http://dev.w3.org/csswg/css-grid/#auto-margins Not sure if there's anything specific to implement here, or if it just falls out from implementing other sections. We should add tests for this though.

Comment on attachment 8690086 [details] [diff] [review] part 1 - [css-grid] Implement margin:auto for grid items Review of attachment 8690086 [details] [diff] [review]: ----------------------------------------------------------------- Tentative r=me, though the writing-mode thing labeled (1) below ends up being complex to address, another round of review might be merited. r+ with it addressed or with an explanation for why the code there is OK. ::: layout/generic/nsGridContainerFrame.cpp @@ +922,5 @@ > + } else { > + hasAutoMarginStart = styleMargin.GetIStartUnit(wm) == eStyleUnit_Auto; > + hasAutoMarginEnd = styleMargin.GetIEndUnit(wm) == eStyleUnit_Auto; > + } > + drop whitespace on blank line ::: layout/generic/nsHTMLReflowState.cpp @@ +2314,5 @@ > auto justifySelf = mStylePosition->ComputedJustifySelf(mStyleDisplay, > frame->StyleContext()->GetParent()); > + if (justifySelf != NS_STYLE_JUSTIFY_STRETCH || > + mStyleMargin->mMargin.GetIStartUnit(wm) == eStyleUnit_Auto || > + mStyleMargin->mMargin.GetIEndUnit(wm) == eStyleUnit_Auto) { Two things: (1) I think the GetIStartUnit/GetIEndUnit calls here need to use "cbwm", not "wm", right? Given that we're considering "justify-self" here, then that means we want the *grid's* inline axis, not the grid *item's* inline axis. (It looks like you have tests for this sort of thing, though [haven't looked in too much detail], so maybe I'm misunderstanding?) (2) This chunk (starting with the parentFrameType check) has gotten a bit complex now. Consider adding an explanatory comment summing up what we're doing - something like: // If a grid item has justify-self:stretch & no auto margins in grid's inline // axis, it'll be stretched in that axis. Otherwise, it's shrinkwrapped.

> (1) I think the GetIStartUnit/GetIEndUnit calls here need to use "cbwm", > not "wm", right? No, I think you're misunderstanding. The inline-axis margin for any element applies to its own axis regardless of which axis that happens to be for the container.

... but this clause is all about "justify-self: stretch", which is something that applies along the *grid container's* inline axis (if the margins in that axis are non-auto). So surely we should be actually checking the margins *in that axis* (the grid container's inline axis), not the child's inline axis, for autoness?

