Bug 1351359 - Make nsGridContainerFrame call ReflowInput::SetBResize(true) because of auto-block-size swapping between measuring reflows and regular reflows. r=mats, a=gchang
authorL. David Baron <dbaron@dbaron.org>
Thu, 30 Mar 2017 22:56:14 -0400
changeset 395892 e984689093ed389415affc819776a6eede5d44d2
parent 395891 659b949948787450acaf0466acea822f2cbfc169
child 395893 bdce123aa2c2aeb4ccd3848a961fbfd97b6d0ec7
push id1468
push userasasaki@mozilla.com
push dateMon, 05 Jun 2017 19:31:07 +0000
treeherdermozilla-release@0641fc6ee9d1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmats, gchang
bugs1351359, 1308876, 8849771
milestone54.0a2
Bug 1351359 - Make nsGridContainerFrame call ReflowInput::SetBResize(true) because of auto-block-size swapping between measuring reflows and regular reflows. r=mats, a=gchang This fixes the failure of layout/reftests/css-grid/grid-min-max-content-sizing-002.html with the primary patch in bug 1308876 (which causes a child whose parent is dirty to pick up the dirty bit from the parent only the first reflow of the child if the parent reflows the child multiple times). A simplified testcase for that failure is https://bugzilla.mozilla.org/attachment.cgi?id=8849771 . The failure was caused by an error in height calculation of the first <x> in the test. The div that is the parent of that x has a definite height (presumably due to rules in grid), and the x has a specified height. The div gets three reflows: two measuring reflows (from MinContentContribution and then from MaxContentContribution) and then a final reflow from nsGridContainerFrame::ReflowInFlowChild. Prior to the primary patch in this bug, the div was marked dirty on all three reflows, but with it it is marked dirty only on the first. This means that, without the block-resize flag, the div optimizes away the reflow of its children, since ShouldReflowAllKids returns false because IsBResize() is false, even though NS_FRAME_CONTAINS_RELATIVE_BSIZE is correctly set. In order to fix this, we need to make sure the BResize flag on the reflow state in at least some cases (see the comments in the patch for when, and for how the cases could be optimized in the future). Note that: * when the dirty bit is set on the grid container, the new behavior (with the combination of the patches) is strictly more efficient than the old, since we will sometimes do non-dirty reflows on the grid items (with the b-resize flag) * when the dirty bit is *not* set on the grid container, the new behavior is less efficient than the old, since we will set the b-resize flag when we did not do so before. However, this slowdown fixes existing bugs such as the one in the reftest. Given that I was able to construct a reftest that triggers the failure without the changes from bug 1308876, I've moved this to a separate bug. Without the patch, grid-measuring-reflow-resize-dynamic-001.html fails, but grid-measuring-reflow-resize-static-001.html passes. With the patch both tests pass. (And without the patch, doing a text zoom on the dynamic test fixes the layout error.) MozReview-Commit-ID: JQOdVTQIkU0
layout/generic/nsGridContainerFrame.cpp
layout/reftests/css-grid/grid-measuring-reflow-resize-001-ref.html
layout/reftests/css-grid/grid-measuring-reflow-resize-dynamic-001.html
layout/reftests/css-grid/grid-measuring-reflow-resize-static-001.html
layout/reftests/css-grid/reftest.list
--- a/layout/generic/nsGridContainerFrame.cpp
+++ b/layout/generic/nsGridContainerFrame.cpp
@@ -3707,16 +3707,26 @@ MeasuringReflow(nsIFrame*           aChi
   if (aBMinSizeClamp != NS_MAXSIZE) {
     riFlags |= ReflowInput::B_CLAMP_MARGIN_BOX_MIN_SIZE;
     aChild->Properties().Set(nsIFrame::BClampMarginBoxMinSizeProperty(),
                              aBMinSizeClamp);
   } else {
     aChild->Properties().Delete(nsIFrame::BClampMarginBoxMinSizeProperty());
   }
   ReflowInput childRI(pc, *rs, aChild, aAvailableSize, &aCBSize, riFlags);
+
+  // Because we pass ReflowInput::COMPUTE_SIZE_USE_AUTO_BSIZE, and the
+  // previous reflow of the child might not have, set the child's
+  // block-resize flag to true.
+  // FIXME (perf): It would be faster to do this only if the previous
+  // reflow of the child was not a measuring reflow, and only if the
+  // child does some of the things that are affected by
+  // ReflowInput::COMPUTE_SIZE_USE_AUTO_BSIZE.
+  childRI.SetBResize(true);
+
   ReflowOutput childSize(childRI);
   nsReflowStatus childStatus;
   const uint32_t flags = NS_FRAME_NO_MOVE_FRAME | NS_FRAME_NO_SIZE_VIEW;
   WritingMode wm = childRI.GetWritingMode();
   parent->ReflowChild(aChild, pc, childSize, childRI, wm,
                       LogicalPoint(wm), nsSize(), flags, childStatus);
   parent->FinishReflowChild(aChild, pc, childSize, &childRI, wm,
                             LogicalPoint(wm), nsSize(), flags);
@@ -5236,16 +5246,25 @@ nsGridContainerFrame::ReflowInFlowChild(
   if (!isConstrainedBSize) {
     childCBSize.BSize(childWM) = NS_UNCONSTRAINEDSIZE;
   }
   LogicalSize percentBasis(cb.Size(wm).ConvertTo(childWM, wm));
   ReflowInput childRI(pc, *aState.mReflowInput, aChild, childCBSize,
                       &percentBasis, flags);
   childRI.mFlags.mIsTopOfPage = aFragmentainer ? aFragmentainer->mIsTopOfPage : false;
 
+  // Because we pass ReflowInput::COMPUTE_SIZE_USE_AUTO_BSIZE, and the
+  // previous reflow of the child might not have, set the child's
+  // block-resize flag to true.
+  // FIXME (perf): It would be faster to do this only if the previous
+  // reflow of the child was a measuring reflow, and only if the child
+  // does some of the things that are affected by
+  // ReflowInput::COMPUTE_SIZE_USE_AUTO_BSIZE.
+  childRI.SetBResize(true);
+
   // A table-wrapper needs to propagate the CB size we give it to its
   // inner table frame later.  @see nsTableWrapperFrame::InitChildReflowInput.
   if (childType == nsGkAtoms::tableWrapperFrame) {
     const auto& props = aChild->Properties();
     LogicalSize* cb = props.Get(nsTableWrapperFrame::GridItemCBSizeProperty());
     if (!cb) {
       cb = new LogicalSize(childWM);
       props.Set(nsTableWrapperFrame::GridItemCBSizeProperty(), cb);
new file mode 100644
--- /dev/null
+++ b/layout/reftests/css-grid/grid-measuring-reflow-resize-001-ref.html
@@ -0,0 +1,24 @@
+<!DOCTYPE HTML>
+<title>Testcase simplified from layout/reftests/css-grid/grid-min-max-content-sizing-002.html</title>
+<style type="text/css">
+
+html { overflow: hidden }
+body { margin: 0 }
+
+div {
+  display: inline-block;
+  border: 1px solid fuchsia;
+}
+
+span {
+  display: block;
+  border: 4px solid blue;
+  padding: 0 8px 8px 0;
+  margin: 0 -8px -8px 0;
+}
+
+</style>
+
+<div>
+  <span>blue should overflow fuchsia on right/bottom</span>
+</div>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/css-grid/grid-measuring-reflow-resize-dynamic-001.html
@@ -0,0 +1,37 @@
+<!DOCTYPE HTML>
+<title>Testcase simplified from layout/reftests/css-grid/grid-min-max-content-sizing-002.html</title>
+<style type="text/css">
+
+html { overflow: hidden }
+body { margin: 0 }
+
+.grid {
+  display: grid;
+  grid-template-columns: minmax(min-content,max-content);
+  grid-template-rows: minmax(min-content,max-content);
+}
+
+.grid > div {
+  border: 1px solid fuchsia;
+}
+
+span {
+  display: block;
+  border: 4px solid blue;
+  width: 100%;
+  height: 100%;
+}
+
+</style>
+
+<div class="grid">
+  <div>
+    <span id="s"></span>
+  </div>
+</div>
+
+<script>
+var s = document.getElementById("s");
+s.offsetWidth; // flush layout
+s.textContent = "blue should overflow fuchsia on right/bottom";
+</script>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/css-grid/grid-measuring-reflow-resize-static-001.html
@@ -0,0 +1,32 @@
+<!DOCTYPE HTML>
+<title>Testcase simplified from layout/reftests/css-grid/grid-min-max-content-sizing-002.html</title>
+<style type="text/css">
+
+html { overflow: hidden }
+body { margin: 0 }
+
+.grid {
+  display: grid;
+  grid-template-columns: minmax(min-content,max-content);
+  grid-template-rows: minmax(min-content,max-content);
+}
+
+.grid > div {
+  border: 1px solid fuchsia;
+}
+
+span {
+  display: block;
+  border: 4px solid blue;
+  width: 100%;
+  height: 100%;
+}
+
+</style>
+
+<div class="grid">
+  <div>
+    <span>blue should overflow fuchsia on right/bottom</span>
+  </div>
+</div>
+
--- a/layout/reftests/css-grid/reftest.list
+++ b/layout/reftests/css-grid/reftest.list
@@ -270,8 +270,10 @@ asserts(1-10) == grid-fragmentation-dyn4
 == grid-fragmentation-dyn4-028.html grid-fragmentation-028-ref.html
 == grid-fragmentation-dyn5-028.html grid-fragmentation-028-ref.html
 == grid-fragmentation-dyn1-029.html grid-fragmentation-029-ref.html
 == grid-fragmentation-dyn2-029.html grid-fragmentation-029-ref.html
 == grid-fragmentation-dyn2-030.html grid-fragmentation-030-ref.html
 == grid-fragmentation-dyn2-031.html grid-fragmentation-031-ref.html
 == bug1306106.html bug1306106-ref.html
 == grid-percent-intrinsic-sizing-001.html grid-percent-intrinsic-sizing-001-ref.html
+== grid-measuring-reflow-resize-static-001.html grid-measuring-reflow-resize-001-ref.html
+== grid-measuring-reflow-resize-dynamic-001.html grid-measuring-reflow-resize-001-ref.html