-
Notifications
You must be signed in to change notification settings - Fork 94
Implementing customized builtin elements. #88
Conversation
@@ -4,7 +4,7 @@ import CEState from './CustomElementState.js'; | |||
export default class CustomElementInternals { | |||
constructor() { | |||
/** @type {!Map<string, !CustomElementDefinition>} */ | |||
this._localNameToDefinition = new Map(); | |||
this._nameToDefinition = new Map(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For autonomous custom elements, the custom element name is the same as localName
. But for customized builtin elements, the localName
is different than the name of the custom element. I have changed this Map so that it looks thing up by name
instead of localName
, so that it works with both autonomous and customized builtin elements.
src/CustomElementInternals.js
Outdated
* @param {!Node} node | ||
* @return {!CustomElementDefinition|null} | ||
*/ | ||
nodeToDefinition(node) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting the CE definition for a DOM element used to be as straightforward as looking things up by localName
. But now it's a bit more complicated since you have to deal with customized builtins where the localName is not the same as the name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we tweak the naming here to indicate that the definition returned isn't necessarily the definition that has been applied to that element (__CE_definition
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can do. What do you rthink of nodeToCustomElementDefinition
? Since the return type is of CustomElementDefinition
type
src/CustomElementInternals.js
Outdated
* @return {!CustomElementDefinition|null} | ||
*/ | ||
nodeToDefinition(node) { | ||
const name = typeof node.__CE_is === 'string' ? node.__CE_is : node.localName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__CE_is
is a new variable I have created on nodes that are customized builtin elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: const name = node.__CE_is || node.localName;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 will change.
When I wrote this, I was thinking somehow that __CE_is would not always be a string and that we wanted to ignore it in that case. But we should instead just ensure that __CE_is
is always a string.
src/CustomElementInternals.js
Outdated
nodeToDefinition(node) { | ||
const name = typeof node.__CE_is === 'string' ? node.__CE_is : node.localName; | ||
const definition = this.nameToDefinition(name); | ||
if (definition && definition.localName === node.localName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is possible that someone tries to apply a customized builtin element (via is="my-builtin"
) to a dom element that the customized builtin was never meant to extend. In that case, we should not upgrade the element to be the customized builtin.
This is why there is the definition.localName === node.localName
check.
src/CustomElementInternals.js
Outdated
@@ -208,6 +222,11 @@ export default class CustomElementInternals { | |||
this.patchAndUpgradeTree(importNode, visitedImports); | |||
}); | |||
} | |||
} else if (element.hasAttribute('is') || element.is) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The is=""
attribute should only be looked at when an element is being created, but never again. This function is run when innerHTML
occurs, so it is when elements are newly created. So this is the only place we respect what is in the is
attribute.
src/CustomElementRegistry.js
Outdated
let localName = name; | ||
|
||
if (options && options.extends) { | ||
if (!this.enableCustomizedBuiltins) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what puts customized builtins behind a flag.
|
||
let localName = name; | ||
|
||
if (options && options.extends) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See 6.
and 7.
in the spec for define
/** | ||
* @param {!CustomElementInternals} internals | ||
*/ | ||
export default function(internals) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Browsers currently throw errors if you try to subclass native DOM elements. So we patch each of the DOM element constructors so that they don't throw errors.
@@ -28,4 +28,71 @@ export default { | |||
HTMLElement: window.HTMLElement, | |||
HTMLElement_innerHTML: Object.getOwnPropertyDescriptor(window.HTMLElement.prototype, 'innerHTML'), | |||
HTMLElement_insertAdjacentElement: window.HTMLElement.prototype['insertAdjacentElement'], | |||
HTMLElement_subclasses: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is used when patching all the native elements.
src/custom-elements.js
Outdated
@@ -35,6 +37,7 @@ if (!priorCustomElements || | |||
|
|||
/** @type {!CustomElementRegistry} */ | |||
const customElements = new CustomElementRegistry(internals); | |||
customElements.enableCustomizedBuiltins = priorCustomElements.enableCustomizedBuiltins; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If somebody set enableCustomizedBuiltins
before the polyfill executes, we need to respect that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated this to work with Closure's ADVANCED
mode. It was mangling enableCustomizedBuiltins
, but I fixed that.
This is why the tests were failing in the build. Shouldn't happen anymore
src/CustomElementInternals.js
Outdated
element.__CE_is = element.getAttribute('is') || element.is; | ||
element.removeAttribute('is'); | ||
delete element.is; | ||
elements.push(element); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could limit the calls to hasAttribute, getAttribute, removeAttribute
to have better perf. Also, avoid delete element.is
as it's slow and all we need here is to unset it.
e.g.
} else {
// if getAttribute returns an empty string consider it null.
const is = element.is || element.getAttribute('is') || null;
if (is) {
element.__CE_is = is;
element.is = null;
element.removeAttribute('is');
}
elements.push(element);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the is
attribute reflect to .is
even if the element hasn't been upgraded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@valdrinkoshi I realized that element.is
is not actually a real property according to the spec. So I can avoid delete element.is
and looking up element.is
altogether.
Also I have removed the call to removeAttribute
as suggested by @bicknellr in his comment above.
Finally I decided to combine the hasAttribute
and getAttribute
calls into just one getAttribute
(hopefully maybe trading a little bit of speed for the memory required to store the temp variable)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I left a handful of comments, PTAL.
I think __CE_is
might be unnecessary since __CE_definition
will contain name
and localName
in it. Although, maybe there are other places where we need to know the element's is
value that I'm not thinking about?
src/CustomElementInternals.js
Outdated
@@ -208,6 +222,11 @@ export default class CustomElementInternals { | |||
this.patchAndUpgradeTree(importNode, visitedImports); | |||
}); | |||
} | |||
} else if (element.hasAttribute('is') || element.is) { | |||
element.__CE_is = element.getAttribute('is') || element.is; | |||
element.removeAttribute('is'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should remove it: if we're not actually telling the browser anything about any custom element definitions, then the browser shouldn't do anything special with is
, right? Also, I think people will expect to be able to style their custom elements with rules containing attribute selectors for is
([is=my-element]
) since it's the closest equivalent to styling with a raw tag name for autonomous custom elements.
Have you found a situation where setting the is
attribute of an element (without registering the value of that attribute as an extension) causes distinct behavior?
src/CustomElementInternals.js
Outdated
element.__CE_is = element.getAttribute('is') || element.is; | ||
element.removeAttribute('is'); | ||
delete element.is; | ||
elements.push(element); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the is
attribute reflect to .is
even if the element hasn't been upgraded?
@@ -47,7 +47,7 @@ export default class CustomElementRegistry { | |||
* @private | |||
* @type {!Array<string>} | |||
*/ | |||
this._unflushedLocalNames = []; | |||
this._unflushedNames = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
src/CustomElementInternals.js
Outdated
* @param {!Node} node | ||
* @return {!CustomElementDefinition|null} | ||
*/ | ||
nodeToDefinition(node) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we tweak the naming here to indicate that the definition returned isn't necessarily the definition that has been applied to that element (__CE_definition
)?
src/CustomElementRegistry.js
Outdated
const el = document.createElement(options.extends); | ||
if (el instanceof window['HTMLUnknownElement']) { | ||
throw new Error(`Cannot extend '${options.extends}': is not a read HTML element`); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this method of checking if a custom element exists is a bit strange, particularly if there is already a defined custom element with that name, which this check might produce an instance of.
'is not a read HTML element': Was read
meant to be real
? If so, could be reworded to be more clear about what a 'real' HTML element is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 yeah that was a typo. Also I agree the wording could probably have been better so I have updated it.
The purpose of this check is to follow 7.2
in https://html.spec.whatwg.org/multipage/scripting.html#custom-elements-api. As far as I know, there is no way of checking the element interface of a localName without actually creating an element. If anyone knows of another way to check if the element interface of localName
isHTMLUnknownElement, would love to know. But this was the only way I knew how to check.
src/Patch/HTMLElementSubclasses.js
Outdated
|
||
function patchElement(elementName, NativeElement) { | ||
if (!NativeElement) { | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I wish this could throw but I'm guessing it's not just in case the element isn't implemented in the browser we're running in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this happens when the browser doesn't implement a particular subclass of HTMLElement. The good news is that when that is the case, nobody will be trying to subclass that class anyway, so we don't need to patch it.
src/CustomElementInternals.js
Outdated
@@ -208,6 +222,11 @@ export default class CustomElementInternals { | |||
this.patchAndUpgradeTree(importNode, visitedImports); | |||
}); | |||
} | |||
} else if (element.hasAttribute('is') || element.is) { | |||
element.__CE_is = element.getAttribute('is') || element.is; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add __CE_is
to the externs added to Element
(here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 updated
@@ -110,6 +129,7 @@ export default class CustomElementRegistry { | |||
} | |||
|
|||
const definition = { | |||
name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add name: string
to CustomElementDefinition
(here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 updated
src/Patch/HTMLElementSubclasses.js
Outdated
// in ES5. Assuming the user keeps the default value of the constructor's | ||
// prototype's `constructor` property, this is equivalent. | ||
/** @type {!Function} */ | ||
const constructor = this.constructor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to move the annotation so that it looks like a cast (/** @type {!Function} */ (this.constructor)
) because closure is complaining that this.constructor
might be null
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
src/Patch/HTMLElementSubclasses.js
Outdated
element.__CE_state = CEState.custom; | ||
element.__CE_definition = definition; | ||
internals.patch(element); | ||
return element; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Closure is complaining about the type of element
; any ideas? Admittedly, there are 5 closure warnings in master but I'd like to keep that number as low as possible. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed the warning by changing it to /** @type {!HTMLElement} */ (element)
, similar to line 61.
The warning was that Native.Document_createElement
returns a Node{....}
instead of an HTMLElement
like is expected.
@seaneking I've updated the pr yesterday to address all the comments, look forward to seeing what other thoughts @bicknellr has |
@bicknellr Can we merge this in? Is there anything else missing here? |
Sorry about the delay! We're still full steam ahead on hybrid porting stuff over in Polymer element land so I won't be able to give this a thorough review for a little bit. However, we're really close and I definitely want to see this landed so, once that's all settled, I'll be able to give this PR more attention. |
ping |
Pong? Is there anything we (the community) can do to help this get merged? It's holding up a lot of us from migrating existing type-extended elements to WC v1 / Polymer 2. |
I am a little bit worried about this PR. My understanding is that is= isn't implemented on any browser right now, not even Chrome, which means you would force the polyfill in Chrome for your entire app, which pre-this PR it wouldn't be necessary. Secondly, Apple has been pretty clear they don't want to implement is=, which again means you would always have to force the polyfill. Forcing the polyfill makes me sad, because it's slower and more expensive than the native platforms :( In Polymer 2 we've just worked with the assumption that is= is not available, and that you have to wrap all your native elements in a custom element for the same effect. /cc @sorvell |
I guess the people who are waiting on this don't mind working with polyfills. Is Mozilla planning on implementing html imports? That will have to be polyfilled for the forseeable future, right? Is there any plan to remove customized builtins from the spec? For my purposes, wrapping a native element in a custom element doesn't give the same effect. I gather this holds for others, but I'll let them speak for themselves! |
AFAIK customized builtins are in-progress in Chrome? There's also mentions in FF sourcecode about customized builtins, and Edge have said that they're 'basically okay' with them. At the end of the day though Then it's up to developers to determine whether they need to use customized builtins, with the extra weight that may incur, rather than have that decision forced upon them upstream. Because there are still many use-cases that just aren't possible without extending builtins. If anything the type extension fiasco has forced some people away from web components into DSLs, which is what makes me sad. Anyway, IMO if we stopped using something every time a vendor didn't ship it then we wouldn't have WC at all - case in point: following this reasoning shouldn't HTML imports be dropped from polyfills (and Polymer) since FF have refused to implement? |
As the author of this pr, I know that there is real risk that Safari will never implement customized built-in elements, but believe that it is worth having the polyfill meet the spec. See #42 for previous discussion, also https://twitter.com/Joelbdenning/status/839241923139268608 for some of the discussion and thoughts as I was deciding about making this pr or not. In the end, I believe that as long as users of the polyfill are aware of the risks associated with using customized builtins, that they should be able to. See https://github.com/webcomponents/custom-elements/pull/88/files#diff-04c6e90faac2675aa89e2176d2eec7d8R84 for where we warn the user in the Readme. Also note that I have built this feature behind a feature flag, as discussed in #42. |
+1 I wonder if it would be more palatable if it were offered as an opt-in fill. Meaning, the consumer has to deliberately include a separate polyfill for customised built-ins (that assumes standard custom element support is provided). I think Safari currently has around 10% market share. Considering that customised built-ins are extremely useful, it might be sane to add support for it, especially since the spec has it. |
@treshugart Customized builtins already are opt-in in this pull request. You have to run The only advantage I can think of to separating it into a different script is to try to reduce the bundle size for the polyfill for those who are not using customized builtins. Looking at the bundle size, though, the polyfill is 14.1kb right now and my pull request adds only 3.5kb. That difference is negligible, in my opinion, and not worth causing the extra nuisance of users having to include two separate files. |
If its part of the spec it should be built into the bundle. I'm not sure what advantage having to activate it would have, possibly speed for non-builtin sites? How much time would be saved I wonder. More switches and files causes more confusion DX. |
Ping! What is the process for this to go from here to Polymer 2? Looking forward to trying it out. |
Any update on this? At least confirmation on consensus of whether this should be merged into mainline or not, or if the community should start investigating forking/splitting it out? This is still a blocker for a lot of people in migrating to v1 specs / Polymer 2, would be great to have a final word either way from maintainers. |
Status updateLast night we had a small discussion on twitter about what to do with this pr. It looks like I perhaps misunderstood the reasoning for splitting out the customized builtins into its own script. I thought it was to prevent people from using customized builtins accidentally without knowing the risks associated with this part of the spec. But it sounds like it also is motivated by a desire to use native custom element behavior for autonomous custom elements where possible (ie Chrome) and only use the polyfill code for customized builtins. Question for the reviewers:
Implementation options:Splitting things out into separate scripts creates some new implementation challenges. Specifically, in order for any type of custom element to be polyfilled, mutation observers must be set up and the window/DOM needs to be patched. So if we want two separate bundles (one for autonomous and one for customized builtins), a (maybe naive) implementation would create a situation where the mutation observers and window patching would happen twice if you used both scripts in your project. To me, that does not seem ideal. With that said, here are the options I see for pulling off what we want:
I lean towards 2) because feature checking native behavior before polyfilling is something that is expected from a polyfill, plus doing it that way avoids the double mutation observer situation. Which option do you prefer? Are there other options I am missing? |
cc @notwaldorf @justinfagnani. Would love to hear your response to the above comment! |
Bump. Hate to be a nag, but would be great to know final stance from maintainers on how this PR is going to be treated (re: @joeldenning's comment above), so that the developers who rely on it to migrate to v1 specs can plan their work. Is there anything we can do to help move this along? Do you need usage/use-case data of customized builtins? Perf testing? Anything? |
@seaneking I haven't been the active maintainer here for a little bit, but in my mind we would ideally have three bundles/entrypoints:
This would allow libraries that depend on is= to document that they need the is= polyfill on Safari. I would generally oppose adding is= directly to the main polyfill and only having one bundle, as I think it would encourage using is= without necessarily knowing that it required the full Custom Elements polyfill on Safari. |
@justinfagnani, that sounds like a good idea; splitting the polyfill's entry point into (1) and (2) would make the choice more explicit than a flag and would add less overhead for authors that aren't using @joeldenning, maybe we should look into how we could refactor |
@bicknellr @justinfagnani that sounds good. Creating three different bundles for those use cases will be a fair amount of work, but I think that it will help. I'll start work on it, and yeah maybe subclassing CustomElementInternals and/or CustomElementRegistry will be a good solution for this. |
Isn't this what the If |
Yes, the flag certainly communicates that but I think there are a couple of things that make splitting into two bundles more desirable: users that don't intend to use I think (3), the separate [1]: I'm trying to gather whatever information I can but I'm in the dark and the implementor actions I'm aware of seem balanced. How do you compare a strong no against two partial but recently dormant implementations? |
If (3) isn't feasible, and we still made separate bundles, then would webcomponents.js pull in the one that included |
HTML imports IS (pun intended) "being dropped" somewhat conceptually (if i'm reading this properly). Is a topic of discussion in the next TPAC meeting. As @notwaldorf states... perhaps unfortunately. Concensus is 🔑 . We've come a long way to let this fall apart. /cc @bicknellr |
Also discussing here as well @seaneking @notwaldorf @bicknellr #StayWoke WICG/webcomponents#645 |
Hopefully this is not forgotten in the Polymer Summit craziness. |
@bicknellr Do I understand correctly that you agree to merge implementation into the current code base/single bundle (behind the flag) once it will be somewhat separated internally and indicate clearly parts required only for |
@joeldenning are you working on this? I would be happy to help, to push it sooner than later :) |
I don't mean to be rude, and at the risk of getting banned: you people need to s**t or get off the pot. Customized built-in elements are part of the spec. The spec clearly recommends using them even in preference to autonomous elements. Let's get this done. |
"Getting this done" means locking some element users into the polyfill forever on Safari, and even Chrome for who knows how long. A polyfill that never goes away isn't really a polyfill. Getting off the pot, without creating a separate bundle with is= support, to me would simply mean closing this PR. |
I apologize for my absence, the last while -- I hopefully will be able to work on this soon to create a separate bundle for is= support. @tomalec if you'd like to help out, let's collaborate! You can contact me on twitter at @joelbdenning. |
Hi @joeldenning, is there any news about the customized built-in polyfill support? |
Thank you for having invested time in this. Chrome 67 ships with support for extending native elements. To leverage this feature in Chrome, we need to include this polyfill for other browsers. Any behind-the-scenes progress on this? |
This PR is very old, and now that we're moving to a monorepo, it has to be remade. We have a tracking issue for this topic at webcomponents/polyfills#102, would you mind remaking the PR in the monorepo? Thanks! |
@azakus agreed - this PR is very old. I am no longer able to put time into this, but welcome someone else taking the lead on it. |
See #42 for previous discussion about customized builtin elements.
Note that I was unable to run some of the html tests locally (I think related to this issue in wct), and because of that was also unable to add more tests to several files. Let me know what you want me to do about that. Also note that I'm not very well versed in Closure types and welcome any help with that.
Fixes https://github.com/webcomponents/custom-elements/issues/13