Note: There are a few cases of duplicates in user autocompletion which are being worked on.

cloneNode on a customElement should call createdCallback

RESOLVED FIXED in mozilla37

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: krizsa, Assigned: wchen)

Tracking

(Blocks: 1 bug)

unspecified
mozilla37
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

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

Updated

3 years ago
Blocks: 811542

Comment 1

3 years ago
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?
(Assignee)

Comment 2

3 years ago
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.

Comment 3

3 years ago
What should happen on adoptNode()?  Worth adding a testcase for that...
(Reporter)

Comment 4

3 years ago
(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?
(Assignee)

Comment 5

3 years ago
Created attachment 8531754 [details] [diff] [review]
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.
Attachment #8504511 - Attachment is obsolete: true
Attachment #8531754 - Flags: review?(bugs)

Comment 6

3 years ago
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?
(Reporter)

Comment 7

3 years ago
(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

3 years ago
> 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.
(Assignee)

Comment 9

3 years ago
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 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?
Attachment #8531754 - Flags: review?(bugs) → review-
(Assignee)

Comment 11

3 years ago
Created attachment 8538195 [details] [diff] [review]
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.
Attachment #8531754 - Attachment is obsolete: true
Attachment #8538195 - Flags: review?(bugs)
(Assignee)

Comment 12

3 years ago
Created attachment 8538197 [details] [diff] [review]
v1 diff v2
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
Attachment #8538195 - Flags: review?(bugs) → review+
(Assignee)

Comment 14

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3eee7f05cbb
Assignee: nobody → wchen
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/d3eee7f05cbb
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Depends on: 1116087
You need to log in before you can comment on or make changes to this bug.