1261002 - Implement Node.isConnected

Status: RESOLVED FIXED

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

Description

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

https://github.com/w3c/webcomponents/issues/81

The spec https://dom.spec.whatwg.org/ doesn't seem to have it quite yet, but I assume it'll behave just like Gecko's IsInComposedDoc().

isConnected is a bit badly named though (too generic), IMO, so I'd prefer to keep Gecko to use
IsInComposedDoc(). That would be consistent with other *Doc() methods we have.

So to expose the new property to JS, one needs to just add the property to Node.webidl 
[BinaryName=getComposedDoc]
readonly attribute boolean isConnected;
and write tests.

Comment 1

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

And looks like the spec was just updated.
https://dom.spec.whatwg.org/#dom-node-isconnected

Comment 2

[Ben Hsu [:HoPang]]

If you don't mind, I'd like to take this bug :)

Comment 3

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

Thanks.

FYI, looks like blink is also implementing this
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/0b5Jgk47uMU

Comment 4

[Ben Hsu [:HoPang]]

Attachment: Part 1: Implement node.isConnected.

(Uploading an WIP patch.)

Comment 5

[Ben Hsu [:HoPang]]

Attachment: Part 2: Add a testcase for node.isConnected.

(Uploading an WIP patch.)

Comment 6

[Ben Hsu [:HoPang]]

Attachment: Part 1: Implement node.isConnected.

Comment 7

[Ben Hsu [:HoPang]]

Attachment: Part 2: Add a testcase for node.isConnected.

Comment 8

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hey Ben, any updates here?

Comment 9

[Ben Hsu [:HoPang]]

Currently, I've exposed |getComposedDoc| in the WebIDL, and it works perfectly with the tests for ordinary DOM child and shadow DOM. However, it fails the tests for iframes. I did a modification to pass the tests for iframes, but the tests end up with crash. I think I should provide more detailed information to call for help later today :(

Comment 10

[Ben Hsu [:HoPang]]

Attachment: Part 2: Add a testcase for node.isConnected.

Comment 11

[Ben Hsu [:HoPang]]

Attachment: Part 2: Add a testcase for node.isConnected.

Comment 12

[Ben Hsu [:HoPang]]

Attachment: Part 3: Propagate Node.isConnected through iframes.

Comment 13

[Ben Hsu [:HoPang]]

Attachment: backtrace.txt

Comment 14

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

Comment on attachment 8752747 [details] [diff] [review]
Part 3: Propagate Node.isConnected through iframes.

I don't understand this part. Why is this needed? Node.isConnected is about nodes in one document, not about sub-documents at all.

Comment 15

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

In other words, just follow
https://dom.spec.whatwg.org/#dom-node-isconnected

Comment 16

[Ben Hsu [:HoPang]]

(In reply to Olli Pettay [:smaug] from comment #15)
> In other words, just follow
> https://dom.spec.whatwg.org/#dom-node-isconnected

Hi Olli,

Thanks for your reply! I misunderstood what the spec means. I would like to reserve the tests for iframes, since there might be somebody like me that doesn't know if Node.isConnected should be true if it's under a document including under the document of an iframe.

Comment 17

[Ben Hsu [:HoPang]]

Attachment: Part 1: Implement node.isConnected. r=smaug

Hi Olli,

I think these patches are ready, do you mind reviewing them? In this part, I exposed an existing method to the WedIDL as Node.isConnected.

Comment 18

[Ben Hsu [:HoPang]]

Attachment: Part 2: Add a testcase for Node.isConnected.

This testcase is composed of three groups of tests, ordinary children, iframes and shadow DOM. In side each of the groups, I firstly create nodes, append them to the DOM tree, and finally remove them from the DOM tree. To test how Node.isConnected propogates, multiple layer of nodes would be appended or removed together.

Comment 19

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

May take a bit time to get used to the spec jargon :)

Comment 20

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

Comment on attachment 8753212 [details] [diff] [review]
Part 2: Add a testcase for Node.isConnected.

