Implementing customized built-in elements #42

joeldenning · 2017-01-09T22:21:25Z

This fixes #13. This implementation is hopefully very close to the spec for customized built-in elements.
Note that I based my branch off of #36 because the tests are failing on the master branch and I didn't want to duplicate the work that @bicknellr has already done in fixing the tests. This means that #36 should probably be merged in before this pull request.

… match native, rather than being specifically true. They should be true per spec but it seems like Safari sets writable to false.

* fix-tests: Remove unnecessary `HTMLElement#constructor` descriptor test. Test for 'HTMLElement#constructor' writable / configurable flags that match native, rather than being specifically true. Add promise polyfill for tests. Add bower dependency and installation. Update WCT to 6.0.0-prerelease.3 for ES2015 compilation. Remove use of `String#endsWith`. Update HTMLImports polyfill path. Update WCT and webcomponentsjs. Use one-liner for forcing the polyfill. Remove test assumption that `customElements` exists. `raw` -> `_native` `_orig` -> `_native` Use `HTMLNS`. `doc` -> `document`; `win` -> `window` Fix instanceof for built-in elements. 📝 Fix gramatical error in comment

joeldenning · 2017-01-09T22:26:05Z

bower.json

-    "webcomponentsjs": "webcomponents/webcomponentsjs#^0.7.22"
+    "web-component-tester": "6.0.0-prerelease.3",
+    "webcomponentsjs": "webcomponents/webcomponentsjs#v1",
+    "es6-promise": "stefanpenner/es6-promise#^4.0.0"


These changes are from #36

joeldenning · 2017-01-09T22:26:15Z

package.json

    "google-closure-compiler": "^20160822.1.0",
    "gulp": "^3.8.8",
    "gulp-sourcemaps": "^1.6.0",
-    "web-component-tester": "^4.0.1"
+    "web-component-tester": "6.0.0-prerelease.3"


All changes to package.json are from #36

joeldenning · 2017-01-09T22:27:04Z

src/custom-elements.js

@@ -138,7 +138,7 @@ let Deferred;
      /** @private {!Map<string, !CustomElementDefinition>} **/
      this._definitions = new Map();

-      /** @private {!Map<Function, string>} **/
+      /** @private {!Map<Function, !CustomElementDefinition>} **/


I have changed this._constructors to so that it has the entire CustomElementDefinition, since we need more than the tagName now when looking up custom elements by their constructor

joeldenning · 2017-01-09T22:27:29Z

src/custom-elements.js

      const nameError = checkValidCustomElementName(name);
      if (nameError) throw nameError;

-      // 4, 5:
+      // 3:


The numbering has changed in the spec for customElements.define, so I updated it.

joeldenning · 2017-01-09T22:28:02Z

src/custom-elements.js

