1271392 - [css-grid] Make grid item 'stretch' not require an extra reflow

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Comment 1

[Mats Palmgren (inactive)]

Attachment: part 1 - [css-grid] Make grid item 'stretch' not require an extra reflow.

This part removes the 'stretch' logic in AlignJustifySelf and implements
it in nsLayoutUtils::ComputeSizeWithIntrinsicDimensions /
nsFrame::ComputeSize instead.

This also fixes bug 1258443 - 'stretch' should shrink items below their
min-content size.

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-grid] When pulling up grid item, make sure its next-in-flow (if any) is in our next-in-flow.

This fixes an assertion which occurred after a series of Reflow calls
of fragments combined with removing a (grid item) DOM node:

GridContainer  <---- Reflow
  item1
  item2

# we decide to push item2 ...

=================

GridContainer:frag1
  item1
  OverflowList<
    item2
  >
GridContainer:frag2  <---- Reflow
 <empty>

# item2 fits but has some child overflow so we create an EOC for it ...

=================

GridContainer:frag1
  item1
GridContainer@7f2165e30260:frag2
  item2:frag1
  ExcessOverflowContainersList<
    item2:frag2
  >
GridContainer:frag3  <---- Reflow
 <empty>

# frag 3 pulls in item2:frag2 to its OC list ...

=================

# script removes item1

=================

GridContainer:frag1      <---- Reflow
  <empty>
GridContainer:frag2
  item2:frag1
GridContainer:frag3
  OverflowContainersList<
    item2:frag2
  >

# GridContainer:frag1 pulls up item2:frag1 ...

=================

GridContainer:frag1
  item2:frag1
GridContainer:frag2      <---- Reflow
  <empty>
GridContainer:frag3
  OverflowContainersList<
    item2:frag2
  >

fails assert in SanityCheckGridItemsBeforeReflow:

  MOZ_ASSERT(!childNIF || mFrames.ContainsFrame(childNIF) ||
             (pifEOC && pifEOC->ContainsFrame(childNIF)) ||
             (oc && oc->ContainsFrame(childNIF)) ||
             (eoc && eoc->ContainsFrame(childNIF)));

where childNIF == item2:frag2 because we expect it to be
a child of GridContainer:frag2.

Comment 3

[Mats Palmgren (inactive)]

Comment on attachment 8750481 [details] [diff] [review]
part 1 - [css-grid] Make grid item 'stretch' not require an extra reflow.

I forgot to say that the "#if 0" block comes off in the last patch,
so please ignore that bit for now.

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 3 - Only merge in children from the EOC list that doesn't already have a prev-in-flow in this frame.

Not directly Grid related, but maybe it can only be triggered by fragmented
grid layout currently...

The frame tree looked something like this:

<grid-container:frag1
  ...
  OverflowList<
    item1:frag1
  >
>

<grid-container:frag2
  ...
  ExcessOverflowContainersList<
    item1:frag2
  >
>

then we reflow grid-container:frag2 whcih picked up item1:frag1,
so now we have:
<grid-container:frag2
  item1:frag1
  ExcessOverflowContainersList<
    item1:frag2
  >
>

Then nsContainerFrame::ReflowOverflowContainerChildren would drain EOC:
https://hg.mozilla.org/mozilla-central/annotate/043082cb7bd8/layout/generic/nsContainerFrame.cpp#l1675
so the reflowed grid-container:frag2 frame tree is:
<grid-container:frag2
  OverflowContainersList<
    item1:frag2
  >
  item1:frag1
>

item1's fragments are now in the "wrong order" since frag2 will be reflowed
before frag1.  This leads to the same assertion as above.  Apart from that,
I don't think anything bad can happen from this other than minor layout
errors (and also the item1:frag2 reflow is wasted) in very rare edge cases.

We should fix this so that we always have correct frame trees though.

Comment 5

[Mats Palmgren (inactive)]

Attachment: part 4 - [css-grid] Make sure a grid item's last fragment fills its grid area.

This fixes a fragmentation edge case where a grid item's first fragment
overflows its row which makes its computed bsize no longer fill up
its grid area at the end.

Comment 6

[Mats Palmgren (inactive)]

Attachment: part 5 - [css-grid] Reftest tweaks.

Comment 7

[Mats Palmgren (inactive)]

Attachment: part 6 - [css-grid] Remove now dead code.

Comment 8

[Mats Palmgren (inactive)]

That's all the parts.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=78cfbfaec6d7

Comment 9

[Daniel Holbert [:dholbert]]

Sorry for review delay; in CSSWG meetings for most of this week, and haven't found much time for reviews. I hope to get to this soon, though.

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8750481 [details] [diff] [review]
part 1 - [css-grid] Make grid item 'stretch' not require an extra reflow.

Review of attachment 8750481 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry again for the delay here. r=me on this part, with nits addressed:

::: layout/base/nsLayoutUtils.cpp
@@ +5474,5 @@
> +  } else if (MOZ_UNLIKELY(isGridItem)) {
> +    MOZ_ASSERT(!IS_TRUE_OVERFLOW_CONTAINER(aFrame));
> +    // 'auto' block-size for grid-level box - apply 'stretch' as needed:
> +    auto cbSize = aCBSize.BSize(aWM);
> +    const auto& styleMargin = aFrame->StyleMargin()->mMargin;

Can't we simply use the "aMargin" arg here? I think that's the same as this new variable, and it saves us an extra lookup.

@@ +5478,5 @@
> +    const auto& styleMargin = aFrame->StyleMargin()->mMargin;
> +    if (cbSize != NS_AUTOHEIGHT &&
> +        styleMargin.GetBStartUnit(aWM) != eStyleUnit_Auto &&
> +        styleMargin.GetBEndUnit(aWM) != eStyleUnit_Auto) {
> +      auto blockAxisAlignment = !aWM.IsOrthogonalTo(aFrame->GetParent()->GetWritingMode()) ?

Two things:
 (1) This line is 93 characters long. Probably needs a wrap after the "=".

 (2) This assignment is pretty complex (multiple writing modes, style contexts, alignment properties, and possible block axes in play).  Consider adding a comment to sum up what we're doing here, e.g.:
  // Get aFrame's css-align property for its parent's block axis:

@@ +5484,5 @@
> +        stylePos->ComputedJustifySelf(aFrame->StyleContext()->GetParent());
> +      if (blockAxisAlignment == NS_STYLE_ALIGN_NORMAL ||
> +          blockAxisAlignment == NS_STYLE_ALIGN_STRETCH) {
> +        bSize = std::max(nscoord(0), cbSize -
> +                                     aPadding.BSize(aWM) -

The indentation of "aPadding" here (and subsequent lines) isn't consistent with the similar code at the end of your nsFrame.cpp chunk here. Please make those consistent, one way or another.

(I'm not 100% sure which is correct, but emacs auto-indentation says the style you've got here is wrong -- it seems to prefer having aPadding de-indented back to the open-paren.  That matches the style you're using in nsFrame.cpp, too.  So, this chunk here in nsLayoutUtils is probably the chunk that needs fixing.)

::: layout/generic/nsFrame.cpp
@@ +4715,5 @@
> +               blockStyleCoord->GetUnit() == eStyleUnit_Auto &&
> +               !IS_TRUE_OVERFLOW_CONTAINER(this)) {
> +      // 'auto' block-size for grid-level box - apply 'stretch' as needed:
> +      auto cbSize = aCBSize.BSize(aWM);
> +      const auto& styleMargin = StyleMargin()->mMargin;

As in nsLayoutUtils, we have |aMargin| here; let's use that, instead of looking it up again.

@@ +4719,5 @@
> +      const auto& styleMargin = StyleMargin()->mMargin;
> +      if (cbSize != NS_AUTOHEIGHT &&
> +          styleMargin.GetBStartUnit(aWM) != eStyleUnit_Auto &&
> +          styleMargin.GetBEndUnit(aWM) != eStyleUnit_Auto) {
> +        auto blockAxisAlignment = !aWM.IsOrthogonalTo(GetParent()->GetWritingMode()) ?

As noted above for similar code in nsLayoutUtils: (1) this line is too long, and (2) its multi-line assignment might benefit from a comment.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8750482 [details] [diff] [review]
part 2 - [css-grid] When pulling up grid item, make sure its next-in-flow (if any) is in our next-in-flow.

Review of attachment 8750482 [details] [diff] [review]:
-----------------------------------------------------------------

This part makes sense, though I don't immediately see its connection to this bug about "stretch". Maybe because this bug lets grid items be a bit smaller (via fixing bug 1258443), and that makes one of our existing reftests trigger the scenario you describe in comment 2?

(If that's the case: hopefully that means we've already got test coverage for the scenario described in comment 2. :)  If we don't, I'd very much like to have test coverage, since this stuff is tricky.)

If you like, feel free to spin this patch off into a helper-bug, whose fix might even want to land before the rest of the patches here, since it seems orthogonal & seems like it'd already be a problem regardless of the other patches here.

Anyway, r=me regardless.

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #11)
> This part makes sense, though I don't immediately see its connection to this
> bug about "stretch".

(This applies to parts 3 and 4, I guess, too. :))

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8750494 [details] [diff] [review]
part 3 - Only merge in children from the EOC list that doesn't already have a prev-in-flow in this frame.

Review of attachment 8750494 [details] [diff] [review]:
-----------------------------------------------------------------

Commit message grammar nit:
> Bug 1271392 part 3 - Only merge in children from the EOC list that doesn't already have a prev-in-flow in this frame. r=dholbert

s/doesn't/don't/  (so it'll end up "children ... that don't")

::: layout/generic/nsContainerFrame.cpp
@@ +1673,5 @@
>    }
>  
>    // Our own excess overflow containers from a previous reflow can still be
> +  // present if our next-in-flow hasn't been reflown yet.  Move any children
> +  // from it that doesn't have a continuation in this frame to the

Similar to the commit message, this needs s/doesn't/don't/

("Move any children...that don't have")

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8750500 [details] [diff] [review]
part 4 - [css-grid] Make sure a grid item's last fragment fills its grid area.

Review of attachment 8750500 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not as sure about this part -- notes below:

::: layout/generic/nsBlockFrame.cpp
@@ +1626,5 @@
> +    }
> +  } else if (aReflowState.AvailableBSize() != NS_UNCONSTRAINEDSIZE &&
> +             !NS_INLINE_IS_BREAK_BEFORE(aState.mReflowStatus) &&
> +             NS_FRAME_IS_COMPLETE(aState.mReflowStatus)) {
> +    // Currently only used for grid items, but could be used in other contexts.

I don't fully understand what's going on in this new code.  Please add an explanation for what we're actually doing here (i.e. we're imposing a block-size passed in by our container, I think?) -- and also, please hint at why we're checking these specific conditions apply.  i.e. it seems we only do this for non-overflow-container frames that are being reflowed with a constrained bsize & that don't have "break-before" and that are complete. Why?)

::: layout/generic/nsGridContainerFrame.cpp
@@ +4133,5 @@
>    Maybe<nsHTMLReflowState> childRS; // Maybe<> so we can reuse the space
>    childRS.emplace(pc, *aState.mReflowState, aChild, childCBSize, &percentBasis);
>    childRS->mFlags.mIsTopOfPage = aFragmentainer ? aFragmentainer->mIsTopOfPage : false;
> +
> +  // If the child is stretching in its block axis, then setup a frame

This first clause needs an additional condition:
  "...and we might be fragmenting it in that axis,"

(I think that's a large part of what your "isConstrainedBSize && !wm.IsOrthogonalTo(childWM)" check is getting at, right?)

@@ +4139,5 @@
> +  if (isConstrainedBSize && !wm.IsOrthogonalTo(childWM)) {
> +    bool stretchBSize = false;
> +    const auto& styleMargin = childRS->mStyleMargin->mMargin;
> +    if (styleMargin.GetBStartUnit(childWM) != eStyleUnit_Auto &&
> +        styleMargin.GetBEndUnit(childWM) != eStyleUnit_Auto &&

With this code ^ and a few copies in Part 1, we've now got this pattern repeated 3+ times.  And each instance involves some repetition (the name of the margin, the wm, and eStyleUnit_Auto).

I think we should add a convenience method to collapse this pattern into a simpler one-liner, and remove this repetition.

Maybe something like:
  nsLayoutUtils::MarginHasAutoInBlockAxis(myLogicalMargin, wm)
...or:
  myLogicalaArgin.HasAutoInAxis(wm, eLogicalAxisBlock);
...or something like that.

Could you file a followup (or post a followup patch here) to add an convenience accessor like that?

@@ +4527,5 @@
> +    aDesiredSize.BSize(aState.mWM) =
> +      aState.mRows.GridLineEdge(std::min(aEndRow, info->mArea.mRows.mEnd),
> +                                GridLineSide::eBeforeGridGap) -
> +      aState.mRows.GridLineEdge(std::max(aStartRow, row),
> +                                GridLineSide::eAfterGridGap);

This seems like an odd use of aDesiredSize -- I think it's an outparam at this point (all the way up the stack, pretty much), but this seems like it's co-opting aDesiredSize and turning it into an in/out-param (but only at this one level in the callstack).

PLUS, other callers to ReflowInFlowChild don't do this, so presumably for them aDesiredSize.BSize() is uninitialized (probably at 0?)  So from those callsites, ReflowInFlowChild could end up using that uninitialized/0 value, if it inspects this piece of aDesiredSize.

SO: Could you drop this (ab)use of aDesiredSize, and instead pass this "stretch size" in via an explicit dedicated ReflowInFlowChild arg? Perhaps as "Maybe<nscoord> aBlockStretchSize", and other callers (besides this callsite here) would pass Nothing(), and ReflowInFlowChild() would check (or assert) that this value isSome() before it uses it?

Comment 15

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> > +    const auto& styleMargin = aFrame->StyleMargin()->mMargin;
> 
> Can't we simply use the "aMargin" arg here? I think that's the same as this
> new variable, and it saves us an extra lookup.

aMargin is a LogicalSize, it doesn't have the style units we need.

Comment 16

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #11)
> (If that's the case: hopefully that means we've already got test coverage
> for the scenario described in comment 2. :)

Yes, part 2 and 3 fixes bugs that fell out from part 1 making us
take new and exciting paths through the reflow code :-)
(I'm not sure if it's posibble to trigger these bugs without
part 1.)

Comment 17

[Mats Palmgren (inactive)]

Attachment: part 4, v2 - [css-grid] Make sure a grid item's last fragment fills its grid area.

(In reply to Daniel Holbert [:dholbert] from comment #14)
> > +  } else if (aReflowState.AvailableBSize() != NS_UNCONSTRAINEDSIZE &&
> > +             !NS_INLINE_IS_BREAK_BEFORE(aState.mReflowStatus) &&
> > +             NS_FRAME_IS_COMPLETE(aState.mReflowStatus)) {
> > +    // Currently only used for grid items, but could be used in other contexts.
> 
> I don't fully understand what's going on in this new code.  Please add an
> explanation for what we're actually doing here (i.e. we're imposing a
> block-size passed in by our container, I think?) -- and also, please hint at
> why we're checking these specific conditions apply.  i.e. it seems we only
> do this for non-overflow-container frames that are being reflowed with a
> constrained bsize & that don't have "break-before" and that are complete.
> Why?)

The !NS_INLINE_IS_BREAK_BEFORE check is simply because
COMPLETE/INCOMPLETE is undefined when it is set (But yeah,
since BREAK_BEFORE always triggers another reflow most places
don't bother to check).

I added a longer code comment explaining what the code is about.

> ::: layout/generic/nsGridContainerFrame.cpp
> This first clause needs an additional condition:
>   "...and we might be fragmenting it in that axis,"
> 
> (I think that's a large part of what your "isConstrainedBSize &&
> !wm.IsOrthogonalTo(childWM)" check is getting at, right?)

Yes, fixed.

> I think we should add a convenience method to collapse this pattern into a
> simpler one-liner, and remove this repetition.

I filed bug 1273705, and replaced all three occurrences.

> This seems like an odd use of aDesiredSize -- I think it's an outparam at
> this point (all the way up the stack, pretty much), but this seems like it's
> co-opting aDesiredSize and turning it into an in/out-param (but only at this
> one level in the callstack).

Fair enough, I added a Maybe<nscoord> param instead.

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 8753600 [details] [diff] [review]
part 4, v2 - [css-grid] Make sure a grid item's last fragment fills its grid area.

Review of attachment 8753600 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! This looks good.

r=me, modulo some Maybe<> usage nits (dropping explicit Maybe<> variables from the callsites, basically).

::: layout/generic/nsGridContainerFrame.cpp
@@ +4296,5 @@
>    // Reflow our placeholder children; they must all be complete.
>    for (auto child : placeholders) {
>      nsReflowStatus childStatus;
> +    Maybe<nscoord> nothing;
> +    ReflowInFlowChild(child, nullptr, aContainerSize, nothing, &aFragmentainer,

You should be able to just pass in the expression "Nothing()" directly -- no need to create a local variable. See http://mxr.mozilla.org/mozilla-central/source/mfbt/Maybe.h?rev=de42116d5ef3#38

@@ +4573,5 @@
> +    bSize.emplace(
> +      aState.mRows.GridLineEdge(std::min(aEndRow, info->mArea.mRows.mEnd),
> +                                GridLineSide::eBeforeGridGap) -
> +      aState.mRows.GridLineEdge(std::max(aStartRow, row),
> +                                GridLineSide::eAfterGridGap));

Given that we *always* emplace bSize here, better to just have it be a nscoord, and wrap it in a Maybe<> for the function call using "Some()".

So:
 nscoord bSize = [stuff] -
   [stuff];
 ReflowInFlowChild(child, info, aContainerSize, Some(bSize), &aFragmentainer,
                   [...])

@@ +4795,5 @@
>        if (child->GetType() != nsGkAtoms::placeholderFrame) {
>          info = &aState.mGridItems[aState.mIter.GridItemIndex()];
>        }
> +      Maybe<nscoord> nothing;
> +      ReflowInFlowChild(*aState.mIter, info, containerSize, nothing, nullptr,

As above, just pass Nothing() directly here.

::: layout/generic/nsGridContainerFrame.h
@@ +233,5 @@
>    // Helper for ReflowChildren / ReflowInFragmentainer
>    void ReflowInFlowChild(nsIFrame*              aChild,
>                           const GridItemInfo*    aGridItemInfo,
>                           nsSize                 aContainerSize,
> +                         mozilla::Maybe<nscoord>aStretchBSize,

Needs a space between ">" and "a", for readability if not compilability. (which probably means the rest of the args want to be reindented; feel free to do that in a oneoff whitespace-only followup, if you wanted to avoid all of those changes in this patch)

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8753600 [details] [diff] [review]
part 4, v2 - [css-grid] Make sure a grid item's last fragment fills its grid area.

Review of attachment 8753600 [details] [diff] [review]:
-----------------------------------------------------------------

sorry, one more review nit -- just a comment typo:

::: layout/generic/nsBlockFrame.cpp
@@ +1630,5 @@
> +    // Currently only used for grid items, but could be used in other contexts.
> +    // The FragStretchBSizeProperty is our expected non-fragmented block-size
> +    // we should stretch to (for align-self:stretch etc).  In some fragmentation
> +    // cases though, the last fragment (this frame since we're complete), needs
> +    // to have extra size applied because earlier fragments consumed to much of

s/to much/too much/

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/141c11f55522
https://hg.mozilla.org/integration/mozilla-inbound/rev/98710f78e04d
https://hg.mozilla.org/integration/mozilla-inbound/rev/017a3d4acd7f
https://hg.mozilla.org/integration/mozilla-inbound/rev/9377969bb65f
https://hg.mozilla.org/integration/mozilla-inbound/rev/24f0b01fc31d
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e11a2a81e37

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/141c11f55522
https://hg.mozilla.org/mozilla-central/rev/98710f78e04d
https://hg.mozilla.org/mozilla-central/rev/017a3d4acd7f
https://hg.mozilla.org/mozilla-central/rev/9377969bb65f
https://hg.mozilla.org/mozilla-central/rev/24f0b01fc31d
https://hg.mozilla.org/mozilla-central/rev/5e11a2a81e37