If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Implement Node.isConnected

RESOLVED FIXED in Firefox 49

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: smaug, Assigned: HoPang)

Tracking

36 Branch
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

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

Attachments

(2 attachments, 12 obsolete attachments)

913 bytes, patch
HoPang
: review+
Details | Diff | Splinter Review
6.61 KB, patch
HoPang
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
And looks like the spec was just updated.
https://dom.spec.whatwg.org/#dom-node-isconnected
(Assignee)

Comment 2

2 years ago
If you don't mind, I'd like to take this bug :)
Assignee: nobody → bhsu
(Reporter)

Comment 3

2 years ago
Thanks.

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

Comment 4

2 years ago
Created attachment 8739747 [details] [diff] [review]
Part 1: Implement node.isConnected.

(Uploading an WIP patch.)
(Assignee)

Comment 5

2 years ago
Created attachment 8739748 [details] [diff] [review]
Part 2: Add a testcase for node.isConnected.

(Uploading an WIP patch.)
(Assignee)

Updated

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

Comment 6

a year ago
Created attachment 8749504 [details] [diff] [review]
Part 1: Implement node.isConnected.
Attachment #8739747 - Attachment is obsolete: true
(Assignee)

Comment 7

a year ago
Created attachment 8749505 [details] [diff] [review]
Part 2: Add a testcase for node.isConnected.
Hey Ben, any updates here?
Flags: needinfo?(bhsu)
(Assignee)

Updated

a year ago
Attachment #8739748 - Attachment is obsolete: true
Flags: needinfo?(bhsu)
(Assignee)

Comment 9

a year ago
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 :(
(Assignee)

Comment 10

a year ago
Created attachment 8752745 [details] [diff] [review]
Part 2: Add a testcase for node.isConnected.
Attachment #8749505 - Attachment is obsolete: true
(Assignee)

Comment 11

a year ago
Created attachment 8752746 [details] [diff] [review]
Part 2: Add a testcase for node.isConnected.
Attachment #8752745 - Attachment is obsolete: true
(Assignee)

Comment 12

a year ago
Created attachment 8752747 [details] [diff] [review]
Part 3: Propagate Node.isConnected through iframes.
(Assignee)

Comment 13

a year ago
Created attachment 8752748 [details]
backtrace.txt
(Reporter)

Comment 14

a year ago
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.
(Reporter)

Comment 15

a year ago
In other words, just follow
https://dom.spec.whatwg.org/#dom-node-isconnected
(Assignee)

Comment 16

a year ago
(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.
(Assignee)

Updated

a year ago
Attachment #8752747 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8752748 - Attachment is obsolete: true
(Assignee)

Comment 17

a year ago
Created attachment 8753208 [details] [diff] [review]
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.
Attachment #8749504 - Attachment is obsolete: true
Attachment #8753208 - Flags: review?(bugs)
(Assignee)

Comment 18

a year ago
Created attachment 8753212 [details] [diff] [review]
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.
Attachment #8752746 - Attachment is obsolete: true
Attachment #8753212 - Flags: review?(bugs)
(Assignee)

Updated

a year ago
Attachment #8753212 - Attachment description: Bug 1261002 - Part 2: Add a testcase for Node.isConnected. → Bug 1261002 - for Node.isConnected.
(Assignee)

Updated

a year ago
Attachment #8753212 - Attachment description: Bug 1261002 - for Node.isConnected. → Part 2: Add a testcase for Node.isConnected.
(Reporter)

Comment 19

a year ago
May take a bit time to get used to the spec jargon :)
(Reporter)

Updated

a year ago
Attachment #8753208 - Flags: review?(bugs) → review+
(Reporter)

Comment 20

a year ago
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+
(Assignee)

Comment 21

a year ago
Created attachment 8753328 [details] [diff] [review]
(V2) Part 2: Add a testcase for Node.isConnected. r=smaug

Comments addressed :)
Attachment #8753212 - Attachment is obsolete: true
Attachment #8753328 - Flags: review+
(Assignee)

Updated

a year ago
Attachment #8753208 - Attachment description: Part 1: Implement node.isConnected. → Part 1: Implement node.isConnected. r=smaug
Comment hidden (obsolete)
(Assignee)

Comment 23

a year ago
Created attachment 8753692 [details] [diff] [review]
(V3) Part 2: Add a testcase for Node.isConnected. r=smaug

Sorry, I've made a terribly stupid mistake :(
Attachment #8753328 - Attachment is obsolete: true
Attachment #8753692 - Flags: review+
Comment hidden (obsolete)
(Assignee)

Comment 25

a year ago
Created attachment 8755089 [details] [diff] [review]
(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.
Attachment #8753208 - Attachment is obsolete: true
Attachment #8755089 - Flags: review+
(Assignee)

Comment 26

a year ago
Created attachment 8755090 [details] [diff] [review]
(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
Attachment #8753692 - Attachment is obsolete: true
Attachment #8755090 - Flags: review+
(Assignee)

Comment 27

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5df2d3317f4a
Keywords: checkin-needed

Comment 28

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/51865375fb83
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e9913cfd157
Keywords: checkin-needed

Comment 29

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

Comment 30

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

Comment 31

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a5aea694837a
https://hg.mozilla.org/mozilla-central/rev/1cc416ae06a0
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.