Skip to content
This repository has been archived by the owner on Apr 4, 2019. It is now read-only.

input.type IE issue #380

Closed
stefanpenner opened this issue Jul 6, 2015 · 32 comments
Closed

input.type IE issue #380

stefanpenner opened this issue Jul 6, 2015 · 32 comments
Assignees

Comments

@stefanpenner
Copy link
Collaborator

source: emberjs/ember.js#11553

TL;DR input.type must be set before input is appended to any tree (fragment, or body ancestored)

var input = document.createElement('input');
var docFragment = document.createDocumentFragment();
docFragment.appendChild(input);

input.setAttribute('type', 'text');  // throws in IE8
@jridgewell
Copy link

This extends a bit further: IE < 9 can't change the input's type after it's been inserted into any DOM tree:

var input = document.createElement('input');
input.type = 'text';
var form = document.createElement('form');
form.appendChild(input);

input.setAttribute('type', 'password'); // Throws error

@stefanpenner
Copy link
Collaborator Author

i have removed fragment or body ancestored, and left merely any tree. Does that sound accurate?

@jridgewell
Copy link

I was actually trying to point that type can't be changed vs. that it must be set beforehand. But, it also happens after it's inserted into any tree. 😃

@stefanpenner
Copy link
Collaborator Author

I was actually trying to point that type can't be changed vs. that it must be set beforehand.

Is that for only < IE9, if so. Im not sure what we should do about that other then tell me its just isn't supported.

@jridgewell
Copy link

Is that for only < IE9

I've never tested on IE 9. jQuery claims it's just 8 and lower:

Attempting to change the type attribute on an input or button element created via document.createElement() will throw an exception on Internet Explorer 8 or older.


I'm not sure what we should do about that other then tell me its just isn't supported.

I think you can get around it with #cloneNode, then just replace the old with the cloned (with new type).

@MichaelVdheeren
Copy link

Where are we with this issue? I've tried the alternative to use <input > and then actions to update the value but even then I can't set the type via type={{type}}.

@XrXr
Copy link

XrXr commented Aug 3, 2015

@stefanpenner any updates? it's been quite a bit

@MichaelVdheeren
Copy link

By now we have decided (together with business reps) to drop support for IE8 in future version (thank you, finally) hence we are not depending on a fix anymore. But I can imagine others are...

@xcskier56
Copy link

@stefanpenner this seems to be an issue for us also. Any word on a fix? Thanks.

@rwjblue
Copy link
Collaborator

rwjblue commented Aug 25, 2015

I do not believe that anyone has worked on this yet. A good first start would be to submit a failing test case (we run our test suite against IE9 so submitting a failing test here should show the failure).

@xcskier56
Copy link

Ok. I haven't dug much into htmlbars before. Any suggested files to look at?

@XrXr
Copy link

XrXr commented Aug 25, 2015

@rwjblue This is an IE8 specific issue, do tests run against it?

@xcskier56
Copy link

After digging into this, I'm not really sure what a failing test should look like because I'm not sure what should be tested and changed. My two thoughts are:

  • Instead of outputting a comment and a createMorphAt, the fragementOpcodeCompiler and hydrationOpcodeCompiler treated the input helper as if it was an element, putting it in the dom before attaching the morph.
  • In buildRenderNodes, change the createMorphAt so that is creates the input element, sets the attrs, and then appends to the dom.

I'm not sure which approach is a seems like a better option, or if I'm barking up the completely wrong tree. @rwjblue which direction do you think I should go? I'll try to write a test for that.

@xcskier56
Copy link

@rwjblue I've been working on this for the last day and a half straight, and I don't think I'm anywhere near a solution or even a idea of what to test for. Some guidance would be very appreciated.

@rwjblue
Copy link
Collaborator

rwjblue commented Aug 27, 2015

@mmun - We could use your advice here....

@xcskier56
Copy link

After a lot of digging, I have modified manualElement in htmlbars-runtime/render to add the type attr in the template's buildFragement method and remove it from statements and buildRenderedNodes This solved the IE8 issue of changing that type of the input after the element has become a child node. However, this has raised a few issues.

By doing this, type becomes a static attribute, it cannot be passed in later. I've quickly found places where type gets set as ['get', 'view.type']. This raised a bigger question, does IE8's inability to change the type attr after render mean that we can never have type as a bound attribute?

@ef2k
Copy link

ef2k commented Sep 2, 2015

I've been able to reproduce this in IE8 by simply using an input helper with a bound value attribute. @xcskier56 mind sharing your code? I'd like to look into this a bit further and help however possible.

