Bug 204578 - [css-grid] Failures on css/css-grid/grid-model/grid-box-sizing-001.html
Summary: [css-grid] Failures on css/css-grid/grid-model/grid-box-sizing-001.html
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
: 209652 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-11-25 07:14 PST by Manuel Rego Casasnovas
Modified: 2021-02-03 00:43 PST (History)
17 users (show)

See Also:


Attachments
Patch (7.53 KB, patch)
2021-01-28 08:29 PST, zsun
no flags Details | Formatted Diff | Diff
Patch (11.69 KB, patch)
2021-01-29 08:46 PST, zsun
no flags Details | Formatted Diff | Diff
Patch (8.51 KB, patch)
2021-02-01 06:19 PST, zsun
no flags Details | Formatted Diff | Diff
Patch (8.48 KB, patch)
2021-02-02 10:08 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 Manuel Rego Casasnovas 2019-11-25 07:14:21 PST
We have a few failures in the following WPT test:
http://w3c-test.org/css/css-grid/grid-model/grid-box-sizing-001.html

Which is working fine in Chromium and Firefox.

This might be related to the following patch on Chromium that was the one adding the test:
https://chromium-review.googlesource.com/c/chromium/src/+/735141/
Comment 1 Oriol Brufau 2020-05-25 11:53:22 PDT
*** Bug 209652 has been marked as a duplicate of this bug. ***
Comment 2 zsun 2021-01-28 08:29:09 PST
Created attachment 418643 [details]
Patch
Comment 3 zsun 2021-01-29 08:46:07 PST
Created attachment 418730 [details]
Patch
Comment 4 Javier Fernandez 2021-02-01 03:03:53 PST
Comment on attachment 418730 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:194
> +        // FIXME: We should use RenderBlock::hasDefiniteLogicalHeight() only but it does not work for positioned stuff.

Why it doesn't work for positioned elements ? If that's the case, is there a WPT that still fails due to this issue ? If there isn't, would it be possible to define a test case and file a bug ? We could add such bug number as part of this comment to clarify the issue even more.

> Source/WebCore/rendering/RenderGrid.cpp:195
> +        m_hasDefiniteLogicalHeight = hasDefiniteLogicalHeight() || hasOverridingLogicalHeight() || computeContentLogicalHeight(MainOrPreferredSize, style().logicalHeight(), WTF::nullopt);

It's indeed true that if a box having an "override height" should be treated as if it had a definite size. However, this override height is set to apply the 'stretch' alignment, which is only implemented for grid and flexbox items. Is there any test case of the ones we pass now, thanks to this change, covering such situation ?

> Source/WebCore/rendering/RenderGrid.cpp:249
> +        if (cachedHasDefiniteLogicalHeight())

