Add a fast-path for independent inherited properties

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

 /*
  * Copyright (C) 2011 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Neither the name of Google Inc. nor the names of its
@@ skipping 116 matching lines @@
         replaceChildrenWithFragment(this, fragment, exceptionState);
 }
 
 void ShadowRoot::recalcStyle(StyleRecalcChange change)
 {
     // ShadowRoot doesn't support custom callbacks.
     DCHECK(!hasCustomStyleCallbacks());
 
     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();
 
     recalcDescendantStyles(change);
     clearChildNeedsStyleRecalc();
 }
 
 void ShadowRoot::attachLayoutTree(const AttachContext& context)
 {
@@ skipping 186 matching lines @@
         ostream << "ShadowRootType::Open";
         break;
     case ShadowRootType::Closed:
         ostream << "ShadowRootType::Closed";
         break;
     }
     return ostream;
 }
 
 } // namespace blink