>+test(function() {
>+  var nodes = [];
>+  nodes.push([document.createElement("div"),
>+              document.createElement("div"),
>+              document.createElement("div")]);
>+  nodes.push([document.createElement("div"),
>+              document.createElement("div"),
>+              document.createElement("div")]);
>+  nodes.push([document.createElement("div"),
>+              document.createElement("div"),
>+              document.createElement("div")]);
>+
>+  var shadowHosts = [document.createElement("div"),
>+                     nodes[0][1],
>+                     nodes[1][1]];
>+
>+  var trees = nodes.map(aNodes => {
>+    // Create a list-like tree.
>+    aNodes.reduce((aPrev, aCurr) => {
>+      aPrev.appendChild(aCurr);
>+      return aCurr;
>+    });
>+    // Return the root of the tree.
>+    return aNodes[0];
>+  });
>+  checkNodes([], [shadowHosts[0], ...nodes[0], ...nodes[1], ...nodes[2]]);
>+
>+  // Add the 1st layer shadow tree
>+  var shadowRoot0 = shadowHosts[0].createShadowRoot();
>+  shadowRoot0.appendChild(trees[0]);
>+  checkNodes([], [shadowHosts[0], ...nodes[0], ...nodes[1], ...nodes[2]]);
>+
>+  document.body.appendChild(shadowHosts[0]);
>+  checkNodes([shadowHosts[0], ...nodes[0]],
>+             [...nodes[1], ...nodes[2]]);
>+
>+  // Add the 2nd and the 3rd layer shadow tree together
>+  var shadowRoot2 = shadowHosts[2].createShadowRoot();
>+  shadowRoot2.appendChild(trees[2]);
>+  checkNodes([shadowHosts[0], ...nodes[0]],
>+             [...nodes[1], ...nodes[2]]);
>+
>+  var shadowRoot1 = shadowHosts[1].createShadowRoot();
>+  shadowRoot1.appendChild(trees[1]);
>+  checkNodes([shadowHosts[0], ...nodes[0], ...nodes[1], ...nodes[2]], []);
>+
>+  // Remove the 3rd layer shadow tree.
>+  shadowHosts[2].remove();
>+  checkNodes([shadowHosts[0], ...nodes[0], nodes[1][0]],
>+             [nodes[1][1], nodes[1][2], ...nodes[2]]);
>+
>+  // Remove the 1st and the 2nd layer shadow tree together
>+  shadowHosts[0].remove();
>+  checkNodes([], [shadowHosts[0], ...nodes[0], ...nodes[1], ...nodes[2]]);
>+}, "Test with shadow DOM.");
Please remove this test, since Shadow DOM spec has been in flux and the latest spec has attachShadow and not createShadowRoot.
We'll add more tests once attachShadow is implemented (and createShadowRoot removed).

Comment 21

[Ben Hsu [:HoPang]]

Attachment: (V2) Part 2: Add a testcase for Node.isConnected. r=smaug

Comments addressed :)

Comment 22

[Ben Hsu [:HoPang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1918437c990

Comment 23

[Ben Hsu [:HoPang]]

Attachment: (V3) Part 2: Add a testcase for Node.isConnected. r=smaug

Sorry, I've made a terribly stupid mistake :(

Comment 24

[Ben Hsu [:HoPang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=546f16a712cf

Comment 25

[Ben Hsu [:HoPang]]

Attachment: (V2) Part 1: Implement node.isConnected. r=smaug

Hi Olli,

I moved the code to another place to match the order in the interface defined in web-platform-tests. Since it's a quite simple modification, I've r+ on my own. If you have any concern, please feel free to cancel it.

Comment 26

[Ben Hsu [:HoPang]]

Attachment: (V4) Part 2: Add a testcase for Node.isConnected. r=smaug

Hi Olli,

Since I am not so familiar with web-platform-tests, they took me some time to pass try server. It's another simple update, so I also r+ this patch myself ;P

Comment 27

[Ben Hsu [:HoPang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5df2d3317f4a

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51865375fb83
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e9913cfd157

Comment 29

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc15d1035a0d
https://hg.mozilla.org/integration/mozilla-inbound/rev/08a0ff25afd6

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5aea694837a
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cc416ae06a0

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a5aea694837a
https://hg.mozilla.org/mozilla-central/rev/1cc416ae06a0