Bug 1174003 part 5: [css-flexbox] Remove is-{main,cross}-axis-horizontal checks from ReflowFlexItem. r?mats draft
authorDaniel Holbert <dholbert@cs.stanford.edu>
Tue, 27 Feb 2018 15:40:18 -0800
changeset 760683 fb87314e2c86924c78de50440fd97686ebc043b3
parent 760682 ebdefc1a94f6df0ee048de330e9477fc5264eb8d
child 760684 3ca18cd51d5160366d9297d2c4bae3f4a06dc365
child 760690 4f9e912b1903a92c9fa1d4e42e6d180cf57cbdb5
push id100718
push userdholbert@mozilla.com
push dateTue, 27 Feb 2018 23:41:18 +0000
reviewersmats
bugs1174003, 1441348
milestone60.0a1
Bug 1174003 part 5: [css-flexbox] Remove is-{main,cross}-axis-horizontal checks from ReflowFlexItem. r?mats This patch mostly[1] doesn't affect behavior. It just changes some ReflowInput width/height-setting logic to use ISize/BSize setters instead of width/height setters. To help with this, I'm also adding some more getters to answer the question "is this flex item's {inline,block} axis the same as the flex container's {main,cross} axis", for convenience/readability at callsites. (All of these getters are simply giving a different view of the same underlying single bit of information.) [1] One way this patch *kind of* affects behavior: it generalizes the conditions under which we set the NS_FRAME_CONTAINS_RELATIVE_BSIZE flag on a flex item. But that doesn't actually have any user-visible effects right now, because that flag's only purpose is to determine whether we should reflow, and currently we always reflow all flex items. If/when that changes, though (i.e. when we start reflowing flex items more selectively), this patch is adding a reftest that may test this generalized behavior. (The reftest actually trivially passes right now, due to unrelated bug 1441348.) MozReview-Commit-ID: 4NEKLBAjowh
layout/generic/nsFlexContainerFrame.cpp
layout/reftests/flexbox/flexbox-resizeviewport-2-helper.html
layout/reftests/flexbox/flexbox-resizeviewport-2-ref.xhtml
layout/reftests/flexbox/flexbox-resizeviewport-2.xhtml
layout/reftests/flexbox/reftest.list
--- a/layout/generic/nsFlexContainerFrame.cpp
+++ b/layout/generic/nsFlexContainerFrame.cpp
@@ -565,24 +565,27 @@ public:
   // min-[width|height] property.
   bool NeedsMinSizeAutoResolution() const
     { return mNeedsMinSizeAutoResolution; }
 
   // Indicates whether this item is a "strut" left behind by an element with
   // visibility:collapse.
   bool IsStrut() const             { return mIsStrut; }
 
-  // Returns true if this item's inline axis is parallel (or antiparallel)
-  // to the container's main axis. Otherwise (i.e. if this item's inline axis
-  // is orthogonal to the container's main axis), this function returns false.
-  bool IsInlineAxisMainAxis() const { return mIsInlineAxisMainAxis; }
-
-  // Same as above, but for cross axis. Equivalent to !IsInlineAxisMainAxis().
-  // This just exists for convenience/readability at callsites.
+  // IsInlineAxisMainAxis() returns true if this item's inline axis is parallel
+  // (or antiparallel) to the container's main axis. Otherwise (i.e. if this
+  // item's inline axis is orthogonal to the container's main axis), this
+  // function returns false. The next 3 methods are all other ways of asking
+  // the same question, and only exist for readability at callsites (depending
+  // on which axes those callsites are reasoning about).
+  bool IsInlineAxisMainAxis() const  { return mIsInlineAxisMainAxis;  }
   bool IsInlineAxisCrossAxis() const { return !mIsInlineAxisMainAxis; }
