Skip to content
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

Merged
merged 4 commits into from
Nov 12, 2015

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Nov 12, 2015

Closes #12351.
Fixes #11553.
Fixes #12295.
Closes tildeio/htmlbars#380.

xcskier56 and others added 3 commits November 12, 2015 10:41
* 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.
@rwjblue
Copy link
Member Author

rwjblue commented Nov 12, 2015

I have pushed builds for this to https://github.com/rwjblue/ember/tree/ie8-input-type to ease testing.

rwjblue added a commit to rwjblue/ember that referenced this pull request Nov 12, 2015
@rwjblue
Copy link
Member Author

rwjblue commented Nov 12, 2015

@gauthierm / @xcskier56 - Please test the assets provided above, and confirm that the various permutations work properly.

@gauthierm
Copy link

@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);
Copy link
Member

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.

Copy link
Member Author

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).
rwjblue added a commit that referenced this pull request Nov 12, 2015
[BUGFIX release-1-13] Fix {{input}} helper on IE8.
@rwjblue rwjblue merged commit 4566dc9 into emberjs:release-1-13 Nov 12, 2015
@rwjblue rwjblue deleted the ie8-input-type branch November 12, 2015 18:11
@jerel
Copy link
Contributor

jerel commented Nov 12, 2015

@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.

@gauthierm
Copy link

@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.

@gauthierm
Copy link

@jerel fwiw, you can get IE8 VMs for testing from modern.ie.

@jerel
Copy link
Contributor

jerel commented Nov 12, 2015

@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.

@gauthierm
Copy link

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.

@rwjblue
Copy link
Member Author

rwjblue commented Nov 13, 2015

This fix feels a little shaky since it doesn't work in Edge

How can the fact that Edge is not properly emulating IE8 be this patches fault?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants