crysalead-js / dom-layer

Virtual DOM implementation.
MIT License
30 stars 1 forks source link

Input value on IE #61

Closed ghost closed 9 years ago

ghost commented 9 years ago

Have a look here: https://github.com/Matt-Esch/virtual-dom/issues/287

Same issue for dom-layer, even in MS Edge.

jails commented 9 years ago

I don't have IE so I created an ie-fix banch. Would you mind testing it for me ? the dist files are up to date.

ghost commented 9 years ago

@jails Confirmed not working!! After I started to study your code. There is no patch spec for props so didn't test that. However, look at your code? There is an unused value attribute there? I can get it to work in ie if I change the code to this, but then you attribute spec fails for me. BANG!

var value = element.getAttribute('value');
                element.setAttribute(attrs[name], value);
                element[name] = value;

I checked in all IE now, and IE10 give other issues to with your code. I need to double check. With my change it wanted checkbox to become hello and fails.

ghost commented 9 years ago

Other IE issues was only in IE9 and IE10. When patching boolean attributes... Expecting "" to be a true value. and for patching SVG it want "" to be a null, same for unsetting a xml namespace. It want "" to be null.

jails commented 9 years ago

Ah yeah indeed, not totally awake... just fixed that (I force pushed on the same branch). Otherwise according to global stats IE<11 are less used than IE8. And since IE8 is way to old for being supported, I don't think supporting all IE<11 worth the effort.

ghost commented 9 years ago

issue are if value are set before type in IE, so you need to reverse the order to type first and value last

jails commented 9 years ago

In IE updating the type attribute reset the value. So to become generic, I'm just saving the element value before updating its type and then applying the previous value on it to restore it. It should work whatever is the applying order.

jails commented 9 years ago

During patching you are not always patching both type and value at the same time. Indeed patching is only done when an attribute is modified. So if at some point you only change the type attribute value, you'll need to take care of the value attribute which will be reseted on IE. so the following fix should work now.

ghost commented 9 years ago

I understand you, but IE are not agree :) Your spec works, but in MS Edge there is an issue. To demonstrate:

Works!! attrs: { value: "test", type: "submit" }

Not working!! attrs: { type: "submit", value: "test" }

To get it working, value has to come before type

jails commented 9 years ago

Can you gist me the example you are using which doesn't work on MS Edge ?

ghost commented 9 years ago

Just see my updated comment.

jails commented 9 years ago

I asked a friend to test this fiddle on IE11 https://jsfiddle.net/1eb65aoz/ and it worked. You are probably not using the last dist file of the ie-fix branch.

ghost commented 9 years ago

Jsfiddle works. So I guess this is solved.

jails commented 9 years ago

Yup, I'll merge that.

ghost commented 9 years ago

Another issue raises. When you have a separate hook for type, this would give another IE issues I figured out when you getting the value with getAttribute. Or I'm wrong

See jquery code here and see if we see the same:

var val = jQuery.find.attr( elem, "value" );
                return val != null ?
                    val :
                    // Support: IE10-11+
                    // option.text throws exceptions (#14686, #14858)
                    jQuery.trim( jQuery.text( elem ) );
jails commented 9 years ago

It's more a bug related to <option> tags and the jquery val() abstraction. I don't see any issue here for dom-layer.

ghost commented 9 years ago

Ok. I was just reading - not only in jquery - that the issue was getAttribute sometimes can return null for type. as your hooks is type. Another example are the workaround done in AngularJS