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

attr.split() causes errors in IE10,11 in live.js on CanJS 2.2.6 when using jQuery #1790

Closed
gregfleury opened this Issue Jul 17, 2015 · 16 comments

Comments

Projects
None yet
4 participants
@gregfleury

gregfleury commented Jul 17, 2015

Line 302 on live.js causes errors in IE10,11 (and presumably 9). The reason is that IE will format "attr" as an integer if the value is not a string:

elements.getAttr(el, attributeName) <-- cannot assume string in IE

The fix for it is simple:
Wrap the line in a String() cast.

Line 302 live.js: var attr = String(elements.getAttr(el, attributeName));

@daffl daffl added this to the 2.2.7 milestone Jul 17, 2015

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Jul 17, 2015

Contributor

I think that makes sense. What's the breaking test case (maybe as a JSBin) that causes this issue?

Contributor

daffl commented Jul 17, 2015

I think that makes sense. What's the breaking test case (maybe as a JSBin) that causes this issue?

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jul 17, 2015

Contributor

@gregfleury thanks for positing! What do you mean "format attr as an integer"?

Do you mean it will return the attribute as a number?

If so, shouldn't elements.getAttr be doing the type conversion?

Contributor

justinbmeyer commented Jul 17, 2015

@gregfleury thanks for positing! What do you mean "format attr as an integer"?

Do you mean it will return the attribute as a number?

If so, shouldn't elements.getAttr be doing the type conversion?

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jul 24, 2015

Contributor

@gregfleury can you let us know what causes that error? Some code that will show it? We need to add a test to fix this and I'm not sure what that test looks like.

Contributor

justinbmeyer commented Jul 24, 2015

@gregfleury can you let us know what causes that error? Some code that will show it? We need to add a test to fix this and I'm not sure what that test looks like.

@daffl daffl modified the milestones: 2.2.8, 2.2.7 Jul 24, 2015

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jul 28, 2015

Contributor

@nlundquist did you look into this? Thanks!

Contributor

justinbmeyer commented Jul 28, 2015

@nlundquist did you look into this? Thanks!

@nlundquist

This comment has been minimized.

Show comment
Hide comment
@nlundquist

nlundquist Jul 29, 2015

Collaborator

I did look into this and it doesn't appear to be IE specific. I made some fiddles that demonstrated consistent types returned by CanJS attr and DOM getAttribute on IE & Chrome in several cases.

However attr.get has a case where instead of the attribute value being written and retrieved on the element using 'getAttribute' / 'setAttribute' it's actually written as an object property on the element object itself, i.e. 'el[prop] = val;'. In that case it would be easily conceivable that an number type might be stored and retrieved rather than a string as might be expected when getting an element attribute. I'm trying to determine when this case is used and reverse engineer from that a reasonable test case that reproduces an error like the one reported.

Collaborator

nlundquist commented Jul 29, 2015

I did look into this and it doesn't appear to be IE specific. I made some fiddles that demonstrated consistent types returned by CanJS attr and DOM getAttribute on IE & Chrome in several cases.

However attr.get has a case where instead of the attribute value being written and retrieved on the element using 'getAttribute' / 'setAttribute' it's actually written as an object property on the element object itself, i.e. 'el[prop] = val;'. In that case it would be easily conceivable that an number type might be stored and retrieved rather than a string as might be expected when getting an element attribute. I'm trying to determine when this case is used and reverse engineer from that a reasonable test case that reproduces an error like the one reported.

@gregfleury

This comment has been minimized.

Show comment
Hide comment
@gregfleury

gregfleury Jul 29, 2015

I was away on project work. nlundquist is entirely correct -- we found the same situation. I will provide follow up source shortly.

gregfleury commented Jul 29, 2015

I was away on project work. nlundquist is entirely correct -- we found the same situation. I will provide follow up source shortly.

@nlundquist

This comment has been minimized.

Show comment
Hide comment
@nlundquist

nlundquist Jul 29, 2015

Collaborator

