Bug 191463 - [css-grid][css-flex] <table> grid item should fill the grid area for 'stretch'/'normal' self alignment
Summary: [css-grid][css-flex] <table> grid item should fill the grid area for 'stretch...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-09 04:02 PST by Javier Fernandez
Modified: 2021-02-03 06:05 PST (History)
14 users (show)

See Also:


Attachments
Test case to reproduce the issue (642 bytes, text/html)
2018-11-09 04:02 PST, Javier Fernandez
no flags Details
Patch (3.95 KB, patch)
2021-01-21 23:52 PST, zsun
no flags Details | Formatted Diff | Diff
Patch (3.98 KB, patch)
2021-01-28 08:49 PST, zsun
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Javier Fernandez 2018-11-09 04:02:43 PST
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/
Comment 1 zsun 2021-01-21 23:52:48 PST
Created attachment 418110 [details]
Patch
Comment 2 Sergio Villar Senin 2021-01-25 01:50:02 PST
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"
Comment 3 Sergio Villar Senin 2021-01-25 03:15:13 PST
(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.
Comment 4 Javier Fernandez 2021-01-25 03:25:53 PST
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, ...)
Comment 5 zsun 2021-01-28 08:49:23 PST
Created attachment 418649 [details]
Patch
Comment 6 zsun 2021-02-01 07:09:52 PST
(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!
Comment 7 EWS 2021-02-03 05:56:35 PST
zsun@igalia.com 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.
Comment 8 EWS 2021-02-03 06:04:00 PST
Committed r272308: <https://trac.webkit.org/changeset/272308>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418649 [details].
Comment 9 Radar WebKit Bug Importer 2021-02-03 06:05:45 PST
<rdar://problem/73929027>