1419662 - Autonomous custom element get confused if it contains 'is' attribute

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Edgar Chen [:edgar]]

For example, if we have <x-foo is="x-bar"></x-foo>, we will treat this custom element as 'x-bar' type, it seems wrong to me. But I need to look carefully through the spec steps to see if this is an implementation issue. If we do follow the spec, it probably requires filing a spec bug to clarify. 

Test script: http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=5534.

Comment 1

[John Dai[:jdai]]

In Chrome and Safari, it will treat <x-foo is="x-bar"></x-foo>[1] as autonomous custom element, not customized built-in element.

[1] http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=5534

Comment 2

[Edgar Chen [:edgar]]

(In reply to John Dai[:jdai] from comment #1)
> In Chrome and Safari, it will treat <x-foo is="x-bar"></x-foo>[1] as
> autonomous custom element, not customized built-in element.
> 
> [1] http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=5534

But Chrome and Safari don't support customized built-in element yet, AFAIK.

Comment 3

[John Dai[:jdai]]

(In reply to Edgar Chen [:edgar] from comment #0)
> For example, if we have <x-foo is="x-bar"></x-foo>, we will treat this
> custom element as 'x-bar' type, it seems wrong to me. But I need to look
> carefully through the spec steps to see if this is an implementation issue.
> If we do follow the spec, it probably requires filing a spec bug to clarify. 
> 
The problem is because we didn't follow look up a custom element definition spec[1]. Per spec, we need to pass localName and |is| value, then it'll check if there is custom element definition in registry with name and local name both equal to localName, return that custom element definition ([1] step 4). However, we put localName and |is| value into one parameter[2], if there is a |is| value, we will pass |is| value as parameter in most of cases[3].

[1] https://html.spec.whatwg.org/multipage/custom-elements.html#look-up-a-custom-element-definition
[2] https://searchfox.org/mozilla-central/rev/797c93d81fe446f78babf20894f0729f15f71ee6/dom/base/CustomElementRegistry.cpp#238-249
[3] https://searchfox.org/mozilla-central/rev/797c93d81fe446f78babf20894f0729f15f71ee6/parser/html/nsHtml5TreeOperation.cpp#410-412

Comment 4

[Edgar Chen [:edgar]]

(In reply to John Dai[:jdai] from comment #3)
> However, we put localName and |is| value into one parameter[2], if
> there is a |is| value, we will pass |is| value as parameter in most of
> cases[3].

We do pass another parameter for localname, and LookupCustomElementDefinition() will check the passed parameter with the one stored in CustomElementDefintion. In http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=5534 case, it won't pass the check and returns nullptr (though this is still not expected behavior), right? And I am also curious why prototype of <x-foo> is Bar in this case ...

Comment 5

[John Dai[:jdai]]

(In reply to Edgar Chen [:edgar] from comment #4)
> (In reply to John Dai[:jdai] from comment #3)
> > However, we put localName and |is| value into one parameter[2], if
> > there is a |is| value, we will pass |is| value as parameter in most of
> > cases[3].
> 
> We do pass another parameter for localname, and
> LookupCustomElementDefinition() will check the passed parameter with the one
> stored in CustomElementDefintion. In
> http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=5534 case, it
> won't pass the check and returns nullptr (though this is still not expected
> behavior), right? And I am also curious why prototype of <x-foo> is Bar in
> this case ...

The prototype of <x-foo> is Bar because when parser run NS_NewHTMLElement if there is no definition, but it's a valid custom element, we'll set CustomElementData[1]. Hence, we will do prototype swizzling at Element::WrapObject[2]. It'll be fixed at bug 1416999. However, we still need to fix LookupCustomElementDefinition return incorrect definition in this bug.

[1] https://searchfox.org/mozilla-central/rev/33c90c196bc405e628bc868a4f4ba29b992478c0/dom/html/nsHTMLContentSink.cpp#370-373
[2] https://searchfox.org/mozilla-central/rev/33c90c196bc405e628bc868a4f4ba29b992478c0/dom/base/Element.cpp#521-541

Comment 6

[Edgar Chen [:edgar]]

(In reply to John Dai[:jdai] from comment #5)
> we'll set CustomElementData[1]. Hence, we will do prototype swizzling at
> Element::WrapObject[2].

That means we also set incorrect custom element type in CustomElementData, we should also fix this.

Comment 7

[John Dai[:jdai]]

(In reply to Edgar Chen [:edgar] from comment #6)
> (In reply to John Dai[:jdai] from comment #5)
> > we'll set CustomElementData[1]. Hence, we will do prototype swizzling at
> > Element::WrapObject[2].
> 
> That means we also set incorrect custom element type in CustomElementData,
> we should also fix this.

Thanks to notice this!

Comment 8

[John Dai[:jdai]]

Attachment: patch, v1

Comment 9

[Edgar Chen [:edgar]]

Comment on attachment 8936800 [details] [diff] [review]
patch, v1

Review of attachment 8936800 [details] [diff] [review]:
-----------------------------------------------------------------

Could you add some comments in https://searchfox.org/mozilla-central/rev/b0098afaeaad24a6752aa418ca3e86eade5fab17/dom/base/nsNodeUtils.cpp#465 to explain why we are good for clone case.

::: dom/base/nsContentUtils.cpp
@@ +10121,5 @@
>      isCustomElementName = nsContentUtils::IsCustomElementName(name);
>    }
> +
> +  RefPtr<nsAtom> tagAtom = nodeInfo->NameAtom();
> +  RefPtr<nsAtom> typeAtom = (isCustomElementName || !aIs) ? tagAtom : NS_Atomize(*aIs);

Hmm, I think we probably could be better here. The typeAtom seems could still be a weird value, for example, the value of the button element without `is` attribute, in this case we set it to 'button', but this element is not a custom elements at all. I think we should set typeAtom to nullptr to prevent someone misuse it in the future. And if it is possible adding some assertions, the will be great.

::: dom/tests/mochitest/webcomponents/test_autonomous_custom_element_contain_is_attribute.html
@@ +30,5 @@
> +
> +</script>
> +<x-foo is="x-bar"></x-foo>
> +<script>
> +ok(xfoo instanceof XFoo, "Parser create an autonomous custom element which contains is attribute should create as autonomous custom element");

I don't quite understand the test here,
1. The `xfoo` is still first <x-foo> that is removed from container, right?
2. And the first <x-foo> is also created from parser, why we test the parser created case again?
   Or you meant to test the definition come before/after creation case?

And please also add test for document.createElement() and clonNode().

Last thing, could this test be a web platform test? If yes, you could do in this bug or file a follow-up bug to move this test to a web platform test.

::: parser/html/nsHtml5TreeOperation.cpp
@@ +408,5 @@
>      isCustomElement = (aCreator == NS_NewCustomElement || !isValue.IsEmpty());
>      if (isCustomElement && aFromParser != dom::FROM_PARSER_FRAGMENT) {
>        RefPtr<nsAtom> tagAtom = nodeInfo->NameAtom();
>        RefPtr<nsAtom> typeAtom =
> +        (aCreator == NS_NewCustomElement || isValue.IsEmpty()) ? tagAtom : NS_Atomize(isValue);

Ditto.

Comment 10

[John Dai[:jdai]]

(In reply to Edgar Chen [:edgar] from comment #9)
> Comment on attachment 8936800 [details] [diff] [review]
> patch, v1
> 
> Review of attachment 8936800 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Could you add some comments in
> https://searchfox.org/mozilla-central/rev/
> b0098afaeaad24a6752aa418ca3e86eade5fab17/dom/base/nsNodeUtils.cpp#465 to
> explain why we are good for clone case.
> 
Will do.

> ::: dom/base/nsContentUtils.cpp
> @@ +10121,5 @@
> >      isCustomElementName = nsContentUtils::IsCustomElementName(name);
> >    }
> > +
> > +  RefPtr<nsAtom> tagAtom = nodeInfo->NameAtom();
> > +  RefPtr<nsAtom> typeAtom = (isCustomElementName || !aIs) ? tagAtom : NS_Atomize(*aIs);
> 
> Hmm, I think we probably could be better here. The typeAtom seems could
> still be a weird value, for example, the value of the button element without
> `is` attribute, in this case we set it to 'button', but this element is not
> a custom elements at all. I think we should set typeAtom to nullptr to
> prevent someone misuse it in the future. And if it is possible adding some
> assertions, the will be great.
> 
Will do.

> :::
> dom/tests/mochitest/webcomponents/
> test_autonomous_custom_element_contain_is_attribute.html
> @@ +30,5 @@
> > +
> > +</script>
> > +<x-foo is="x-bar"></x-foo>
> > +<script>
> > +ok(xfoo instanceof XFoo, "Parser create an autonomous custom element which contains is attribute should create as autonomous custom element");
> 
> I don't quite understand the test here,
> 1. The `xfoo` is still first <x-foo> that is removed from container, right?
> 2. And the first <x-foo> is also created from parser, why we test the parser
> created case again?
>    Or you meant to test the definition come before/after creation case?
> 
I want to test the definition come before/after creation, I'll revise test message for better understanding. 

> And please also add test for document.createElement() and clonNode().
>
Will do.
 
> Last thing, could this test be a web platform test? If yes, you could do in
> this bug or file a follow-up bug to move this test to a web platform test.
> 
Will do.

> ::: parser/html/nsHtml5TreeOperation.cpp
> @@ +408,5 @@
> >      isCustomElement = (aCreator == NS_NewCustomElement || !isValue.IsEmpty());
> >      if (isCustomElement && aFromParser != dom::FROM_PARSER_FRAGMENT) {
> >        RefPtr<nsAtom> tagAtom = nodeInfo->NameAtom();
> >        RefPtr<nsAtom> typeAtom =
> > +        (aCreator == NS_NewCustomElement || isValue.IsEmpty()) ? tagAtom : NS_Atomize(isValue);
> 
> Ditto.
Will do.

Comment 11

[John Dai[:jdai]]

Attachment: patch, v2

Address comment 9.

Comment 12

[Edgar Chen [:edgar]]

Comment on attachment 8939488 [details] [diff] [review]
patch, v2

Review of attachment 8939488 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsNodeUtils.cpp
@@ +513,1 @@
>          RefPtr<nsAtom> typeAtom = extension.IsEmpty() ? tagAtom : NS_Atomize(extension);

I just realized that we still possibly have incorrect CustomElementData setup on the cloned node,

var foo = document.createElement("button", { is: "x-foo" });
foo.removeAttribute("is");
var bar = foo.cloneNode();

In above case, we will set CustomElementData with 'button' type to 'bar', but 'bar' is not a custom element because the 'is' attribute on 'foo' have been removed when cloning, and the 'button' is not a correct custom element type, either.

Maybe adding some sanity checks in Element::SetCustomElementData() would help to catch the incorrectness of custom element type?

::: parser/html/nsHtml5TreeOperation.cpp
@@ +408,5 @@
>      isCustomElement = (aCreator == NS_NewCustomElement || !isValue.IsEmpty());
>      if (isCustomElement && aFromParser != dom::FROM_PARSER_FRAGMENT) {
>        RefPtr<nsAtom> tagAtom = nodeInfo->NameAtom();
> +      RefPtr<nsAtom> typeAtom;
> +      if (isCustomElement) {

We already check isCustomElement in above if, so maybe just

RefPtr<nsAtom> typeAtom = (aCreator == NS_NewCustomElement) ? tagAtom : NS_Atomize(isValue);

::: testing/web-platform/meta/MANIFEST.json
@@ +523704,5 @@
>     "d1661ab1734f7d1a252030aeac7e9842a7a4cb3b",
>     "testharness"
>    ],
>    "custom-elements/Document-createElement.html": [
> +   "06c2041a52cccc068fa2001af8d205088302f562",

Node-cloneNode.html also needs an update.

Comment 13

[John Dai[:jdai]]

(In reply to Edgar Chen [:edgar] from comment #12)
> Comment on attachment 8939488 [details] [diff] [review]
> patch, v2
> 
> Review of attachment 8939488 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsNodeUtils.cpp
> @@ +513,1 @@
> >          RefPtr<nsAtom> typeAtom = extension.IsEmpty() ? tagAtom : NS_Atomize(extension);
> 
> I just realized that we still possibly have incorrect CustomElementData
> setup on the cloned node,
> 
> var foo = document.createElement("button", { is: "x-foo" });
> foo.removeAttribute("is");
> var bar = foo.cloneNode();
> 
> In above case, we will set CustomElementData with 'button' type to 'bar',
> but 'bar' is not a custom element because the 'is' attribute on 'foo' have
> been removed when cloning, and the 'button' is not a correct custom element
> type, either.
> 
Thank you for catching this. This is a case we should consider.

> Maybe adding some sanity checks in Element::SetCustomElementData() would
> help to catch the incorrectness of custom element type?
> 
Will do.

> ::: parser/html/nsHtml5TreeOperation.cpp
> @@ +408,5 @@
> >      isCustomElement = (aCreator == NS_NewCustomElement || !isValue.IsEmpty());
> >      if (isCustomElement && aFromParser != dom::FROM_PARSER_FRAGMENT) {
> >        RefPtr<nsAtom> tagAtom = nodeInfo->NameAtom();
> > +      RefPtr<nsAtom> typeAtom;
> > +      if (isCustomElement) {
> 
> We already check isCustomElement in above if, so maybe just
> 
> RefPtr<nsAtom> typeAtom = (aCreator == NS_NewCustomElement) ? tagAtom :
> NS_Atomize(isValue);
> 
Will do.

> ::: testing/web-platform/meta/MANIFEST.json
> @@ +523704,5 @@
> >     "d1661ab1734f7d1a252030aeac7e9842a7a4cb3b",
> >     "testharness"
> >    ],
> >    "custom-elements/Document-createElement.html": [
> > +   "06c2041a52cccc068fa2001af8d205088302f562",
> 
> Node-cloneNode.html also needs an update.
Will do.

Comment 14

[John Dai[:jdai]]

Attachment: patch, v3

Address comment 12.

Comment 15

[Edgar Chen [:edgar]]

Comment on attachment 8939748 [details] [diff] [review]
patch, v3

Review of attachment 8939748 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thank you.

::: dom/base/nsNodeUtils.cpp
@@ +508,5 @@
> +          !extension.IsEmpty()) {
> +        // The typeAtom can be determined by extension, because we only need to
> +        // consider two cases: 1) Original node is a autonomous custom element
> +        // which has CustomElementData. 2) Original node has extension which is
> +        // a built-in custom element or it's not a custom element, but it

Maybe: 2) Original node is a built-in custom element or normal element, but it has `is` attribute when it is being cloned.

Comment 16

[John Dai[:jdai]]

Attachment: Bug 1419662 - Fix incorrect custom element type in CustomElementData.

Address comment 15. In this bug, we set correct custom element type. If we have a autonomous custom element, but it has is attribute, e.g. <x-foo is="x-bar"></x-foo>. Hence, we will treat this custom element as 'x-foo' type.

Test script: http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=5534

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5ebaef1895f986d90fbe83695d948a33657a5b8&group_state=expanded&filter-tier=1

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8940076 [details] [diff] [review]
Bug 1419662 - Fix incorrect custom element type in CustomElementData.

The spec seems to be clear on how this all should work. It is hard to follow, but still clear.


>+<autonomous-custom-element id="instance2" is="is-custom-element"></my-custom-element>
Fix the closing tag to use the same name as opening tag

Comment 18

[John Dai[:jdai]]

Attachment: Bug 1419662 - Fix incorrect custom element type in CustomElementData. f=echen, r=smaug

Address comment 17, and carrying r+.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48bb9add84a8
Fix incorrect custom element type in CustomElementData. f=echen, r=smaug

Comment 20

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/48bb9add84a8