Skip to content
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

custom-elements: A step to add "is" content attribute in "new" #3402

Closed
tkent-google opened this issue Jan 25, 2018 · 22 comments
Closed

custom-elements: A step to add "is" content attribute in "new" #3402

tkent-google opened this issue Jan 25, 2018 · 22 comments
Assignees
Labels
topic: custom elements Relates to custom elements (as defined in DOM and HTML)

Comments

@tkent-google
Copy link
Contributor

4.13.1.2 Creating a customized built-in element says:

const plasticButton2 = new PlasticButton();
console.log(plasticButton2.localName);          // will output "button"
console.log(plasticButton2.getAttribute("is")); // will output "plastic-button"

However, I couldn't find a step to add is attribute in new case though I found it in document.createElement() case

@annevk annevk added the topic: custom elements Relates to custom elements (as defined in DOM and HTML) label Jan 25, 2018
@annevk
Copy link
Member

annevk commented Jan 25, 2018

Hmm yeah, in particular https://html.spec.whatwg.org/multipage/dom.html#htmlconstructor doesn't mention the is attribute. I thought that all element allocation went through https://dom.spec.whatwg.org/#concept-create-element but I guess that's not the case?

@domenic
Copy link
Member

domenic commented Jan 31, 2018

@annevk unfortunately directly calling the constructor does not go through that. Rather, concept-element-create calls the constructor, at least for custom elements.

@tkent-google
Copy link
Contributor Author

tkent-google commented Feb 1, 2018

Because clone algorithm refers to is attribute value, cloneNode() for customized built-in elements created by constructors produce non-customized elements according to the current specifications.

@annevk
Copy link
Member

annevk commented Feb 1, 2018

@domenic how does that work then if you have your own constructor for a builtin?

@domenic
Copy link
Member

domenic commented Feb 1, 2018

@annevk I'm not sure what your question is. How does the existing spec work? It works pretty well; just follow the algorithms. (In particular the super-call to [HTMLConstructor] is what creates the element.) But, it fails to add the is="" attribute, as @tkent-google noted.

I started on this yesterday, but got a bit stuck as there's a clash with the requirement in create-an-element which disallows new attributes being added by the constructor. I'll try again today.

@domenic
Copy link
Member

domenic commented Feb 1, 2018

Cloning using the is attribute instead of the is value also seems like a bug.

@domenic
Copy link
Member

domenic commented Feb 1, 2018

I've wrote up most of a patch for this, but it's getting fairly complicated. I'd like implementer opinions from @tkent-google and anyone else working customized built-ins; I've previously seen @cbrewster and @bzbarsky from Mozilla pop in.

Here are some important background constraints. Assume we have a setup like customElements.define("plastic-button", class PlasticButton extends HTMLButtonElement { ... }, { extends: "button" }).

  • When doing new PlasticButton(), our only opportunity to run implementation code is during the super() call to HTMLButtonElement's constructor, i.e. during the [HTMLConstructor] algorithm.
  • Everything but new PlasticButton() goes through a central "create an element" algorithm. (For example, the parser or createElement().) In this algorithm, if the custom element is registered, we eventually call into the custom element custom constructor, and thus [HTMLConstructor].

