[BUGFIX release-1-13] Make type attr on inputs immutable for IE8 #12351

xcskier56 · 2015-09-16T02:59:39Z

This fixes #11553 and #12295 as well as tildeio/htmlbars#380. This works by detecting if the browser can change the type attr on an input after it has been appended to a node, and then if it cannot, unbinding the type attr so that it is set on the input immediately before being appended to a node.

stavarotti · 2015-09-17T22:56:25Z

👍 I was able to build and run in IE 8 using the VM's on https://dev.modern.ie/tools/vms/windows/. I was however, not able to run the tests in IE 8 due to various exceptions. @rwjblue is the test suite for 1.13.x supposed to run on IE 8?

ghost · 2015-09-22T09:54:16Z

I can confirm that this fixes the IE8 input incompatibility.

jerel · 2015-09-25T20:30:41Z

Any word on when a bugfix release will happen with this fix in it? I've got some pretty ugly hacks in place trying to workaround all components that use {{input}} and they aren't working too well.

ef2k · 2015-09-25T20:37:53Z

👍 great work! Thanks @xcskier56 for taking the time to fix this, can't wait to see it in the next release.

ef2k · 2015-09-29T17:14:37Z

Bump. Will the next release include this bugfix? Any ETA on it? Thanks in advance.

rwjblue · 2015-09-29T21:19:51Z

packages/ember-views/lib/system/build-component-template.js

