Bug 1388877. Fix insertions under a ::first-line in stylo. r=heycam
☠☠ backed out by 16f4c4efa148 ☠ ☠
authorBoris Zbarsky <bzbarsky@mit.edu>
Fri, 11 Aug 2017 03:12:44 -0400
changeset 644833 6c23895588581092613c8d6a054bfe54478f1462
parent 644832 7581c0c154deb931d3b1f5c81b1f548aef19672c
child 644834 3a1ec0cb0728a391d2d3d7619050e609a4bdb796
push id73555
push userbmo:emilio+bugs@crisal.io
push dateFri, 11 Aug 2017 10:47:23 +0000
reviewersheycam
bugs1388877
milestone57.0a1
Bug 1388877. Fix insertions under a ::first-line in stylo. r=heycam MozReview-Commit-ID: CDolJpTtGki
layout/base/nsCSSFrameConstructor.cpp
layout/base/nsCSSFrameConstructor.h
layout/reftests/bugs/reftest.list
layout/reftests/first-line/caption-insert-in-first-line-1.html
layout/reftests/first-line/caption-insert-in-first-line-2.html
layout/reftests/first-line/caption-insert-in-first-line-ref.html
layout/reftests/first-line/insertion-in-first-line-1.html
layout/reftests/first-line/insertion-in-first-line-10.html
layout/reftests/first-line/insertion-in-first-line-11.html
layout/reftests/first-line/insertion-in-first-line-12.html
layout/reftests/first-line/insertion-in-first-line-2.html
layout/reftests/first-line/insertion-in-first-line-3.html
layout/reftests/first-line/insertion-in-first-line-4.html
layout/reftests/first-line/insertion-in-first-line-5.html
layout/reftests/first-line/insertion-in-first-line-6.html
layout/reftests/first-line/insertion-in-first-line-7.html
layout/reftests/first-line/insertion-in-first-line-8.html
layout/reftests/first-line/insertion-in-first-line-9.html
layout/reftests/first-line/insertion-in-first-line-ref.html
layout/reftests/first-line/reftest.list
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -7891,21 +7891,33 @@ nsCSSFrameConstructor::ContentAppended(n
     // because processing a single caption can add a whole bunch of things to
     // the frame items due to pseudoframe processing.  So we'd have to pull
     // captions from a list anyway; might as well do that here.
     // XXXbz this is no longer true; we could pull captions directly out of the
     // FrameConstructionItemList now.
     PullOutCaptionFrames(frameItems, captionItems);
   }
 
+  bool dealtWithFirstLine = false;
   if (haveFirstLineStyle && parentFrame == containingBlock) {
     // It's possible that some of the new frames go into a
     // first-line frame. Look at them and see...
     AppendFirstLineFrames(state, containingBlock->GetContent(),
                           containingBlock, frameItems);
+    // That moved things into line frames as needed, reparenting their
+    // styles.  Nothing else needs to be done.
+    dealtWithFirstLine = true;
+  }
+
+  if (!dealtWithFirstLine &&
+      parentFrame->StyleContext()->HasPseudoElementData()) {
+    // parentFrame might be inside a ::first-line frame.  Check whether it is,
+    // and if so fix up our styles.
+    CheckForFirstLineInsertion(parentFrame, frameItems);
+    CheckForFirstLineInsertion(parentFrame, captionItems);
   }
 
   // Notify the parent frame passing it the list of new frames
   // Append the flowed frames to the principal child list; captions
   // need special treatment
   if (captionItems.NotEmpty()) { // append the caption to the table wrapper
     NS_ASSERTION(LayoutFrameType::Table == frameType, "how did that happen?");
     nsContainerFrame* outerTable = parentFrame->GetParent();
@@ -8491,34 +8503,46 @@ nsCSSFrameConstructor::ContentRangeInser
     nsIFrame* appendAfterFrame;
     insertion.mParentFrame =
       ::AdjustAppendParentForAfterContent(this, container,
                                           frameItems.FirstChild()->GetParent(),
                                           aStartChild, &appendAfterFrame);
     prevSibling = ::FindAppendPrevSibling(insertion.mParentFrame, appendAfterFrame);
   }
 
+  bool dealtWithFirstLine = false;
   if (haveFirstLineStyle && insertion.mParentFrame == containingBlock) {
     // It's possible that the new frame goes into a first-line
     // frame. Look at it and see...
     if (isAppend) {
       // Use append logic when appending
       AppendFirstLineFrames(state, containingBlock->GetContent(),
                             containingBlock, frameItems);
+      // That moved things into line frames as needed, reparenting their
+      // styles.  Nothing else needs to be done to handle ::first-line.
+      dealtWithFirstLine = true;
     }
     else {
       // Use more complicated insert logic when inserting
       // XXXbz this method is a no-op, so it's easy for the args being passed
       // here to make no sense without anyone noticing...  If it ever stops
       // being a no-op, vet them carefully!
+      // XXXbz Can this code even get hit?  I'd think/hope not, since any
+      // insert would go into an existing lineframe if we have them..
       InsertFirstLineFrames(state, container, containingBlock, &insertion.mParentFrame,
                             prevSibling, frameItems);
     }
   }
 
