#908 - Fix Safari TP and @webcomponents/custom-element polyfill #909
Conversation
|
It did because of WICG/webcomponents#604. |
| @@ -89,7 +89,7 @@ const attributesContext = propContext(attributes, { | |||
|
|
|||
| // Default attribute applicator. | |||
| [symbols.default] (elem, name, value) { | |||
| const { props, prototype } = customElements.get(elem.tagName) || { | |||
| const { props, prototype } = customElements.get(elem.tagName.toLowerCase()) || { | |||
There was a problem hiding this comment.
@bengummer this is what Safari TP would fail on before.
…nd it's breaking some because i Also updated readme to recommend the @webcomponents polyfills. #908
| const $prevOldValue = createSymbol(); | ||
| const $prevNewValue = createSymbol(); | ||
|
|
||
| function preventDoubleCalling (elem, name, oldValue, newValue) { |
There was a problem hiding this comment.
This fixes the polyfill.
| } | ||
|
|
||
| // Set data so we can prevent double calling if the polyfill. | ||
| this[$prevName] = name; |
There was a problem hiding this comment.
The other part of being defensive.
| const $prevNewValue = createSymbol(); | ||
|
|
||
| function preventDoubleCalling (elem, name, oldValue, newValue) { | ||
| return name === elem[$prevName] |
There was a problem hiding this comment.
@justinfagnani it would be good to get your thoughts here. I thought about fixing it at the polyfill level because it seems a result of how the Mutation Observer handler tries to check to see if it was called by the setAttribute() override. The problem seems to exist because of the microtask delay and the fact that we sync properties back to attributes if { attribute: true }.
There was a problem hiding this comment.
Can you send me a repro of the original issue?
There was a problem hiding this comment.
I've created a basic reproduction of what we're doing internally: http://www.webpackbin.com/EyVC4-Agz.
I think we should just continue being defensive, but this does raise some questions. In that bin:
- Chrome CE v1 text content is 1
- Safari CE v1 text content is 1772 (or whatever, call stack exceeded)
- In polyfill text content is 2
Which scenario is correct? I assume the polyfill to be incorrect against the native behaviour, but is WebKit or Blink correct here?
There was a problem hiding this comment.
This came up today in a thread with the WebKit team. It seems like WebKit is correct and Chrome and the poyfill will change to match it - attributeChangedCallback should be called even when the new value is the same as the old value. The polyfill also has to take extra care not to double notify on one change because of sync vs async mutation detection.
| @@ -89,7 +89,7 @@ const attributesContext = propContext(attributes, { | |||
|
|
|||
| // Default attribute applicator. | |||
| [symbols.default] (elem, name, value) { | |||
| const { props, prototype } = customElements.get(elem.tagName) || { | |||
| const { props, prototype } = customElements.get(elem.localName) || { | |||
There was a problem hiding this comment.
Fun fact, seems localName returns whatever case the element was created with.
There was a problem hiding this comment.
should we be normalising it here then? Or is customElements.get not case sensitive?
There was a problem hiding this comment.
It's case-sensitive, but localName should return the exact name of the element how it was specified in HTML, as I understand. More info here WICG/webcomponents#604.
| "skatejs-web-components": "3.x" | ||
| "@webcomponents/custom-elements": "https://github.com/webcomponents/custom-elements/archive/fc022e8.tar.gz", | ||
| "@webcomponents/shadycss": "https://github.com/webcomponents/shadycss/archive/19a46c9.tar.gz", | ||
| "@webcomponents/shadydom": "https://github.com/webcomponents/shadydom/archive/c162774.tar.gz" |
There was a problem hiding this comment.
would we keep skatejs/web-components up to date with these?
|
I'll update the skatejs-web-components stuff later. That PR is taking a bit longer because we need o figure out a way to consume the native-shim in both native and non-native browsers. Conditional requiring is difficult. |
fixes #908