Bug 1269017 part 3 - [css-grid] Reduce scope of a mStaticPosIsCBOrigin check to *just* cover hypothetical-position calculation, and update comments. r=mats
authorDaniel Holbert <dholbert@cs.stanford.edu>
Tue, 08 Nov 2016 16:30:00 -0800
changeset 351833 ab70dc086596dc45abba729112247a7b6c684b57
parent 351832 5e6f0bf39062d35752108049fb4b70cc5732f2bd
child 351834 9f2bdebbdb89d5be01e11a148812f01211dcae64
push id6795
push userjlund@mozilla.com
push dateMon, 23 Jan 2017 14:19:46 +0000
treeherdermozilla-esr52@76101b503191 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmats
bugs1269017
milestone52.0a1
Bug 1269017 part 3 - [css-grid] Reduce scope of a mStaticPosIsCBOrigin check to *just* cover hypothetical-position calculation, and update comments. r=mats Previously, I'd thought the "mStaticPosIsCBOrigin" flag was going to become obsolete -- but now I've realized it's quite useful to avert mixup between the coordinate space of the grid vs. the coordinate space of grid-areas-acting-as-abspos-containing-blocks. So, this patch clarifies/removes some stale comments about this flag, and also pulls out some code that was unnecessarily in an "else" clause, so that it happens regardless of whether this flag is set. (Note: the InitAbsoluteConstraints changes are basically just code-reordering & deindentation.) MozReview-Commit-ID: 9TFrOuldVBe
layout/generic/ReflowInput.cpp
layout/generic/ReflowInput.h
layout/generic/nsAbsoluteContainingBlock.cpp
--- a/layout/generic/ReflowInput.cpp
+++ b/layout/generic/ReflowInput.cpp
@@ -1559,42 +1559,41 @@ ReflowInput::InitAbsoluteConstraints(nsP
   bool bStartIsAuto = styleOffset.GetBStartUnit(cbwm) == eStyleUnit_Auto;
   bool bEndIsAuto   = styleOffset.GetBEndUnit(cbwm) == eStyleUnit_Auto;
 
   // If both 'left' and 'right' are 'auto' or both 'top' and 'bottom' are
   // 'auto', then compute the hypothetical box position where the element would
   // have been if it had been in the flow
   nsHypotheticalPosition hypotheticalPos;
   if ((iStartIsAuto && iEndIsAuto) || (bStartIsAuto && bEndIsAuto)) {
+    nsIFrame* placeholderFrame =
+      aPresContext->PresShell()->GetPlaceholderFrameFor(mFrame);
+    NS_ASSERTION(placeholderFrame, "no placeholder frame");
+
+    if (placeholderFrame->HasAnyStateBits(
+          PLACEHOLDER_STATICPOS_NEEDS_CSSALIGN)) {
+      DebugOnly<nsIFrame*> placeholderParent = placeholderFrame->GetParent();
+      MOZ_ASSERT(placeholderParent, "shouldn't have unparented placeholders");
+      MOZ_ASSERT(placeholderParent->IsFlexOrGridContainer(),
+                 "This flag should only be set on grid/flex children");
+
+      // If the (as-yet unknown) static position will determine the inline
+      // and/or block offsets, set flags to note those offsets aren't valid
+      // until we can do CSS Box Alignment on the OOF frame.
+      mFlags.mIOffsetsNeedCSSAlign = (iStartIsAuto && iEndIsAuto);
+      mFlags.mBOffsetsNeedCSSAlign = (bStartIsAuto && bEndIsAuto);
+    }
+
     if (mFlags.mStaticPosIsCBOrigin) {
-      // XXXdholbert This whole clause should be removed in bug 1269017, where
-      // we'll be making abpsos grid children share our CSS Box Alignment code.
       hypotheticalPos.mWritingMode = cbwm;
       hypotheticalPos.mIStart = nscoord(0);
       hypotheticalPos.mBStart = nscoord(0);
     } else {
-      nsIFrame* placeholderFrame =
-        aPresContext->PresShell()->GetPlaceholderFrameFor(mFrame);
-      NS_ASSERTION(placeholderFrame, "no placeholder frame");
       CalculateHypotheticalPosition(aPresContext, placeholderFrame, cbrs,
                                     hypotheticalPos, aFrameType);
-
-      if (placeholderFrame->HasAnyStateBits(
-            PLACEHOLDER_STATICPOS_NEEDS_CSSALIGN)) {
-        DebugOnly<nsIFrame*> placeholderParent = placeholderFrame->GetParent();
-        MOZ_ASSERT(placeholderParent, "shouldn't have unparented placeholders");
-        MOZ_ASSERT(placeholderParent->IsFlexOrGridContainer(),
-                   "This flag should only be set on grid/flex children");
-
-        // If the (as-yet unknown) static position will determine the inline
-        // and/or block offsets, set flags to note those offsets aren't valid
-        // until we can do CSS Box Alignment on the OOF frame.
-        mFlags.mIOffsetsNeedCSSAlign = (iStartIsAuto && iEndIsAuto);
-        mFlags.mBOffsetsNeedCSSAlign = (bStartIsAuto && bEndIsAuto);
-      }
     }
   }
 
   // Initialize the 'left' and 'right' computed offsets
   // XXX Handle new 'static-position' value...
 
   // Size of the containing block in its writing mode
   LogicalSize cbSize = aCBSize;
--- a/layout/generic/ReflowInput.h
+++ b/layout/generic/ReflowInput.h
@@ -229,18 +229,16 @@ public:
     // When these bits are set, the offset values (IStart/IEnd, BStart/BEnd)
     // represent the "start" edge of the frame's CSS Box Alignment container
     // area, in that axis -- and these offsets need to be further-resolved
     // (with CSS Box Alignment) after we know the OOF frame's size.
     // NOTE: The "I" and "B" (for "Inline" and "Block") refer the axes of the
     // *containing block's writing-mode*, NOT mFrame's own writing-mode. This
     // is purely for convenience, since that's the writing-mode we're dealing
     // with when we set & react to these bits.
-    // XXXdholbert These new bits will probably make the "mStaticPosIsCBOrigin"
-    // bit obsolete -- consider removing it in bug 1269017.
     uint32_t mIOffsetsNeedCSSAlign:1;
     uint32_t mBOffsetsNeedCSSAlign:1;
   };
 
 #ifdef DEBUG
   // Reflow trace methods.  Defined in nsFrame.cpp so they have access
   // to the display-reflow infrastructure.
   static void* DisplayInitOffsetsEnter(
@@ -724,18 +722,20 @@ public:
     // The caller wants shrink-wrap behavior (i.e. ComputeSizeFlags::eShrinkWrap
     // will be passed to ComputeSize()).
     COMPUTE_SIZE_SHRINK_WRAP = (1<<2),
 
     // The caller wants 'auto' bsize behavior (ComputeSizeFlags::eUseAutoBSize
     // will be be passed to ComputeSize()).
     COMPUTE_SIZE_USE_AUTO_BSIZE = (1<<3),
 
-    // The caller wants the abs.pos. static-position resolved at the origin
-    // of the containing block, i.e. at LogicalPoint(0, 0).
+    // The caller wants the abs.pos. static-position resolved at the origin of
+    // the containing block, i.e. at LogicalPoint(0, 0). (Note that this
+    // doesn't necessarily mean that (0, 0) is the *correct* static position
+    // for the frame in question.)
     STATIC_POS_IS_CB_ORIGIN = (1<<4),
 
     // Pass ComputeSizeFlags::eIClampMarginBoxMinSize to ComputeSize().
     I_CLAMP_MARGIN_BOX_MIN_SIZE = (1<<5),
 
     // Pass ComputeSizeFlags::eBClampMarginBoxMinSize to ComputeSize().
     B_CLAMP_MARGIN_BOX_MIN_SIZE = (1<<6),
   };