My in-progress patch ends up looking like the following:

  • [HTMLConstructor], when it notices it's dealing with a registered customized built-in element, adds the is="" attribute (and sets the "is value").
  • "create an element", when passed an is argument (e.g. via document.createElement(..., { is }) or the parser finding an is="" attribute in the token stream) need to add a check: did constructing the element cause the is="" attribute to be added? If so, great; the [HTMLConstructor] must have done that for us, so don't add it. If not, add it during this algorithm, and set the "is value" as well; this allows the element to get upgraded later if an appropriate definition is registered.
  • The parser omits is="" attributes from the list that it appends to the element due to parsing. By the time the element is created, via "create an element", the is="" attribute already exists, per the previous bullet. (This has impact on the ordering in the NamedNodeMap, making is="" always first, even if in source order it's not.)
  • Cloning looks at "is value", not is="" attribute; this is just a bugfix.

A simpler alternative would be the follows:

  • If you use new PlasticButton(), you're opting out of the is="" attribute reflecting the correct state. The internal "is value" is still correct. But in this world the is="" attribute is more of a tool for markup to trigger the correct element creation.
  • I guess in this world it would be more consistent to not set the attribute from createElement() either? So only the parser cares about is="", and only about reading it.
  • In this version, the spec mostly stays as it is today, except:
    • We probably remove setting the is="" attribute from createElement()
    • We fix cloning to look at "is value", not is="" attribute.
    • We fix the example quoted in the OP.

Basically the question is whether we want try hard to ensure that customized built-in elements, after they are created, have an is="" attribute that matches their is value. I think it might be OK to say "no"; we're already fairly clear that is="" is not a dynamic thing, e.g. modifying it does not have any effect on the element. (Except for, due to a current spec bug, what happens when you clone it.)

What do folks think?

@bzbarsky
Copy link
Contributor

bzbarsky commented Feb 1, 2018

My gut instinct is that changing parser behavior is pretty fishy....

The current custom element situation as Mozilla is ... temporarily in flux. @smag---- do you know who's on top of them right now?

@bzbarsky
Copy link
Contributor

bzbarsky commented Feb 1, 2018

Er, @smaug---- see above.

@annevk
Copy link
Member

annevk commented Feb 2, 2018

I like the simpler alternative much better. We should perhaps complement it by adding readonly attribute DOMString internalIs;, although writing it out I realize that's a terrible name due to the l and uppercase I.

@tkent-google
Copy link
Contributor Author

Should we support serialization&parsing round-trip? e.g.

element.appendChild(new PlasticButton());
element.innerHTML = element.innerHTML;
element.lastChild instanceof PlasticButton; => should be true?

@annevk
Copy link
Member

annevk commented Feb 2, 2018

I think we should. With the simpler alternative that would mean special casing the serializer. That still seems better than further special casing the parser and might be reasonable anyway to override later-set is attributes on such elements?

@smaug----
Copy link

Definitely the simpler thing. Having inconsistent attribute handling in parser feels rather bad.
That does mean inconsistency in serializer, but I guess I'd rather take that.
Something like, when serializing, and there isn't is attribute set explicitly, but the element is customized built-in, inject is attribute.

@domenic
Copy link
Member

domenic commented Feb 2, 2018

Fascinating, I hadn't anticipated people would want to change the serializer. OK, great, I'll work on this path.

@annevk I thought about adding an is getter, but I couldn't think of a use case, so I'll leave it out for now?

@annevk
Copy link
Member

annevk commented Feb 2, 2018

@domenic fine with me.

domenic added a commit to whatwg/dom that referenced this issue Feb 2, 2018
Part of fixing whatwg/html#3402. This changes two things:

* It fixes the cloning steps to consult the internal is value, instead of the is="" content attribute.
* It removes the step from createElement()/createElementNS() that would insert an is="" attribute in the created element, as we've now decided that is="" is a way of communicating to the parser, and not a general-purpose indicator of customized built-in-ness.
domenic added a commit that referenced this issue Feb 2, 2018
This follows the DOM changes in whatwg/dom#566,
and is part of fixing #3402.

The normative changes are to the serialization algorithm, which now
writes out the element's is value as its is="" attribute, if no actual
is="" attribute is present. The rest of the changes are to introductory
text about customized built-in elements.
@domenic
Copy link
Member

domenic commented Feb 2, 2018

I've done two PRs that people could take a look at:

@annevk
Copy link
Member

annevk commented Feb 3, 2018

Copying @whatwg/components to ensure nobody gets surprised by this change.

domenic added a commit to whatwg/dom that referenced this issue Feb 14, 2018
Part of fixing whatwg/html#3402. This changes two things:

* It fixes the cloning steps to consult the internal is value, instead of the is="" content attribute.
* It removes the step from createElement()/createElementNS() that would insert an is="" attribute in the created element, as we've now decided that is="" is a way of communicating to the parser, and not a general-purpose indicator of customized built-in-ness.

See also whatwg/html#3444 for the other half of the fix.

Tests: web-platform-tests/wpt#9505
domenic added a commit that referenced this issue Feb 14, 2018
This follows the DOM changes in whatwg/dom#566,
and is part of fixing #3402.

The normative changes are to the serialization algorithm, which now
writes out the element's is value as its is="" attribute, if no actual
is="" attribute is present. The rest of the changes are to introductory
text about customized built-in elements.

Tests: web-platform-tests/wpt#9508
@annevk
Copy link
Member

annevk commented Feb 16, 2018

Is the only remaining thing here filing a bug against Firefox and Safari? (I'm assuming @tkent-google took care of Chrome.)

@dstorey does Edge need a bug for this or are you earlier in your implementation stage?

@dstorey
Copy link
Member

dstorey commented Feb 16, 2018

We're not at that point yet, so as long as there are tests covering this it should be fine without a bug.

@tkent-google
Copy link
Contributor Author

Yes, I take care of Google Chrome.

@annevk
Copy link
Member

annevk commented Feb 22, 2018

@domenic could you please link the Firefox and Safari bugs?

@annevk annevk reopened this Feb 22, 2018
@domenic
Copy link
Member

domenic commented Feb 22, 2018

Safari does not implement customized built-ins. Gecko's bug is https://bugzilla.mozilla.org/show_bug.cgi?id=1440382.

@domenic domenic closed this as completed Feb 22, 2018
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
This follows the DOM changes in whatwg/dom#566,
and is part of fixing whatwg#3402.

The normative changes are to the serialization algorithm, which now
writes out the element's is value as its is="" attribute, if no actual
is="" attribute is present. The rest of the changes are to introductory
text about customized built-in elements.

Tests: web-platform-tests/wpt#9508
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: custom elements Relates to custom elements (as defined in DOM and HTML)
Development

No branches or pull requests

6 participants