Additional resolutions from CSSWG meeting: https://lists.w3.org/Archives/Public/www-style/2015May/0280.html

Taking this, patch coming up...

(also idempotent patch)

Comment on attachment 8697618 [details] [diff] [review] part 3 - [css-grid] Add a generic nsHTMLReflowState::STATIC_POS_IS_CB_ORIGIN flag to place the static-position at the CB origin, and make nsAbsoluteContainingBlock use it in Grid containers BTW, this part does not add any code for child placeholders when the grid container isn't the abs.pos. CB for them. For that case, 10.2 says: "The static position [CSS21] of an absolutely-positioned child of a grid container is determined as if it were the sole grid item in a grid area whose edges coincide with the padding edges of the grid container." This seems a bit odd to me, and Chrome doesn't do that either so I'm contemplating raising a spec issue. We currently use the content edge (same as Chrome) which seems more natural as a static-position (in the "that's where it would be if it were in-flow" sense). But then again, its position isn't influenced by distribution so that might be odd too. Anyway, there might come a follow-up patch here for that case...

Added a one-liner to put placeholders at the padding edge, so we do what the spec says for now.

Moved a code change here that was mistakenly included in the reftest patch.

Comment on attachment 8697609 [details] [diff] [review] part 1 - Replace three bool params for nsAbsoluteContainingBlock::Reflow with a flag param (idempotent patch). Review of attachment 8697609 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsAbsoluteContainingBlock.h @@ +68,5 @@ > void RemoveFrame(nsIFrame* aDelegatingFrame, > ChildListID aListID, > nsIFrame* aOldFrame); > > + enum AbsPosReflowFlags { I think this should be "enum class" (not just "enum"), because: (1) It's more type-safe and hence foolproof. (2) You're using MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS, which I think only makes sense with an enum class. (With a traditional enum, it's just a glorified "int" or "uint", so bitwise operators Just Work -- no need for the macro.) (The downside of "enum class" is that the enum-names get a bit more verbose -- they might end up being something like "nsAbsoluteContainingBlock::AbsPosReflowFlags::eConstrainHeight". To avoid that, you could either move this enum outside of the nsAbsoluteContainingBlock {...} class-scope, or -- probably better -- you could add a local typedef in each .cpp file that uses these values (something like "typedef nsAbsoluteContainingBlock::AbsPosReflowFlags AbsPosReflowFlags"). @@ +69,5 @@ > ChildListID aListID, > nsIFrame* aOldFrame); > > + enum AbsPosReflowFlags { > + eConstrainHeight = 0x1, Fix trailing whitespace here. @@ +96,5 @@ > nsPresContext* aPresContext, > const nsHTMLReflowState& aReflowState, > nsReflowStatus& aReflowStatus, > const nsRect& aContainingBlock, > + AbsPosReflowFlags aFlags, Fix odd indentation here.

Comment on attachment 8697615 [details] [diff] [review] part 2 - [css-grid] Add a eIsGridContainerCB flag for nsAbsoluteContainingBlock::Reflow to trigger Grid specific code (rather than checking GetType()). Review of attachment 8697615 [details] [diff] [review]: ----------------------------------------------------------------- r=me, nits below: ::: layout/generic/nsAbsoluteContainingBlock.cpp @@ +123,5 @@ > // The 'width' check below is an optimization to avoid the virtual GetType() > // call in most cases. 'aContainingBlock' isn't used for grid items, > // each item has its own CB on a frame property instead. > // @see nsGridContainerFrame::ReflowChildren > + const bool isGrid = aFlags & eIsGridContainerCB; The comment before this line needs removing/adjusting. (It's about the VERY_LIKELY_A_GRID_CONTAINER width-check, which you've now removed.) ::: layout/generic/nsGridContainerFrame.cpp @@ +3241,5 @@ > *cb = itemCB.GetPhysicalRect(wm, gridCBPhysicalSize); > } > + // We pass a dummy rect as CB because each child has its own CB rect. > + // The eIsGridContainerCB flag tells nsAbsoluteContainingBlock::Reflow to > + // use those instead. Consider mentioning the grid area (which is the CB you're hinting at) in the first sentence here -- e.g. add "(from its grid area)" at the end of that sentence.

Comment on attachment 8697635 [details] [diff] [review] part 3 - [css-grid] Add a generic nsHTMLReflowState::STATIC_POS_IS_CB_ORIGIN flag to place the static-position at the CB origin, and make nsAbsoluteContainingBlock use it in Grid containers Review of attachment 8697635 [details] [diff] [review]: ----------------------------------------------------------------- r=me, nits below: ::: layout/generic/nsAbsoluteContainingBlock.cpp @@ +388,5 @@ > aReflowState.ComputedSizeWithPadding(wm).ISize(wm); > } > > + uint32_t flags = 0; > + if (aFlags & eIsGridContainerCB) { Maybe rename "flags" to "rsFlags" (rs for reflow state), to make them more clearly-distinct from "aFlags"? To avoid confusion, I'd like to avoid having "foo" and "aFoo" variables in-scope simultaneously, particularly if they represent different things (as is the case here). @@ +391,5 @@ > + uint32_t flags = 0; > + if (aFlags & eIsGridContainerCB) { > + // For a grid container child that has its parent as the abs.pos. CB > + // the static-position is the CB origin. > + // https://drafts.csswg.org/css-grid/#static-position The phrase "its parent as the abs.pos. CB" is misleading/incorrect here, I think. If we get to this code, IIUC, it means we have an abspos element that's being positioned with its *grid area* being the abspos CB -- right? (not its parent) The spec distinguishes this in 10.1 by saying the grid *generates* the abspos CB, rather than the grid *being* the abspos CB. [Separately: there are also cases where we have e.g. <positioned block> <grid> <abspos child, positioned w.r.t. grandparent> ...and we need to find the static position of the abspos thing, which happens to be a child of a grid. I think that's the case that the #static-position section (10.2) initially focuses on, RE using the actual grid container's padding-box origin. But I don't think that scenario ends up in this code, does it? I'd expect we address that scenario by putting the placeholder (a child frame of the grid) at position 0,0 in our padding box. It looks like you're adding code to handle that in nsGridContainerFrame; cool.] IN ANY CASE: please clarify this code-comment to make it clearer what containing-block you're talking about & whether you actually mean the grid itself (I don't think you do) vs. a grid area, i.e. a CB that's "generated by the grid". @@ +489,1 @@ > r.x += aContainingBlock.x; Please update the comment above this block. Right now, the comment declares that we don't need to add an offset if the frame has "auto" on both sides -- but you're adding an exception to that rule. ::: layout/generic/nsGridContainerFrame.cpp @@ +3206,5 @@ > LogicalPoint(childWM), dummyContainerSize, 0, childStatus); > } > + } else { > + // Put a placeholder at the padding edge, in case we're not its CB. > + childPos -= padStart; Similar to my comment above for nsAbsoluteContainingBlock.cpp, "in case we're not its CB" is misleading here -- "we" (the grid) are generally *never* the CB, even when we're positioned. It'd be some grid area that would be the CB in that case. Maybe adjust to say "in case we aren't the frame that generates its CB"? ::: layout/generic/nsHTMLReflowState.h @@ +682,5 @@ > COMPUTE_SIZE_SHRINK_WRAP = (1<<2), > + > + // The caller wants the abs.pos. static-position resolved at the origin > + // of the containing block, i.e. at LogicalPoint(0, 0). > + STATIC_POS_IS_CB_ORIGIN = (1<<4), I think this should be 1<<3, right? That's the next unused bit here. (We have 1<<0, 1<<1, 1<<2, and the next bit would be 1<<3. I suspect you're thinking in terms of 0xWhatever instead of 1<<Whatever; the 2 to 4 jump is an easy mistake to make, from that mindset.)

Comment on attachment 8697638 [details] [diff] [review] part 4 - Some code cleanup in nsHTMLReflowState::CalculateHypotheticalPosition, and make a few methods 'const' (idempotent patch). Review of attachment 8697638 [details] [diff] [review]: ----------------------------------------------------------------- This cleanup looks nice -- thanks! r=me

https://hg.mozilla.org/integration/mozilla-inbound/rev/0937bbc778f3 https://hg.mozilla.org/integration/mozilla-inbound/rev/a296d149c69e https://hg.mozilla.org/integration/mozilla-inbound/rev/0f8a939a1213 https://hg.mozilla.org/integration/mozilla-inbound/rev/4cf8b95e28d0 https://hg.mozilla.org/integration/mozilla-inbound/rev/1849d8dd6361

https://hg.mozilla.org/mozilla-central/rev/0937bbc778f3 https://hg.mozilla.org/mozilla-central/rev/a296d149c69e https://hg.mozilla.org/mozilla-central/rev/0f8a939a1213 https://hg.mozilla.org/mozilla-central/rev/4cf8b95e28d0 https://hg.mozilla.org/mozilla-central/rev/1849d8dd6361