1465290 - [css-grid-2] Add a few frame bits to keep track of subgrid state

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P2)

Comment 1

[Mats Palmgren (inactive)]

Attachment: part 1 - [css-grid-2] Add frame bits to nsGridContainerFrame that is set if it's a subgrid in an axis

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-grid-2] Add bits to the GridItemInfo state that is set if the item is a subgrid in the container's axis

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-grid-2] Add frame bits to nsGridContainerFrame that is set if it has an item that is a subgrid in that axis

Comment 4

[Mats Palmgren (inactive)]

(FYI, the bits in part 3 will be set in bug 1465296, which comes next...)

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8981718 [details] [diff] [review]
part 1 - [css-grid-2] Add frame bits to nsGridContainerFrame that is set if it's a subgrid in an axis

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

r=me

::: layout/generic/nsGridContainerFrame.cpp
@@ +6346,5 @@
> +    }
> +    if (parent && parent->IsGridContainerFrame()) {
> +      const auto* pos = StylePosition();
> +      if (pos->GridTemplateColumns().mIsSubgrid) {
> +        bits = NS_STATE_GRID_IS_COL_SUBGRID;

Nit: consider using "bits |=" (not "=") here, to be more consistent with the |= in the other case here, and to be more future-proof against possible further tweaks or reorderings of these statements.

(note that you've initialized bits to 0, so |= is equivalent to = here, since this is the first assignment after that initialization.)

::: layout/generic/nsGridContainerFrame.h
@@ +218,5 @@
>    ExplicitNamedAreas* GetExplicitNamedAreas() const {
>      return GetProperty(ExplicitNamedAreasProperty());
>    }
>  
> +  /** Return true if this frame subgridded in its aAxis. */

Maybe s/subgridded/is subgridded/?  (or "is a subgrid" to be clearer still?  I'm not sure that "subgridded" is defined anywhere...)

@@ +226,5 @@
> +                                          : NS_STATE_GRID_IS_COL_SUBGRID);
> +  }
> +  bool IsColSubgrid() const { return IsSubgrid(mozilla::eLogicalAxisInline); }
> +  bool IsRowSubgrid() const { return IsSubgrid(mozilla::eLogicalAxisBlock); }
> +  /** Return true if this frame subgridded in any axis. */

(as above, the phrase "if this frame subgridded" feels odd to me, but not a big deal)

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8981720 [details] [diff] [review]
part 2 - [css-grid-2] Add bits to the GridItemInfo state that is set if the item is a subgrid in the container's axis

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

Commit message nit:
> Add bits ... that is set

s/that is/that are/

::: layout/generic/nsGridContainerFrame.cpp
@@ +562,5 @@
> +      auto parentWM = aFrame->GetParent()->GetWritingMode();
> +      bool isOrthogonal = parentWM.IsOrthogonalTo(f->GetWritingMode());
> +      if (f->IsColSubgrid()) {
> +        mState[isOrthogonal ? eLogicalAxisBlock : eLogicalAxisInline] =
> +          StateBits::eIsSubgrid;

It'd probably be best to use "|=" rather than "=" here, to just selectively set this bit without touching the others.

(It happens to be the case that mState is 0 at this point, so stomping on it doesn't matter; but that's kind of fragile to depend on, because it's possible we might set other bits in between in the future.)

This applies twice (for the IsColSubgrid and the IsRowSubgrid case)

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8981722 [details] [diff] [review]
part 3 - [css-grid-2] Add frame bits to nsGridContainerFrame that is set if it has an item that is a subgrid in that axis

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

Similar to part 2, commit message nit:
> Add frame bits...that is set
s/that is/that are/

::: layout/generic/nsFrameStateBits.h
@@ +354,5 @@
>  
> +// True if the container contains items subgridded in its inline axis
> +FRAME_STATE_BIT(GridContainer, 26, NS_STATE_GRID_HAS_COL_SUBGRID_ITEMS)
> +
> +// True if the container contains items subgridded in its block axis

3 nits:
 (1) add period at the end of each comment here.

 (2) In the bit's name as well as in the comment, I think you might mean "ITEM" / "item" -- there's no guarantee we have plural "items", right?  (Maybe there are plural, but in general, not necessarily.)  Conveniently, that makes the bit name 1 character shorter, too. :))

 (3) It's not clear why we're using different terminology ("is a subgrid" vs. "subgridded") between the first two bits [part 1] vs. these latter 2 bits [part 3].  Maybe make that terminology consistent? I think "is a subgrid" is well defined, vs. "subgridded" isn't really, so I'd lean towards the former phrasing...

::: layout/generic/nsGridContainerFrame.cpp
@@ +6400,5 @@
>    } else {
>      bits = aPrevInFlow->GetStateBits() & (NS_STATE_GRID_IS_COL_SUBGRID |
> +                                          NS_STATE_GRID_IS_ROW_SUBGRID |
> +                                          NS_STATE_GRID_HAS_COL_SUBGRID_ITEMS |
> +                                          NS_STATE_GRID_HAS_ROW_SUBGRID_ITEMS);

Nit: this cloning doesn't make 100% sense -- because technically, it'd be possible for our prev-in-flow to have a subgrid without us having a subgrid, right?  (if the subgrid is entirely inside one of the parent grid's fragments, then subsequent fragments don't technically have a subgrid)

Maybe that doesn't matter for our purposes, though; if these state-bits are just to prevent us doing expensive stuff in the common no-subgrids-at-all case, then this seems fine.

::: layout/generic/nsGridContainerFrame.h
@@ +232,5 @@
>      return HasAnyStateBits(NS_STATE_GRID_IS_ROW_SUBGRID |
>                             NS_STATE_GRID_IS_COL_SUBGRID);
>    }
>  
> +  /** Return true if this frame have subgrid items subgridded in our aAxis. */

2 nits:
 (1)  s/this frame have/this frame has/

 (2) "subgrid items subgridded" is confusing (and also incorrectly plural when we don't necessarily know it's plural).  Maybe "Return true if this frame has at least one item that is a subgrid in our aAxis"?

@@ +238,5 @@
> +    return HasAnyStateBits(
> +      aAxis == mozilla::eLogicalAxisBlock ? NS_STATE_GRID_HAS_ROW_SUBGRID_ITEMS
> +                                          : NS_STATE_GRID_HAS_COL_SUBGRID_ITEMS);
> +  }
> +  /** Return true if this frame have any subgrid items. */

s/have/has/

Comment 8

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> > +        mState[isOrthogonal ? eLogicalAxisBlock : eLogicalAxisInline] =
> > +          StateBits::eIsSubgrid;
> 
> It'd probably be best to use "|=" rather than "=" here, to just selectively
> set this bit without touching the others.

That doesn't compile though, so I left it as is.
"error: assigning to 'nsGridContainerFrame::GridItemInfo::StateBits' from incompatible type 'int'"

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fbed517647c
part 1 - [css-grid-2] Add frame bits to nsGridContainerFrame that is set if it's a subgrid in an axis.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/427ecb724099
part 2 - [css-grid-2] Add bits to the GridItemInfo state that are set if the item is a subgrid in the container's axis.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d4e28bd3162
part 3 - [css-grid-2] Add frame bits to nsGridContainerFrame that are set if it has an item that is a subgrid in that axis.  r=dholbert

Comment 10

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/8fbed517647c
https://hg.mozilla.org/mozilla-central/rev/427ecb724099
https://hg.mozilla.org/mozilla-central/rev/6d4e28bd3162

Comment 11

[Mats Palmgren (inactive)]

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

Comment 12

[Mats Palmgren (inactive)]

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

Comment 13

[Mats Palmgren (inactive)]

Attachment: part 6 - [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 14

[Mats Palmgren (inactive)]

Sorry, wrong bug... sigh