-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BUGFIX release-1-13] Fix {{input}} helper on IE8. #12596
Conversation
(cherry picked from commit dd57452)
(cherry picked from commit 7921711)
* Avoid adding new public API for internal testing purposes. * Remove warnings (IE8 is not a supported platform in Ember 2.0, and this issue does not apply so we shouldn't be warning). * Limit the "locking" down of `type` attribute to `<input>` tags (previously all `type` attributes would have been forced to be static). * Refactor code that forces static values for type when required.
I have pushed builds for this to https://github.com/rwjblue/ember/tree/ie8-input-type to ease testing. |
@gauthierm / @xcskier56 - Please test the assets provided above, and confirm that the various permutations work properly. |
@rwjblue I tested the build provided above and it works for both attributes in the template and for attributes specified in the component definition. Your refactor looks great. |
expression = ['get', 'view.' + attrProperty]; | ||
|
||
if (attrName === 'type' && hardCodeType) { | ||
expression = component.get(attrProperty); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting value to expression
here works? I am a bit flummoxed by that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mixonic and I discussed this in chat. When set to a string HTMLBars manualElement
calls dom.setAttribute
before appending to the fragment, any value other than a string here would result in the element being created and appended to a fragment, then the attributes would be set which would trigger the error on IE8.
Relevant code paths:
Without this coercion to a string, folks could exploit the changes made for IE8 support to push arbitrary statements (by returning an array for type property on the component).
[BUGFIX release-1-13] Fix {{input}} helper on IE8.
@rwjblue per my testing this does not yet fix #12295. Here is a jsbin using the code from https://github.com/rwjblue/ember/tree/ie8-input-type: http://output.jsbin.com/tofelezaze/1 Fire up Edge in IE8 mode or use IE8 and it's still a text input. |
@jerel Your jsbin in IE8 on Windows XP works properly. Edge in emulated IE8 mode does not do a good job of emulating this bug. The type attribute is mutable in emulated IE8 mode in Edge. |
@jerel fwiw, you can get IE8 VMs for testing from modern.ie. |
@gauthierm well, interesting. I can confirm that it works properly in genuine IE8 on WIndows 7. This fix feels a little shaky since it doesn't work in Edge: I do IE8 debugging in Edge when possible because the tools are so much better and now all radio inputs are broken there but not in IE8. |
Unfortunately Edge doesn't emulate IE8 properly and can't be relied on for IE8 JS testing or debugging. This fix is correct in that is does feature detection to see if the type attribute is mutable. In Edge emulated IE8, the type attribute is mutable. In real IE8 the type attribute is not mutable. |
How can the fact that Edge is not properly emulating IE8 be this patches fault? |
Closes #12351.
Fixes #11553.
Fixes #12295.
Closes tildeio/htmlbars#380.