+  if (!dealtWithFirstLine &&
+      insertion.mParentFrame->StyleContext()->HasPseudoElementData()) {
+    CheckForFirstLineInsertion(insertion.mParentFrame, frameItems);
+    CheckForFirstLineInsertion(insertion.mParentFrame, captionItems);
+  }
+
   // We might have captions; put them into the caption list of the
   // table wrapper frame.
   if (captionItems.NotEmpty()) {
     NS_ASSERTION(LayoutFrameType::Table == frameType ||
                  LayoutFrameType::TableWrapper == frameType,
                  "parent for caption is not table?");
     // We need to determine where to put the caption items; start with the
     // the parent frame that has already been determined and get the insertion
@@ -11704,16 +11728,66 @@ nsCSSFrameConstructor::InsertFirstLineFr
         }
       }
     }
   }
 
 #endif
 }
 
+void
+nsCSSFrameConstructor::CheckForFirstLineInsertion(nsIFrame* aParentFrame,
+                                                  nsFrameItems& aFrameItems)
+{
+  MOZ_ASSERT(aParentFrame->StyleContext()->HasPseudoElementData(),
+             "Why were we called?");
+
+  if (aFrameItems.IsEmpty()) {
+    // Happens often enough, with the caption stuff.  No need to do the ancestor
+    // walk here.
+    return;
+  }
+
+  class RestyleManager* restyleManager = RestyleManager();
+  if (!restyleManager->IsServo()) {
+    // Gecko's style resolution is frame-based, so already has the right styles
+    // even in the ::first-line case.
+    return;
+  }
+
+  // Check whether there's a ::first-line on the path up from aParentFrame.
+  // Note that we can't stop until we've run out of ancestors with
+  // pseudo-element data, because the first-letter might be somewhere way up the
+  // tree; in particular it might be past our containing block.
+  nsIFrame* ancestor = aParentFrame;
+  while (ancestor) {
+    if (!ancestor->StyleContext()->HasPseudoElementData()) {
+      // We know we won't find a ::first-line now.
+      return;
+    }
+
+    if (!ancestor->IsLineFrame()) {
+      ancestor = ancestor->GetParent();
+      continue;
+    }
+
+    if (!ancestor->StyleContext()->IsPseudoElement()) {
+      // This is a continuation lineframe, not the first line; no need to do
+      // anything to the styles.
+      return;
+    }
+
+    // Fix up the styles of aFrameItems for ::first-line.
+    for (nsIFrame* f : aFrameItems) {
+      restyleManager->ReparentStyleContext(f);
+    }
+    return;
+  }
+}
+
 //----------------------------------------------------------------------
 
 // First-letter support
 
 // Determine how many characters in the text fragment apply to the
 // first letter
 static int32_t
 FirstLetterCount(const nsTextFragment* aFragment)
