BindToTree/UnbindFromTree of shadow root host does not call BindToTree/UnbindFromTree on ShadowRoot children.

RESOLVED FIXED in mozilla35

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: wchen, Assigned: wchen)

Tracking

(Blocks: 2 bugs)

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

3 years ago
We currently have code in various places in BindToTree/UnbindFromTree that doesn't run in certain cases because when a shadow root host is bound/unbound, the calls to BindToTree/UnbindFromTree are not called on the shadow root children.
(Assignee)

Comment 1

3 years ago
Created attachment 8485331 [details] [diff] [review]
BindToTree/UnbindFromTree wip

This patch calls BindToTree/UnbindFromTree on shadow root children. Nodes in the shadow tree will get BindTo/UnbindFromTree calls at least twice for hosts that are in the composed document. Once when the node is inserted in the shadow root subtree, and once when the host is inserted into the composed document.

Comment 2

3 years ago
That is not different to random DOM subtrees. The nodes in such get BindToTree when the subtree is created, and then if the subtree is added to document, all the elements get another BindToTree call so
that they can update their state.
Blocks: 1025724
(Assignee)

Comment 3

3 years ago
Created attachment 8488213 [details] [diff] [review]
Call BindToTree/UnbindFromTree on shadow root children when host is bound/unbound from tree.
Attachment #8485331 - Attachment is obsolete: true
Attachment #8488213 - Flags: review?(bugs)
Blocks: 1066554

Comment 4

3 years ago
Comment on attachment 8488213 [details] [diff] [review]
Call BindToTree/UnbindFromTree on shadow root children when host is bound/unbound from tree.