+      let _extends = options ? options.extends : null;
+
+      // 7:
+      if (_extends) {


This is stuff specific to customizedBuiltInElements

joeldenning · 2017-01-09T22:36:32Z

src/custom-elements.js

+    if (options && options.is) {
+        // We're going to take care of setting the is attribute ourselves
+        isAttr = options.is;
+        delete options.is;


Delete options.is So that the native browser implementation doesn't freak out

joeldenning · 2017-01-09T22:36:51Z

src/custom-elements.js

-    const definition = customElements._definitions.get(tagName.toLowerCase());
+    if (isAttr) {
+        element.setAttribute('is', isAttr);
+        element.is = isAttr;


Set the is attribute and property on the newly created node

I don' think this property exists.

Good catch -- I think I confused the "is value" with a property on the dom element. But it seems like the "is value" is just an internal thing not exposed to userland.

joeldenning · 2017-01-09T22:37:01Z

src/native-shim.js

@@ -61,6 +61,9 @@
 (() => {
  'use strict';

+  // Do nothing if `customElements` does not exist.
+  if (!window.customElements) return;


This is from #36

joeldenning · 2017-01-09T22:41:26Z

tests/js/reactions.js

@@ -181,6 +246,72 @@ suite('Custom Element Reactions', function() {

  });

+  suite('customized built-in elements', function() {
+    test('inherits native behavior such as a button\'s disabled property', function(done) {


Tests that you can actually get the native behavior for specific elements (like disabled for buttons) to work just like it's native.

joeldenning · 2017-01-09T22:42:39Z

tests/js/reactions.js

+      });
+    });
+
+    test('does not upgrade elements that don\'t match what extends was at definition', function() {


This is something I am actually not 100% sure on in the spec. It seems like you should not be able to <div is="x-foo" /> if you say customElements.define('x-foo', XFoo, {extends: 'span'}). But I was not able to find a definitive answer in the spec. Am happy to change if I'm wrong on this.

This appears to be correct. Spec breadcrumbs: https://html.spec.whatwg.org/#concept-try-upgrade will give a null definition in step 1 since https://html.spec.whatwg.org/#look-up-a-custom-element-definition will return null if name + local name do not match (in its step 5).

blittle · 2017-01-09T23:27:51Z

Do the tests need to be retried?

joeldenning · 2017-01-10T17:15:10Z

Yeah the tests pass locally for me but timed out in travis. Retrying would probably get them to run.

joeldenning · 2017-01-19T20:13:16Z

Ping

blittle · 2017-03-02T21:20:00Z

I'd love to hear an update about this!

joeldenning · 2017-03-02T21:20:24Z

Ping.

Are there concerns around this pr that are preventing it from being reviewed? Maybe that webcomponents/custom-elements is hesitant about implementing customized built-in elements? Or is it just that since it's a big pull request it is hard to review?

bicknellr · 2017-03-07T21:51:59Z

Sorry, @joeldenning, I owe you an explanation here. I recently ended up overhauling the polyfill so that we could remove most of our dependence on MutationObservers for the sake of performance and timing correctness. Unfortunately, this makes this PR effectively unmergable as there's no common code anymore. I should have paid more attention to what was already happening in the repo and let you know about these changes earlier.

I didn't end up implementing extension for built-ins during that process because, AFAIK, WebKit does not intend to implement it. I'd rather not have people end up stuck on the polyfill because they become dependent on this feature. Also, we won't have to worry about supporting a feature that's already been baked in if it does end up being dropped from the spec.

However, if it does make it through and WebKit decides to implement, then we'll definitely want it here. If you're still interested in implementing this on top of the new polyfill, I'd be glad to help explain how the new polyfill works and point you to the relevant spots. (I'm hoping that the recent updates made the polyfill a bit easier to navigate.) I just can't guarantee it's going to make it in at the moment - you'll have to keep an eye on the spec discussion for that.

joeldenning · 2017-03-07T22:27:33Z

@bicknellr thanks for the explanation. Do you know if/where there is more discussion about Webkit's disagreement with customized built in elements (beyond what is described in this issue in the CE spec)? I have been following it as best as I can, and as far as I can tell what you said is correct about webkit not planning to implement customized built ins.

About whether this polyfill should implement customized built-ins, though, would you be opposed to including support for customized built-ins if it came with a disclaimer in the documentation? Explaining the current status of the spec (v1 finalized) but that Webkit doesn't plan on implementing a specific part of the spec (customized built-in). Or perhaps as a separate script that is optionally included to support customized built ins?

I think that there is still value in fully implementing the v1 spec, as long as users know what they are getting themselves into. I also think that this polyfill is a great place to educate people on the spec and its browser support, since many developers will come here to learn about custom elements without ever having read the spec or the discussions surrounding it. Once people know that there is a risk that they will be stuck on the polyfill forever if they choose to use customized builtins, I feel that we have given them enough information to make that decision.

All that said, I understand that I'm merely an interested individual, who doesn't represent the vast swaths of Polymer users or the large corporations interested in the CE spec 😃 . Interested to hear others' thoughts on whether a polyfill should support a feature that is in the spec but may never be supported by all browsers.

bedeoverend · 2017-03-07T23:24:09Z

@bicknellr If I understand correctly, Chrome, Firefox (written into gecko source), and Edge (expressed intent in last TPAC meeting - potentially could now have changed) all intend to implement built-ins via is="". If it is the case that the majority of vendors intend to implement, will the polyfill implement also?

I understand wanting to hold off until at least one implementation exists, but if Safari is the only one planning on not implementing, and that was the reason the polyfill held off, the polyfill's functionality would differ from most browsers.

madeleineostoja · 2017-03-07T23:27:10Z

I strongly agree with @joeldenning here - extending built-ins is firmly in the v1 spec, and being actively shipped by multiple vendors. We'd be making a very big assumption that users don't want this polyfilled, because they'd be carrying the polyfill on Webkit, and making that decision for them.

This polyfill should follow the spec, regardless of vendors, and include a disclaimer in the docs that using a certain feature of the spec (extending built-ins) will have performance ramifications on Webkit. Then users are free to make up their own mind.

In practical terms - there are a lot of use-cases that just aren't possible without extending builtins, and having native perf in most vendors with a shim in webkit, vs. hacking something together with mutation observers for all vendors to meet these use-cases, is a compromise that I think a lot of devs would be happy making.

But, at the end of the day, it should be their choice. A polyfill's job should just be to follow the spec as best as it can and document any edge-cases / caveats. Not to mention that Webkit is the odd one out here, and there is vocal opposition to their position from a lot of people. A polyfill shouldn't take sides or dictate usage.

dominiccooney

Drive by comments. I'm not super familiar with this polyfill so take these with appropriate skepticism.

dominiccooney · 2017-03-09T00:15:42Z

src/custom-elements.js

@@ -451,6 +459,9 @@ let Deferred;
        const walker = createTreeWalker(root);
        do {
          const node = /** @type {!HTMLElement} */ (walker.currentNode);


FWIW I think this annotation is incorrect; you may have non-HTMLElements in a document, usually SVG.

dominiccooney · 2017-03-09T00:17:48Z

src/custom-elements.js

@@ -451,6 +459,9 @@ let Deferred;
        const walker = createTreeWalker(root);
        do {
          const node = /** @type {!HTMLElement} */ (walker.currentNode);
+          if (node.getAttribute('is')) {


I'm not super familiar with this polyfill, but if it hooks element creation it's more accurate to cache 'is' at creation time. The HTML spec calls this the element's "is value" IIRC.

I guess you probably need to look at attributes for parsed elements.

You may want to keep those values in a side WeakMap of Element -> string. That will avoid the closure type problem you mention below.

I agree that not looking at the is attribute would be ideal, but like you pointed out I don't think it's possible to avoid it for parsed elements. Since (per the spec) updating the is attribute after creation should not cause an element to be upgraded to a custom element, what I did here was only look at the is attribute when the MutationObserver says that a node was added, and never look at the is attribute after that.

Since this polyfill has been rewritten to rely on monkeypatched methods instead of mutation observers, I'll have to figure out how to get the same functionality without the MutationObserver telling me if it is a newly added node or not.

dominiccooney · 2017-03-09T00:22:33Z

src/custom-elements.js

-    const definition = customElements._definitions.get(tagName.toLowerCase());
+    if (isAttr) {
+        element.setAttribute('is', isAttr);
+        element.is = isAttr;


I don' think this property exists.

dominiccooney · 2017-03-09T00:29:05Z

src/custom-elements.js

@@ -562,7 +573,7 @@ let Deferred;
          const node = walker.currentNode;
          if (node[_upgradedProp] && node[_attachedProp]) {
            node[_attachedProp] = false;
-            const definition = this._definitions.get(node.localName);
+            const definition = getDefinition(this._definitions, node);


FWIW the native implementation in Chrome does this differently: once a definition is associated with an element (at creation or upgrade) we store a pointer directly from the element to the definition.

Given that getDefinition is more complicated now, it might be worth revisiting the space/speed tradeoff.

I note that you're storing "is" on the element. If you wanted to save space you could switch the storage space for "is" to point to the full definition once you have it. If you ever happened to need is (I don't think you do; cloning goes back to the attribute value for example) you could retrieve it from the definition.

👍 I like that idea. Putting it on the element instead of the looking it up would improve performance. And getting rid of the is property will offset the new storage space for the definition.

And regarding the is property, just reviewed the spec and looks like I got confused between the "is value" and an actual userland property on the dom element. So you're right, it should be removed from this polyfill

joeldenning · 2017-03-09T01:48:18Z

@dominiccooney thanks for the feedback, will definitely incorporate it!

@bicknellr do you have thoughts on the discussion above about whether to implement customized built-ins for this polyfill? I am more than happy to redo my implementation to work with the monkeypatching approach, if you feel comfortable with that. As discussed above, I think that if we educate users of the polyfill about the potentially shaky status of customized builtins, we've given them the power to choose whether to use customized built-ins or not. I also agree with @bedeoverend that it would be unexpected if the polyfill does not have feature parity with the native implementation of the majority of browsers (Chrome, Firefox, Edge). What are your thoughts?

bicknellr · 2017-03-09T18:57:36Z

@joeldenning Yeah, having a polyfill that doesn't fully implement the spec feels wrong. More so if that spec is ends up implemented by multiple browsers. @dominiccooney mentioned to me that Chrome's built-in extension is behind a flag (maybe that's just because it's not totally there yet?). I think following suit by also putting the polyfill's implementation behind a flag would make sense. That way it's available but still requires opt-in and we can point out the continued debate about the feature where the flag is documented. I think that strikes a reasonable balance between being spec complete and being cautious about allowing users to write code that could potentially be non-portable in the future. WDYT?

joeldenning · 2017-03-09T19:45:03Z

👍 I like that. Maybe something like customElements.enableCustomizedBuiltIns = true?

I'll start work on this again (based on the new monkeypatched way) in a new pr.

avinoamr · 2017-03-26T04:53:25Z

@joeldenning How is it looking? Are you still actively working on it? Any estimation on when you'll have something ready?

joeldenning · 2017-03-26T04:57:32Z

Yeah, I have been working on it. I have it partially working, and am just adding tests and fixing bugs. I hope to have it finished and create a pr this upcoming week

avinoamr · 2017-03-26T09:13:05Z

@joeldenning thanks. can't wait :)

bicknellr and others added 18 commits December 16, 2016 15:55

Remove test assumption that customElements exists.

a7073c4

Use one-liner for forcing the polyfill.

ee326ff

Update WCT and webcomponentsjs.

f0eb664

Update HTMLImports polyfill path.

8ffa12b

Remove use of String#endsWith.

555e61e

Update WCT to 6.0.0-prerelease.3 for ES2015 compilation.

e089691

Add bower dependency and installation.

0265c2d

Add promise polyfill for tests.

775b224

Test for 'HTMLElement#constructor' writable / configurable flags that…

e6eddb5

… match native, rather than being specifically true. They should be true per spec but it seems like Safari sets writable to false.

Remove unnecessary HTMLElement#constructor descriptor test.

0c82e24

It's sort of working

4485b37

subclassing things

851fd1b

Tests passing

93d812a

Google closure type fixing

5e15bb8

Removing built files

270f834

Self review 1

4259dda

Self review 2

2a6a70d

joeldenning changed the title ~~Customized builtin~~ Implementing customized built-in elements Jan 9, 2017

treshugart mentioned this pull request Jan 9, 2017

Support for attributes, type-extension CEs, and those loaded by HTML import webcomponents/react-integration#60

Closed

Fixing bug where autonomous elements had is="" attribute

5dce865

joeldenning commented Jan 9, 2017

View reviewed changes

joeldenning added 3 commits January 10, 2017 10:46

Removing custom-elements.min.js changes

2feec3e

Domenic's feedback - use node.localName instead of tagName

c8c6627

Fixing closure compiler warnings

7233ea6

madeleineostoja mentioned this pull request Feb 25, 2017

Type-extension polyfill for Polymer 2.0 Polymer/polymer#3925

Closed

dominiccooney reviewed Mar 9, 2017

View reviewed changes

joeldenning closed this Mar 9, 2017

web-padawan mentioned this pull request Mar 15, 2017

Built-in element extension in V2. Polymer/polymer#4433

Closed

joeldenning mentioned this pull request Apr 1, 2017

Implementing customized builtin elements. #88

Closed

Implementing customized built-in elements #42

Implementing customized built-in elements #42

Conversation

joeldenning commented Jan 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blittle commented Jan 9, 2017

joeldenning commented Jan 10, 2017

joeldenning commented Jan 19, 2017

blittle commented Mar 2, 2017

joeldenning commented Mar 2, 2017

bicknellr commented Mar 7, 2017

joeldenning commented Mar 7, 2017 • edited Loading

bedeoverend commented Mar 7, 2017 • edited Loading

madeleineostoja commented Mar 7, 2017

dominiccooney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeldenning commented Mar 9, 2017

bicknellr commented Mar 9, 2017 • edited Loading

joeldenning commented Mar 9, 2017

avinoamr commented Mar 26, 2017

joeldenning commented Mar 26, 2017

avinoamr commented Mar 26, 2017

joeldenning commented Jan 9, 2017 •

edited

Loading

joeldenning commented Mar 7, 2017 •

edited

Loading

bedeoverend commented Mar 7, 2017 •

edited

Loading

bicknellr commented Mar 9, 2017 •

edited

Loading