Add a fast-path for independent inherited properties

third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp

Index: third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp
diff --git a/third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp b/third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp
index af3defa3ebec4c8926879ad994004671287d9e59..9e60a6188ced64e3c2eae38344d91d2d4a051b53 100644
--- a/third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp
+++ b/third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp
@@ -134,8 +134,13 @@ void ShadowRoot::recalcStyle(StyleRecalcChange change)
 
     StyleSharingDepthScope sharingScope(*this);
 
-    if (getStyleChangeType() >= SubtreeStyleChange)
+    if (getStyleChangeType() >= SubtreeStyleChange) {
         change = Force;
+    } else if (change == IndependentInherit) {
+        // Independent inheritance can't continue through shadow roots, since we
+        // assume inheritance from the parent element which isn't the case here.
+        change = Inherit;

[sashab, 2016/08/18 04:47:15]

Fix for shadow roots problem - crbug.com/633859. I can't recreate a minimal test
case for this bug and I'm still not sure I fully understand why it's happening.
I think it's because propagateInheritedProperties() assumes we inherit styles
from the parent element, but in a shadow root this isn't the case.

An alternative fix for this would probably be to change
propagateInheritedProperties to check if the parent is a shadow root, and not
run the fast path in this case. Either way we've effectively disabling
IndependentInherit changes across ShadowRoot boundaries, which I guess makes
sense since we shouldn't be propagating changes across to shadow doms from the
main dom anyway.

[rune, 2016/08/18 08:06:49]

I think I know what's happening. We traverse the full dom in tree-of-trees order
and not flat-tree order. Inheritance happens in the flat-tree order. For
instance:

== tree-of-trees:

<div id="host">
  <:shadow-root>
    <span>
      <slot></slot>
    </span>
  <:shadow-root>
  <div id="c1"></div>
  <div id="c2"></div>
</div>

== flat-tree:

<div id="host">
  <span>
    <div id="c1"></div>
    <div id="c2"></div>
  </span>
</div>


So, inheritance should happen from span to the distributed c1/c2 children. When
we recalculate style for a shadow host, we recalculate style for the shadow
tree(s) first. When we reach an insertion point (<slot> for v1, <content> for
v0), we check the StyleRecalcChange value and mark the distributed nodes (here
c1/c2) for recalc if the change is >= Inherit
(HTMLSlotElement::willRecalcStyle() and InsertionPoint::willRecalcStyle()).

Propagation should be fine down the shadow tree's light children, but the light
tree children of the host cannot use propagated inherit based on its light tree
parent. In fact, that's sub-optimal today because we use the StyleRecalcChange
of the shadow host when we figure out if we need to recalc the distributed
nodes. If the recalc of #host in the example above yields an Inherit, we
recalculate the style of c1/c2, but if the span recalc ends up in NoInherit or
NoChange, we shouldn't need to recalc for inheritance for c1/c2.

I think the conclusion for your changes, is that it should work to change the
implementation of HTMLSlotElement::willRecalcStyle and
InsertionPoint::willRecalcStyle to set LocalStyleChange for IndependentInherit.


A test-case that should trigger the bug is to change an independently inherited
property on the span in the example above and see that it is inherited into the
c1/c2 child(ren).

[sashab, 2016/08/19 05:35:25]

Thanks for the excellent summary Rune! You were absolutely right -- and I
understand why as well -- added a test and validated that this fix also fixes
the user manager bug.

In hindsight I'm surprised I didn't notice this when looking for usages of
StyleRecalcChange::Inherit.

Updated the CL description too. :)

+    }
 
     // There's no style to update so just calling recalcStyle means we're updated.
     clearNeedsStyleRecalc();