Bug 204578 – [css-grid] Failures on css/css-grid/grid-model/grid-box-sizing-001.html

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Manuel Rego Casasnovas]

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/

[Oriol Brufau]

*** Bug 209652 has been marked as a duplicate of this bug. ***

Created attachment 418643 [details]
Patch

Created attachment 418730 [details]
Patch

[Javier Fernandez]

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.

Created attachment 418866 [details]
Patch

(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.

[Javier Fernandez]

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.

Created attachment 419022 [details]
Patch

(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!

[EWS]

Committed r272302: <https://trac.webkit.org/changeset/272302>

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

[Radar WebKit Bug Importer]

<rdar://problem/73920674>

[Sam Sneddon [:gsnedders]]

*** Bug 188502 has been marked as a duplicate of this bug. ***