+  bool IsBlockAxisMainAxis() const   { return !mIsInlineAxisMainAxis; }
+  bool IsBlockAxisCrossAxis() const  { return mIsInlineAxisMainAxis;  }
+
 
   WritingMode GetWritingMode() const { return mWM; }
   uint8_t GetAlignSelf() const     { return mAlignSelf; }
 
   // Returns the flex factor (flex-grow or flex-shrink), depending on
   // 'aIsUsingFlexGrow'.
   //
   // Asserts fatally if called on a frozen item (since frozen items are not
@@ -4858,71 +4861,79 @@ nsFlexContainerFrame::ReflowFlexItem(nsP
 {
   WritingMode outerWM = aReflowInput.GetWritingMode();
   WritingMode wm = aItem.Frame()->GetWritingMode();
   LogicalSize availSize = aReflowInput.ComputedSize(wm);
   availSize.BSize(wm) = NS_UNCONSTRAINEDSIZE;
   ReflowInput childReflowInput(aPresContext, aReflowInput,
                                      aItem.Frame(), availSize);
 
-  // Keep track of whether we've overriden the child's computed height
-  // and/or width, so we can set its resize flags accordingly.
-  bool didOverrideComputedWidth = false;
-  bool didOverrideComputedHeight = false;
+  // Keep track of whether we've overriden the child's computed ISize
+  // and/or BSize, so we can set its resize flags accordingly.
+  bool didOverrideComputedISize = false;
+  bool didOverrideComputedBSize = false;
 
   // Override computed main-size
-  if (aAxisTracker.IsMainAxisHorizontal()) {
-    childReflowInput.SetComputedWidth(aItem.GetMainSize());
-    didOverrideComputedWidth = true;
+  if (aItem.IsInlineAxisMainAxis()) {
+    childReflowInput.SetComputedISize(aItem.GetMainSize());
+    didOverrideComputedISize = true;
   } else {
-    childReflowInput.SetComputedHeight(aItem.GetMainSize());
-    didOverrideComputedHeight = true;
+    childReflowInput.SetComputedBSize(aItem.GetMainSize());
+    didOverrideComputedBSize = true;
   }
 
   // Override reflow state's computed cross-size if either:
   // - the item was stretched (in which case we're imposing a cross size)
   // ...or...
   // - the item it has an aspect ratio (in which case the cross-size that's
   // currently in the reflow state is based on arithmetic involving a stale
   // main-size value that we just stomped on above). (Note that we could handle
   // this case using an AutoFlexItemMainSizeOverride, as we do elsewhere; but
   // given that we *already know* the correct cross size to use here, it's
   // cheaper to just directly set it instead of setting a frame property.)
   if (aItem.IsStretched() ||
       aItem.HasIntrinsicRatio()) {
-    if (aAxisTracker.IsCrossAxisHorizontal()) {
-      childReflowInput.SetComputedWidth(aItem.GetCrossSize());
-      didOverrideComputedWidth = true;
+    if (aItem.IsInlineAxisCrossAxis()) {
+      childReflowInput.SetComputedISize(aItem.GetCrossSize());
+      didOverrideComputedISize = true;
     } else {
-      childReflowInput.SetComputedHeight(aItem.GetCrossSize());
-      didOverrideComputedHeight = true;
+      childReflowInput.SetComputedBSize(aItem.GetCrossSize());
+      didOverrideComputedBSize = true;
     }
   }
-  if (aItem.IsStretched() && !aAxisTracker.IsCrossAxisHorizontal()) {
-    // If this item's height is stretched, it's a relative height.
+  if (aItem.IsStretched() && aItem.IsBlockAxisCrossAxis()) {
+    // This item is stretched (in the cross axis), and that axis is its block
+    // axis.  That stretching effectively gives it a relative BSize.
+    // XXXdholbert This flag only makes a difference if we use the flex items'
+    // frame-state when deciding whether to reflow them -- and we don't, as of
+    // the changes in bug 851607. So this has no effect right now, but it might
+    // make a difference if we optimize to use dirty bits in the
+    // future. (Reftests flexbox-resizeviewport-1.xhtml and -2.xhtml are
+    // intended to catch any regressions here, if we end up relying on this bit
+    // & neglecting to set it.)
     aItem.Frame()->AddStateBits(NS_FRAME_CONTAINS_RELATIVE_BSIZE);
   }
 
   // XXXdholbert Might need to actually set the correct margins in the
   // reflow state at some point, so that they can be saved on the frame for
   // UsedMarginProperty().  Maybe doesn't matter though...?
 
   // If we're overriding the computed width or height, *and* we had an
   // earlier "measuring" reflow, then this upcoming reflow needs to be
   // treated as a resize.
   if (aItem.HadMeasuringReflow()) {
-    if (didOverrideComputedWidth) {
-      // (This is somewhat redundant, since the reflow state already
-      // sets mHResize whenever our computed width has changed since the
-      // previous reflow. Still, it's nice for symmetry, and it may become
-      // necessary once we support orthogonal flows.)
-      childReflowInput.SetHResize(true);
+    if (didOverrideComputedISize) {
+      // (This is somewhat redundant, since ReflowInput::InitResizeFlags()
+      // already calls SetIResize() whenever our computed ISize has changed
+      // since the previous reflow. Still, it's nice for symmetry, and it might
+      // be necessary for some edge cases.)
+      childReflowInput.SetIResize(true);
     }
-    if (didOverrideComputedHeight) {
-      childReflowInput.SetVResize(true);
+    if (didOverrideComputedBSize) {
+      childReflowInput.SetBResize(true);
     }
   }
   // NOTE: Be very careful about doing anything else with childReflowInput
   // after this point, because some of its methods (e.g. SetComputedWidth)
   // internally call InitResizeFlags and stomp on mVResize & mHResize.
 
   ReflowOutput childDesiredSize(childReflowInput);
   nsReflowStatus childReflowStatus;
copy from layout/reftests/flexbox/flexbox-resizeviewport-1-helper.html
copy to layout/reftests/flexbox/flexbox-resizeviewport-2-helper.html
--- a/layout/reftests/flexbox/flexbox-resizeviewport-1-helper.html
+++ b/layout/reftests/flexbox/flexbox-resizeviewport-2-helper.html
@@ -1,21 +1,22 @@
 <!--
      Any copyright is dedicated to the Public Domain.
      http://creativecommons.org/publicdomain/zero/1.0/
 -->
-<!-- Helper-file for reftest flexbox-resizeviewport-1.xhtml
+<!-- Helper-file for reftest flexbox-resizeviewport-2.xhtml
      I'm intentionally using quirks-mode (no doctype), so that
-     a 100% height will work. -->
+     a 100% width (block-size) will work. -->
 <html>
   <head>
     <style>
+      html { writing-mode: vertical-rl; }
       div.flexbox {
         display: flex;
-        height: 100%;
+        width: 100%;
         border: 2px dashed black;
       }
       div.a {
         flex: 1;
         background: pink;
       }
       div.b {
         flex: 1;
copy from layout/reftests/flexbox/flexbox-resizeviewport-1-ref.xhtml
copy to layout/reftests/flexbox/flexbox-resizeviewport-2-ref.xhtml
--- a/layout/reftests/flexbox/flexbox-resizeviewport-1-ref.xhtml
+++ b/layout/reftests/flexbox/flexbox-resizeviewport-2-ref.xhtml
@@ -8,15 +8,15 @@
     <style>
       iframe {
         width:  75px;
         height: 75px;
       }
     </style>
   </head>
   <body>
-    <iframe src="flexbox-resizeviewport-1-helper.html" style="width: 50px"/>
-    <iframe src="flexbox-resizeviewport-1-helper.html" style="width: 125px"/>
+    <iframe src="flexbox-resizeviewport-2-helper.html" style="width: 50px"/>
+    <iframe src="flexbox-resizeviewport-2-helper.html" style="width: 125px"/>
     <br/>
-    <iframe src="flexbox-resizeviewport-1-helper.html" style="height: 50px"/>
-    <iframe src="flexbox-resizeviewport-1-helper.html" style="height: 125px"/>
+    <iframe src="flexbox-resizeviewport-2-helper.html" style="height: 50px"/>
+    <iframe src="flexbox-resizeviewport-2-helper.html" style="height: 125px"/>
   </body>
 </html>
copy from layout/reftests/flexbox/flexbox-resizeviewport-1.xhtml
copy to layout/reftests/flexbox/flexbox-resizeviewport-2.xhtml
--- a/layout/reftests/flexbox/flexbox-resizeviewport-1.xhtml
+++ b/layout/reftests/flexbox/flexbox-resizeviewport-2.xhtml
@@ -1,15 +1,19 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <!--
      Any copyright is dedicated to the Public Domain.
      http://creativecommons.org/publicdomain/zero/1.0/
 -->
 <!-- Testcase to be sure a flex container gets reflowed properly when its
      iframe changes size. -->
+<!-- XXXdholbert This testcase can't actually test the scenario it's intended
+     to test right now (quirks-mode percent-BSize resolution in a vertical
+     writing mode), due to Bug 1441348.
+ -->
 <html xmlns="http://www.w3.org/1999/xhtml"
       class="reftest-wait">
   <head>
     <style>
       iframe {
         width:  75px;
         height: 75px;
       }
@@ -26,15 +30,15 @@
         setElementPropertyTo("c", "height", "50px");
         setElementPropertyTo("d", "height", "125px");
         document.documentElement.removeAttribute("class");
       }
       window.addEventListener("MozReftestInvalidate", tweak, false);
     </script>
   </head>
   <body>
-    <iframe id="a" src="flexbox-resizeviewport-1-helper.html"/>
-    <iframe id="b" src="flexbox-resizeviewport-1-helper.html"/>
+    <iframe id="a" src="flexbox-resizeviewport-2-helper.html"/>
+    <iframe id="b" src="flexbox-resizeviewport-2-helper.html"/>
     <br/>
-    <iframe id="c" src="flexbox-resizeviewport-1-helper.html"/>
-    <iframe id="d" src="flexbox-resizeviewport-1-helper.html"/>
+    <iframe id="c" src="flexbox-resizeviewport-2-helper.html"/>
+    <iframe id="d" src="flexbox-resizeviewport-2-helper.html"/>
   </body>
 </html>
--- a/layout/reftests/flexbox/reftest.list
+++ b/layout/reftests/flexbox/reftest.list
@@ -106,16 +106,17 @@ fails == flexbox-inlinecontent-horiz-1b.
 == flexbox-intrinsic-sizing-horiz-2a.xhtml flexbox-intrinsic-sizing-horiz-2-ref.xhtml
 == flexbox-intrinsic-sizing-horiz-2b.xhtml flexbox-intrinsic-sizing-horiz-2-ref.xhtml
 
 # Tests for invalidation after dynamic modifications
 == flexbox-invalidation-1.html flexbox-invalidation-1-ref.html
 
 # Tests for flexbox in an iframe that gets resized.
 fuzzy-if(skiaContent,1,5) == flexbox-resizeviewport-1.xhtml flexbox-resizeviewport-1-ref.xhtml
+fuzzy-if(skiaContent,1,5) == flexbox-resizeviewport-2.xhtml flexbox-resizeviewport-2-ref.xhtml
 
 # Tests for flexbox styling on things that don't support it
 == flexbox-styling-on-svg-1.svg flexbox-styling-on-svg-1-ref.svg
 
 # Tests with widgets as flex items
 fuzzy-if(gtkWidget,1,66) == flexbox-widget-flex-items-1.html flexbox-widget-flex-items-1-ref.html
 fuzzy-if(gtkWidget,1,74) == flexbox-widget-flex-items-2.html flexbox-widget-flex-items-2-ref.html
 skip-if(gtkWidget) == flexbox-widget-flex-items-3.html flexbox-widget-flex-items-3-ref.html # bug 1260965