@xcskier56
Copy link

@eddflrs, I've pushed some really basic code to my fork here here. My basic strategy is to not create any morphs for a type attr. This way we will not have to change the type after it has been appended to a node. But for this to work, when we build the element, we need some sort of knowledge about the environment so that we can correctly set type attributes that are bound.

@ef2k
Copy link

ef2k commented Sep 4, 2015

@xcskier56 thanks for sharing! I'll have a closer look over the weekend.

@jerel
Copy link

jerel commented Sep 10, 2015

Is anybody close to having a fix or a workaround for this? As far as I can tell all form inputs have been broken in IE8 for a couple months now. A basic {{input}} causes my app to whitescreen

@stavarotti
Copy link

@rwjblue @xcskier56 I have bandwidth to work on this issue as I need to get IE 8 working for a client project. I am available 8 hours a day starting 8/15. Any pointers regarding where to start?

@xcskier56
Copy link

@stavarotti I think I actually have a way to solve this. If we are ok limiting the type attr to being set once and only once at first render, then this works. Which is a requirement for this to work with IE8 anyways

Basically, the idea is to modify the function that creates the attributes to be added to the component so that the type attr behaves like id attr.

I think that this actually belongs in Ember.js, but I'll describe it here. In ember.js/ember-views/build-component-template.js we need to modify the normalizeComponentAttributes function to turn the type property from a hash to a string.

Something like this will work

if (attrs.type) {
  attrs.type = attrs.type[1]
}

This needs to be placed after the loop through the attribute bindings so that it has the correct information. This way when the manualElement function gets called, attrs['type'] is a string instead of a array and it will then set the attribute on the element before it has a parent node.

@xcskier56
Copy link

I am currently working on getting tests to pass with more or less what I outlined above. The real question is how should we handle the change in behavior that this will necessitate.

In the tests, we expect that we can set the type attr on an input with a dynamic property, and then when this property gets changed after the input is rendered, the input's type should change to the new value.

This is exactly the behavior that will error in IE8.

I think that including some sort of flag on the input component like ie8Safe which would turn on and off the new behavior. Then the only question is what should the default be.

@rwjblue How do you think we should handle this change in behavior?

@rwjblue
Copy link
Collaborator

rwjblue commented Sep 15, 2015

In the tests, we expect that we can set the type attr on an input with a dynamic property, and then when this property gets changed after the input is rendered, the input's type should change to the new value.

The test in Ember that confirms type can change was added in 1.13 (see emberjs/ember.js@e6f3174). We could detect if input can change type after creation (only once, not for every {{input}}) and toggle that test off in scenarios that don't work.

@jerel
Copy link

jerel commented Sep 15, 2015

Would it make sense to just detect IE8 to decide whether it should be a static or dynamic attribute? document.documentMode === 8 in IE8 and its value is undefined in most others

@XrXr
Copy link

XrXr commented Sep 15, 2015

Would it make sense to just detect IE8 to decide whether it should be a static or dynamic attribute? document.documentMode === 8 in IE8 and its value is undefined in most others

A better way to detect it would be just catching the exception, I think.

@xcskier56
Copy link

@rwjblue where should I put this so that it is evaluated once?

Also, is there a way to optionally make this the default behavior for an app? In our case, we support IE8, therefore I would like to make the way inputs behave in modern browsers the same as in IE8 so that I can catch issues that might arise from different add ons much sooner.

@rwjblue
Copy link
Collaborator

rwjblue commented Sep 15, 2015

Also, is there a way to optionally make this the default behavior for an app? In our case, we support IE8, therefore I would like to make the way inputs behave in modern browsers the same as in IE8 so that I can catch issues that might arise from different add ons much sooner.

Yep, don't use a bound property for type.

@xcskier56
Copy link

Where should this feature detection code go? Is there a standard place that browser oddities are?

@rwjblue
Copy link
Collaborator

rwjblue commented Sep 15, 2015

I was thinking something similar to: https://github.com/emberjs/ember.js/blob/master/packages/ember-views/lib/views/text_field.js#L13-L35

Also, the PR for Ember should target release-1-13 branch (since we don't need to include it in 2.x).

@stavarotti
Copy link

@xcskier56 Thanks for doing the leg work. I got pulled into other projects was not able to dedicate the time originally stated.

@rwjblue
Copy link
Collaborator

rwjblue commented Nov 12, 2015

Fixed by emberjs/ember.js#12596.

@rwjblue rwjblue closed this as completed Nov 12, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants