Bug 1168921 - Stop cloning block direction margins for box-decoration-break:clone. r=mats
authorTing-Yu Lin <tlin@mozilla.com>
Thu, 10 Oct 2019 13:38:07 +0000
changeset 497153 e9783a644016aa9b317887076618425586730d73
parent 497152 be107549b845d2699572cabb5868dacec6514e1c
child 497154 f6b08fc17ebc72b40b29d1c5939740f0245f3fd8
push id36677
push userrmaries@mozilla.com
push dateThu, 10 Oct 2019 21:40:19 +0000
treeherdermozilla-central@e9783a644016 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmats
bugs1168921, 1586470, 1025669
milestone71.0a1
first release with
nightly linux32
e9783a644016 / 71.0a1 / 20191010214019 / files
nightly linux64
e9783a644016 / 71.0a1 / 20191010214019 / files
nightly mac
e9783a644016 / 71.0a1 / 20191010214019 / files
nightly win32
e9783a644016 / 71.0a1 / 20191010214019 / files
nightly win64
e9783a644016 / 71.0a1 / 20191010214019 / files
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
releases
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1168921 - Stop cloning block direction margins for box-decoration-break:clone. r=mats This can also fix bug 1586470. This change basically reverts Bug 1025669 Part 1. https://hg.mozilla.org/mozilla-central/rev/ae2fd5b2defb0df1bd30521f4793de6757d1e98b In box-decoration-break-block-margin.html, the `height` in `.inner` is changed to 79px so that 79px plus 7px margin top and 1px margin end, total 87px, can be divided by 3 (columns). The modification to reference file reflects what we currently rendered. Co-authored-by: Mats Palmgren <mats@mozilla.com> Differential Revision: https://phabricator.services.mozilla.com/D48484
layout/generic/BlockReflowInput.cpp
layout/generic/crashtests/1586470.html
layout/generic/crashtests/crashtests.list
layout/generic/nsBlockFrame.cpp
layout/generic/nsBlockFrame.h
layout/reftests/css-break/box-decoration-break-block-margin-ref.html
layout/reftests/css-break/box-decoration-break-block-margin.html
--- a/layout/generic/BlockReflowInput.cpp
+++ b/layout/generic/BlockReflowInput.cpp
@@ -72,36 +72,21 @@ BlockReflowInput::BlockReflowInput(const
   mContainerSize.width += mBorderPadding.LeftRight(wm);
 
   // For now at least, we don't do that fix-up for mContainerHeight.
   // It's only used in nsBidiUtils::ReorderFrames for vertical rtl
   // writing modes, which aren't fully supported for the time being.
   mContainerSize.height =
       aReflowInput.ComputedHeight() + mBorderPadding.TopBottom(wm);
 
-  // A column-content after the column-span split has a previous non-fluid
-  // continuation, and has its block-start side skipped. However, if this
-  // column-content is the first column in a ColumnSet, we want its first child
-  // to have a chance to apply its block-start margin.
-  //
-  // XXX: We might want to separate two ideas: apply this block's own
-  // block-start margin versus allow this block's first child apply its
-  // block-start margin.
-  const bool isFirstColumnContentInMulticolLine =
-      StaticPrefs::layout_css_column_span_enabled() &&
-      aFrame->Style()->GetPseudoType() == PseudoStyleType::columnContent &&
-      !aFrame->GetPrevInFlow();
-  if ((aBStartMarginRoot &&
-       (!logicalSkipSides.BStart() || isFirstColumnContentInMulticolLine)) ||
-      0 != mBorderPadding.BStart(wm)) {
+  if (aBStartMarginRoot || 0 != mBorderPadding.BStart(wm)) {
     mFlags.mIsBStartMarginRoot = true;
     mFlags.mShouldApplyBStartMargin = true;
   }
-  if ((aBEndMarginRoot && !logicalSkipSides.BEnd()) ||
-      0 != mBorderPadding.BEnd(wm)) {
+  if (aBEndMarginRoot || 0 != mBorderPadding.BEnd(wm)) {
     mFlags.mIsBEndMarginRoot = true;
   }
   if (aBlockNeedsFloatManager) {
     mFlags.mBlockNeedsFloatManager = true;
   }
 
   // We need to check mInsideLineClamp here since we are here before the block
   // has been reflowed, and CanHaveOverflowMarkers() relies on the block's
@@ -188,50 +173,31 @@ void BlockReflowInput::ComputeReplacedBl
     iEndOffset =
         std::max(iEndFloatIOffset, frameMargin.IEnd(wm)) - frameMargin.IEnd(wm);
     iEndOffset = std::max(iEndOffset, 0);  // in case of negative margin
   }
   aIStartResult = iStartOffset;
   aIEndResult = iEndOffset;
 }
 
-static nscoord GetBEndMarginClone(nsIFrame* aFrame,
-                                  gfxContext* aRenderingContext,
-                                  const LogicalRect& aContentArea,
-                                  WritingMode aWritingMode) {
-  if (aFrame->StyleBorder()->mBoxDecorationBreak ==
-      StyleBoxDecorationBreak::Clone) {
-    SizeComputationInput os(aFrame, aRenderingContext, aWritingMode,
-                            aContentArea.ISize(aWritingMode));
-    return os.ComputedLogicalMargin()
-        .ConvertTo(aWritingMode, aFrame->GetWritingMode())
-        .BEnd(aWritingMode);
-  }
-  return 0;
-}
-
 // Compute the amount of available space for reflowing a block frame
 // at the current block-direction coordinate. This method assumes that
 // GetFloatAvailableSpace has already been called.
 void BlockReflowInput::ComputeBlockAvailSpace(
     nsIFrame* aFrame, const nsFlowAreaRect& aFloatAvailableSpace,
     bool aBlockAvoidsFloats, LogicalRect& aResult) {
 #ifdef REALLY_NOISY_REFLOW
   printf("CBAS frame=%p has floats %d\n", aFrame,
          aFloatAvailableSpace.HasFloats());
 #endif
   WritingMode wm = mReflowInput.GetWritingMode();
   const nscoord availBSize = mReflowInput.AvailableBSize();
   aResult.BStart(wm) = mBCoord;
-  aResult.BSize(wm) =
-      availBSize == NS_UNCONSTRAINEDSIZE
-          ? NS_UNCONSTRAINEDSIZE
-          : availBSize - mBCoord -
-                GetBEndMarginClone(aFrame, mReflowInput.mRenderingContext,
-                                   mContentArea, wm);
+  aResult.BSize(wm) = availBSize == NS_UNCONSTRAINEDSIZE ? NS_UNCONSTRAINEDSIZE
+                                                         : availBSize - mBCoord;
   // mBCoord might be greater than mBEndEdge if the block's top margin pushes
   // it off the page/column. Negative available block-size can confuse other
   // code and is nonsense in principle.
 
   // XXX Do we really want this condition to be this restrictive (i.e.,
   // more restrictive than it used to be)?  The |else| here is allowed
   // by the CSS spec, but only out of desperation given implementations,
   // and the behavior it leads to is quite undesirable (it can cause
new file mode 100644
--- /dev/null
+++ b/layout/generic/crashtests/1586470.html
@@ -0,0 +1,9 @@
+<style>
+.a {
+  max-height: 0vw;
+  writing-mode: vertical-rl;
+  box-decoration-break: clone
+}
+</style>
+<dl style="column-width:3em">
+<dd class="a" dir="RTL">A</dd>
--- a/layout/generic/crashtests/crashtests.list
+++ b/layout/generic/crashtests/crashtests.list
@@ -748,8 +748,9 @@ load 1569639.html
 pref(layout.css.column-span.enabled,true) load 1571239.html
 load 1571460.html
 pref(layout.css.column-span.enabled,true) load 1571598.html
 pref(layout.css.column-span.enabled,true) load 1571897.html
 pref(layout.css.column-span.enabled,true) load 1572901.html
 pref(layout.css.column-span.enabled,true) load 1573216.html
 load 1574552.html
 pref(layout.css.column-span.enabled,true) load 1574993.html
+load 1586470.html
--- a/layout/generic/nsBlockFrame.cpp
+++ b/layout/generic/nsBlockFrame.cpp
@@ -3256,26 +3256,23 @@ bool nsBlockFrame::IsEmpty() {
        line != line_end; ++line) {
     if (!line->IsEmpty()) return false;
   }
 
   return true;
 }
 
 bool nsBlockFrame::ShouldApplyBStartMargin(BlockReflowInput& aState,
-                                           nsLineBox* aLine,
-                                           nsIFrame* aChildFrame) {
+                                           nsLineBox* aLine) {
   if (aState.mFlags.mShouldApplyBStartMargin) {
     // Apply short-circuit check to avoid searching the line list
     return true;
   }
 
-  if (!aState.IsAdjacentWithTop() ||
-      aChildFrame->StyleBorder()->mBoxDecorationBreak ==
-          StyleBoxDecorationBreak::Clone) {
+  if (!aState.IsAdjacentWithTop()) {
     // If we aren't at the start block-coordinate then something of non-zero
     // height must have been placed. Therefore the childs block-start margin
     // applies.
     aState.mFlags.mShouldApplyBStartMargin = true;
     return true;
   }
 
   // Determine if this line is "essentially" the first line
@@ -3324,22 +3321,19 @@ void nsBlockFrame::ReflowBlockFrame(Bloc
     aState.mFloatBreakType = StyleClear::None;
   }
 
   // Clear past floats before the block if the clear style is not none
   aLine->SetBreakTypeBefore(breakType);
 
   // See if we should apply the block-start margin. If the block frame being
   // reflowed is a continuation, then we don't apply its block-start margin
-  // because it's not significant unless it has 'box-decoration-break:clone'.
-  // Otherwise, dig deeper.
-  bool applyBStartMargin = (frame->StyleBorder()->mBoxDecorationBreak ==
-                                StyleBoxDecorationBreak::Clone ||
-                            !frame->GetPrevContinuation()) &&
-                           ShouldApplyBStartMargin(aState, aLine, frame);
+  // because it's not significant. Otherwise, dig deeper.
+  bool applyBStartMargin =
+      !frame->GetPrevContinuation() && ShouldApplyBStartMargin(aState, aLine);
   if (applyBStartMargin) {
     // The HasClearance setting is only valid if ShouldApplyBStartMargin
     // returned false (in which case the block-start margin-root set our
     // clearance flag). Otherwise clear it now. We'll set it later on
     // ourselves if necessary.
     aLine->ClearHasClearance();
   }
   bool treatWithClearance = aLine->HasClearance();
@@ -3979,17 +3973,17 @@ void nsBlockFrame::ReflowInlineFrames(Bl
                                       LineIterator aLine,
                                       bool* aKeepReflowGoing) {
   *aKeepReflowGoing = true;
 
   aLine->SetLineIsImpactedByFloat(false);
 
   // Setup initial coordinate system for reflowing the inline frames
   // into. Apply a previous block frame's block-end margin first.
-  if (ShouldApplyBStartMargin(aState, aLine, aLine->mFirstChild)) {
+  if (ShouldApplyBStartMargin(aState, aLine)) {
     aState.mBCoord += aState.mPrevBEndMargin.get();
   }
   nsFlowAreaRect floatAvailableSpace = aState.GetFloatAvailableSpace();
 
   LineReflowStatus lineReflowStatus;
   do {
     nscoord availableSpaceBSize = 0;
     aState.mLineBSize.reset();
@@ -7362,33 +7356,32 @@ void nsBlockFrame::CheckFloats(BlockRefl
     // used for its XMost and YMost, not to place new floats and
     // lines.
     aState.FloatManager()->RemoveTrailingRegions(oofs->FirstChild());
   }
 }
 
 void nsBlockFrame::IsMarginRoot(bool* aBStartMarginRoot,
                                 bool* aBEndMarginRoot) {
+  nsIFrame* parent = GetParent();
   if (!(GetStateBits() & NS_BLOCK_MARGIN_ROOT)) {
-    nsIFrame* parent = GetParent();
     if (!parent || parent->IsFloatContainingBlock()) {
       *aBStartMarginRoot = false;
       *aBEndMarginRoot = false;
       return;
     }
-    // Bug 1499281: The following margin root calculation is for legacy
-    // multi-column container. Remove it after removing the column-span
-    // preference.
-    if (parent->IsColumnSetFrame()) {
-      MOZ_ASSERT(!StaticPrefs::layout_css_column_span_enabled(),
-                 "Column contents always have BFC set!");
-      *aBStartMarginRoot = GetPrevInFlow() == nullptr;
-      *aBEndMarginRoot = GetNextInFlow() == nullptr;
-      return;
-    }
+  }
+
+  if (parent && parent->IsColumnSetFrame()) {
+    // The first column is a start margin root and the last column is an end
+    // margin root.  (If the column-set is split by a column-span:all box then
+    // the first and last column in each column-set fragment are margin roots.)
+    *aBStartMarginRoot = GetPrevInFlow() == nullptr;
+    *aBEndMarginRoot = GetNextInFlow() == nullptr;
+    return;
   }
 
   *aBStartMarginRoot = true;
   *aBEndMarginRoot = true;
 }
 
 /* static */
 bool nsBlockFrame::BlockNeedsFloatManager(nsIFrame* aBlock) {
--- a/layout/generic/nsBlockFrame.h
+++ b/layout/generic/nsBlockFrame.h
@@ -725,18 +725,17 @@ class nsBlockFrame : public nsContainerF
   bool IsLastLine(BlockReflowInput& aState, LineIterator aLine);
 
   void DeleteLine(BlockReflowInput& aState, nsLineList::iterator aLine,
                   nsLineList::iterator aLineEnd);
 
   //----------------------------------------
   // Methods for individual frame reflow
 
-  bool ShouldApplyBStartMargin(BlockReflowInput& aState, nsLineBox* aLine,
-                               nsIFrame* aChildFrame);
+  bool ShouldApplyBStartMargin(BlockReflowInput& aState, nsLineBox* aLine);
 
   void ReflowBlockFrame(BlockReflowInput& aState, LineIterator aLine,
                         bool* aKeepGoing);
 
   void ReflowInlineFrames(BlockReflowInput& aState, LineIterator aLine,
                           bool* aKeepLineGoing);
 
   void DoReflowInlineFrames(
--- a/layout/reftests/css-break/box-decoration-break-block-margin-ref.html
+++ b/layout/reftests/css-break/box-decoration-break-block-margin-ref.html
@@ -10,22 +10,24 @@
   <meta charset="utf-8">
   <style type="text/css">
     html,body {
       color:black; background-color:white; font-size:16px; padding:10px; margin:0;
     }
 
     .inner {
       border: 5px dashed blue;
-      height: 26px;
       width: 40px;
       background: lime;
       background-clip: padding-box;
       margin: 0px 3px 15px 1px;
       position: relative;
+    }
+
+    .inner.first {
       top: 7px;
     }
 
     .columns {
       columns: 3;
       width: 250px;
       height: 50px;
       background: grey;
@@ -37,31 +39,27 @@
       width: 250px;
       background: grey;
     }
 
     .unbalanced {
       column-fill: auto;
     }
 
-    .unbalanced .inner { height: 28px; }
-    .x .inner { height: 28px; }
-    .x.unbalanced .inner { height: 28px; }
-
     #t2 .inner { border-style:none; height: 38px; }
   </style>
 </head>
 <body>
 
-<div class="outer"><div class="columns"><div class="inner"></div><div class="inner"></div><div class="inner"></div></div></div>
-<div class="outer"><div class="columns unbalanced"><div class="inner"></div><div class="inner"></div><div class="inner" style="height:22px"></div></div></div>
-<div class="outer"><div class="columns x"><div class="p inner"></div><div class="inner"></div><div class="inner" style="height:31px"></div></div></div>
-<div class="outer"><div class="columns x unbalanced"><div class="p inner"></div><div class="inner"></div><div class="inner"></div></div></div>
+<div class="outer"><div class="columns"><div class="inner first" style="height:22px;" ></div><div class="inner" style="height:29px;"></div><div class="inner" style="height:28px;"></div></div></div>
+<div class="outer"><div class="columns unbalanced"><div class="inner first" style="height:33px;"></div><div class="inner" style="height:40px;"></div><div class="inner" style="height:6px"></div></div></div>
+<div class="outer"><div class="columns"><div class="inner first" style="height:25px;"></div><div class="inner" style="height:32px"></div><div class="inner" style="height:30px"></div></div></div>
+<div class="outer"><div class="columns unbalanced"><div class="inner first" style="height:33px;"></div><div class="inner" style="height:40px;"></div><div class="inner" style="height:11px"></div></div></div>
 
 <div id="t2">
-<div class="outer"><div class="columns unbalanced"><div class="inner"></div><div class="inner"></div><div class="inner" style="height:2px"></div></div></div>
-<div class="outer"><div class="columns unbalanced"><div style="height:4px"></div><div class="inner" style="height:34px"></div><div class="inner"></div><div class="inner" style="height:6px"></div></div></div>
-<div class="outer"><div class="columns unbalanced x"><div class="p inner"></div><div class="inner"></div><div class="inner" style="height:2px"></div></div></div>
-<div class="outer"><div class="columns unbalanced"><div style="height:4px"></div><div class="inner" style="height:34px"></div><div class="inner"></div><div class="inner" style="height:6px"></div></div></div>
+<div class="outer"><div class="columns unbalanced"><div class="inner first" style="height:43px"></div><div class="inner" style="height:36px"></div></div></div>
+<div class="outer"><div class="columns unbalanced"><div style="height:4px"></div><div class="inner first" style="height:39px"></div><div class="inner" style="height:40px"></div></div></div>
+<div class="outer"><div class="columns unbalanced"><div class="inner first" style="height:43px"></div><div class="inner" style="height:36px"></div></div></div>
+<div class="outer"><div class="columns unbalanced"><div style="height:4px"></div><div class="inner first" style="height:39px"></div><div class="inner" style="height:40px"></div></div></div>
 </div>
 
 </body>
 </html>
--- a/layout/reftests/css-break/box-decoration-break-block-margin.html
+++ b/layout/reftests/css-break/box-decoration-break-block-margin.html
@@ -14,17 +14,17 @@
     html,body {
       color:black; background-color:white; font-size:16px; padding:10px; margin:0;
     }
 
     .inner {
       box-decoration-break: clone;
       border: 5px dashed blue;
       margin: 7px 3px 5px 1px;
-      height: 78px;
+      height: 79px;
       width: 40px;
       background: lime;
       background-clip: content-box;
     }
 
     .columns {
       columns: 3;
       width: 250px;