> nsINode::GetComposedDocInternal() const
> {
>   MOZ_ASSERT(HasFlag(NODE_IS_IN_SHADOW_TREE) && IsContent(),
>              "Should only be caled on nodes in the shadow tree.");
> 
>   // Cross ShadowRoot boundary.
>   ShadowRoot* containingShadow = AsContent()->GetContainingShadow();
>+
>+  if (!containingShadow) {
>+    // May have been nulled during unlinking.
>+    return nullptr;
>+  }
This is very unfortunate. Could we not clear the flag during unlinking?


So we don't bind/unbind more than just the youngest shadow tree.

Shouldn't HTMLShadowElement propagate the state to old trees?
Attachment #8488213 - Flags: review?(bugs) → review-
(Assignee)

Comment 5

3 years ago
Created attachment 8491273 [details] [diff] [review]
Call BindToTree/UnbindFromTree on shadow root children when host is bound/unbound from tree. v2

(In reply to Olli Pettay [:smaug] from comment #4)
> Comment on attachment 8488213 [details] [diff] [review]
> This is very unfortunate. Could we not clear the flag during unlinking?

The flag was used by nsXBLBinding during unlinking, I've made some slight changes so that we can clear the flag during unlinking.

> Shouldn't HTMLShadowElement propagate the state to old trees?

Done.

A couple other related changes in the updated patch:

- Nodes in an older shadow root used to return non-null from GetComposedDoc() when the nodes were not projected into a shadow insertion point (thus not appearing in the composed tree), it now will return null.

- A current bug that manifested after the GetComposedDoc() tweak is that currently the shadow root style sheet list [1] only contains sheets from style elements in the composed tree. It has been changed to always contain the sheets from style elements in the shadow tree.

[1] http://w3c.github.io/webcomponents/spec/shadow/#widl-ShadowRoot-styleSheets
Attachment #8488213 - Attachment is obsolete: true
Attachment #8491273 - Flags: review?(bugs)
(Assignee)

Comment 6

3 years ago
Created attachment 8491274 [details] [diff] [review]
v1 diff v2

Comment 7

3 years ago
Comment on attachment 8491274 [details] [diff] [review]
v1 diff v2

>@@ -829,16 +829,23 @@ Element::CreateShadowRoot(ErrorResult& a
>   // Replace the old ShadowRoot with the new one and let the old
>   // ShadowRoot know about the younger ShadowRoot because the old
>   // ShadowRoot is projected into the younger ShadowRoot's shadow
>   // insertion point (if it exists).
>   ShadowRoot* olderShadow = GetShadowRoot();
>   SetShadowRoot(shadowRoot);
>   if (olderShadow) {
>     olderShadow->SetYoungerShadow(shadowRoot);
>+
>+    // Unbind children of older shadow root because they
>+    // are no longer in the composed tree.
>+    for (nsIContent* child = olderShadow->GetFirstChild(); child;
>+         child = child->GetNextSibling()) {
>+      child->UnbindFromTree(true, false);
>+    }
Hmm, the comment is a bit misleading. nodes in the older shadow tree will be just
in a different place in the composed tree, assuming there is <shadow> somewhere in the new
shadow tree, right?
But actually, does the spec define this case at all?

https://www.w3.org/Bugs/Public/show_bug.cgi?id=26846

>+++ b/content/base/src/nsStyleLinkElement.cpp
>@@ -203,17 +203,18 @@ nsStyleLinkElement::UpdateStyleSheet(nsI
>                                      bool* aWillNotify,
>                                      bool* aIsAlternate,
>                                      bool aForceReload)
> {
>   if (aForceReload) {
>     // We remove this stylesheet from the cache to load a new version.
>     nsCOMPtr<nsIContent> thisContent;
>     CallQueryInterface(this, getter_AddRefs(thisContent));
>-    nsIDocument* doc = thisContent->GetCrossShadowCurrentDoc();
>+    nsCOMPtr<nsIDocument> doc = thisContent->IsInShadowTree() ?
>+      thisContent->OwnerDoc() : thisContent->GetUncomposedDoc();
The spec is _crazy_ if it requires this behavior.
I don't see anything in the spec requiring this behavior.
So, either just use composedDoc, or explain why this behavior is needed.


>-  nsCOMPtr<nsIDocument> doc = thisContent->GetCrossShadowCurrentDoc();
>+  nsCOMPtr<nsIDocument> doc = thisContent->IsInShadowTree() ?
>+    thisContent->OwnerDoc() : thisContent->GetUncomposedDoc();
>   if (!doc || !doc->CSSLoader()->GetEnabled()) {

ditto.

>+    // Propagate UnbindFromTree call to previous projected shadow
>+    // root children.
>+    ShadowRoot* projectedShadow = oldContainingShadow->GetOlderShadow();
>+    if (projectedShadow) {
>+      for (nsIContent* child = projectedShadow->GetFirstChild(); child;
>+           child = child->GetNextSibling()) {
>+        child->UnbindFromTree(true, false);
>+      }
>+    }
Shouldn't we propagate UnbindFromTree to older shadow even if 
GetContainingShadow() returns true.

Comment 8

3 years ago
Comment on attachment 8491273 [details] [diff] [review]
Call BindToTree/UnbindFromTree on shadow root children when host is bound/unbound from tree. v2

So, minor tweaking and clarifications needed.
Attachment #8491273 - Flags: review?(bugs) → review-
(Assignee)

Comment 9

3 years ago
Created attachment 8491813 [details] [diff] [review]
Call BindToTree/UnbindFromTree on shadow root children when host is bound/unbound from tree. v3

(In reply to Olli Pettay [:smaug] from comment #7)
> Comment on attachment 8491274 [details] [diff] [review]
> v1 diff v2
> 
> Hmm, the comment is a bit misleading. nodes in the older shadow tree will be
> just
> in a different place in the composed tree, assuming there is <shadow>
> somewhere in the new
> shadow tree, right?
> But actually, does the spec define this case at all?

When you call createShadowRoot(), it creates a new empty shadow root without a <shadow>, so nodes in the old shadow root don't get distributed anywhere and thus do not appear in the composed tree as specified by the "composed tree children calculation algorithm".

> The spec is _crazy_ if it requires this behavior.
> I don't see anything in the spec requiring this behavior.
> So, either just use composedDoc, or explain why this behavior is needed.

This spec is not well defined here. "On getting [the .styleSheet property], the attribute must return a StyleSheetList sequence containing the shadow root style sheets."

The spec doesn't mention anything about when to create and add style sheets in shadow DOM.

I made it behave the way it does in the patch so that this works:

var shadow = document.createElement("div").createShadowRoot();
shadow.innerHTML = "<style></style>";
console.log(shadow.styleSheets.length == 1);

It just seemed to make sense that shadow.styleSheets.length always return 1 regardless of whether the host is in the composed document. But I don't have a strong opinion about it right now so if you prefer only creating sheets when in a composed document, I'll update the implementation and tests.

https://www.w3.org/Bugs/Public/show_bug.cgi?id=26850

> Shouldn't we propagate UnbindFromTree to older shadow even if 
> GetContainingShadow() returns true.

Yeah, that's right.
Attachment #8491273 - Attachment is obsolete: true
Attachment #8491274 - Attachment is obsolete: true
Attachment #8491813 - Flags: review?(bugs)
(Assignee)

Comment 10

3 years ago
Created attachment 8491814 [details] [diff] [review]
v2 diff v3
Comment on attachment 8491813 [details] [diff] [review]
Call BindToTree/UnbindFromTree on shadow root children when host is bound/unbound from tree. v3

>@@ -829,16 +829,23 @@ Element::CreateShadowRoot(ErrorResult& a
>   // Replace the old ShadowRoot with the new one and let the old
>   // ShadowRoot know about the younger ShadowRoot because the old
>   // ShadowRoot is projected into the younger ShadowRoot's shadow
>   // insertion point (if it exists).
>   ShadowRoot* olderShadow = GetShadowRoot();
>   SetShadowRoot(shadowRoot);
>   if (olderShadow) {
>     olderShadow->SetYoungerShadow(shadowRoot);
>+
>+    // Unbind children of older shadow root because they
>+    // are no longer in the composed tree.
>+    for (nsIContent* child = olderShadow->GetFirstChild(); child;
>+         child = child->GetNextSibling()) {
>+      child->UnbindFromTree(true, false);
>+    }

It is not clear from the spec how the older shadow trees should behave here, but I think this behavior makes sense, 
since older trees aren't in the composed tree.
Attachment #8491813 - Flags: review?(bugs) → review+
(Assignee)

Comment 12

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d7ccb243db5
Assignee: nobody → wchen
Flags: in-testsuite+
sorry had to backout this change for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=48933233&tree=Mozilla-Inbound
(Assignee)

Comment 14

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/82d07982831c

We are now passing a test that was previous failing. Pushed the same thing but removed the file for expected failure.
https://hg.mozilla.org/mozilla-central/rev/82d07982831c
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.