--- a/layout/generic/nsAbsoluteContainingBlock.cpp
+++ b/layout/generic/nsAbsoluteContainingBlock.cpp
@@ -629,18 +629,21 @@ nsAbsoluteContainingBlock::ReflowAbsolut
                  "Must have a useful inline-size _somewhere_");
     availISize =
       aReflowInput.ComputedSizeWithPadding(wm).ISize(wm);
   }
 
   uint32_t rsFlags = 0;
   if (aFlags & AbsPosReflowFlags::eIsGridContainerCB) {
     // When a grid container generates the abs.pos. CB for a *child* then
-    // the static-position is the CB origin (i.e. of the grid area rect).
-    // https://drafts.csswg.org/css-grid/#static-position
+    // the static position is determined via CSS Box Alignment within the
+    // abs.pos. CB (a grid area, i.e. a piece of the grid). In this scenario,
+    // due to the multiple coordinate spaces in play, we use a convenience flag
+    // to simply have the child's ReflowInput give it a static position at its
+    // abs.pos. CB origin, and then we'll align & offset it from there.
     nsIFrame* placeholder =
       aPresContext->PresShell()->GetPlaceholderFrameFor(aKidFrame);
     if (placeholder && placeholder->GetParent() == aDelegatingFrame) {
       rsFlags |= ReflowInput::STATIC_POS_IS_CB_ORIGIN;
     }
   }
   ReflowInput kidReflowInput(aPresContext, aReflowInput, aKidFrame,
                                    LogicalSize(wm, availISize,