Bug 236122 – Inherit explicit grid size from parent for subgrids

• Component: CSS
• Status: RESOLVED FIXED

[Matt Woodrow]

Subgrids should use their grid span in the parent grid to determine the size of the explicit grids, and don't allow implicit tracks to be created.

[Matt Woodrow]

Created attachment 450924 [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 450925 [details]
Patch for EWS

[Matt Woodrow]

Created attachment 451001 [details]
Patch

[Matt Woodrow]

Created attachment 451002 [details]
Patch for EWS

[Radar WebKit Bug Importer]

<rdar://problem/88795234>

[Matt Woodrow]

Created attachment 451946 [details]
Patch

[Matt Woodrow]

Created attachment 451954 [details]
Patch

[Oriol Brufau]

Comment on attachment 451954 [details]
Patch

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

> Source/WebCore/rendering/GridLayoutFunctions.cpp:80
> +bool isOrthogonalChild(const RenderGrid& grid, const RenderElement& child)

This change seems unnecessary?

> Source/WebCore/rendering/GridLayoutFunctions.cpp:90
> +GridTrackSizingDirection flowAwareDirectionForChild(const RenderGrid& grid, const RenderElement& child, GridTrackSizingDirection direction)

Ditto

> Source/WebCore/rendering/GridLayoutFunctions.h:33
> +class RenderElement;

Ditto

> Source/WebCore/rendering/GridLayoutFunctions.h:43
> +bool isOrthogonalChild(const RenderGrid&, const RenderElement&);

Ditto

> Source/WebCore/rendering/GridLayoutFunctions.h:45
> +GridTrackSizingDirection flowAwareDirectionForChild(const RenderGrid&, const RenderElement&, GridTrackSizingDirection);

Ditto

> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/subgrid/line-names-002-expected.html:17
> +  grid: auto / [a] 50px 40px [a] 10px 50px [a];

Changing the expectation makes the test fail? So not sure if this is the right patch to change it.

[Matt Woodrow]

(In reply to Oriol Brufau from comment #9)
> Comment on attachment 451954 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=451954&action=review
> 
> > Source/WebCore/rendering/GridLayoutFunctions.cpp:80
> > +bool isOrthogonalChild(const RenderGrid& grid, const RenderElement& child)
> 
> This change seems unnecessary?

These all make it easier to check for orthogonal changes between grid/subgrid using parent() (without needing to both casting to a RenderGrid). Looks like there isn't actually an instance here, but there are in the other patches.

> > LayoutTests/imported/w3c/web-platform-tests/css/css-grid/subgrid/line-names-002-expected.html:17
> > +  grid: auto / [a] 50px 40px [a] 10px 50px [a];
> 
> Changing the expectation makes the test fail? So not sure if this is the
> right patch to change it.

What do you mean by makes the test fail? It still fails at this point in the patch queue, since it depends on other functionality.

This patch is the one that implements the new CSSWG resolution about how to handle indefinite spans, so I put the relevant test change with it.

[Oriol Brufau]

Comment on attachment 451954 [details]
Patch

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

>>> Source/WebCore/rendering/GridLayoutFunctions.cpp:80
>>> +bool isOrthogonalChild(const RenderGrid& grid, const RenderElement& child)
>> 
>> This change seems unnecessary?
> 
> These all make it easier to check for orthogonal changes between grid/subgrid using parent() (without needing to both casting to a RenderGrid). Looks like there isn't actually an instance here, but there are in the other patches.

But then it would be easier if the grid was the RenderElement, right? So you could do

  isOrthogonalChild(child->parent(), child)

without having to cast child->parent().

But this is changing the child. Will there be a child which is not already provided as a RenderBox?

>>> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/subgrid/line-names-002-expected.html:17
>>> +  grid: auto / [a] 50px 40px [a] 10px 50px [a];
>> 
>> Changing the expectation makes the test fail? So not sure if this is the right patch to change it.
> 
> What do you mean by makes the test fail? It still fails at this point in the patch queue, since it depends on other functionality.
> 
> This patch is the one that implements the new CSSWG resolution about how to handle indefinite spans, so I put the relevant test change with it.

If I run the test locally (removing the ImageOnlyFailure entry from TestExpectations) then it fails with this change, and without the change it tends to pass (though I also got one crash).

[Matt Woodrow]

(In reply to Oriol Brufau from comment #11)
> Comment on attachment 451954 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=451954&action=review
> 
> >>> Source/WebCore/rendering/GridLayoutFunctions.cpp:80
> >>> +bool isOrthogonalChild(const RenderGrid& grid, const RenderElement& child)
> >> 
> >> This change seems unnecessary?
> > 
> > These all make it easier to check for orthogonal changes between grid/subgrid using parent() (without needing to both casting to a RenderGrid). Looks like there isn't actually an instance here, but there are in the other patches.
> 
> But then it would be easier if the grid was the RenderElement, right? So you
> could do
> 
>   isOrthogonalChild(child->parent(), child)
> 
> without having to cast child->parent().
> 
> But this is changing the child. Will there be a child which is not already
> provided as a RenderBox?

I think this is largely because I'm using it in the reverse direction (since we only care if the writing mode changes boolean state, it works both ways), which is sort of cheating. It might be better to have an aliased version that takes two RenderGrids.

> 
> >>> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/subgrid/line-names-002-expected.html:17
> >>> +  grid: auto / [a] 50px 40px [a] 10px 50px [a];
> >> 
> >> Changing the expectation makes the test fail? So not sure if this is the right patch to change it.
> > 
> > What do you mean by makes the test fail? It still fails at this point in the patch queue, since it depends on other functionality.
> > 
> > This patch is the one that implements the new CSSWG resolution about how to handle indefinite spans, so I put the relevant test change with it.
> 
> If I run the test locally (removing the ImageOnlyFailure entry from
> TestExpectations) then it fails with this change, and without the change it
> tends to pass (though I also got one crash).

I'm not entirely sure why that is, would need to figure out why exactly we start passing later on. I'll look into it tomorrow.

[Matt Woodrow]

(In reply to Matt Woodrow from comment #12)
> I think this is largely because I'm using it in the reverse direction (since
> we only care if the writing mode changes boolean state, it works both ways),
> which is sort of cheating. It might be better to have an aliased version
> that takes two RenderGrids.

I'll add a second version of this function 'flowAwareDirectionForParent' (which does the exact same thing), so that it's clearer which direction we're going.

[Matt Woodrow]

(In reply to Matt Woodrow from comment #12) 
> > >>> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/subgrid/line-names-002-expected.html:17
> > >>> +  grid: auto / [a] 50px 40px [a] 10px 50px [a];
> > >> 
> > >> Changing the expectation makes the test fail? So not sure if this is the right patch to change it.
> > > 
> > > What do you mean by makes the test fail? It still fails at this point in the patch queue, since it depends on other functionality.
> > > 
> > > This patch is the one that implements the new CSSWG resolution about how to handle indefinite spans, so I put the relevant test change with it.
> > 
> > If I run the test locally (removing the ImageOnlyFailure entry from
> > TestExpectations) then it fails with this change, and without the change it
> > tends to pass (though I also got one crash).
> 
> I'm not entirely sure why that is, would need to figure out why exactly we
> start passing later on. I'll look into it tomorrow.

Looks like we start passing properly when the subgrid track sizing starts using the sizes from the parent grid, rather than computing its own.

The changes here make us select the right lines for the subgrid, but since we compute new track sizes (everything in the subgrid is currently computing as 'auto' sized), the results are nonsensical.

I still think it's better to have the test changes closer to the relevant implementation details.

[Matt Woodrow]

Created attachment 452067 [details]
Patch

[Oriol Brufau]

Comment on attachment 452067 [details]
Patch

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

Overall it looks reasonable, but some comments.

> Source/WebCore/rendering/RenderGrid.cpp:1620
> +bool RenderGrid::isSubgridRows() const

Nit: since they are just trivial syntax sugar for isSubgrid(), maybe make isSubgridRows() and isSubgridColumns() inline in the header?

> Source/WebCore/rendering/RenderGrid.cpp:1996
> +bool RenderGrid::computeGridSpanForOutOfFlowChild(const RenderBox& child, GridTrackSizingDirection direction, int& startLine, bool& startIsAuto, int& endLine, bool& endIsAuto) const

It seems a bit strange to me that a method called computeGridSpanForSomething doesn't provide the caller with a GridSpan.
Also startIsAuto can be set to true even it it's not actually auto but needs to be clamped, lastLine is recomputed, etc.
It seems there is a single caller, so maybe merge this into that?

GridSpan RenderGrid::gridSpanForOutOfFlowChild(const RenderBox& child, GridTrackSizingDirection direction) const
{
    ASSERT(child.isOutOfFlowPositioned());
    int lastLine = numTracks(direction, m_grid);
    GridSpan span = GridPositionsResolver::resolveGridPositionsFromStyle(child, direction);
    if (span.isIndefinite())
        return GridSpan::translatedDefiniteGridSpan(0, lastLine);

    unsigned explicitStart = m_grid.explicitGridStart(direction);
    int startLine = span.untranslatedStartLine() + explicitStart;
    int endLine = span.untranslatedEndLine() + explicitStart;

    GridPosition startPosition = direction == ForColumns ? child.style().gridItemColumnStart() : child.style().gridItemRowStart();
    GridPosition endPosition = direction == ForColumns ? child.style().gridItemColumnEnd() : child.style().gridItemRowEnd();

    if (startPosition.isAuto() || startLine < 0 || startLine > lastLine)
        startLine = 0;
    if (endPosition.isAuto() || endLine < 0 || endLine > lastLine)
        endLine = lastLine;
    return GridSpan::translatedDefiniteGridSpan(startLine, endLine);
}

> Source/WebCore/rendering/RenderGrid.cpp:2040
> +        bool isSubgrid = direction == ForColumns ? renderGrid->isSubgridColumns() : renderGrid->isSubgridRows();

bool isSubgrid = renderGrid->isSubgrid(direction);

> Source/WebCore/rendering/style/GridArea.h:68
> +        ASSERT(isTranslatedDefinite() || m_type == UntranslatedDefinite);

So basically ASSERT(!isIndefinite())?

> Source/WebCore/rendering/style/GridArea.h:152
> +            m_startLine = parent.integerSpan() - m_endLine + parent.startLine();

If I get it right, parent.integerSpan()+parent.startLine() is parent.endLine(), so

    m_startLine = parent.endLine() - m_endLine;
    m_endLine = parent.endLine() - start;

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:159
> +    if (!initialPosition.isAuto() && !finalPosition.isAuto())

Nit: the code can check the type of each position twice. I think it could be:

    if (initialPosition.isAuto())
        return !finalPosition.isSpan();
    if (finalPosition.isAuto())
        return !initialPosition.isSpan();
    return false;

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:192
> +            if (finalPosition.isSpan())

I don't think this can be true, or isIndefiniteSpan would have returned false?

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:197
> +            if (initialPosition.isSpan())

Ditto

[Manuel Rego Casasnovas]

Comment on attachment 452067 [details]
Patch

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

> Source/WebCore/rendering/Grid.cpp:66
> +    GridArea clamped = area;

Nit: s/campled/campledArea/

> Source/WebCore/rendering/GridLayoutFunctions.cpp:113
> +{

Nit: Could we ASSERT that subgrid is actually a subgrid?

> Source/WebCore/rendering/RenderGrid.cpp:671
> +        grid.clampAndTranslateToImplicitGrid(area);

This name is a little bit confusing to me, there is no implicit grid in subgrids, we're not adding extra columns/rows, so we're always in the explicit grid defined by our parent. Why you call this "TranslateToImplicitGrid"? Should we use a different name here?

> Source/WebCore/rendering/RenderGrid.cpp:2036
> +    while (renderGrid != this) {

When we need this loop? Could an if be enough?

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:187
> +        int lineCount = direction == ForColumns ? gridItem.style().gridColumns().size() : gridItem.style().gridRows().size();

Why int?

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:206
> +    if (specifiedSubgrid && gridContainer.isSubgridColumns()) {

Isn't enough to check gridContainer.isSubgridColumns()?

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:211
> +    auto specifiedColumns = specifiedSubgrid ? 0 : gridContainer.style().gridColumns().size();

Could you explain this condition? If it's not a subgrid, what do we need to do?

> LayoutTests/imported/w3c/ChangeLog:8
> +        Test changes to match latest spec, submitted upstream as https://github.com/web-platform-tests/wpt/pull/32629

Nit: Could you link the spec issue here too?

> LayoutTests/imported/w3c/ChangeLog:11
> +        * web-platform-tests/css/css-grid/subgrid/line-names-005-expected.html:

So we don't start passing any test after this change?

[Dean Jackson]

Oriol, Manuel, thanks for these reviews! If Matt addressed the feedback would you be ok with the patch landing in the next couple of days, and then us doing any cleanup next week? The same applies to the subsequent patches Matt has already posted and I reviewed. I'm not the best reviewer, so we'd still love your eyes on the work.

[Manuel Rego Casasnovas]

(In reply to Dean Jackson from comment #18)
> Oriol, Manuel, thanks for these reviews! If Matt addressed the feedback
> would you be ok with the patch landing in the next couple of days, and then
> us doing any cleanup next week? The same applies to the subsequent patches
> Matt has already posted and I reviewed. I'm not the best reviewer, so we'd
> still love your eyes on the work.

This patch looks good to land.

Though I'm not sure I understand the urgency, I believe we haven't reviewed a bunch of the patches yet. And Oriol's reviews have been helping to find issues.

What would be the plan? Those patches get merged this week, and we do post-reviews later on in bugzilla?

[Matt Woodrow]

Created attachment 452234 [details]
Patch

[Matt Woodrow]

(In reply to Manuel Rego Casasnovas from comment #17)
 
> > Source/WebCore/rendering/RenderGrid.cpp:2036
> > +    while (renderGrid != this) {
> 
> When we need this loop? Could an if be enough?

We need to loop because sub grids can be nested arbitrarily many times, and we want to keep converting coordinate spaces until we get to 'this'.

> 
> > Source/WebCore/rendering/style/GridPositionsResolver.cpp:187
> > +        int lineCount = direction == ForColumns ? gridItem.style().gridColumns().size() : gridItem.style().gridRows().size();
> 
> Why int?

Since we do a -1 below, and it could be 0. I could have used a ternary below instead of std::max, no strong opinion :)

[Matt Woodrow]

(In reply to Manuel Rego Casasnovas from comment #19)
> 
> What would be the plan? Those patches get merged this week, and we do
> post-reviews later on in bugzilla?

I believe so, yes. I'll be more than happy to open follow-up bugs and address any issues that are found.

[Simon Fraser (smfr)]

Comment on attachment 452234 [details]
Patch

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

> Source/WebCore/rendering/Grid.cpp:66
> +    GridArea clampedArea = area;

auto

> Source/WebCore/rendering/RenderGrid.cpp:754
> +        GridSpan rowPositions = GridPositionsResolver::resolveGridPositionsFromStyle(*child, ForRows);

auto

> Source/WebCore/rendering/RenderGrid.cpp:766
> +        GridSpan columnPositions = GridPositionsResolver::resolveGridPositionsFromStyle(*child, ForColumns);

auto

> Source/WebCore/rendering/RenderGrid.cpp:1988
> +    GridSpan span = GridPositionsResolver::resolveGridPositionsFromStyle(child, direction);

auto

> Source/WebCore/rendering/RenderGrid.cpp:1997
> +    GridPosition startPosition = direction == ForColumns ? child.style().gridItemColumnStart() : child.style().gridItemRowStart();
> +    GridPosition endPosition = direction == ForColumns ? child.style().gridItemColumnEnd() : child.style().gridItemRowEnd();

auto

> Source/WebCore/rendering/RenderGrid.h:90
> +    bool isSubgridRows() const

I can't parse this. What does it mean to be "is subgrid rows"?

> Source/WebCore/rendering/RenderGrid.h:94
> +    bool isSubgridColumns() const

Same

> Source/WebCore/rendering/style/GridPositionsResolver.cpp:201
> +        const RenderGrid& parent = *downcast<RenderGrid>(gridContainer.parent());
> +        GridTrackSizingDirection direction = GridLayoutFunctions::flowAwareDirectionForChild(parent, gridContainer, ForColumns);

auto

[EWS]

Committed r289986 (247372@main): <https://commits.webkit.org/247372@main>

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