Trigger attribute-changed-callback for different attribute capitalization #168
Conversation
| @@ -132,7 +132,7 @@ export default function(internals) { | |||
| const oldValue = Native.Element_getAttribute.call(this, name); | |||
| Native.Element_setAttribute.call(this, name, newValue); | |||
| newValue = Native.Element_getAttribute.call(this, name); | |||
| internals.attributeChangedCallback(this, name, oldValue, newValue, null); | |||
| internals.attributeChangedCallback(this, name.toLowerCase(), oldValue, newValue, null); | |||
There was a problem hiding this comment.
I think it would be clearer to create the lowercase version of this string before the const oldValue = ... line so that it's used it in place of name when calling the native API. Even though the behavior should be the same, I think it'll be more explicit that the effects are intended for an attribute with that particular name. (Also, below in removeAttribute.)
There was a problem hiding this comment.
There are attributes that do rely on capitalization, for example viewBox on svg: https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/viewBox If we therefore lowercase all attribute names, we would break svg.setAttribute('viewBox', '....'). I think the lowercasing should therefore only be in our callback, as we check it there with observedAttributes.
There was a problem hiding this comment.
I thought viewBox was in the SVG namespace, so you would need to use setAttributeNS for that one? Although, I've definitely been confused when running into this sort of thing before, so I'll have to double check.
There was a problem hiding this comment.
Shall we move forward with this PR?
|
Issue moved to webcomponents/polyfills#110. |
|
Remaking PR in monorepo |
This patches
setAttributeandremoveAttributeto lowercase when setting attributes. I have confirmed with the implementation in Chrome thatsetAttributeNSand remove alike do NOT trigger the callback with different capitalization.Fixes #167