I'm not really sure why caching the result of the "hasDefiniteLogicalHeight" between layouts is the solution to this bug. Did you verify that the layout is correct in the first ons but wrong in the subsequent layouts ? I thin it'd be a good idea to explain in the ChangeLog why this is happening. Even if we eventually decide to land this patch, this information would be useful to implement a different solution, perhaps avoiding the inconsistencies between subsequent layout operations.
Comment 5 zsun 2021-02-01 06:19:27 PST
Created attachment 418866 [details]
Patch
Comment 6 zsun 2021-02-01 06:22:31 PST
(In reply to Javier Fernandez from comment #4)
> Comment on attachment 418730 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418730&action=review
> 
> > Source/WebCore/rendering/RenderGrid.cpp:194
> > +        // FIXME: We should use RenderBlock::hasDefiniteLogicalHeight() only but it does not work for positioned stuff.
> 
> Why it doesn't work for positioned elements ? If that's the case, is there a
> WPT that still fails due to this issue ? If there isn't, would it be
> possible to define a test case and file a bug ? We could add such bug number
> as part of this comment to clarify the issue even more.
> 

This comments has been in the original code and not to do with this patch. I tried tracing it back but couldn't find out why it's not explained in the first place. I guess that I can investigate this in a separate thread.

> > Source/WebCore/rendering/RenderGrid.cpp:195
> > +        m_hasDefiniteLogicalHeight = hasDefiniteLogicalHeight() || hasOverridingLogicalHeight() || computeContentLogicalHeight(MainOrPreferredSize, style().logicalHeight(), WTF::nullopt);
> 
> It's indeed true that if a box having an "override height" should be treated
> as if it had a definite size. However, this override height is set to apply
> the 'stretch' alignment, which is only implemented for grid and flexbox
> items. Is there any test case of the ones we pass now, thanks to this
> change, covering such situation ?
> 

The check is on "hasDefiniteLogicalHeight()", which basically calls "availableLogicalHeightForPercentageComputation()". For the test cases here top and bottom are both defined and it should return with an specific value of logical-height.

> > Source/WebCore/rendering/RenderGrid.cpp:249
> > +        if (cachedHasDefiniteLogicalHeight())
> 
> I'm not really sure why caching the result of the "hasDefiniteLogicalHeight"
> between layouts is the solution to this bug. Did you verify that the layout
> is correct in the first ons but wrong in the subsequent layouts ? I thin
> it'd be a good idea to explain in the ChangeLog why this is happening. Even
> if we eventually decide to land this patch, this information would be useful
> to implement a different solution, perhaps avoiding the inconsistencies
> between subsequent layout operations.

The cache logic could help to avoid the performance impact that hasDefiniteLogicalHeight() might introduce (see chromium change at https://codereview.chromium.org/2334133002/). As we discussed, I will address this in a separate patch.
Comment 7 Javier Fernandez 2021-02-01 12:28:39 PST
Comment on attachment 418866 [details]
Patch

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

r=me

> Source/WebCore/ChangeLog:14
> +

Nit: sometimes it helps to add a line here to describe the change in this file.

> Source/WebCore/rendering/RenderGrid.cpp:196
> +        bool hasDefiniteLogicalHeight = RenderBlock::hasDefiniteLogicalHeight() || hasOverridingLogicalHeight() || computeContentLogicalHeight(MainOrPreferredSize, style().logicalHeight(), WTF::nullopt);

Maybe we could add here  FIXME comment to suggest the idea of caching the hasDefiniteLogicalHeight if this new call causes a relevant performance regression.

> Source/WebCore/rendering/RenderGrid.cpp:253
> +

Remove this empty line, please.

> LayoutTests/imported/w3c/ChangeLog:8
> +        * web-platform-tests/css/css-grid/grid-model/grid-box-sizing-001-expected.txt:

You could mention here that some test cases pass now thanks to this change.
Comment 8 zsun 2021-02-02 10:08:58 PST
Created attachment 419022 [details]
Patch
Comment 9 zsun 2021-02-02 10:09:54 PST
(In reply to Javier Fernandez from comment #7)
> Comment on attachment 418866 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418866&action=review
> 
> r=me
> 
> > Source/WebCore/ChangeLog:14
> > +
> 
> Nit: sometimes it helps to add a line here to describe the change in this
> file.
> 
> > Source/WebCore/rendering/RenderGrid.cpp:196
> > +        bool hasDefiniteLogicalHeight = RenderBlock::hasDefiniteLogicalHeight() || hasOverridingLogicalHeight() || computeContentLogicalHeight(MainOrPreferredSize, style().logicalHeight(), WTF::nullopt);
> 
> Maybe we could add here  FIXME comment to suggest the idea of caching the
> hasDefiniteLogicalHeight if this new call causes a relevant performance
> regression.
> 
> > Source/WebCore/rendering/RenderGrid.cpp:253
> > +
> 
> Remove this empty line, please.
> 
> > LayoutTests/imported/w3c/ChangeLog:8
> > +        * web-platform-tests/css/css-grid/grid-model/grid-box-sizing-001-expected.txt:
> 
> You could mention here that some test cases pass now thanks to this change.

All comments have been addressed. Thank you!
Comment 10 EWS 2021-02-03 00:42:08 PST
Committed r272302: <https://trac.webkit.org/changeset/272302>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419022 [details].
Comment 11 Radar WebKit Bug Importer 2021-02-03 00:43:13 PST
<rdar://problem/73920674>