Closed Bug 1261002 Opened 4 years ago Closed 3 years ago

Implement Node.isConnected

Categories

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

36 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: smaug, Assigned: bhsu)

Details

(Whiteboard: [tw-dom] btpp-active)

Attachments

(2 files, 12 obsolete files)

913 bytes, patch
bhsu
: review+
Details | Diff | Splinter Review
6.61 KB, patch
bhsu
: review+
Details | Diff | Splinter Review
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.
And looks like the spec was just updated.
https://dom.spec.whatwg.org/#dom-node-isconnected
If you don't mind, I'd like to take this bug :)
Assignee: nobody → bhsu
Thanks.

FYI, looks like blink is also implementing this
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/0b5Jgk47uMU
(Uploading an WIP patch.)
(Uploading an WIP patch.)
Attachment #8739748 - Attachment description: Bug 1261002 - Part 2: Add a testcase for node.isConnected. → Part 2: Add a testcase for node.isConnected.
Whiteboard: [tw-dom] → [tw-dom] btpp-active
Attachment #8739747 - Attachment is obsolete: true
Hey Ben, any updates here?
Flags: needinfo?(bhsu)
Attachment #8739748 - Attachment is obsolete: true
Flags: needinfo?(bhsu)
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 :(
Attachment #8749505 - Attachment is obsolete: true
Attachment #8752745 - Attachment is obsolete: true
Attached file backtrace.txt (obsolete) —
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.
In other words, just follow
https://dom.spec.whatwg.org/#dom-node-isconnected
(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.
Attachment #8752747 - Attachment is obsolete: true
Attachment #8752748 - Attachment is obsolete: true
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.
Attachment #8749504 - Attachment is obsolete: true
Attachment #8753208 - Flags: review?(bugs)
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.
Attachment #8752746 - Attachment is obsolete: true
Attachment #8753212 - Flags: review?(bugs)
Attachment #8753212 - Attachment description: Bug 1261002 - Part 2: Add a testcase for Node.isConnected. → Bug 1261002 - for Node.isConnected.
Attachment #8753212 - Attachment description: Bug 1261002 - for Node.isConnected. → Part 2: Add a testcase for Node.isConnected.
May take a bit time to get used to the spec jargon :)
Attachment #8753208 - Flags: review?(bugs) → review+
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).
Attachment #8753212 - Flags: review?(bugs) → review+
Comments addressed :)
Attachment #8753212 - Attachment is obsolete: true
Attachment #8753328 - Flags: review+
Attachment #8753208 - Attachment description: Part 1: Implement node.isConnected. → Part 1: Implement node.isConnected. r=smaug
Sorry, I've made a terribly stupid mistake :(
Attachment #8753328 - Attachment is obsolete: true
Attachment #8753692 - Flags: review+
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.
Attachment #8753208 - Attachment is obsolete: true
Attachment #8755089 - Flags: review+
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
Attachment #8753692 - Attachment is obsolete: true
Attachment #8755090 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/a5aea694837a
https://hg.mozilla.org/mozilla-central/rev/1cc416ae06a0
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.