Bug 1324618 part 7. Set our new style context on all our continuations in ServoRestyleManager. r?emilio draft
authorBoris Zbarsky <bzbarsky@mit.edu>
Mon, 26 Jun 2017 23:35:08 -0700
changeset 600504 933c8d807ca43911df37368f6eca0f166a118c7b
parent 600503 6d38ee68c1152a9147f210bffb4ff0006444df90
child 600505 410f00b19f805bcbe83101252311b7e16986a56d
push id65783
push userbzbarsky@mozilla.com
push dateTue, 27 Jun 2017 06:35:27 +0000
reviewersemilio
bugs1324618
milestone56.0a1
Bug 1324618 part 7. Set our new style context on all our continuations in ServoRestyleManager. r?emilio MozReview-Commit-ID: 4gVXPDCPZnq
layout/base/ServoRestyleManager.cpp
layout/generic/nsInlineFrame.cpp
layout/reftests/ib-split/insert-into-split-inline-17-ref.html
layout/reftests/ib-split/insert-into-split-inline-17.html
layout/reftests/ib-split/reftest.list
--- a/layout/base/ServoRestyleManager.cpp
+++ b/layout/base/ServoRestyleManager.cpp
@@ -336,19 +336,18 @@ ServoRestyleManager::ProcessPostTraversa
     ClearRestyleStateFromSubtree(aElement);
     return true;
   }
 
   // TODO(emilio): We could avoid some refcount traffic here, specially in the
   // ServoComputedValues case, which uses atomic refcounting.
   //
   // Hold the old style context alive, because it could become a dangling
-  // pointer during the replacement. In practice it's not a huge deal (on
-  // GetNextContinuationWithSameStyle the pointer is not dereferenced, only
-  // compared), but better not playing with dangling pointers if not needed.
+  // pointer during the replacement. In practice it's not a huge deal, but
+  // better not playing with dangling pointers if not needed.
   RefPtr<nsStyleContext> oldStyleContext =
     styleFrame ? styleFrame->StyleContext() : nullptr;
 
   UndisplayedNode* displayContentsNode = nullptr;
   // FIXME(emilio, bug 1303605): This can be simpler for Servo.
   // Note that we intentionally don't check for display: none content.
   if (!oldStyleContext) {
     displayContentsNode =
@@ -384,24 +383,27 @@ ServoRestyleManager::ProcessPostTraversa
     nsIAtom* pseudoTag = pseudo == CSSPseudoElementType::NotPseudo
       ? nullptr : nsCSSPseudoElements::GetPseudoAtom(pseudo);
 
     newContext = aRestyleState.StyleSet().GetContext(
       computedValues.forget(), aParentContext, pseudoTag, pseudo, aElement);
 
     newContext->EnsureSameStructsCached(oldStyleContext);
 
-    // XXX This could not always work as expected: there are kinds of content
-    // with the first split and the last sharing style, but others not. We
-    // should handle those properly.
-    // XXXbz I think the UpdateStyleOfOwnedAnonBoxes call below handles _that_
-    // right, but not other cases where we happen to have different styles on
-    // different continuations... (e.g. first-line).
-    for (nsIFrame* f = styleFrame; f;
-         f = GetNextContinuationWithSameStyle(f, oldStyleContext)) {
+    // We want to walk all the continuations here, even the ones with different
+    // styles.  In practice, the only reason we get continuations with different
+    // styles here is ::first-line (::first-letter never affects element
+    // styles).  But in that case, newContext is the right context for the
+    // _later_ continuations anyway (the ones not affected by ::first-line), not
+    // the earlier ones, so there is no point stopping right at the point when
+    // we'd actually be setting the right style context.
+    //
+    // This does mean that we may be setting the wrong style context on our
+    // initial continuations; ::first-line fixes that up after the fact.
+    for (nsIFrame* f = styleFrame; f; f = f->GetNextContinuation()) {
       f->SetStyleContext(newContext);
     }
 
     if (MOZ_UNLIKELY(displayContentsNode)) {
       MOZ_ASSERT(!styleFrame);
       displayContentsNode->mStyle = newContext;
     }
 
@@ -488,18 +490,28 @@ ServoRestyleManager::ProcessPostTraversa
   if (!primaryFrame) {
     return false;
   }
 
   RefPtr<nsStyleContext> oldStyleContext = primaryFrame->StyleContext();
   nsStyleContext& newContext = aPostTraversalState.ComputeStyle(aTextNode);
   aPostTraversalState.ComputeHintIfNeeded(aTextNode, primaryFrame, newContext);
 
-  for (nsIFrame* f = primaryFrame; f;
-       f = GetNextContinuationWithSameStyle(f, oldStyleContext)) {
+  // We want to walk all the continuations here, even the ones with different
+  // styles.  In practice, the only reasons we get continuations with different
+  // styles are ::first-line and ::first-letter.  But in those cases,
+  // newContext is the right context for the _later_ continuations anyway (the
+  // ones not affected by ::first-line/::first-letter), not the earlier ones,
+  // so there is no point stopping right at the point when we'd actually be
+  // setting the right style context.
+  //
+  // This does mean that we may be setting the wrong style context on our
+  // initial continuations; ::first-line/::first-letter fix that up after the
+  // fact.
+  for (nsIFrame* f = primaryFrame; f; f = f->GetNextContinuation()) {
     f->SetStyleContext(&newContext);
   }
 
   return true;
 }
 
 void
 ServoRestyleManager::ClearSnapshots()
--- a/layout/generic/nsInlineFrame.cpp
+++ b/layout/generic/nsInlineFrame.cpp
@@ -1027,46 +1027,52 @@ nsInlineFrame::UpdateStyleOfOwnedAnonBox
              "Only the primary frame of the inline in a block-inside-inline "
              "split should have NS_FRAME_OWNS_ANON_BOXES");
   MOZ_ASSERT(mContent->GetPrimaryFrame() == this,
              "We should be the primary frame for our element");
 
   nsIFrame* blockFrame = GetProperty(nsIFrame::IBSplitSibling());
   MOZ_ASSERT(blockFrame, "Why did we have an IB split?");
 
+  // The later inlines need to get our style.
+  nsStyleContext* ourStyle = StyleContext();
+
   // The anonymous block's style inherits from ours, and we already have our new
   // style context.
   RefPtr<nsStyleContext> newContext =
     aRestyleState.StyleSet().ResolveInheritingAnonymousBoxStyle(
-      nsCSSAnonBoxes::mozBlockInsideInlineWrapper, StyleContext());
+      nsCSSAnonBoxes::mozBlockInsideInlineWrapper, ourStyle);
 
   // We're guaranteed that newContext only differs from the old style context on
   // the block in things they might inherit from us.  And changehint processing
   // guarantees walking the continuation and ib-sibling chains, so our existing
   // changehint being in aChangeList is good enough.  So we don't need to touch
   // aChangeList at all here.
 
   while (blockFrame) {
     MOZ_ASSERT(!blockFrame->GetPrevContinuation(),
                "Must be first continuation");
 
     MOZ_ASSERT(blockFrame->StyleContext()->GetPseudo() ==
                nsCSSAnonBoxes::mozBlockInsideInlineWrapper,
                "Unexpected kind of style context");
 
-    // We _could_ just walk along using GetNextContinuationWithSameStyle here,
-    // but it would involve going back to the first continuation every so often,
-    // which is a bit silly when we can just keep track of our first
-    // continuations.
+    // We don't want to just walk through using GetNextContinuationWithSameStyle
+    // here, because we want to set updated style contexts on both our
+    // ib-sibling blocks and inlines.
     for (nsIFrame* cont = blockFrame; cont; cont = cont->GetNextContinuation()) {
       cont->SetStyleContext(newContext);
     }
 
     nsIFrame* nextInline = blockFrame->GetProperty(nsIFrame::IBSplitSibling());
     MOZ_ASSERT(nextInline, "There is always a trailing inline in an IB split");
+
+    for (nsIFrame* cont = nextInline; cont; cont = cont->GetNextContinuation()) {
+      cont->SetStyleContext(ourStyle);
+    }
     blockFrame = nextInline->GetProperty(nsIFrame::IBSplitSibling());
   }
 }
 
 //////////////////////////////////////////////////////////////////////
 
 // nsLineFrame implementation
 