I'm thinking about a case like this: <grid> <grid-item style="justify-self: stretch; writing-mode: vertical-lr; margin-top: auto"> I'm assuming your "mStyleMargin->mMargin.GetIStartUnit(wm) == eStyleUnit_Auto" check will return true here (because the grid item's inline-start margin [margin-top] is "auto"). And I don't see why we should care about that fact (the margin-top's autoness) at all here. Really, we should be caring about margin-left / margin-right when determining how "justify-self" will apply (since it applies in the horizontal axis here, for our horizontal-writing-mode grid.) In other words, I'd expect we should be passing the grid's writing mode to GetIStartUnit/GetIEndUnit.

Well, not exactly. The auto-margin test here has nothing to do with the justify-self != stretch test, they are completely orthogonal. The code could be written: if (mStyleMargin->mMargin.GetIStartUnit(wm) == eStyleUnit_Auto || mStyleMargin->mMargin.GetIEndUnit(wm) == eStyleUnit_Auto) flags |= eShrinkWrap; if (justifySelf != NS_STYLE_JUSTIFY_STRETCH) flags |= eShrinkWrap; So I think the auto-margin test is correct. But the justify-self:stretch test here is actually slightly wrong... When the writing-modes are orthogonal it's actually align-self we should test (this is probably what you we're getting at). There are two cases we get wrong: <item style="justify-self:stretch; align-self:end; writing-mode:vertical-lr"> In this case we don't set eShrinkWrap although we really should. It seems to work fine when I test it though, but I have no clue why to be honest. :-) Perhaps some writing-mode specific code already set eShrinkWrap somewhere else? <item style="justify-self:end; align-self:stretch; writing-mode:vertical-lr"> In this case we do set eShrinkWrap but we shouldn't because we really want the item's inline-axis to stretch in this case. It's "merely" a performance problem though, because the alignment code in nsGridContainerFrame will detect that the item doesn't fill its available space and force it to stretch. I'll fix the justifySelf test while we're here...

(In reply to Mats Palmgren (:mats) from comment #7) > When the writing-modes are orthogonal it's actually align-self we should > test (this is probably what you we're getting at). Yeah -- that makes more sense. > I'll fix the justifySelf test while we're here... Thanks! (I think you're saying now you'll be checking align-self here instead of justify-self, if our wm is orthogonal to the grid; that seems good to me, but please correct me if I'm misunderstanding.)

Correct.

> Perhaps some writing-mode specific code already set eShrinkWrap somewhere else? Yep, just a few lines above actually: http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp?rev=73e519e9ec5e#2284 Also, if you use <div class="grid"><span>... then isBlock will be false! http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp?rev=73e519e9ec5e#2279 because the item's "frame type" is NS_CSS_FRAME_TYPE_INLINE: http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp?rev=73e519e9ec5e#778 sigh...

Wow, the display:inline grid item ends up with mDisplay == NS_STYLE_DISPLAY_INLINE_BLOCK. First we blockify it here (to NS_STYLE_DISPLAY_BLOCK): http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleContext.cpp?rev=41e9257c073d#715 then we overwrite it again at the end for orthogonal flows: http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleContext.cpp?rev=41e9257c073d#772 the reason for that is that it's testing |disp| there which points at the original style struct (which might actually be deallocated at this point?) not the one we allocated and assigned NS_STYLE_DISPLAY_BLOCK to. That seems like a bug to me. Is https://drafts.csswg.org/css-writing-modes-3/#block-flow really supposed to override flex/grid item blockification?

Also, do you know any reason why we can't suppress setting eShrinkWrap here for orthogonal grid items that has align/justify-self:stretch in the relevant axis? http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp?rev=73e519e9ec5e#2284 That's a performance problem since the item will be forced to stretch later anyway and thus require another reflow.

(In reply to Mats Palmgren (:mats) from comment #11) > Wow, the display:inline grid item ends up with mDisplay == > NS_STYLE_DISPLAY_INLINE_BLOCK. > First we blockify it here (to NS_STYLE_DISPLAY_BLOCK): > http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleContext. > cpp?rev=41e9257c073d#715 > then we overwrite it again at the end for orthogonal flows: > http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleContext. > cpp?rev=41e9257c073d#772 > the reason for that is that it's testing |disp| there which points at the > original style struct (which might actually be deallocated at this point?) > not the one we allocated and assigned NS_STYLE_DISPLAY_BLOCK to. > That seems like a bug to me. > > Is https://drafts.csswg.org/css-writing-modes-3/#block-flow really supposed > to override flex/grid item blockification? I think this is a bug; if the item was blockified already, there's no reason writing-mode should override that to make it inline-block. This seems like a Writing Modes spec bug, as it currently says "If the box has a specified display of inline, its display computes to inline-block", ignoring the possibility that the "specified display of inline" may be overridden due to other properties. So I think this should be clarified; but in the meantime I'd be happy to take a patch that anticipates a sensible spec change here. (Just curious, have you checked what other browsers do?) (In reply to Mats Palmgren (:mats) from comment #12) > Also, do you know any reason why we can't suppress setting eShrinkWrap here... No reason I can think of offhand. Let's try it?

(this depends on bug 1227917 for the "isBlock" assertions) Now tests align-/justify-self:stretch based on the writing-modes as discussed above. Also, I moved the code block for grid upfront to avoid the "shrink-wrap all orthogonal flows" case setting it when it's not needed. I'll attach a wdiff version which is probably easier to review...

You only need to look at the nsHTMLReflowState.cpp changes in this patch.

BTW, it looks like all the branches that set eShrinkWrap have isBlock==true as a condition. So we could wrap everything in an outer "if (isBlock)" which would avoid the virtual parent->GetType() call when it's false. Perhaps not worth it though, I don't know. Let me know what you think.

Whoa, lot's of Try failures there. I guess that assertion does not hold for some reason. (It appears <input type=number> is using display:flex internally and we don't appear to blockify those items for some reason?) I'll just remove the assertions for now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=03c3124d3e0d

Comment on attachment 8692016 [details] [diff] [review] part 1 (wdiff) Review of attachment 8692016 [details] [diff] [review]: ----------------------------------------------------------------- r=me, w/ encouragement to add a comment; see below. (Sorry for the delay here -- was AFK over Thanksgiving holiday weekend) ::: mc1.1816d3f26814/layout/generic/nsHTMLReflowState.cpp @@ +2291,5 @@ > > + nsIFrame* parent = frame->GetParent(); > + nsIAtom* parentFrameType = parent ? parent->GetType() : nullptr; > + if (parentFrameType == nsGkAtoms::gridContainerFrame) { > + MOZ_ASSERT(isBlock || frame->GetType() == nsGkAtoms::placeholderFrame, As noted at end of comment 3, this whole clause would benefit from a summarizing code-comment, I think, now that it's getting more complex. Something like this might be better than what I suggested in comment 3: // Use shrinkwrapped size, if we can tell our grid will be aligning // (rather than stretching) us in our grid area's inline axis.

(In reply to Mats Palmgren (:mats) from comment #18) > (It appears <input type=number> is using display:flex internally and > we don't appear to blockify those items for some reason?) (That's probably because they're inside of a form control. We suppress flex-item-blockification inside of form controls, so that we don't accidentally blockify styles for the mandatory XUL junk that lives inside of an <audio style="display:flex"> element, for example. See bug 812822 and bug 844529.) (It's surprising that <input type="number">'s flex-item pieces aren't already block-level, though... I'd expect that we should hardcode their display values correctly so that this works out.)

Poked around in a debugger, & filed bug 1229212 for the <input type="number"> assertion-failures. Pushed a Try run with the patches on this bug here, all alone (expected to have assertion-failures): https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b37e2c01f39 ...and with the fix for bug 1229212 (hopefully no assertion-failures): https://treeherder.mozilla.org/#/jobs?repo=try&revision=d47a2f6401a4 If that second Try run goes well, I'm hoping we can keep the assertions here. (so we won't need to remove them, as thought in comment 18)

https://hg.mozilla.org/integration/mozilla-inbound/rev/daf6598a5d7a https://hg.mozilla.org/integration/mozilla-inbound/rev/e52c8b7764e2

OK, I added: // Shrink-wrap grid items that will be aligned (rather than stretched) // in its inline axis.

Sounds good, thanks!

https://hg.mozilla.org/mozilla-central/rev/daf6598a5d7a https://hg.mozilla.org/mozilla-central/rev/e52c8b7764e2