1081039 - cloneNode on a customElement should call createdCallback

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Gabor Krizsanits (INACTIVE)]

I don't know what does the spec say about this but polymer seems to rely on this, so we should check this.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Yeah, it looks like we only enqueue the callback in RegisterElement, SwizzleCustomElement, and the NS_NewHTMLElement in the content sink.

Should we be calling SwizzleCustomElement on clone, or enqueueing the callback directly?

Comment 2

[William Chen [:wchen]]

Attachment: cloneNode on a custom element should call createdCallback

The spec doesn't say that cloneNode should call the created callback per se, but part of cloneNode is to create a new node in the document of the cloned node and this happens to be a code path that was missed for enqueuing a created callback. This patch should fix the problem.

Comment 3

[Boris Zbarsky [:bzbarsky]]

What should happen on adoptNode()?  Worth adding a testcase for that...

Comment 4

[Gabor Krizsanits (INACTIVE)]

(In reply to William Chen [:wchen] from comment #2)
> Created attachment 8504511 [details] [diff] [review]
> cloneNode on a custom element should call createdCallback
> 
> The spec doesn't say that cloneNode should call the created callback per se,
> but part of cloneNode is to create a new node in the document of the cloned
> node and this happens to be a code path that was missed for enqueuing a
> created callback. This patch should fix the problem.

Should this patch get in the tree?

Comment 5

[William Chen [:wchen]]

Attachment: cloneNode on a custom element should call createdCallback if cloned in a document with custom element definition

(In reply to Boris Zbarsky [:bz] from comment #3)
> What should happen on adoptNode()?  Worth adding a testcase for that...

The created callback doesn't get called for adoptNode because it doesn't create a new node.

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK, but does the old custom element definition go away when the proto changes?  Is the new custom element definition from the new document applied?

Comment 7

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky [:bz] from comment #6)
> OK, but does the old custom element definition go away when the proto
> changes?  Is the new custom element definition from the new document applied?

I'm trying to understand this question, but I guess I'm missing something. What do you mean exactly by "proto changes"? Do we do some proto swapping in adoptNode? Do you mean we should register the proto in the new document? Sorry, I might misinterpret your question entirely...

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Do we do some proto swapping in adoptNode?

Yes.  We swap to a proto from the new document.

In fact, we just create an entirely new JS wrapper for the object, which I assume will end up triggering attachment of custom element definitions from the new document.

Comment 9

[William Chen [:wchen]]

On adoptNode, we lose the prototype from the old custom element definition because we don't use the same prototype on the new wrapper and we don't set up the prototype of the custom element definition in the new document because per spec, it is set right before where we would call the created callback (which doesn't happen with an adopted node because it's not created in the new document).

We currently differ in behavior from chrome here because chrome keeps the prototype on the adopted node. There is a bug filed for an adopted callback that could help resolve the differences. https://www.w3.org/Bugs/Public/show_bug.cgi?id=24577

Comment 10

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

Comment on attachment 8531754 [details] [diff] [review]
cloneNode on a custom element should call createdCallback if cloned in a document with custom element definition

Having these two almost identical SetupCustomElement methods feels rather
error prone.
Could we perhaps make aTypeExtension const nsAString* so that one could pass null if needed. Perhaps move it to be the last param and default to null?


Do we have any tests for cases when one clones non-html elements with an is-attribute?

Comment 11

[William Chen [:wchen]]

Attachment: cloneNode on a custom element should call createdCallback if cloned in a document with custom element definition. v2

Got rid of the second similar method and added test.

Comment 12

[William Chen [:wchen]]

Attachment: v1 diff v2

Comment 13

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

Comment on attachment 8538195 [details] [diff] [review]
cloneNode on a custom element should call createdCallback if cloned in a document with custom element definition. v2

>+  if (definition->mLocalName != typeAtom) {
>+    // This element is a custom element by extenion, thus we need to
extension

>+    if (clone->IsElement()) {
>+      // The cloned node may be a custom element that may require
>+      // enqueing created callback and prototype swizzling.
>+      Element* elem = clone->AsElement();
>+      if (nsContentUtils::IsCustomElementName(nodeInfo->NameAtom())) {
>+        elem->OwnerDoc()->SetupCustomElement(elem, nodeInfo->NamespaceID());
>+      } else {
>+        // Check if node may be custom element by type extension.
>+        // ex. <button is="x-button">
>+        nsAutoString extension;
>+        elem->GetAttr(kNameSpaceID_None, nsGkAtoms::is, extension);
>+        if (!extension.IsEmpty()) {
Maybe
if (elem->GetAttr(kNameSpaceID_None, nsGkAtoms::is, extension) && !extension.IsEmpty()) ...
>+          elem->OwnerDoc()->SetupCustomElement(elem, nodeInfo->NamespaceID(),
>+                                               &extension);
>+        }
>+      }
We really don't have any faster way to check if element is a customelement? Looks like no.
And 'is' handling kind of requires this setup.
It is perhaps a bit surprising that if one removes 'is' from an element and then clones it, the clone is rather different.
But that is what the spec seems to require.




>+var p = Object.create(HTMLElement.prototype);
>+p.createdCallback = function() {
>+  if (this == fooClone) {
>+    // Callback called for the element created before registering the custom element.
>+    // Should be called on element upgrade.
>+    is(callbackCalledOnUpgrade, false, "Upgrade should only be called once per clone.");
>+    callbackCalledOnUpgrade = true;
Could you check somewhere here the prototype of the clone is the right onw

Comment 14

[William Chen [:wchen]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3eee7f05cbb

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d3eee7f05cbb