new file mode 100644
--- /dev/null
+++ b/layout/reftests/ib-split/insert-into-split-inline-17-ref.html
@@ -0,0 +1,9 @@
+<!DOCTYPE html>
+<style>
+  span { color: green; }
+</style>
+<span>
+  <div></div>
+  First Second Third
+  <div></div>
+</span>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/ib-split/insert-into-split-inline-17.html
@@ -0,0 +1,20 @@
+<!DOCTYPE html>
+<style>
+  #target { color: red; }
+  #target.green { color: green; }
+</style>
+<span id="target">
+  <div></div>
+  First <span id="next"> Third</span>
+  <div></div>
+</span>
+<script>
+  onload = function() {
+    var s = document.querySelector("#target");
+    window.color = getComputedStyle(s).color;
+    s.className = "green";
+    window.newColor = getComputedStyle(s).color;
+    s.insertBefore(document.createTextNode("Second"),
+                   s.querySelector("#next"));
+  }
+</script>
--- a/layout/reftests/ib-split/reftest.list
+++ b/layout/reftests/ib-split/reftest.list
@@ -59,16 +59,17 @@
 == insert-into-split-inline-13-ref.html insert-into-split-inline-13-noib-ref.html
 == insert-into-split-inline-14.html insert-into-split-inline-14-ref.html
 == insert-into-split-inline-14-ref.html insert-into-split-inline-14-noib-ref.html
 == insert-into-split-inline-15.html insert-into-split-inline-15-ref.html
 == insert-into-split-inline-15-ref.html insert-into-split-inline-15-noib-ref.html
 == insert-into-split-inline-16a.html insert-into-split-inline-16-ref.html
 == insert-into-split-inline-16b.html insert-into-split-inline-16-ref.html
 == insert-into-split-inline-16-ref.html insert-into-split-inline-16-noib-ref.html
+== insert-into-split-inline-17.html insert-into-split-inline-17-ref.html
 == float-inside-inline-between-blocks-1.html float-inside-inline-between-blocks-1-ref.html
 == table-pseudo-in-part3-1.html table-pseudo-in-part3-1-ref.html
 == emptyspan-1.html emptyspan-1-ref.html
 == emptyspan-2.html emptyspan-2-ref.html
 == emptyspan-3.html emptyspan-3-ref.html
 == emptyspan-4.html emptyspan-4-ref.html
 == split-inner-inline-1.html split-inner-inline-1-ref.html
 == split-inner-inline-2.html split-inner-inline-2-ref.html