+  // IE8 Support: IE8 cannot change the type attr of an input after it has been appended to
+  // any node. Therefore, we detect if the browser cannot change the type of the input after
+  // being appended, and unbind type.
+  if (normalized.type && !canChangeType) {


This seems more complicated than I would have assumed.

I was thinking something like:

if (attrs.type && !canChangeType) { attrs.type = getValue(attrs.type); // forces a static value }

Done around here would do the trick.

The simpler version works but you don't get warnings. The warnings are a nice feature.

Simpler is much less likely to cause other issues down the line. We should ensure that a deprecation is printed when you use IE8 (I vaguely think that there is already), and remove the extra complication here.

The simple version does not address #12295. For the case where the component definition defines the type attribute instead of the template we need to compute the property value.

xcskier56 · 2015-09-30T18:33:46Z

@rwhblue, thanks for the feedback. I will get to work on this. Is there an estimated date for the next release that this could make it into?

rwjblue · 2015-09-30T18:54:29Z

I do not have a specific timeline for the next release. This seems to be a big enough fix to justify a releas shortly after landing,

Also, remember that you can use the latests builds from release-1-13 branch at any time (it is available as both bower install --save ember#release-1-13 and from builds.emberjs.com...).

stavarotti · 2015-10-20T01:31:32Z

@xcskier56 Any progress on the changes? I can help were needed.

gauthierm · 2015-10-23T15:48:40Z

I opened xcskier56#1 to clean up this PR slightly.

gauthierm · 2015-10-23T18:12:27Z

Does anyone know if the code in views/text_field.js is obsolete if this PR is merged?

Only compute mutable type flag once and clean up var initilization.

xcskier56 · 2015-10-27T19:51:10Z

@gauthierm, thanks for the help in finishing up the PR. Looking forward to landing this.

benkonrath · 2015-10-28T10:25:41Z

@xcskier56 Thanks for working on this issue. I'm eager to see this merged. Does @gauthierm's change address all of the concerns that @rwjblue had? Let me know if I can help in any way.

xcambar · 2015-10-28T10:27:58Z

ping @xcskier56 (wrong autocomplete in #12351 (comment))

benkonrath · 2015-10-28T10:42:55Z

Thanks @xcambar - sorry for the noise.

gauthierm · 2015-11-02T18:11:55Z

@rwjblue do you have time to look at this PR again?

jerel · 2015-11-03T19:39:18Z

Is there anything more we as the community can do to help get this merged? #12351 (comment) says to bower install from release-1-13 but if I'm not mistaken this PR has to be merged before it's on that branch :)

gauthierm · 2015-11-03T20:34:03Z

@jerel It's not merged so using the latest build won't include the bug fix. You'll have to:

fork the ember.js repo
apply this PR to the 1.13 branch
do a build (remember to update the version before running the build)
fork the ember shim repo
branch from the release-1-13 branch
copy the build into the shim repo
commit, tag and push to your own remote

I've done all that at https://github.com/silverorange/ember/tree/1.13.10-so.1. You can fork off that if you want. I do not recommend using it directly in your bower config as it may disappear after this PR is merged.

jerel · 2015-11-06T20:56:20Z

Has anyone else explicitly tested a radio button with this patch?

Inputs don't cause IE8 to crash now but I'm still seeing a text input like is reported in #12295 with {{input type="radio" name="foo" value=false}}. The markup that gets rendered is:

<input name="foo" class="ember-view ember-text-field" id="ember644" type="radio" value="false"/>`

and it shows up as a text input.

And @gauthierm, thanks so much for sharing that release.

gauthierm · 2015-11-06T21:20:13Z

@jerel I have not tried a radio input. I've only used "email" and "text".

gauthierm · 2015-11-09T14:15:27Z

@jerel I've tried this PR with radio inputs in IE8 on Windows XP and it works fine.

gauthierm · 2015-11-09T14:21:01Z

@jerel okay, I see the behaviour of #12295 when using a component. The built-in {{input}} helper does render a radio button in IE8 but a custom component does not.

xcskier56 · 2015-11-09T15:42:14Z

@gauthierm, this is the situation where I was hoping the attrs.ie8SafeInput would help. In your custom component, you can add this flag to see how it will behave in IE8. In my app, I've added that to all of the inputs and other components so that I catch issues like this earlier.

You can see it here around line 274

gauthierm · 2015-11-09T16:57:35Z

@xcskier56 I think it can be handled as part of this PR. I have a fix for #12295 which I'll submit a PR for in a sec.

jerel · 2015-11-09T17:03:15Z

@gauthierm I just rechecked here and the built in helper {{input type="radio"}} does not work for me on IE8. {{input type="radio" ie8SafeInput=true}} does work however but components like https://github.com/yapplabs/ember-radio-button won't work because they don't handle the attribute.

gauthierm · 2015-11-09T17:46:56Z

@xcskier56 @jerel xcskier56#2 should fix default type attribute values on custom components.

rwjblue · 2015-11-10T16:43:57Z

packages/ember-views/lib/system/build-component-template.js

+
+function canChangeTypeAfterRender(attrs) {
+  // This permits testing of the unbound type attr behavior outside of IE8.
+  if (attrs.ie8SafeInput) {


A [BUGFIX release] should not add new API. I would be fine with either prevent any changing of type after initial render (this was asserted in some versions of Ember prior to 1.13 anyways) or simply ignoring and deferring the testing to applications if they need to confirm things work...

rwjblue · 2015-11-10T16:49:40Z

I left a couple more comments. If someone can address them (either by updating this PR or pulling these commits into another), we can try to finally get this dealt with.

Sorry for the delays here.

rwjblue · 2015-11-12T14:22:32Z

Does anyone know if the code in views/text_field.js is obsolete if this PR is merged?

Which code specifically? The entire file is certainly not obsolete (it is what defines the component used by {{input type="text"}} helper)...

gauthierm · 2015-11-12T15:59:51Z

Does anyone know if the code in views/text_field.js is obsolete if this PR is merged?

Which code specifically? The entire file is certainly not obsolete (it is what defines the component used by {{input type="text"}} helper)...

https://github.com/emberjs/ember.js/blob/release-1-13/packages/ember-views/lib/views/text_field.js#L13-L35 and associated code. It looks like it's doing some extra checking of whether the type attribute can be bound.

rwjblue · 2015-11-12T18:13:01Z

Merged in under #12596.

[BUGFIX release-1-13] Make type attr on inputs immutable for IE8

dd57452

rwjblue reviewed Sep 29, 2015
View reviewed changes

Only compute mutable type flag once and clean up var initilization.

7921711

gauthierm mentioned this pull request Oct 23, 2015

Only compute mutable type flag once and clean up var initilization. xcskier56/ember.js#1

Merged

Merge pull request #1 from gauthierm/cleanup-ie8-immutable-type-fix

d6510a8

Only compute mutable type flag once and clean up var initilization.

gauthierm mentioned this pull request Nov 9, 2015

Radio buttons render as a text input in IE8 #12295

Closed

rwjblue reviewed Nov 10, 2015
View reviewed changes

rwjblue mentioned this pull request Nov 12, 2015

[BUGFIX release-1-13] Fix {{input}} helper on IE8. #12596

Merged

rwjblue closed this Nov 12, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUGFIX release-1-13] Make type attr on inputs immutable for IE8 #12351

[BUGFIX release-1-13] Make type attr on inputs immutable for IE8 #12351

xcskier56 commented Sep 16, 2015

stavarotti commented Sep 17, 2015

ghost commented Sep 22, 2015

jerel commented Sep 25, 2015

ef2k commented Sep 25, 2015

ef2k commented Sep 29, 2015

rwjblue Sep 29, 2015

gauthierm Oct 23, 2015

rwjblue Nov 10, 2015

gauthierm Nov 11, 2015

xcskier56 commented Sep 30, 2015

rwjblue commented Sep 30, 2015

stavarotti commented Oct 20, 2015

gauthierm commented Oct 23, 2015

gauthierm commented Oct 23, 2015

xcskier56 commented Oct 27, 2015

benkonrath commented Oct 28, 2015

xcambar commented Oct 28, 2015

benkonrath commented Oct 28, 2015

gauthierm commented Nov 2, 2015

jerel commented Nov 3, 2015

gauthierm commented Nov 3, 2015

jerel commented Nov 6, 2015

gauthierm commented Nov 6, 2015

gauthierm commented Nov 9, 2015

gauthierm commented Nov 9, 2015

xcskier56 commented Nov 9, 2015

gauthierm commented Nov 9, 2015

jerel commented Nov 9, 2015

gauthierm commented Nov 9, 2015

rwjblue Nov 10, 2015

rwjblue commented Nov 10, 2015

rwjblue commented Nov 12, 2015

gauthierm commented Nov 12, 2015

rwjblue commented Nov 12, 2015

[BUGFIX release-1-13] Make type attr on inputs immutable for IE8 #12351

[BUGFIX release-1-13] Make type attr on inputs immutable for IE8 #12351

Conversation

xcskier56 commented Sep 16, 2015

stavarotti commented Sep 17, 2015

ghost commented Sep 22, 2015

jerel commented Sep 25, 2015

ef2k commented Sep 25, 2015

ef2k commented Sep 29, 2015

rwjblue Sep 29, 2015

Choose a reason for hiding this comment

gauthierm Oct 23, 2015

Choose a reason for hiding this comment

rwjblue Nov 10, 2015

Choose a reason for hiding this comment

gauthierm Nov 11, 2015

Choose a reason for hiding this comment

xcskier56 commented Sep 30, 2015

rwjblue commented Sep 30, 2015

stavarotti commented Oct 20, 2015

gauthierm commented Oct 23, 2015

gauthierm commented Oct 23, 2015

xcskier56 commented Oct 27, 2015

benkonrath commented Oct 28, 2015

xcambar commented Oct 28, 2015

benkonrath commented Oct 28, 2015

gauthierm commented Nov 2, 2015

jerel commented Nov 3, 2015

gauthierm commented Nov 3, 2015

jerel commented Nov 6, 2015

gauthierm commented Nov 6, 2015

gauthierm commented Nov 9, 2015

gauthierm commented Nov 9, 2015

xcskier56 commented Nov 9, 2015

gauthierm commented Nov 9, 2015

jerel commented Nov 9, 2015

gauthierm commented Nov 9, 2015

rwjblue Nov 10, 2015

Choose a reason for hiding this comment

rwjblue commented Nov 10, 2015

rwjblue commented Nov 12, 2015

gauthierm commented Nov 12, 2015

rwjblue commented Nov 12, 2015