Bug 191463 – [css-grid][css-flex] <table> grid item should fill the grid area for 'stretch'/'normal' self alignment

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Javier Fernandez]

Created attachment 354326 [details]
Test case to reproduce the issue

What steps will reproduce the problem?
(1) load the attached testcase
(2)
(3)

What is the expected result?
The <table> grid items should fill their grid area.
There should be no red areas visible.

Fwiw, the testcase works correctly in Firefox:
https://nightly.mozilla.org/

Created attachment 418110 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 418110 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        grid are imported.

In the Blink patch the author was also unskipping some other tests that are not imported in WebKit tree yet, we should probably do that before landing this change or even as part of this change.

> Source/WebCore/rendering/RenderTable.cpp:243
> +        // needed and that we're not skipping anything essential due to the early return here.

WebKit uses FIXME instead of TODO(name). In any case I think we can completely remove it at this point.

In any case I am not sure about the change itself because this is skipping the table width calculation for all the cases when the table is a grid item, but if the grid item is not stretched then we do need this computation because we need the table intrinsic size.

> LayoutTests/ChangeLog:8
> +        * TestExpectations:

You should fill in this ChangeLog as well. Something like "Unskipped a test that is now passing"

[Sergio Villar Senin]

(In reply to Sergio Villar Senin from comment #2)
> Comment on attachment 418110 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418110&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        grid are imported.
> 
> In the Blink patch the author was also unskipping some other tests that are
> not imported in WebKit tree yet, we should probably do that before landing
> this change or even as part of this change.

I think I messed it up, the other tests are for flexbox, sorry about the noise.

> 
> > Source/WebCore/rendering/RenderTable.cpp:243
> > +        // needed and that we're not skipping anything essential due to the early return here.
> 
> WebKit uses FIXME instead of TODO(name). In any case I think we can
> completely remove it at this point.
> 
> In any case I am not sure about the change itself because this is skipping
> the table width calculation for all the cases when the table is a grid item,
> but if the grid item is not stretched then we do need this computation
> because we need the table intrinsic size.

Looks like RenderBox ends up calling the preferred logical widths computation in any case so I guess this should indeed work. Let me just review again the test in order to fully understand what's going on.

[Javier Fernandez]

Comment on attachment 418110 [details]
Patch

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

>> Source/WebCore/rendering/RenderTable.cpp:243
>> +        // needed and that we're not skipping anything essential due to the early return here.
> 
> WebKit uses FIXME instead of TODO(name). In any case I think we can completely remove it at this point.
> 
> In any case I am not sure about the change itself because this is skipping the table width calculation for all the cases when the table is a grid item, but if the grid item is not stretched then we do need this computation because we need the table intrinsic size.

I think the change is correct, at least for the cases we were testing in blink. The idea is that in case the table has justify-self different from auto/stretch, we use the LayoutBox logic, which will eventually run the PreferredLogicalWidth call. However, as the table is a grid item, it should use its corresponding grid area as "container" when computing the available size. 

The FIXME still stands, because while I think the current RenderGrid logic deals with this correctly for regular boxes as grid item, I'm not completely sure it covers all the peculiarities a table could have (eg. margins, padding, ...)

Created attachment 418649 [details]
Patch

(In reply to Sergio Villar Senin from comment #2)
> Comment on attachment 418110 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418110&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        grid are imported.
> 
> In the Blink patch the author was also unskipping some other tests that are
> not imported in WebKit tree yet, we should probably do that before landing
> this change or even as part of this change.
> 
> > Source/WebCore/rendering/RenderTable.cpp:243
> > +        // needed and that we're not skipping anything essential due to the early return here.
> 
> WebKit uses FIXME instead of TODO(name). In any case I think we can
> completely remove it at this point.
> 
> In any case I am not sure about the change itself because this is skipping
> the table width calculation for all the cases when the table is a grid item,
> but if the grid item is not stretched then we do need this computation
> because we need the table intrinsic size.
> 
> > LayoutTests/ChangeLog:8
> > +        * TestExpectations:
> 
> You should fill in this ChangeLog as well. Something like "Unskipped a test
> that is now passing"

Updated. Thank you!

[EWS]

[e-mail] does not have committer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/Tools/Scripts/webkitpy/common/config/contributors.json.

Rejecting attachment 418649 [details] from commit queue.

[EWS]

Committed r272308: <https://trac.webkit.org/changeset/272308>

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

[Radar WebKit Bug Importer]

<rdar://problem/73929027>