--- a/layout/base/nsCSSFrameConstructor.h
+++ b/layout/base/nsCSSFrameConstructor.h
@@ -2047,16 +2047,28 @@ private:
   void InsertFirstLineFrames(nsFrameConstructorState& aState,
                              nsIContent*              aContent,
                              nsIFrame*                aBlockFrame,
                              nsContainerFrame**       aParentFrame,
                              nsIFrame*                aPrevSibling,
                              nsFrameItems&            aFrameItems);
 
   /**
+   * When aFrameItems is being inserted into aParentFrame, and aParentFrame has
+   * pseudo-element-affected styles, it's possible that we're inserting under a
+   * ::first-line frame.  In that case, with servo's style system, the styles we
+   * resolved for aFrameItems are wrong (they don't take ::first-line into
+   * account), and we should fix them up, which is what this method does.
+   *
+   * This method does not mutate aFrameItems.
+   */
+  void CheckForFirstLineInsertion(nsIFrame* aParentFrame,
+                                  nsFrameItems& aFrameItems);
+
+  /**
    * Find the right frame to use for aContent when looking for sibling
    * frames for aTargetContent.  If aPrevSibling is true, this
    * will look for last continuations, etc, as necessary.  This calls
    * IsValidSibling as needed; if that returns false it returns null.
    *
    * @param aContent the content to search for frames
    * @param aTargetContent the content we're finding a sibling frame for
    * @param aTargetContentDisplay the CSS display enum for aTargetContent if
--- a/layout/reftests/bugs/reftest.list
+++ b/layout/reftests/bugs/reftest.list
@@ -1378,17 +1378,17 @@ pref(browser.display.focus_ring_width,1)
 == 495385-2d.html 495385-2-ref.html
 == 495385-2e.html 495385-2-ref.html
 pref(dom.use_xbl_scopes_for_remote_xul,true) == 495385-2f.xhtml 495385-2-ref.html
 == 495385-2g.html 495385-2-ref.html
 == 495385-2h.html 495385-2-ref.html
 == 495385-2i.html 495385-2-ref.html
 == 495385-3.html 495385-3-ref.html
 == 495385-4.html 495385-4-ref.html
-fails-if(styloVsGecko||stylo) == 495385-5.html 495385-5-ref.html
+== 495385-5.html 495385-5-ref.html
 == 496032-1.html 496032-1-ref.html
 == 496840-1.html 496840-1-ref.html
 fuzzy-if(skiaContent,1,17000) == 498228-1.xul 498228-1-ref.xul
 == 501037.html 501037-ref.html
 == 501257-1a.html 501257-1-ref.html
 == 501257-1b.html 501257-1-ref.html
 == 501257-1.xhtml 501257-1-ref.xhtml
 == 501627-1.html 501627-1-ref.html
new file mode 100644
--- /dev/null
+++ b/layout/reftests/first-line/caption-insert-in-first-line-1.html
@@ -0,0 +1,20 @@
+<!DOCTYPE html>
+<style>
+  div { color: red; }
+  div::first-line { color: green }
+  #table { display: inline-table; }
+  #caption { display: table-caption; }
+  #tbody { display: table-row-group; }
+</style>
+<div id="x">
+  <span id="table">
+    <span id="tbody">be green</span>
+  </span>
+</div>
+<script>
+  x.offsetWidth;
+  var caption = document.createElement("span");
+  caption.id = "caption";
+  caption.textContent = "This should";
+  table.appendChild(caption);
+</script>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/first-line/caption-insert-in-first-line-2.html
@@ -0,0 +1,20 @@
+<!DOCTYPE html>
+<style>
+  div { color: red; }
+  div::first-line { color: green }
+  #table { display: inline-table; }
+  #caption { display: table-caption; }
+  #tbody { display: table-row-group; }
+</style>
+<div id="x">
+  <span id="table">
+    <span id="tbody">be green</span>
+  </span>
+</div>
+<script>
+  x.offsetWidth;
+  var caption = document.createElement("span");
+  caption.id = "caption";
+  caption.textContent = "This should";
+  table.insertBefore(caption, tbody);
+</script>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/first-line/caption-insert-in-first-line-ref.html
@@ -0,0 +1,13 @@
+<!DOCTYPE html>
+<style>
+  div { color: green; }
+  #table { display: inline-table; }
+  #caption { display: table-caption; }
+  #tbody { display: table-row-group; }
+</style>
+<div>
+  <span id="table">
+    <span id="caption">This should</span>
+    <span id="tbody">be green</span>
+  </span>
+</div>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/first-line/insertion-in-first-line-1.html
@@ -0,0 +1,10 @@
+<!DOCTYPE html>
+<style>
+  div { color: red; }
+  div::first-line { color: green }
+</style>
+<div id="x"></div>
+<script>
+  x.offsetWidth;
+  x.appendChild(document.createTextNode('This should be green'));
+</script>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/first-line/insertion-in-first-line-10.html
@@ -0,0 +1,13 @@
+<!DOCTYPE html>
+<style>
+  div { color: red; }
+  div::first-line { color: green }
+  #y { display: inline-block; }
+</style>
+<div id="x"><span id="y">This should be </span></div>
+<script>
+  x.offsetWidth;
+  var span = document.createElement('span');
+  span.textContent = 'green';
+  y.appendChild(span);
+</script>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/first-line/insertion-in-first-line-11.html
@@ -0,0 +1,11 @@
+<!DOCTYPE html>
+<style>
+  div { color: red; }
+  div::first-line { color: green }
+  #y { display: inline-block; }
+</style>
+<div id="x"><span id="y">green</span></div>
+<script>
+  x.offsetWidth;
+  y.insertBefore(document.createTextNode('This should be '), y.firstChild);
+</script>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/first-line/insertion-in-first-line-12.html
@@ -0,0 +1,13 @@
+<!DOCTYPE html>
+<style>
+  div { color: red; }
+  div::first-line { color: green }
+  #y { display: inline-block; }
+</style>
+<div id="x"><span id="y">green</span></div>
+<script>
+  x.offsetWidth;
+  var span = document.createElement('span');
+  span.textContent = 'This should be ';
+  y.insertBefore(span, y.firstChild);
+</script>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/first-line/insertion-in-first-line-2.html
@@ -0,0 +1,12 @@
+<!DOCTYPE html>
+<style>
+  div { color: red; }
+  div::first-line { color: green }
+</style>
+<div id="x"></div>
+<script>
+  x.offsetWidth;
+  var span = document.createElement('span');
+  span.textContent = 'This should be green';
+  x.appendChild(span);
+</script>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/first-line/insertion-in-first-line-3.html
@@ -0,0 +1,10 @@
+<!DOCTYPE html>
+<style>
+  div { color: red; }
+  div::first-line { color: green }
+</style>
+<div id="x"><span id="y">green</span></div>
+<script>
+  x.offsetWidth;
+  x.insertBefore(document.createTextNode('This should be '), y);
+</script>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/first-line/insertion-in-first-line-4.html
@@ -0,0 +1,12 @@
+<!DOCTYPE html>
+<style>
+  div { color: red; }
+  div::first-line { color: green }
+</style>
+<div id="x"><span id="y">green</span></div>
+<script>
+  x.offsetWidth;
+  var span = document.createElement('span');
+  span.textContent = 'This should be ';
+  x.insertBefore(span, y);
+</script>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/first-line/insertion-in-first-line-5.html
@@ -0,0 +1,10 @@
+<!DOCTYPE html>
+<style>
+  div { color: red; }
+  div::first-line { color: green }
+</style>
+<div id="x"><span id="y">This should be </span></div>
+<script>
+  x.offsetWidth;
+  y.appendChild(document.createTextNode('green'));
+</script>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/first-line/insertion-in-first-line-6.html
@@ -0,0 +1,12 @@
+<!DOCTYPE html>
+<style>
+  div { color: red; }
+  div::first-line { color: green }
+</style>
+<div id="x"><span id="y">This should be </span></div>
+<script>
+  x.offsetWidth;
+  var span = document.createElement('span');
+  span.textContent = 'green';
+  y.appendChild(span);
+</script>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/first-line/insertion-in-first-line-7.html
@@ -0,0 +1,10 @@
+<!DOCTYPE html>
+<style>
+  div { color: red; }
+  div::first-line { color: green }
+</style>
+<div id="x"><span id="y">green</span></div>
+<script>
+  x.offsetWidth;
+  y.insertBefore(document.createTextNode('This should be '), y.firstChild);
+</script>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/first-line/insertion-in-first-line-8.html
@@ -0,0 +1,12 @@
+<!DOCTYPE html>
+<style>
+  div { color: red; }
+  div::first-line { color: green }
+</style>
+<div id="x"><span id="y">green</span></div>
+<script>
+  x.offsetWidth;
+  var span = document.createElement('span');
+  span.textContent = 'This should be ';
+  y.insertBefore(span, y.firstChild);
+</script>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/first-line/insertion-in-first-line-9.html
@@ -0,0 +1,11 @@
+<!DOCTYPE html>
+<style>
+  div { color: red; }
+  div::first-line { color: green }
+  #y { display: inline-block; }
+</style>
+<div id="x"><span id="y">This should be </span></div>
+<script>
+  x.offsetWidth;
+  y.appendChild(document.createTextNode('green'));
+</script>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/first-line/insertion-in-first-line-ref.html
@@ -0,0 +1,5 @@
+<!DOCTYPE html>
+<style>
+  div { color: green; }
+</style>
+<div>This should be green</div>
--- a/layout/reftests/first-line/reftest.list
+++ b/layout/reftests/first-line/reftest.list
@@ -34,8 +34,23 @@ load stress-10.html # crash test
 
 == restyle-inside-first-line.html restyle-inside-first-line-ref.html
 == font-styles.html font-styles-ref.html
 fuzzy-if(OSX==1010,1,2) == font-styles-nooverflow.html font-styles-ref.html
 
 fails-if(!stylo) == ib-split-1.html ib-split-1-ref.html
 
 == first-line-in-columnset-1.html first-line-in-columnset-1-ref.html
+
+== insertion-in-first-line-1.html insertion-in-first-line-ref.html
+== insertion-in-first-line-2.html insertion-in-first-line-ref.html
+== insertion-in-first-line-3.html insertion-in-first-line-ref.html
+== insertion-in-first-line-4.html insertion-in-first-line-ref.html
+== insertion-in-first-line-5.html insertion-in-first-line-ref.html
+== insertion-in-first-line-6.html insertion-in-first-line-ref.html
+== insertion-in-first-line-7.html insertion-in-first-line-ref.html
+== insertion-in-first-line-8.html insertion-in-first-line-ref.html
+== insertion-in-first-line-9.html insertion-in-first-line-ref.html
+== insertion-in-first-line-10.html insertion-in-first-line-ref.html
+== insertion-in-first-line-11.html insertion-in-first-line-ref.html
+== insertion-in-first-line-12.html insertion-in-first-line-ref.html
+== caption-insert-in-first-line-1.html caption-insert-in-first-line-ref.html
+== caption-insert-in-first-line-2.html caption-insert-in-first-line-ref.html