Bug 1764437 - Check for prev/next sibling when propagating start/end page values for frames r=dholbert
authorEmily McDonough <emcdonough@mozilla.com>
Tue, 20 Sep 2022 21:42:34 +0000
changeset 635925 c7d1db6d321f0e46cac8a53e7a40e53ff176a1de
parent 635924 cf7d2eb3996d2b1dc13e8d02001fe9f1bec2ce9a
child 635926 68b253486ec32009db107d63fe26d243ed907891
push id40252
push userimoraru@mozilla.com
push dateWed, 21 Sep 2022 03:56:08 +0000
treeherdermozilla-central@fb7ca98a6881 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdholbert
bugs1764437
milestone107.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1764437 - Check for prev/next sibling when propagating start/end page values for frames r=dholbert This also properly handles placeholder frames, and ensures that when checking next/prev sibling we ignore placeholder frames. To properly test this for multiple levels of page value propagation, we also need to use FirstInFlow to get page values when checking for breaks in block frames. Differential Revision: https://phabricator.services.mozilla.com/D157175
layout/base/nsCSSFrameConstructor.cpp
layout/generic/nsBlockFrame.cpp
layout/reftests/css-page/page-name-propagated-001-ref.html
layout/reftests/css-page/page-name-propagated-001.html
layout/reftests/css-page/page-name-propagated-002-ref.html
layout/reftests/css-page/page-name-propagated-002.html
layout/reftests/css-page/page-name-propagated-003-ref.html
layout/reftests/css-page/page-name-propagated-003.html
layout/reftests/css-page/page-name-propagated-004-ref.html
layout/reftests/css-page/page-name-propagated-004.html
layout/reftests/css-page/page-name-propagated-005-ref.html
layout/reftests/css-page/page-name-propagated-005.html
layout/reftests/css-page/page-name-propagated-006-ref.html
layout/reftests/css-page/page-name-propagated-006.html
layout/reftests/css-page/reftest.list
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -9371,16 +9371,36 @@ static void VerifyGridFlexContainerChild
       prevChildWasAnonItem = true;
     } else {
       prevChildWasAnonItem = false;
     }
   }
 #endif
 }
 
+static bool FrameHasOnlyPlaceholderPrevSiblings(const nsIFrame* aFrame) {
+  // Check for prev siblings, ignoring placeholder frames.
+  MOZ_ASSERT(aFrame, "frame must not be null");
+  const nsIFrame* prevSibling = aFrame;
+  do {
+    prevSibling = prevSibling->GetPrevSibling();
+  } while (prevSibling && prevSibling->IsPlaceholderFrame());
+  return !prevSibling;
+}
+
+static bool FrameHasOnlyPlaceholderNextSiblings(const nsIFrame* aFrame) {
+  // Check for next siblings, ignoring placeholder frames.
+  MOZ_ASSERT(aFrame, "frame must not be null");
+  const nsIFrame* nextSibling = aFrame;
+  do {
+    nextSibling = nextSibling->GetNextSibling();
+  } while (nextSibling && nextSibling->IsPlaceholderFrame());
+  return !nextSibling;
+}
+
 inline void nsCSSFrameConstructor::ConstructFramesFromItemList(
     nsFrameConstructorState& aState, FrameConstructionItemList& aItems,
     nsContainerFrame* aParentFrame, bool aParentIsWrapperAnonBox,
     nsFrameList& aFrameList) {
 #ifdef DEBUG
   if (aParentFrame->StyleContent()->mContent.IsNone() &&
       StaticPrefs::layout_css_element_content_none_enabled()) {
     for (FCItemIterator iter(aItems); !iter.IsDone(); iter.Next()) {
@@ -9457,16 +9477,19 @@ inline void nsCSSFrameConstructor::Const
       StaticPrefs::layout_css_named_pages_enabled() &&
       aParentFrame->IsBlockFrameOrSubclass()) {
     // Set the start/end page values while iterating the frame list, to walk
     // up the frame tree only once after iterating the frame list.
     // This also avoids extra property lookups on these frames.
     const nsAtom* startPageValue = nullptr;
     const nsAtom* endPageValue = nullptr;
     for (nsIFrame* f : aFrameList) {
+      if (f->IsPlaceholderFrame()) {
+        continue;
+      }
       // Resolve auto against the parent frame's used page name, which has been
       // determined and set on aState.mAutoPageNameValue. If this item is not
       // block-level then we use the value that auto resolves to.
       //
       // This is to achieve the propagation behavior described in the spec:
       //
       // "A start page value and end page value is determined for each box as
       //  the value (if any) propagated from its first or last child box
@@ -9504,37 +9527,56 @@ inline void nsCSSFrameConstructor::Const
       if (!startPageValue) {
         startPageValue = pageValues->mStartPageValue;
       }
       endPageValue = pageValues->mEndPageValue;
     }
     MOZ_ASSERT(!startPageValue == !endPageValue,
                "Should have set both or neither page values");
     if (startPageValue) {
-      // TODO: This is slightly incorrect! See Bug 1764437
-      // We should be waiting all of our descendent frames to be constructed.
-      //
-      // Alternatively, we could propagate this back up the frame tree after
-      // constructing this frame's first child, inspecting the parent frames
-      // and rewriting their first child page-name.
-      for (nsContainerFrame* frame = aParentFrame;
-           frame && frame->IsBlockFrameOrSubclass();
-           frame = frame->GetParent()) {
-        nsIFrame::PageValues* const parentPageValues =
-            frame->GetProperty(nsIFrame::PageValuesProperty());
-        // Propagate the start page value back up the frame tree.
-        // If the frame already has mStartPageValue set, then we are not a
-        // descendant of the frame's first child.
-        if (!parentPageValues) {
+      // Walk up the frame tree from our parent frame, propagating start and
+      // end page values.
+      // As we go, if we find that, for a frame, we are not contributing one of
+      // the start/end page values, then our subtree will not contribute this
+      // value from that frame onward.
+      // Stop iterating when we are not contributing either start or end
+      // values, when we hit the root frame (no parent), or when we find a
+      // frame that is not a block frame.
+      for (nsContainerFrame* ancestorFrame = aParentFrame;
+           (startPageValue || endPageValue) && ancestorFrame &&
+           ancestorFrame->IsBlockFrameOrSubclass();
+           ancestorFrame = ancestorFrame->GetParent()) {
+        MOZ_ASSERT(!ancestorFrame->GetPrevInFlow(),
+                   "Should not have fragmentation yet");
+
+        nsIFrame::PageValues* const ancestorPageValues =
+            ancestorFrame->GetProperty(nsIFrame::PageValuesProperty());
+
+        if (!ancestorPageValues) {
           break;
         }
-        if (!parentPageValues->mStartPageValue) {
-          parentPageValues->mStartPageValue = startPageValue;
+
+        // Set start/end values if we are still contributing those values.
+        // Once we stop contributing start/end values, we know there is a
+        // sibling subtree that contributed that value to our shared parent
+        // instead of our starting frame's subtree. This means once
+        // startPageValue/endPageValue becomes null, it should stay null and
+        // we no longer need to check for siblings in that direction.
+        if (startPageValue) {
+          ancestorPageValues->mStartPageValue = startPageValue;
+          if (!FrameHasOnlyPlaceholderPrevSiblings(ancestorFrame)) {
+            startPageValue = nullptr;
+          }
         }
-        parentPageValues->mEndPageValue = endPageValue;
+        if (endPageValue) {
+          ancestorPageValues->mEndPageValue = endPageValue;
+          if (!FrameHasOnlyPlaceholderNextSiblings(ancestorFrame)) {
+            endPageValue = nullptr;
+          }
+        }
       }
     }
   }
 
   if (aParentIsWrapperAnonBox) {
     for (nsIFrame* f : aFrameList) {
       f->SetParentIsWrapperAnonBox();
     }
--- a/layout/generic/nsBlockFrame.cpp
+++ b/layout/generic/nsBlockFrame.cpp
@@ -2819,19 +2819,21 @@ void nsBlockFrame::ReflowDirtyLines(Bloc
     // current page are zero-height. The latter may not be per-spec, but is
     // compatible with Chrome's implementation of named pages.
     bool shouldBreakForPageName = false;
     if (canBreakForPageNames && (!aState.mReflowInput.mFlags.mIsTopOfPage ||
                                  !aState.IsAdjacentWithBStart())) {
       const nsIFrame* const frame = line->mFirstChild;
       if (const nsIFrame* prevFrame = frame->GetPrevSibling()) {
         if (const nsIFrame::PageValues* const pageValues =
-                frame->GetProperty(nsIFrame::PageValuesProperty())) {
+                frame->FirstInFlow()->GetProperty(
+                    nsIFrame::PageValuesProperty())) {
           const nsIFrame::PageValues* const prevPageValues =
-              prevFrame->GetProperty(nsIFrame::PageValuesProperty());
+              prevFrame->FirstInFlow()->GetProperty(
+                  nsIFrame::PageValuesProperty());
           if (prevPageValues &&
               prevPageValues->mEndPageValue != pageValues->mStartPageValue) {
             shouldBreakForPageName = true;
             line->MarkDirty();
           }
         }
       }
     }
new file mode 100644
--- /dev/null
+++ b/layout/reftests/css-page/page-name-propagated-001-ref.html
@@ -0,0 +1,11 @@
+<!DOCTYPE html>
+<html class="reftest-paged">
+  <body>
+    <div>a</div>
+    <div>
+      <div>
+        <div>b</div>
+      </div>
+    </div>
+  </body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/css-page/page-name-propagated-001.html
@@ -0,0 +1,11 @@
+<!DOCTYPE html>
+<html class="reftest-paged">
+  <body>
+    <div style="page: a">a</div>
+    <div style="page: b">
+      <div style="page: c">
+        <div style="page: a">b</div>
+      </div>
+    </div>
+  </body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/css-page/page-name-propagated-002-ref.html
@@ -0,0 +1,13 @@
+<!DOCTYPE html>
+<html class="reftest-paged">
+  <body>
+    <div>a</div>
+    <div>
+      <div>
+        <div>b</div>
+      </div>
+      <div style="break-before: page">c</div>
+    </div>
+    <div style="break-before: page">d</div>
+  </body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/css-page/page-name-propagated-002.html
@@ -0,0 +1,13 @@
+<!DOCTYPE html>
+<html class="reftest-paged">
+  <body>
+    <div style="page: a">a</div>
+    <div style="page: b">
+      <div style="page: e">
+        <div style="page: a">b</div>
+      </div>
+      <div style="page: c">c</div>
+    </div>
+    <div style="page: d">d</div>
+  </body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/css-page/page-name-propagated-003-ref.html
@@ -0,0 +1,12 @@
+<!DOCTYPE html>
+<html class="reftest-paged">
+  <body>
+    <div>a</div>
+    <div>
+      <div>
+        <div style="position: absolute; left: 100px">b</div>
+        <div>c</div>
+      </div>
+    </div>
+  </body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/css-page/page-name-propagated-003.html
@@ -0,0 +1,12 @@
+<!DOCTYPE html>
+<html class="reftest-paged">
+  <body>
+    <div style="page: a">a</div>
+    <div style="page: b">
+      <div style="page: c">
+        <div style="position: absolute; left: 100px">b</div>
+        <div style="page: a">c</div>
+      </div>
+    </div>
+  </body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/css-page/page-name-propagated-004-ref.html
@@ -0,0 +1,12 @@
+<!DOCTYPE html>
+<html class="reftest-paged">
+  <body>
+    <div style="break-after: page">a</div>
+    <div>
+      <div>
+        <div style="position: absolute; left: 100px">b</div>
+        <div>c</div>
+      </div>
+    </div>
+  </body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/css-page/page-name-propagated-004.html
@@ -0,0 +1,12 @@
+<!DOCTYPE html>
+<html class="reftest-paged">
+  <body>
+    <div style="page: a">a</div>
+    <div style="page: b">
+      <div style="page: c">
+        <div style="position: absolute; left: 100px">b</div>
+        <div style="page: x">c</div>
+      </div>
+    </div>
+  </body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/css-page/page-name-propagated-005-ref.html
@@ -0,0 +1,12 @@
+<!DOCTYPE html>
+<html class="reftest-paged">
+  <body>
+    <div>
+      <div>
+        <div>a</div>
+        <div style="position: absolute; left: 100px">b</div>
+      </div>
+    </div>
+    <div>c</div>
+  </body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/css-page/page-name-propagated-005.html
@@ -0,0 +1,12 @@
+<!DOCTYPE html>
+<html class="reftest-paged">
+  <body>
+    <div style="page: b">
+      <div style="page: c">
+        <div style="page: a">a</div>
+        <div style="position: absolute; left: 100px">b</div>
+      </div>
+    </div>
+    <div style="page: a">c</div>
+  </body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/css-page/page-name-propagated-006-ref.html
@@ -0,0 +1,12 @@
+<!DOCTYPE html>
+<html class="reftest-paged">
+  <body>
+    <div>
+      <div>
+        <div>a</div>
+        <div style="position: absolute; left: 100px">b</div>
+      </div>
+    </div>
+    <div style="break-before: page">c</div>
+  </body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/css-page/page-name-propagated-006.html
@@ -0,0 +1,12 @@
+<!DOCTYPE html>
+<html class="reftest-paged">
+  <body>
+    <div style="page: b">
+      <div style="page: c">
+        <div style="page: x">a</div>
+        <div style="position: absolute; left: 100px">b</div>
+      </div>
+    </div>
+    <div style="page: a">c</div>
+  </body>
+</html>
--- a/layout/reftests/css-page/reftest.list
+++ b/layout/reftests/css-page/reftest.list
@@ -23,16 +23,22 @@ defaults pref(layout.css.named-pages.ena
 == page-name-siblings-002.html page-name-siblings-ref.html
 == page-name-siblings-003.html page-name-siblings-ref.html
 == page-name-siblings-004.html page-name-siblings-ref.html
 == page-name-siblings-005.html page-name-siblings-ref.html
 == page-name-orthogonal-writing-001.html page-name-orthogonal-writing-001-ref.html
 == page-name-orthogonal-writing-002.html page-name-orthogonal-writing-002-ref.html
 == page-name-orthogonal-writing-003.html page-name-orthogonal-writing-003-ref.html
 == page-name-orthogonal-writing-004.html page-name-orthogonal-writing-004-ref.html
+== page-name-propagated-001.html page-name-propagated-001-ref.html
+== page-name-propagated-002.html page-name-propagated-002-ref.html
+== page-name-propagated-003.html page-name-propagated-003-ref.html
+== page-name-propagated-004.html page-name-propagated-004-ref.html
+== page-name-propagated-005.html page-name-propagated-005-ref.html
+== page-name-propagated-006.html page-name-propagated-006-ref.html
 # Auto-generated test cases
 == page-name-two-page-001.html page-name-two-page-ref.html
 == page-name-two-page-002.html page-name-two-page-ref.html
 == page-name-two-page-003.html page-name-two-page-ref.html
 == page-name-two-page-004.html page-name-two-page-ref.html
 == page-name-two-page-005.html page-name-two-page-ref.html
 == page-name-two-page-006.html page-name-two-page-ref.html
 == page-name-two-page-007.html page-name-two-page-ref.html