Bug 1269017 part 3 - [css-grid] Reduce scope of a mStaticPosIsCBOrigin check to *just* cover hypothetical-position calculation, and update comments. r=mats draft
authorDaniel Holbert <dholbert@cs.stanford.edu>
Mon, 07 Nov 2016 12:43:01 -0800
changeset 435075 26e6b54fe3ba382815bb517a1d118fb452df3171
parent 435074 6381f10d8409479ed52a843b677feaa06a663c13
child 435076 61c3677ace82a9265a55434aa0ad013838cdb8ec
push id34928
push userdholbert@mozilla.com
push dateTue, 08 Nov 2016 00:57:02 +0000
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,