Closed Bug 1176793 Opened 5 years ago Closed 4 years ago

[css-grid] Aligning grid items with auto margins

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: mats, Assigned: mats)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

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.
Blocks: 1217086
Assignee: nobody → mats
Attachment #8690086 - Flags: review?(dholbert)
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.
Attachment #8690086 - Flags: review?(dholbert) → review+
>  (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?
Flags: needinfo?(jfkthame)
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?
Flags: needinfo?(jfkthame)
Depends on: 1227917
(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...
Attachment #8690086 - Attachment is obsolete: true
Attached patch part 1 (wdiff)Splinter Review
You only need to look at the nsHTMLReflowState.cpp changes in this patch.
Attachment #8692016 - Flags: review?(dholbert)
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.
Attachment #8692016 - Flags: review?(dholbert) → review+
(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)
Depends on: 1229212
OK, I added:
        // Shrink-wrap grid items that will be aligned (rather than stretched)
        // in its inline axis.
Flags: in-testsuite+
Sounds good, thanks!
https://hg.mozilla.org/mozilla-central/rev/daf6598a5d7a
https://hg.mozilla.org/mozilla-central/rev/e52c8b7764e2
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.