I've looked at this a bit closer and I'm still stumped on how an issue like this could be happening. Do you know what attribute was being accessed when this occurred?

The case that I mentioned, where element object property modification is used instead of setAttribute, is only for very specific attributes, those that the browser has some special handling for already, for example:

  • class
  • value
  • src
  • style
  • innertext
  • textcontext

With the exception of value, you wouldn't expect those attributes to ever be an integer, perhaps a string representation of one. All other attributes are set/get by the standard DOM methods which by all accounts appear to work consistently between browsers.

I did some testing with 'element.value' modification across browsers and it worked consistently, implicitly converting numeral values to strings, e.g

<input type="text" />
var input = document.querySelector('input');
input.value = 5;
console.log(input.value); // "5"

I just can't think up a test case using one of the special cased attributes mentioned above resulting in can.attr.get returning an integer. Any ideas?

It could still be that getAttribute itself still returns number types in some edge case, but we haven't found any indication of that.

Collaborator

nlundquist commented Jul 29, 2015

I've looked at this a bit closer and I'm still stumped on how an issue like this could be happening. Do you know what attribute was being accessed when this occurred?

The case that I mentioned, where element object property modification is used instead of setAttribute, is only for very specific attributes, those that the browser has some special handling for already, for example:

  • class
  • value
  • src
  • style
  • innertext
  • textcontext

With the exception of value, you wouldn't expect those attributes to ever be an integer, perhaps a string representation of one. All other attributes are set/get by the standard DOM methods which by all accounts appear to work consistently between browsers.

I did some testing with 'element.value' modification across browsers and it worked consistently, implicitly converting numeral values to strings, e.g

<input type="text" />
var input = document.querySelector('input');
input.value = 5;
console.log(input.value); // "5"

I just can't think up a test case using one of the special cased attributes mentioned above resulting in can.attr.get returning an integer. Any ideas?

It could still be that getAttribute itself still returns number types in some edge case, but we haven't found any indication of that.

@gregfleury

This comment has been minimized.

Show comment
Hide comment
@gregfleury

gregfleury Jul 29, 2015

We initially had this mustache template:

<ul class="dropdown-menu dropdown-menu-left js-contextAction-dropdown" role="menu">
            <li class="dropdown-header">Actions</li>
            {{#contextActions}}
            <li role="presentation" value="{{Index}}"><a role="menuitem" tabindex="-1" href="#">{{Name}}</a></li>
            {{/contextActions}}
        </ul>

All we did was change the li elements value={{Index}} to action-index={{Index}} to fix the problem.

So you cannot use "value" implicitly on an element -- which in itself is not a bug, but an API toolkit usability quirk with CanJS. Internally, something is happening with the Value param that is causing this error that I repeated earlier.

gregfleury commented Jul 29, 2015

We initially had this mustache template:

<ul class="dropdown-menu dropdown-menu-left js-contextAction-dropdown" role="menu">
            <li class="dropdown-header">Actions</li>
            {{#contextActions}}
            <li role="presentation" value="{{Index}}"><a role="menuitem" tabindex="-1" href="#">{{Name}}</a></li>
            {{/contextActions}}
        </ul>

All we did was change the li elements value={{Index}} to action-index={{Index}} to fix the problem.

So you cannot use "value" implicitly on an element -- which in itself is not a bug, but an API toolkit usability quirk with CanJS. Internally, something is happening with the Value param that is causing this error that I repeated earlier.

@nlundquist

This comment has been minimized.

Show comment
Hide comment
@nlundquist

nlundquist Jul 29, 2015

Collaborator

Yeah I've just had that case come to mind and confirmed the error. Like I mention above, the 'value' attribute has special handling that causes it to be written to the element object 'value' property rather than be written like an attribute. This is fine when the element is an input tag. 'inputtag.value = 5;' implicitly converts any non-string type being set to a string, however this is not true for non-input elements and attr.set / attr.get doesn't expect that.

So modifying my example above to reproduce the issue:

<div class="test"></div>
var div = document.querySelector('div.test');
div.value = 5;
console.log(div.value); // 5
// or
can.attr.set(div, 'value', 5);
console.log(can.attr.get(div,'value')); // 5

I'll be making a modification so the special case for 'value' attribute modification is only used for appropriate form elements.

Collaborator

nlundquist commented Jul 29, 2015

Yeah I've just had that case come to mind and confirmed the error. Like I mention above, the 'value' attribute has special handling that causes it to be written to the element object 'value' property rather than be written like an attribute. This is fine when the element is an input tag. 'inputtag.value = 5;' implicitly converts any non-string type being set to a string, however this is not true for non-input elements and attr.set / attr.get doesn't expect that.

So modifying my example above to reproduce the issue:

<div class="test"></div>
var div = document.querySelector('div.test');
div.value = 5;
console.log(div.value); // 5
// or
can.attr.set(div, 'value', 5);
console.log(can.attr.get(div,'value')); // 5

I'll be making a modification so the special case for 'value' attribute modification is only used for appropriate form elements.

@nlundquist

This comment has been minimized.

Show comment
Hide comment
@nlundquist

nlundquist Jul 30, 2015

Collaborator

I've done a short review and the elements with 'native' value attributes are:
button, option, input, li, meter, progress, param

My results for which elements do implicit type conversions and which link the values between 'element.value' & 'element.getAttribute("value")' are shown in this JSBin: http://jsbin.com/hehuqenoya/1/edit?html,js,console

The results themselves are rather surprising. In Safari, button, option, input and param all do implicit string conversion, and all elements but input link the value of the element property to the attribute. This behaviour is consistent with Chrome and IE 11. Only exception is that in IE the value property of the progress element isn't linked to the attribute, but I don't think that's a case worth making special effort for.

I think a reasonable fix, maintaining current behaviour as much as possible, would be to only access 'element.value' for the elements that do implicit type conversions, preventing this case where a number type is returned unexpectedly. However, I'm wondering what the impact would be of removing the special case for 'value' all together, and just using the typical setAttribute/getAttribute behaviour. In my brief testing it accomplishes the same thing, and additionally, though setting 'input.value' doesn't in turn update the value returned by 'input.getAttribute', 'input.setAttribute' does update the value of 'input.value' keeping the state of the element more consistent. Eg: http://jsbin.com/ripinorezu/edit?html,js,console,output

I'll try removing the special case for value tomorrow and see if it has any obvious impact on our test suite.

If anyone has background or insight regarding the purpose of that value attribute special case, if its intended for say backwards compatibility, cross browser compatibility or performance reasons, please join in this thread.

Collaborator

nlundquist commented Jul 30, 2015

I've done a short review and the elements with 'native' value attributes are:
button, option, input, li, meter, progress, param

My results for which elements do implicit type conversions and which link the values between 'element.value' & 'element.getAttribute("value")' are shown in this JSBin: http://jsbin.com/hehuqenoya/1/edit?html,js,console

The results themselves are rather surprising. In Safari, button, option, input and param all do implicit string conversion, and all elements but input link the value of the element property to the attribute. This behaviour is consistent with Chrome and IE 11. Only exception is that in IE the value property of the progress element isn't linked to the attribute, but I don't think that's a case worth making special effort for.

I think a reasonable fix, maintaining current behaviour as much as possible, would be to only access 'element.value' for the elements that do implicit type conversions, preventing this case where a number type is returned unexpectedly. However, I'm wondering what the impact would be of removing the special case for 'value' all together, and just using the typical setAttribute/getAttribute behaviour. In my brief testing it accomplishes the same thing, and additionally, though setting 'input.value' doesn't in turn update the value returned by 'input.getAttribute', 'input.setAttribute' does update the value of 'input.value' keeping the state of the element more consistent. Eg: http://jsbin.com/ripinorezu/edit?html,js,console,output

I'll try removing the special case for value tomorrow and see if it has any obvious impact on our test suite.

If anyone has background or insight regarding the purpose of that value attribute special case, if its intended for say backwards compatibility, cross browser compatibility or performance reasons, please join in this thread.

@nlundquist

This comment has been minimized.

Show comment
Hide comment
@nlundquist

nlundquist Jul 30, 2015

Collaborator

I've answered my own question regarding 'input.value' vs 'input.setAttribute("value", ...)'. It's a quirk of the DOM specs that I was unfamiliar with. For text input types, the HTMLInputElement 'value' property is the current value of the component, while the 'value' attribute is mapped to the defaultValue property. The defaultValue property, when changed, updates the value property, but of course not vice versa.

See http://zvon.org/xxl/DOM2reference/Output/HTML/attribute_value_HTMLInputElement.html for an excerpt from the DOM 2 spec regarding this.

So due to that distinciton my revision will continue to special case setting the value attribute for input elements so we correctly alter the value property instead of the defaultValue via the attribute. For all other elements value will be treated like a normal attribute.

Collaborator

nlundquist commented Jul 30, 2015

I've answered my own question regarding 'input.value' vs 'input.setAttribute("value", ...)'. It's a quirk of the DOM specs that I was unfamiliar with. For text input types, the HTMLInputElement 'value' property is the current value of the component, while the 'value' attribute is mapped to the defaultValue property. The defaultValue property, when changed, updates the value property, but of course not vice versa.

See http://zvon.org/xxl/DOM2reference/Output/HTML/attribute_value_HTMLInputElement.html for an excerpt from the DOM 2 spec regarding this.

So due to that distinciton my revision will continue to special case setting the value attribute for input elements so we correctly alter the value property instead of the defaultValue via the attribute. For all other elements value will be treated like a normal attribute.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jul 30, 2015

Contributor

We should feature detect if value is in the element.

"value" in el

Sent from my iPhone

On Jul 29, 2015, at 9:22 PM, Nils Lundquist notifications@github.com wrote:

I've answered my own question regarding 'input.value' vs 'input.setAttribute("value", ...)'. It's a quirk of the DOM specs that I was unfamiliar with. For text input types, the HTMLInputElement 'value' property is the current value of the component, while the 'value' attribute is mapped to the defaultValue property. The defaultValue property, when changed, updates the value property, but of course not vice versa.

See http://zvon.org/xxl/DOM2reference/Output/HTML/attribute_value_HTMLInputElement.html for an excerpt from the DOM 2 spec regarding this.

So for good semantic reasons we should continue to special case setting the value attribute for input elements so we correctly change the value property directly instead of the defaultValue via the attribute. For all other elements value should be treated like a normal attribute.


Reply to this email directly or view it on GitHub.

Contributor

justinbmeyer commented Jul 30, 2015

We should feature detect if value is in the element.

"value" in el

Sent from my iPhone

On Jul 29, 2015, at 9:22 PM, Nils Lundquist notifications@github.com wrote:

I've answered my own question regarding 'input.value' vs 'input.setAttribute("value", ...)'. It's a quirk of the DOM specs that I was unfamiliar with. For text input types, the HTMLInputElement 'value' property is the current value of the component, while the 'value' attribute is mapped to the defaultValue property. The defaultValue property, when changed, updates the value property, but of course not vice versa.

See http://zvon.org/xxl/DOM2reference/Output/HTML/attribute_value_HTMLInputElement.html for an excerpt from the DOM 2 spec regarding this.

So for good semantic reasons we should continue to special case setting the value attribute for input elements so we correctly change the value property directly instead of the defaultValue via the attribute. For all other elements value should be treated like a normal attribute.


Reply to this email directly or view it on GitHub.

@nlundquist

This comment has been minimized.

Show comment
Hide comment
@nlundquist

nlundquist Jul 30, 2015

Collaborator

That couldn't be the only change however? The core issue is that an integer type is returned unexpectedly from attr.get, which should always return a string. If that were the only change an integer type could still be set / returned on, for example, an li element reproducing the issue at hand. If I'm understanding you correctly. This JSBin demos the issue I'm thinking of http://jsbin.com/sacipomewa/1/edit?html,js,console

Collaborator

nlundquist commented Jul 30, 2015

That couldn't be the only change however? The core issue is that an integer type is returned unexpectedly from attr.get, which should always return a string. If that were the only change an integer type could still be set / returned on, for example, an li element reproducing the issue at hand. If I'm understanding you correctly. This JSBin demos the issue I'm thinking of http://jsbin.com/sacipomewa/1/edit?html,js,console

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jul 30, 2015

Contributor

All I am suggesting is not to hard code what element types to use the value property.

Sent from my iPhone

On Jul 30, 2015, at 12:13 AM, Nils Lundquist notifications@github.com wrote:

That couldn't be the only change however? The core issue is that an integer type is returned unexpectedly from attr.get, which should always return a string. If that were the only change an integer type could still be set / returned on, for example, an li element reproducing the issue at hand. If I'm understanding you correctly. This JSBin demos the issue I'm thinking of http://jsbin.com/jayetogafu/1/edit?html,js,console


Reply to this email directly or view it on GitHub.

Contributor

justinbmeyer commented Jul 30, 2015

All I am suggesting is not to hard code what element types to use the value property.

Sent from my iPhone

On Jul 30, 2015, at 12:13 AM, Nils Lundquist notifications@github.com wrote:

That couldn't be the only change however? The core issue is that an integer type is returned unexpectedly from attr.get, which should always return a string. If that were the only change an integer type could still be set / returned on, for example, an li element reproducing the issue at hand. If I'm understanding you correctly. This JSBin demos the issue I'm thinking of http://jsbin.com/jayetogafu/1/edit?html,js,console


Reply to this email directly or view it on GitHub.

@nlundquist

This comment has been minimized.

Show comment
Hide comment
@nlundquist

nlundquist Jul 30, 2015

Collaborator

Why do you prefer the property over the attribute in the absence of the defaultValue issue unique to input elements?

Collaborator

nlundquist commented Jul 30, 2015

Why do you prefer the property over the attribute in the absence of the defaultValue issue unique to input elements?

@nlundquist

This comment has been minimized.

Show comment
Hide comment
@nlundquist

nlundquist Aug 3, 2015

Collaborator

To summarize some discussions we've been having internally, the issue is that live.js requires a string value for attributes, so it can do manipulations as expected. However, we've concluded that non-string return values are a valid case, so to resolve the issue at hand we'll cast the value to a string when required.

It took some thought to determine why returning a type other than a string from can.attr is correct. The key is, rather than simply accessing attributes, can.attr is designed to get / set state for components which requires, at times, manipulating component properties, which may be any type.

Considering that, we'll be simplifying this behaviour, increasing it's documentation and renaming can.attr & elements.getAttr to something that more accurately reflects the behaviour.

Collaborator

nlundquist commented Aug 3, 2015

To summarize some discussions we've been having internally, the issue is that live.js requires a string value for attributes, so it can do manipulations as expected. However, we've concluded that non-string return values are a valid case, so to resolve the issue at hand we'll cast the value to a string when required.

It took some thought to determine why returning a type other than a string from can.attr is correct. The key is, rather than simply accessing attributes, can.attr is designed to get / set state for components which requires, at times, manipulating component properties, which may be any type.

Considering that, we'll be simplifying this behaviour, increasing it's documentation and renaming can.attr & elements.getAttr to something that more accurately reflects the behaviour.

nlundquist pushed a commit that referenced this issue Sep 11, 2015

Nils Lundquist
Fixes #1790. Elements.getAttr correctly returns other types, but stri…
…ng is expected. Added test case that reproduces a non-string attribute being returned and subsequent error.

@daffl daffl closed this in #1907 Sep 11, 2015

daffl added a commit that referenced this issue Sep 11, 2015

Merge pull request #1907 from bitovi/fix/1790/cast-attr-to-string
Fixes #1790. Cast element.getAttrs to string in live.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment