Bug 237692 – [css-grid] Grid items that establish an independent formatting context should not be subgrids.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Matt Woodrow]

This includes position:absolute, as clarified in https://github.com/w3c/csswg-drafts/issues/7124

[Radar WebKit Bug Importer]

<rdar://problem/90066609>

[Matt Woodrow]

Created attachment 454303 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Matt Woodrow]

Created attachment 454306 [details]
Patch

[zalan]

Comment on attachment 454306 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=454306&action=review

> Source/WebCore/rendering/RenderBox.cpp:4949
> +bool RenderBox::createsNewFormattingContextExcludingGridItems() const
> +{
> +    // Writing-mode changes establish an independent block formatting context
> +    // if the box is a block-container, not a grid-container.
> +    // https://drafts.csswg.org/css-writing-modes/#block-flow

Please do not have a grid specific version of this function in RenderBox. Instead call the generic createsNewFormattingContext() in RenderGrid and deal with the grid related checks there.

> Source/WebCore/rendering/RenderBox.cpp:4951
> +    // FIXME: Is there a way to check for block-containers, instead
> +    // of excluding not-block-containers. Should this be part of isWritingModeRoot?

Yes, there is a way. See Layout::Box::isBlockContainer (it may not be 100% up-to-date though, but surely one can come up with a function that checks all the relevant bits)

[Matt Woodrow]

Created attachment 454310 [details]
Patch

[Matt Woodrow]

Created attachment 454384 [details]
Patch

[zalan]

Comment on attachment 454384 [details]
Patch

This looks a bit mysterious to me. The spec is pretty clear on this:

bool RenderGrid::isSubgrid()
{
    if (!is<RenderGrid>(parent() || establishesIndependentFormattingContext())
        return false;
    return direction == ForColumns ? style().gridSubgridColumns() : style().gridSubgridRows(); 
}

All we need is the establishesIndependentFormattingContext() function (which I already have in LFC as Box::establishesIndependentFormattingContext)
Am I missing something here?

[Matt Woodrow]

(In reply to zalan from comment #8)
> Comment on attachment 454384 [details]
> Patch
> 
> This looks a bit mysterious to me. The spec is pretty clear on this:
> 
> bool RenderGrid::isSubgrid()
> {
>     if (!is<RenderGrid>(parent() ||
> establishesIndependentFormattingContext())
>         return false;
>     return direction == ForColumns ? style().gridSubgridColumns() :
> style().gridSubgridRows(); 
> }
> 
> All we need is the establishesIndependentFormattingContext() function (which
> I already have in LFC as Box::establishesIndependentFormattingContext)
> Am I missing something here?

There's a bit of a circular dependency that needs to be resolved that causes this complexity.

Subgrids are instances of RenderGrid that also return true for isGridItem() (and request to be a subgrid).

Grid items establish an independent formatting context, unless they are also a subgrid, but the subgrid property is ignored if the element already establishes an independent formatting context (from some other property).

The relevant wording from the spec is "If there is no parent grid, or if the grid container is otherwised forced to establish an independent formatting context (for example, due to layout containment [CSS-CONTAIN-2] or absolute positioning [CSS-POSITION-3]), the used value is the initial value, none, and the grid container is not a subgrid" (https://drafts.csswg.org/css-grid-2/#subgrid-listing), particularly the "otherwised" bit.

If we just check the generic establishesIndepdendentFormattingContext function, then it will always return true (and isSubgrid will always be false).

[zalan]

(In reply to Matt Woodrow from comment #9)
> (In reply to zalan from comment #8)
> > Comment on attachment 454384 [details]
> > Patch
> > 
> > This looks a bit mysterious to me. The spec is pretty clear on this:
> > 
> > bool RenderGrid::isSubgrid()
> > {
> >     if (!is<RenderGrid>(parent() ||
> > establishesIndependentFormattingContext())
> >         return false;
> >     return direction == ForColumns ? style().gridSubgridColumns() :
> > style().gridSubgridRows(); 
> > }
> > 
> > All we need is the establishesIndependentFormattingContext() function (which
> > I already have in LFC as Box::establishesIndependentFormattingContext)
> > Am I missing something here?
> 
> There's a bit of a circular dependency that needs to be resolved that causes
> this complexity.
> 
> Subgrids are instances of RenderGrid that also return true for isGridItem()
> (and request to be a subgrid).
> 
> Grid items establish an independent formatting context, unless they are also
> a subgrid, but the subgrid property is ignored if the element already
> establishes an independent formatting context (from some other property).
> 
> The relevant wording from the spec is "If there is no parent grid, or if the
> grid container is otherwised forced to establish an independent formatting
> context (for example, due to layout containment [CSS-CONTAIN-2] or absolute
> positioning [CSS-POSITION-3]), the used value is the initial value, none,
> and the grid container is not a subgrid"
> (https://drafts.csswg.org/css-grid-2/#subgrid-listing), particularly the
> "otherwised" bit.
> 
> If we just check the generic establishesIndepdendentFormattingContext
> function, then it will always return true (and isSubgrid will always be
> false).
yeah, my bad, I meant to add the base class call for establishesIndependentFormattingContext. 

bool RenderBox::isSubgrid()
{
  if (!isGridItem())
    return false;
  if (!is<RenderGrid>(parent()) || RenderElement::establishesIndependentFormattingContext())
    return false;
  return direction == ForColumns ? style().gridSubgridColumns() : style().gridSubgridRows(); 
}

bool RenderBox::establishesIndependentFormattingContext() override
{
  // Unless it is a subgrid, a grid item establishes an independent formatting context.
  if (isGridItem())
    return !isSubgrid();
  return RenderElement::establishesIndependentFormattingContext();
}

[Matt Woodrow]

Created attachment 454612 [details]
Patch

[zalan]

Comment on attachment 454612 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=454612&action=review

> Source/WebCore/rendering/RenderBox.cpp:4932
> +    return isGridItem() || RenderElement::createsNewFormattingContext();

Should this be on the RenderGrid?

> Source/WebCore/rendering/RenderElement.cpp:2398
> +    // Writing-mode changes establish an independent block formatting context
> +    // if the box is a block-container, not a grid-container.
> +    // https://drafts.csswg.org/css-writing-modes/#block-flow
> +    if (isWritingModeRoot() && isBlockContainer())
> +        return true;

The comments does not explain the condition here (e.g. independent formatting context). Also, why move it to RenderElement? (RenderGrid is a RenderBlock).

> Source/WebCore/rendering/RenderGrid.cpp:2075
> +        if (!isSubgridRows() && !isSubgridColumns())
> +            return true;

shouldn't this read: if (!isSubgrid()) ?

[zalan]

I am generally confused as to why this patch changes the behavior of createNewFormattingContext when this should really be about independent formatting context. It'd be great if the code just reflected the spec as is. 
Something like this:

diff --git a/Source/WebCore/rendering/RenderBox.cpp b/Source/WebCore/rendering/RenderBox.cpp
index 2855d0378280..b6ca86884f62 100644
--- a/Source/WebCore/rendering/RenderBox.cpp
+++ b/Source/WebCore/rendering/RenderBox.cpp
@@ -4951,6 +4951,17 @@ bool RenderBox::createsNewFormattingContext() const
         || style().containsLayout() || paintContainmentApplies() || isGridItem() || style().specifiesColumns() || style().columnSpan() == ColumnSpan::All || style().display() == DisplayType::FlowRoot;
 }
 
+bool RenderBox::establishesIndependentFormattingContext() const
+{
+    if (isOutOfFlowPositioned())
+        return true;
+    if (shouldApplyLayoutContainment(*this) || shouldApplyPaintContainment(*this))
+        return true;
+    if (style().isOverflowScroll())
+        return true;
+    return false;
+}
+
 bool RenderBox::avoidsFloats() const
 {
     return isReplacedOrInlineBlock() || isHR() || isLegend() || isFieldset() || createsNewFormattingContext();
diff --git a/Source/WebCore/rendering/RenderBox.h b/Source/WebCore/rendering/RenderBox.h
index ed77770e7e42..d22b2816d0f7 100644
--- a/Source/WebCore/rendering/RenderBox.h
+++ b/Source/WebCore/rendering/RenderBox.h
@@ -684,6 +684,7 @@ protected:
     void willBeDestroyed() override;
 
     bool createsNewFormattingContext() const;
+    virtual bool establishesIndependentFormattingContext() const;
 
     virtual bool shouldResetLogicalHeightBeforeLayout() const { return false; }
     void resetLogicalHeightBeforeLayoutIfNeeded();
diff --git a/Source/WebCore/rendering/RenderGrid.cpp b/Source/WebCore/rendering/RenderGrid.cpp
index d9907f0f7a8f..f185c7ac0b22 100644
--- a/Source/WebCore/rendering/RenderGrid.cpp
+++ b/Source/WebCore/rendering/RenderGrid.cpp
@@ -1650,11 +1650,30 @@ LayoutUnit RenderGrid::rowAxisOffsetForChild(const RenderBox& child) const
     return 0;
 }
 
-bool RenderGrid::isSubgrid(GridTrackSizingDirection direction) const
+bool RenderGrid::isSubgrid(std::optional<GridTrackSizingDirection> direction) const
 {
-    if (!mayBeSubgridExcludingAbsPos(direction))
+    // If there is no parent grid, or if the grid container is forced to establish an independent formatting
+    // context (like contain layout, or position:absolute), then the used value
+    // of grid-template-rows/columns is 'none' and the container is not a subgrid.
+    // https://drafts.csswg.org/css-grid-2/#subgrid-listing
+    if (!is<RenderGrid>(parent()))
+        return false;
+    if (RenderBlock::establishesIndependentFormattingContext())
         return false;
-    return downcast<RenderGrid>(parent())->gridSpanCoversRealTracks(*this, direction);
+
+    auto isSubgridForDirection = [&] {
+        auto& style = this->style();
+        if (!direction)
+            return style.gridSubgridColumns() || style.gridSubgridRows();
+        return *direction == ForColumns ? style.gridSubgridColumns() : style.gridSubgridRows();
+    };
+    return isSubgridForDirection();
+}
+
+bool RenderGrid::establishesIndependentFormattingContext() const
+{
+    // Unless it is a subgrid, a grid item establishes an independent formatting context.
+    return !isSubgrid();
 }
 
 bool RenderGrid::isSubgridInParentDirection(GridTrackSizingDirection parentDirection) const
diff --git a/Source/WebCore/rendering/RenderGrid.h b/Source/WebCore/rendering/RenderGrid.h
index 325ae0eb0d5d..3ad4b38ee4fe 100644
--- a/Source/WebCore/rendering/RenderGrid.h
+++ b/Source/WebCore/rendering/RenderGrid.h
@@ -86,7 +86,8 @@ public:
     // of a descendant subgrid.
     GridSpan gridSpanForChild(const RenderBox&, GridTrackSizingDirection) const;
 
-    bool isSubgrid(GridTrackSizingDirection) const;
+    bool isSubgrid(std::optional<GridTrackSizingDirection> = std::nullopt) const;
+    bool establishesIndependentFormattingContext() const final;
     bool isSubgridRows() const
     {
         return isSubgrid(ForRows);
diff --git a/Source/WebCore/rendering/style/GridPositionsResolver.cpp b/Source/WebCore/rendering/style/GridPositionsResolver.cpp
index 738902af9920..bc8cdbcc897b 100644
--- a/Source/WebCore/rendering/style/GridPositionsResolver.cpp
+++ b/Source/WebCore/rendering/style/GridPositionsResolver.cpp
@@ -380,10 +380,7 @@ static void adjustGridPositionsFromStyle(const RenderBox& gridItem, GridTrackSiz
     if (finalPosition.isAuto() && initialPosition.isSpan() && !initialPosition.namedGridLine().isNull())
         initialPosition.setSpanPosition(1, String());
 
-    // Absolutely positioned items specifying subgrid might not actually be a subgrid if their grid
-    // span doesn't cover any tracks and only the padding area. We don't know if that is the case until
-    // we've figured out their grid position though, which is what we're trying to do now.
-    if (isIndefiniteSpan(initialPosition, finalPosition) && is<RenderGrid>(gridItem) && downcast<RenderGrid>(gridItem).mayBeSubgridExcludingAbsPos(direction)) {
+    if (isIndefiniteSpan(initialPosition, finalPosition) && is<RenderGrid>(gridItem) && downcast<RenderGrid>(gridItem).isSubgrid(direction)) {
         // Indefinite span for an item that is subgridded in this axis.
         int lineCount = (isForColumns ? gridItem.style().orderedNamedGridColumnLines() : gridItem.style().orderedNamedGridRowLines()).size();
 
diff --git a/Source/WebCore/rendering/style/RenderStyle.h b/Source/WebCore/rendering/style/RenderStyle.h
index 5b3614eb9ba1..b2c3540e1ed4 100644
--- a/Source/WebCore/rendering/style/RenderStyle.h
+++ b/Source/WebCore/rendering/style/RenderStyle.h
@@ -340,6 +340,7 @@ public:
     Overflow overflowX() const { return static_cast<Overflow>(m_nonInheritedFlags.overflowX); }
     Overflow overflowY() const { return static_cast<Overflow>(m_nonInheritedFlags.overflowY); }
     bool isOverflowVisible() const { return overflowX() == Overflow::Visible || overflowY() == Overflow::Visible; }
+    bool isOverflowScroll() const { return overflowX() == Overflow::Scroll || overflowY() == Overflow::Scroll; }
 
     OverscrollBehavior overscrollBehaviorX() const { return static_cast<OverscrollBehavior>(m_rareNonInheritedData->overscrollBehaviorX); }
     OverscrollBehavior overscrollBehaviorY() const { return static_cast<OverscrollBehavior>(m_rareNonInheritedData->overscrollBehaviorY); }

[Matt Woodrow]

Comment on attachment 454612 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=454612&action=review

>> Source/WebCore/rendering/RenderBox.cpp:4932
>> +    return isGridItem() || RenderElement::createsNewFormattingContext();
> 
> Should this be on the RenderGrid?

I don't think so? Any RenderBox can be a grid item (an in-flow child of a RenderGrid), so we need to check for that here.

RenderGrid overrides this again to conditionally remove the isGridItem() check, since grid items that are also a RenderGrid (and request to be a subgrid) should not establish an independent formatting context.

>> Source/WebCore/rendering/RenderGrid.cpp:2075
>> +            return true;
> 
> shouldn't this read: if (!isSubgrid()) ?

That's not an existing function (though I could add one?). Subgrid is a per-axis property, so we want to check if it's been specified for either.

[Matt Woodrow]

(In reply to zalan from comment #13)
> I am generally confused as to why this patch changes the behavior of
> createNewFormattingContext when this should really be about independent
> formatting context. It'd be great if the code just reflected the spec as is. 
> Something like this:
> 
> diff --git a/Source/WebCore/rendering/RenderBox.cpp
> b/Source/WebCore/rendering/RenderBox.cpp
> index 2855d0378280..b6ca86884f62 100644
> --- a/Source/WebCore/rendering/RenderBox.cpp
> +++ b/Source/WebCore/rendering/RenderBox.cpp
> @@ -4951,6 +4951,17 @@ bool RenderBox::createsNewFormattingContext() const
>          || style().containsLayout() || paintContainmentApplies() ||
> isGridItem() || style().specifiesColumns() || style().columnSpan() ==
> ColumnSpan::All || style().display() == DisplayType::FlowRoot;
>  }
>  
> +bool RenderBox::establishesIndependentFormattingContext() const
> +{
> +    if (isOutOfFlowPositioned())
> +        return true;
> +    if (shouldApplyLayoutContainment(*this) ||
> shouldApplyPaintContainment(*this))
> +        return true;
> +    if (style().isOverflowScroll())
> +        return true;
> +    return false;
> +}
> +
>  bool RenderBox::avoidsFloats() const
>  {
>      return isReplacedOrInlineBlock() || isHR() || isLegend() ||
> isFieldset() || createsNewFormattingContext();

This doesn't change the behaviour of createsNewFormattingContext though, so existing callers to that function will see 'true' for grid items, even though they shouldn't (if the grid item is a subgrid).

Do you want me to split the independent formatting context subset out of createsNewFormattingContext, and replace it with a call to that new function? Then RenderGrid can override just the independent formatting context piece.

[zalan]

(In reply to Matt Woodrow from comment #15)
> (In reply to zalan from comment #13)
> > I am generally confused as to why this patch changes the behavior of
> > createNewFormattingContext when this should really be about independent
> > formatting context. It'd be great if the code just reflected the spec as is. 
> > Something like this:
> > 
> > diff --git a/Source/WebCore/rendering/RenderBox.cpp
> > b/Source/WebCore/rendering/RenderBox.cpp
> > index 2855d0378280..b6ca86884f62 100644
> > --- a/Source/WebCore/rendering/RenderBox.cpp
> > +++ b/Source/WebCore/rendering/RenderBox.cpp
> > @@ -4951,6 +4951,17 @@ bool RenderBox::createsNewFormattingContext() const
> >          || style().containsLayout() || paintContainmentApplies() ||
> > isGridItem() || style().specifiesColumns() || style().columnSpan() ==
> > ColumnSpan::All || style().display() == DisplayType::FlowRoot;
> >  }
> >  
> > +bool RenderBox::establishesIndependentFormattingContext() const
> > +{
> > +    if (isOutOfFlowPositioned())
> > +        return true;
> > +    if (shouldApplyLayoutContainment(*this) ||
> > shouldApplyPaintContainment(*this))
> > +        return true;
> > +    if (style().isOverflowScroll())
> > +        return true;
> > +    return false;
> > +}
> > +
> >  bool RenderBox::avoidsFloats() const
> >  {
> >      return isReplacedOrInlineBlock() || isHR() || isLegend() ||
> > isFieldset() || createsNewFormattingContext();
> 
> This doesn't change the behaviour of createsNewFormattingContext though, so
> existing callers to that function will see 'true' for grid items, even
> though they shouldn't (if the grid item is a subgrid).
> Do you want me to split the independent formatting context subset out of
> createsNewFormattingContext, and replace it with a call to that new
> function? Then RenderGrid can override just the independent formatting
> context piece.
createsNewFormattingContext should really follow what we have in LFC: 

bool Box::establishesFormattingContext() const
{
    return establishesBlockFormattingContext()
        || establishesInlineFormattingContext()
        || establishesTableFormattingContext()
        || establishesFlexFormattingContext()
        || establishesIndependentFormattingContext();
}

but I think for now
"(isGridItem() && establishesIndependentFormattingContext())"
will do.

[Matt Woodrow]

Created attachment 456650 [details]
Patch

[zalan]

Comment on attachment 456650 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=456650&action=review

> Source/WebCore/rendering/RenderGrid.cpp:2109
> +    // Grid items establish a new (grid) formatting context, unless
> +    // they're a subgrid
> +    // https://drafts.csswg.org/css-grid-2/#grid-item-display

I am not sure if this comment is correct. Grid items normally don't establish grid formatting context (their parent, the grid container does) unless they are grid containers themselves.

> Source/WebCore/rendering/RenderGrid.cpp:2110
> +    if (isGridItem()) {

Ok, so this is the case when the grid item is itself a grid container too (e.g. <div style="display: grid"><div id=grid_item style="display: grid"></div></div>) and it only establishes an independent formatting context if it is not a subgrid.

> Source/WebCore/rendering/RenderGrid.cpp:2111
> +        if (!isSubgridRows() && !isSubgridColumns())

It's a bit confusing that we check for "is this a subgrid" like this when there's already an isSubgrid() function on this class. By just looking at this code, it's hard to tell why we just don't call isSubgrid(). I am sure there's a reason but it's not obvious.

[Matt Woodrow]

(In reply to zalan from comment #18)
> Comment on attachment 456650 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=456650&action=review
> 
> > Source/WebCore/rendering/RenderGrid.cpp:2109
> > +    // Grid items establish a new (grid) formatting context, unless
> > +    // they're a subgrid
> > +    // https://drafts.csswg.org/css-grid-2/#grid-item-display
> 
> I am not sure if this comment is correct. Grid items normally don't
> establish grid formatting context (their parent, the grid container does)
> unless they are grid containers themselves.

Good catch, they establish independent formatting contexts, not grid formatting contexts (unless they're a subgrid, as below). I'll fix that comment.
> 
> > Source/WebCore/rendering/RenderGrid.cpp:2110
> > +    if (isGridItem()) {
> 
> Ok, so this is the case when the grid item is itself a grid container too
> (e.g. <div style="display: grid"><div id=grid_item style="display:
> grid"></div></div>) and it only establishes an independent formatting
> context if it is not a subgrid.
> 
> > Source/WebCore/rendering/RenderGrid.cpp:2111
> > +        if (!isSubgridRows() && !isSubgridColumns())
> 
> It's a bit confusing that we check for "is this a subgrid" like this when
> there's already an isSubgrid() function on this class. By just looking at
> this code, it's hard to tell why we just don't call isSubgrid(). I am sure
> there's a reason but it's not obvious.

I don't see this function, there's isSubgrid(GridTrackSizingDirection)? We'd still have to call that twice, with `ForRows` and `ForColumns`, which is effectively the same thing.

[zalan]

(In reply to Matt Woodrow from comment #19)
> (In reply to zalan from comment #18)
> > Comment on attachment 456650 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=456650&action=review
> > 
> > > Source/WebCore/rendering/RenderGrid.cpp:2109
> > > +    // Grid items establish a new (grid) formatting context, unless
> > > +    // they're a subgrid
> > > +    // https://drafts.csswg.org/css-grid-2/#grid-item-display
> > 
> > I am not sure if this comment is correct. Grid items normally don't
> > establish grid formatting context (their parent, the grid container does)
> > unless they are grid containers themselves.
> 
> Good catch, they establish independent formatting contexts, not grid
> formatting contexts (unless they're a subgrid, as below). I'll fix that
> comment.
> > 
> > > Source/WebCore/rendering/RenderGrid.cpp:2110
> > > +    if (isGridItem()) {
> > 
> > Ok, so this is the case when the grid item is itself a grid container too
> > (e.g. <div style="display: grid"><div id=grid_item style="display:
> > grid"></div></div>) and it only establishes an independent formatting
> > context if it is not a subgrid.
> > 
> > > Source/WebCore/rendering/RenderGrid.cpp:2111
> > > +        if (!isSubgridRows() && !isSubgridColumns())
> > 
> > It's a bit confusing that we check for "is this a subgrid" like this when
> > there's already an isSubgrid() function on this class. By just looking at
> > this code, it's hard to tell why we just don't call isSubgrid(). I am sure
> > there's a reason but it's not obvious.
> 
> I don't see this function, there's isSubgrid(GridTrackSizingDirection)? We'd
> still have to call that twice, with `ForRows` and `ForColumns`, which is
> effectively the same thing.
isSubGrid checks for a lot more things which makes this a bit confusing e.g. under what condition it is ok just to check for "isSubgridRows() || isSubgridColumns()" vs. calling isSubgrid() (I mean I can understand the logic, I am just looking at it as if I were coming back to this code after a year (i.e. trying to find the least ambiguous way of describing spec things.)

[Matt Woodrow]

Created attachment 456889 [details]
Patch for landing

[EWS]

Committed r292524 (249362@main): <https://commits.webkit.org/249362@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 456889 [details].