1466358 - [css-grid-2] Implement subgrid support in track sizing

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P2)

Comment 1

[Mats Palmgren (inactive)]

Attachment: part 1 - [css-grid-2] Add grid item state bits for items in a subgrid and set them when the item occupies the corresponding edge track (idempotent patch)

Comment 2

[Mats Palmgren (inactive)]

Comment on attachment 8984780 [details] [diff] [review]
part 1 - [css-grid-2] Add grid item state bits for items in a subgrid and set them when the item occupies the corresponding edge track (idempotent patch)

Hmm, this doesn't work quite as I intended...

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 1 - [css-grid-2] Add grid item state bits for subgrids and items in a subgrid and set them when the item occupies the corresponding edge track (idempotent patch)

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-grid-2] Add a method to recursively collect relevant items for track sizing from subgrids with their lines and state translated into the caller's coordinates (idempotent patch)

FYI, this will be called for each non-subgrid axis independently to
append descendant subgrid items to 'mGridItems' temporarily during
track sizing.  We're intentionally skipping 'mAbsPosItems' since
those do not affect sizing at all.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8984891 [details] [diff] [review]
part 2 - [css-grid-2] Add a method to recursively collect relevant items for track sizing from subgrids with their lines and state translated into the caller's coordinates (idempotent patch)

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

::: layout/generic/nsGridContainerFrame.cpp
@@ +644,5 @@
>  
> +  /**
> +   * Return a copy of this item with its row/column data swapped.
> +   */
> +  GridItemInfo Transpose() const

I'm slightly uneasy about this naming name right now -- it sounds a lot like this function *performs a Transpose() operation* on the item. Like how this Matrix Transpose() API does:
https://dxr.mozilla.org/mozilla-central/rev/8ab6afabc78cd909ff90ba74c1eab098985f83ef/gfx/2d/Matrix.h#1186-1196 

(It's clear what it really does when I look at it here, but I'm thinking about its callsites being confusing.)

It might be better to name it "TransposedCopy()", or "TransposedVersion()", or "AsTranspose()", so that it's clearer what's happening at callsites (particularly: so that it's clearer that the original item isn't being modified).

@@ +2333,5 @@
> +  static void
> +  CollectSubgridForAxis(LogicalAxis aAxis,
> +                        WritingMode aContainerWM,
> +                        const LineRange& aRangeInAxis,
> +                        const LineRange& aRangeInOppositeAxis,

Perhaps it'd be clearer & more consistent to use "orthog" instead of "opposite"? (for this arg and in variables throughout this patch)

The word "Opposite" feels too ambiguous here, given that subgrid code also has considerations for *reversed same-axis directionality* (e.g. RTL vs LTR) between child and parent.  Arguably, that concept (rtl vs ltr) is closer to a true "opposite axis" - so variables named like this one are easy to misread as being related to that.

In contrast, "orthogonal" (abbreviated if you like) is less ambiguous in that respect, so it seems clearer to stick with that wording, particularly given that it's what our WritingMode / LogicalAxis APIs use.

(If you were worried about ambiguity between these variables & the "isOrthogonal" variable lower down in this function, perhaps the solution is to rename isOrthogonal to something more specific, like "isOrthogToContainer")

@@ +2348,5 @@
> +    bool isSameDirInOppositeAxis =
> +      subgridWM.ParallelAxisStartsOnSameSide(oppositeAxis, aContainerWM);
> +    if (isOrthogonal) {
> +      // We'll Transpose the area below so these needs to be transposed as well.
> +      Swap(isSameDirInAxis, isSameDirInOppositeAxis);

s/needs/need/

But, really: you should probably avoid this Swap() special-case entirely by simply setting up (and interpreting) these bools from the perspective of the container, rather than the perspective of the subgrid -- just swapping the two in these bool's expressions.  Like so, I think:

  bool isSameDirInAxis =
    aContainerWM.ParallelAxisStartsOnSameSide(aAxis, subgridWM);
  bool isSameDirInOppositeAxis =
    aContainerWM.ParallelAxisStartsOnSameSide(oppositeAxis, subgridWM);

That seems better to me, given that the argument aAxis is indeed really meant to be interpreted as an axis of the *container* -- it's not an axis of the subgrid.

@@ +2353,5 @@
> +    }
> +    uint32_t offsetInAxis = aRangeInAxis.mStart;
> +    uint32_t gridEndInAxis = aRangeInAxis.Extent();
> +    uint32_t offsetInOppositeAxis = aRangeInOppositeAxis.mStart;
> +    uint32_t gridEndInOppositeAxis = aRangeInOppositeAxis.Extent();

"gridEnd" feels wrong/confusing here, since
 - this probably is *not* the end of the grid.
 - this really isn't an "end" at all -- it's more of a length, kind of, right?  (It's set by Extent() which ends with "return mEnd - mStart", and that subtraction-result feels strange to be capturing in something called "End".)

Maybe better to call this "extentInAxis" and "extentInOrthogAxis"?  Still not crystal-clear but perhaps a bit clearer and more consistent, concept-naming-wise.

(Same goes for the last arg to the ReverseDirection helper-function, which is what this variable gets passed as.)

@@ +2382,5 @@
> +    }
> +  }
> +
> +  // Copy all descendant items from all our subgrid children that are subgridded
> +  // in aAxis recursively into aResult.  All item grid area's and state are

s/area's/areas/

Comment 6

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-grid-2] Add methods to calculate a subgrid's border+padding+margin

Comment 7

[Mats Palmgren (inactive)]

Attachment: part 4 - [css-grid-2] Make a subgrid contribute its accumulated border+padding+margin to its edge tracks for intrinsically sized tracks

Comment 8

[Mats Palmgren (inactive)]

Attachment: part 5 - [css-grid-2] Add the accumulated border+padding+margin of subgrid ancestors to a subgrid item's content contribution if it spans the relevant edge track(s)

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8985265 [details] [diff] [review]
part 3 - [css-grid-2] Add methods to calculate a subgrid's border+padding+margin

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

Initial notes from a quick skim:

::: layout/generic/nsGridContainerFrame.cpp
@@ +4248,5 @@
> +static nsMargin
> +CalculateSubgridPaddingBorderMargin(nsIFrame* aFrame,
> +                                    nscoord aPercentageBasis)
> +{
> +  nsMargin pbm;

We should probably standardize on an ordering for the margin / border / padding in variables & functions like this one.

In my mind it's "MarginBorderPadding" / "mbp", and that seems to be the more common usage, per these cases:
 - ReflowInput has a method ComputedSizeWithMarginBorderPadding()
 - nsFlexContainer.cpp has a helper function "GetMarginBorderPaddingSizeInAxis" and several "MBP" variables.
 - [ignoring Margin] various files have members/methods called "BorderPadding()" / "mBorderPadding" (but never PaddingBorder)


I found only one exception: nsContainerFrame has a few lines of code that uses "PBM" variables (with a comment saying PaddingBorderMargin).  But aside from that one spot, we're entirely consistent on the MarginBorderPadding spelling.

So it'd probably be worth reordering this new code to standardize on the more common spelling... what do you think?  (Search-And-Replace should make this pretty trivial I hope)

@@ +4367,5 @@
> +    nsIScrollableFrame* scrollFrame =
> +      nsLayoutUtils::GetNearestScrollableFrame(subgridFrame);
> +    if (scrollFrame) {
> +      nsMargin ssz = scrollFrame->GetActualScrollbarSizes();
> +      subgrid->mPaddingBorderMargin += LogicalMargin(cbWM, ssz);

This chunk of code would benefit a brief comment calling out what we're doing and why...  And perhaps we should mention this alongside the definition of this member-variable. (e.g. adding "(and possibly scrollbar sizing)" to the end of the member-variable comment)

(Strictly speaking, scrollbar sizes are *not* part of padding/border/margin, which makes this look confusing/suspect.   I can imagine that it might be useful to account for them in the same quantity, though, so this is probably fine as long as it's briefly explained/documented.)

Comment 10

[Mats Palmgren (inactive)]

I think PaddingBorderMargin is slightly more logical (going outward),
but I don't feel strongly about it so if the MarginBorderPadding
order is already established I don't mind changing it to that.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8985265 [details] [diff] [review]
part 3 - [css-grid-2] Add methods to calculate a subgrid's border+padding+margin

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

Sorry for the delay here!  r- for now, since I think there's a good deal I'd like to better understand and/or see clarified/adjusted.

::: layout/generic/nsGridContainerFrame.cpp
@@ +4250,5 @@
> +                                    nscoord aPercentageBasis)
> +{
> +  nsMargin pbm;
> +  const nsStyleDisplay* disp = aFrame->StyleDisplay();
> +  bool havePadding = false;

Maybe call this "haveWidgetPadding", since that's what this really seems to represent?

(For the non-themed codepath, we never bother to set havePadding = true, even if there's padding.)

@@ +4286,5 @@
> +    auto ResolvePadding = [aPercentageBasis] (const nsStyleCoord& aCoord) {
> +      return nsLayoutUtils::ResolveToLength<true>(aCoord, aPercentageBasis);
> +    };
> +    const nsStylePadding* stylePadding = aFrame->StylePadding();
> +    pbm += nsMargin(ResolvePadding(stylePadding->mPadding.GetTop()),

Hmm... This stuff (particularly the GetWidgetPadding vs. normal-padding stuff, but really this whole method) seems to duplicate some logic from SizeComputationInput::InitOffsets.

Any chance we can share that code?  Perhaps by instantiating a SizeComputationInput at some level of abstraction here and reading precomputed values off of it?  (Note that SizeComputationInput is the cheaper-to-make superclass of ReflowInput, which just does InitOffsets and pretty much nothing else.)

@@ +4315,5 @@
> +  bool skipEndSide = false;
> +  auto axis = aSubgrid->mIsOrthogonal ? GetOrthogonalAxis(aAxis) : aAxis;
> +  while (parent->IsSubgrid(axis)) {
> +    auto* parentSubgrid = parent->GetProperty(Subgrid::Prop());
> +    auto* grandParent = parent->ParentGridContainerForSubgrid();

Can you add a comment somewhere explaining...
 - ...what the concepts of "child" (as used in childRange), "parent", and "grandparent" are meant to represent at some arbitrary iteration of the loop?  (It looks like "parent" is always the subgrid whose margin/border/padding we're currently considering, but that's a bit non-intuitive, so it's nice to call out, if I'm even understanding it correctly).
 - ...what the high-level point of each loop iteration is? (it looks like in a loop iteration, we're getting |parent|'s border/padding/margin and adding it to result, I think...?) 

This is partly a bit confusing because the naming only makes sense to me for the first time through the loop (where parent/grandparent are truly the parent/grandparent of aSubgrid).  For subsequent loop iterations, parent/grandparent are more distant ancestors, so the intuitive takeaway from their naming isn't consistent with how they're actually used.

@@ +4324,5 @@
> +    const auto& parentRange = parentSubgrid->mArea.LineRangeForAxis(axis);
> +    bool sameDir = parentCBWM.ParallelAxisStartsOnSameSide(axis, subgridCBWM);
> +    if (sameDir) {
> +      skipStartSide |= childRange.mStart != 0;
> +      skipEndSide |= childRange.mEnd != parentRange.Extent();

Can you add a comment explaining the point of this side skipping logic? I don't understand why we're skipping sides.

@@ +4357,5 @@
> +                                  const LogicalSize&  aPercentageBasis)
> +{
> +  auto* subgridFrame = aGridItem.SubgridFrame();
> +  auto* subgrid = subgridFrame->GetProperty(Subgrid::Prop());
> +  nscoord percentBasis = aPercentageBasis.ISize(subgridFrame->GetWritingMode());

Shouldn't the percent basis be the ISize from the perspective of the *containing block*?

If we're calculating border/padding/margin on aGridItem.mFrame itself, then I'd expect percentages in those quantities should be resolved against its containing block's ISize... (and the containing block's inline axis could be the subgrid's block axis)

(And really, if we're a nested subgrid, maybe our containing block is ultimately defined by grid lines in the outermost grid, and so that's whose writing mode we should be using for determining what is the ISize to resolve percent margin/padding against?)

In any case, even if I'm misunderstanding and this is fine: a brief explanatory comment would be helpful here.

@@ +4374,5 @@
> +    if (aGridItem.mFrame->IsFieldSetFrame()) {
> +      const auto* f = static_cast<nsFieldSetFrame*>(aGridItem.mFrame);
> +      nsRect r = f->VisualBorderRectRelativeToSelf();
> +      nsMargin topLeft(r.y, 0, 0, r.x);
> +      subgrid->mPaddingBorderMargin += LogicalMargin(cbWM, topLeft);

This seems to be assuming that the legend is only on the top/left side, I think...? (based on naming & the args you're using in nsMargin).  But I don't think that's guaranteed to be the case.  Consider this fieldset with a legend on the right side:

data:text/html,<fieldset style="writing-mode: vertical-rl;direction:rtl"><legend>LLL</legend>CCC

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8985266 [details] [diff] [review]
part 4 - [css-grid-2] Make a subgrid contribute its accumulated border+padding+margin to its edge tracks for intrinsically sized tracks

[--> canceling reviews on parts 4 & 5 to reflect reality -- I didn't get to them back when I was reviewing the rest of this, because I was fuzzy on part 3, which they interact with & build on top of. Not sure if the review feedback in part 3 will influence them, but in any case, I'll gladly review them when we've got an updated patch stack here.]

Comment 13

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-grid-2] Add methods to calculate a subgrid's margin+border+padding

(In reply to Daniel Holbert [:dholbert] from comment #11)
> ::: layout/generic/nsGridContainerFrame.cpp
> > +    const nsStylePadding* stylePadding = aFrame->StylePadding();
> > +    pbm += nsMargin(ResolvePadding(stylePadding->mPadding.GetTop()),
> 
> Hmm... This stuff (particularly the GetWidgetPadding vs. normal-padding
> stuff, but really this whole method) seems to duplicate some logic from
> SizeComputationInput::InitOffsets.

Fair enough, I guess it should work to instantiate a local
SizeComputationInput and get the values off that.

> @@ +4315,5 @@
> Can you add a comment somewhere explaining...
>  - ...what the concepts of "child" (as used in childRange), "parent", and
> "grandparent" are meant to represent at some arbitrary iteration of the
> loop?
[...]
> > +      skipStartSide |= childRange.mStart != 0;
> > +      skipEndSide |= childRange.mEnd != parentRange.Extent();
> 
> Can you add a comment explaining the point of this side skipping logic? I
> don't understand why we're skipping sides.

Sure, I added a comment explaining that we walk up the ancestor
tree (for nested subgrids) and add their MBP on any edges that
are adjacent (i.e. start/end in the same track).

> Shouldn't the percent basis be the ISize from the perspective of the
> *containing block*?

Yes, good catch.

> @@ +4374,5 @@
> > +    if (aGridItem.mFrame->IsFieldSetFrame()) {
> > +      const auto* f = static_cast<nsFieldSetFrame*>(aGridItem.mFrame);
> > +      nsRect r = f->VisualBorderRectRelativeToSelf();
> > +      nsMargin topLeft(r.y, 0, 0, r.x);
> > +      subgrid->mPaddingBorderMargin += LogicalMargin(cbWM, topLeft);
> 
> This seems to be assuming that the legend is only on the top/left side, I
> think...?

Good point, fixed.  Also, we shouldn't use VisualBorderRectRelativeToSelf
here since that only accounts for half the "border area".
I think the full BStart offset to the innner frame is the correct amount.

(Hmm, I wonder if we could generalize that to all wrapper frame cases,
i.e. when "aGridItem.mFrame != subgridFrame".  That would also handle
scrollbars for free I think.  I'll consider that for a follow-up bug...)

Comment 14

[Mats Palmgren (inactive)]

Attachment: part 4 - [css-grid-2] Make a subgrid contribute its accumulated margin+border+padding to its edge tracks for intrinsically sized tracks

Comment 15

[Mats Palmgren (inactive)]

Attachment: part 5 - [css-grid-2] Add the accumulated margin+border+padding of subgrid ancestors to a subgrid item's content contribution if it spans the relevant edge track(s)

Comment 16

[Daniel Holbert [:dholbert]]

Would you mind posting unbitrotted versions of parts 1 and 2 as well, so that I can apply the full patch stack locally to assist in reviewing?

(Or alternately, if you prefer, "moz-phab submit" the stack, & I'll be happy to review over in phabricator)

Comment 17

[Mats Palmgren (inactive)]

Attachment: rollup patch up to and including part 5 in this bug

The patches for bug 1465296 is still sitting in my queue, so perhaps
a rollup patch is easier?

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 9029441 [details] [diff] [review]
part 3 - [css-grid-2] Add methods to calculate a subgrid's margin+border+padding

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

::: layout/generic/nsGridContainerFrame.cpp
@@ +2734,5 @@
> +  auto* subgrid = subgridFrame->GetProperty(Subgrid::Prop());
> +  subgrid->mMarginBorderPadding = LogicalMargin(cbWM, physicalMBP);
> +  if (aGridItem.mFrame != subgridFrame) {
> +    nsIScrollableFrame* scrollFrame =
> +        nsLayoutUtils::GetNearestScrollableFrame(subgridFrame);

I don't think GetNearestScrollableFrame is what we want here. AFAICT, that walks an arbitrary distance[1] up the tree to find the nearest scrollable ancestor -- so if we get here with a non-scrollable subgrid somehow (e.g. with a fieldset or some other frame with a non-scrollframe anon box wrapper), then it looks like this GetNearestScrollableFrame() call will walk up to some arbitrary ancestor here (maybe the parent of the outer grid), and this'll add bogus metrics from that frame's scrollbars.

Perhaps this simply wants to just test whether aGridItem.mFrame is itself a scrollable frame, maybe by calling "GetScrollTargetFrame()" on it? [2]

[1] https://searchfox.org/mozilla-central/rev/69f9d5002c6e3c5c571a348916fb174e6a7b4acd/layout/base/nsLayoutUtils.h#600-602
[2] https://searchfox.org/mozilla-central/rev/69f9d5002c6e3c5c571a348916fb174e6a7b4acd/layout/generic/nsIFrame.h#697-704

@@ +2747,5 @@
> +      auto wm = inner->GetWritingMode();
> +      LogicalPoint pos = inner->GetLogicalPosition(f->GetSize());
> +      // The legend is always on the BStart side.
> +      LogicalMargin offsets(wm, pos.B(wm), 0, 0, 0);
> +      subgrid->mMarginBorderPadding += offsets.ConvertTo(cbWM, wm);

Right now it feels like this comment is expressing an afterthought without explaining the key thing that it's an afterthought for.

Could you maybe extend the comment to say:
      // Include the legend's BSize in margin/border/padding. (The legend's
      // BSize is exactly the block-axis position of fieldset inner frame,
      // because the legend is always on the BStart side of the fieldset.)

@@ +4039,5 @@
> + * Return the accumulated margin+border+padding in aAxis for aFrame (a subgrid)
> + * and its ancestor subgrids.
> + */
> +static LogicalMargin SubgridAccumulatedMarginBorderPadding(
> +    nsIFrame* aFrame, const Subgrid* aSubgrid, WritingMode aCBWM,

Could you document what aCBWM is expected to be here? (which CB is it expected to be?  Is it 'subgrid' CB, or some arbitrary CB?

Note that we immediately figure out "subgridCBWM" which seems easy to confuse with aCBWM, and maybe seems like it's what aCBWM would be expected to be, depending on the interpretation of that variable.

Maybe we should just call it "aResultWM" to indicate that it's the writing mode that the output is expected to be computed with respect to, without having to worry about (& document/reason-about) which/whose containing block(s) this argument happens to be for?

@@ +4053,5 @@
> +  auto axis = aSubgrid->mIsOrthogonal ? GetOrthogonalAxis(aAxis) : aAxis;
> +  // If aFrame's parent is also a subgrid, then add its MBP on the edges that
> +  // are adjacent (i.e. start or end in the same track), recursively.
> +  // ("parent" refers to the frame we're currently adding MBP for,
> +  // and "grandParent" its parent, as we walk up the chain.)

maybe s/frame/grid-frame/ here?

(This would make it clearer that you're talking about "parent" from a ParentGridContainerForSubgrid() perspective, and not from a frame->GetParent() perspective)

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #17)
> The patches for bug 1465296 is still sitting in my queue, so perhaps
> a rollup patch is easier?

(Sure, that's fine -- thanks!)

Comment 20

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-grid-2] Add methods to calculate a subgrid's margin+border+padding

Comment 21

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #18)
> > +        nsLayoutUtils::GetNearestScrollableFrame(subgridFrame);
> Perhaps this simply wants to just test whether aGridItem.mFrame is itself a
> scrollable frame, maybe by calling "GetScrollTargetFrame()" on it? [2]

Yup, good catch.

> Right now it feels like this comment is expressing an afterthought without
> explaining the key thing that it's an afterthought for.
> 
> Could you maybe extend the comment to say:
>       // Include the legend's BSize in margin/border/padding. (The legend's
>       // BSize is exactly the block-axis position of fieldset inner frame,
>       // because the legend is always on the BStart side of the fieldset.)

That's not quite what I wanted to say here. I changed it to:

      // The legend is always on the BStart side and it inflates the fieldset's
      // "border area" size.  The inner frame's b-start pos equals that size.

Does that make it clearer?

> > +static LogicalMargin SubgridAccumulatedMarginBorderPadding(
> > +    nsIFrame* aFrame, const Subgrid* aSubgrid, WritingMode aCBWM,
> 
> Could you document what aCBWM is expected to be here? (which CB is it
> expected to be?  Is it 'subgrid' CB, or some arbitrary CB?

It's the WM the caller desires for the result.

> Maybe we should just call it "aResultWM" to indicate that it's the writing
> mode that the output is expected to be computed with respect to, without
> having to worry about (& document/reason-about) which/whose containing
> block(s) this argument happens to be for?

OK, sounds good.

> maybe s/frame/grid-frame/ here?

Sure, fixed.

Comment 22

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #21)
> [...] I changed it to:
> 
>       // The legend is always on the BStart side and it inflates the fieldset's
>       // "border area" size.  The inner frame's b-start pos equals that size.
> 
> Does that make it clearer?

Yes, thanks!

Comment 23

[Daniel Holbert [:dholbert]]

Comment on attachment 9030315 [details] [diff] [review]
part 3 - [css-grid-2] Add methods to calculate a subgrid's margin+border+padding

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

r=me

Comment 24

[Daniel Holbert [:dholbert]]

Comment on attachment 9029442 [details] [diff] [review]
part 4 - [css-grid-2] Make a subgrid contribute its accumulated margin+border+padding to its edge tracks for intrinsically sized tracks

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

r=me, one nit:

::: layout/generic/nsGridContainerFrame.cpp
@@ +4322,5 @@
> +  if (aSize.mState & TrackSize::eIntrinsicMinSizing) {
> +    aSize.mBase = std::max(aSize.mBase, aMarginBorderPadding);
> +    if (aSize.mLimit < aSize.mBase) {
> +      aSize.mLimit = aSize.mBase;
> +    }

Can you use std::max for this assignment, for consistency with the rest of this function (which is implemented with 2 other std::max calls)?

I think this 3-line "if" block is equivalent to this one-liner:
  aSize.mLimit = std::max(aSize.mLimit, aSize.mBase);

Comment 25

[Daniel Holbert [:dholbert]]

Comment on attachment 9029443 [details] [diff] [review]
part 5 - [css-grid-2] Add the accumulated margin+border+padding of subgrid ancestors to a subgrid item's content contribution if it spans the relevant edge track(s)

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

r=me, one nit:

::: layout/generic/nsGridContainerFrame.cpp
@@ +4102,5 @@
> +  nscoord extraMargin = 0;
> +  nsGridContainerFrame::Subgrid* subgrid = nullptr;
> +  if (child->GetParent() != aState.mFrame &&
> +      (aGridItem.mState[aAxis] & ItemState::eEdgeBits)) {
> +    auto* subgridFrame = child->GetParent();

This added chunk could use a brief code-comment summarizing what it's doing.  In particular, it's non-obvious that the "child->GetParent() != aState.mFrame" comparison is a check for the presence of a subgrid.  (At first glance it looks like it could be a check for anonymous box layers (e.g. for a table or a scrollable frame), or something like that.)

Maybe this would make it clearer precisely what we're checking & doing:
"If |child| is really the child of a subgrid, then it contributes its subgrid-container's MBP for any edge tracks that it spans."

Comment 26

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:mats, could you have a look please?

Comment 27

[Mats Palmgren (inactive)]

Attachment: Bug 1466358 part 6 - [css-grid-2] Always normalize child lists before a child list iterator is used. r=dholbert

Comment 28

[Mats Palmgren (inactive)]

Attachment: Bug 1466358 part 7 - [css-grid-2] Report the right number of tracks for subgrids in ComputedGridTrackInfo. r=dholbert

Depends on D29149

Comment 29

[Mats Palmgren (inactive)]

Attachment: Bug 1466358 part 8 - [css-grid-2] Implement subgrid track sizing.

Comment 30

[Mats Palmgren (inactive)]

Attachment: Bug 1466358 part 9 - [css-grid-2] Percentage basis calculation for subgrid items. r=dholbert

Depends on D29240

Comment 31

[Mats Palmgren (inactive)]

Attachment: Bug 1466358 part 10 - [css-grid-2] Intrinsic sizing tweaks for subgrid. r=dholbert

Depends on D29241

Comment 32

[Mats Palmgren (inactive)]

FYI, Part 8 is still "work in progress" so I intentionally didn't request review
on that part yet. However, the GridReflowInput::CalculateTrackSizesForAxis /
CalculateTrackSizes methods are mostly just refactoring existing code, and
CopyUsedTrackSizes is somewhat stable, so you can review those bits if you want.
I included it so that you can start reviewing part 9/10. (The iffy parts are
the added code in ContentContribution(), that calls UsedTrackSizes::ResolveTrackSizesForAxis
and then ResolveSubgridTrackSizesForAxis, all to determine a CB size in
the opposite axis for min/max-content sizing of a subgrid item. I'm trying
to simplify that code a bit...)
These are all the code parts, BTW. Still working on WPTs...

Comment 33

[Mats Palmgren (inactive)]

Attachment: Bug 1466358 part 11 - [css-grid-2] Update the frame's subgrid state when the style changes. r=dholbert

Comment 34

[Mats Palmgren (inactive)]

Attachment: Bug 1466358 part 8 - [css-grid-2] Implement subgrid track sizing. r=dholbert

Comment 35

[Mats Palmgren (inactive)]

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

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92d9b8b245c4
part 1 - [css-grid-2] Add grid item state bits for subgrids and items in a subgrid and set them when the item occupies the corresponding edge track (idempotent patch).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/9abb8c91398c
part 2 - [css-grid-2] Add a method to recursively collect relevant items for track sizing from subgrids with their lines and state translated into the caller's coordinates (idempotent patch).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a6bf3c650d7
part 3 - [css-grid-2] Add methods to calculate a subgrid's margin+border+padding.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/c619ca5a135b
part 4 - [css-grid-2] Make a subgrid contribute its accumulated margin+border+padding to its edge tracks for intrinsically sized tracks.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf5027913782
part 5 - [css-grid-2] Add the accumulated margin+border+padding of subgrid ancestors to a subgrid item's content contribution if it spans the relevant edge track(s).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/85788d6cb290
part 6 - [css-grid-2] Always normalize child lists before a child list iterator is used.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e4f1fb1ffdd
part 7 - [css-grid-2] Report the right number of tracks for subgrids in ComputedGridTrackInfo.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/003d3db59a1f
part 8 - [css-grid-2] Implement subgrid track sizing.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e90ed77e910
part 9 - [css-grid-2] Percentage basis calculation for subgrid items.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfc4f4f153eb
part 10 - [css-grid-2] Intrinsic sizing tweaks for subgrid.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/f21f62a36c12
part 11 - [css-grid-2] Update the frame's subgrid state when the style changes.  r=dholbert

Comment 37

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/92d9b8b245c4
https://hg.mozilla.org/mozilla-central/rev/9abb8c91398c
https://hg.mozilla.org/mozilla-central/rev/1a6bf3c650d7
https://hg.mozilla.org/mozilla-central/rev/c619ca5a135b
https://hg.mozilla.org/mozilla-central/rev/bf5027913782
https://hg.mozilla.org/mozilla-central/rev/85788d6cb290
https://hg.mozilla.org/mozilla-central/rev/4e4f1fb1ffdd
https://hg.mozilla.org/mozilla-central/rev/003d3db59a1f
https://hg.mozilla.org/mozilla-central/rev/6e90ed77e910
https://hg.mozilla.org/mozilla-central/rev/bfc4f4f153eb
https://hg.mozilla.org/mozilla-central/rev/f21f62a36c12