Bug 220657 – [css-grid] Relayout grid items when definiteness changes.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

Previously RenderGrid performed a simple logical-height change check
to determine if a grid-item needed relayout.

This wasn't correct as when setting an override logical-height the
definiteness can change causing %-height children to resolve
differently.

Created attachment 417699 [details]
Patch

Created attachment 417842 [details]
Patch

Created attachment 417863 [details]
Patch

[Oriol Brufau]

Comment on attachment 417863 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:1147
> +        if (desiredLogicalHeight != child.logicalHeight() || child.maybeHasPercentHeightDescendant()) {

I think it may be possible to use hasPercentHeightDescendant() instead of adding maybeHasPercentHeightDescendant()
In Chromium this wasn't possible because of this quirk: https://quirks.spec.whatwg.org/#the-percentage-height-calculation-quirk
But WebKit doesn't seem to use the quirk for grid according to the image in https://github.com/w3c/csswg-drafts/issues/5545

> Source/WebCore/rendering/RenderGrid.cpp:1149
>              child.setNeedsLayout(MarkOnlyThis);

Just noting that the relayout can worsen performance for deeply nested grids.
https://bugs.chromium.org/p/chromium/issues/detail?id=1146377#c2

Though it doesn't seem possible to do the right thing in a performant way with this rendering engine. Chromium will only address the performance problem in LayoutNG.

> LayoutTests/imported/w3c/ChangeLog:5
> +

Nit: can you mention that you are importing tests, not creating new ones?

> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/relative-grandchild-expected.xht:5
> +  <link rel="author" title="Gérard Talbot" href="http://www.gtalbot.org/BrowserBugsSection/css21testsuite/" />

Bad text encoding in "Gérard"?

Created attachment 417965 [details]
Patch

(In reply to Oriol Brufau from comment #4)
> Comment on attachment 417863 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417863&action=review
> 
> > Source/WebCore/rendering/RenderGrid.cpp:1147
> > +        if (desiredLogicalHeight != child.logicalHeight() || child.maybeHasPercentHeightDescendant()) {
> 
> I think it may be possible to use hasPercentHeightDescendant() instead of
> adding maybeHasPercentHeightDescendant()
> In Chromium this wasn't possible because of this quirk:
> https://quirks.spec.whatwg.org/#the-percentage-height-calculation-quirk
> But WebKit doesn't seem to use the quirk for grid according to the image in
> https://github.com/w3c/csswg-drafts/issues/5545
> 
Updated

> > Source/WebCore/rendering/RenderGrid.cpp:1149
> >              child.setNeedsLayout(MarkOnlyThis);
> 
> Just noting that the relayout can worsen performance for deeply nested grids.
> https://bugs.chromium.org/p/chromium/issues/detail?id=1146377#c2
> 
> Though it doesn't seem possible to do the right thing in a performant way
> with this rendering engine. Chromium will only address the performance
> problem in LayoutNG.
> 
> > LayoutTests/imported/w3c/ChangeLog:5
> > +
> 
> Nit: can you mention that you are importing tests, not creating new ones?
> 

Added

> > LayoutTests/imported/w3c/web-platform-tests/css/css-grid/relative-grandchild-expected.xht:5
> > +  <link rel="author" title="Gérard Talbot" href="http://www.gtalbot.org/BrowserBugsSection/css21testsuite/" />
> 
> Bad text encoding in "Gérard"?

Not sure why. It seems displaying fine for me.

[Manuel Rego Casasnovas]

Comment on attachment 417965 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Reviewed by NOBODY (OOPS!).

Nit: This line is wrongly placed.

> Source/WebCore/ChangeLog:12
> +

Pleaes describe here what the patch is doing. Despite the link to the Chromium patch, it's good to describe things for the change in WebKit as it's not totally equivalent.

> Source/WebCore/ChangeLog:14
> +               imported/w3c/web-platform-tests/css/css-grid/relative-grandchild.html

It looks like Chromium change also has some effects on css/css-grid/grid-child-percent-basis-resize-1.html test, have you checked that on WebKit?

> Source/WebCore/rendering/RenderGrid.cpp:1144
> +        // Checking the logical-height of a child isn't enough. Setting an override
> +        // logical-height changes the definiteness, resulting in percentages to
> +        // resolve differently.

Nit: In WebKit you don't need to wrap comments in 80-columns.

Created attachment 418035 [details]
Patch

(In reply to Manuel Rego Casasnovas from comment #7)
> Comment on attachment 417965 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417965&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        Reviewed by NOBODY (OOPS!).
> 
> Nit: This line is wrongly placed.
> 
> > Source/WebCore/ChangeLog:12
> > +
> 
> Pleaes describe here what the patch is doing. Despite the link to the
> Chromium patch, it's good to describe things for the change in WebKit as
> it's not totally equivalent.
> 
> > Source/WebCore/ChangeLog:14
> > +               imported/w3c/web-platform-tests/css/css-grid/relative-grandchild.html
> 
> It looks like Chromium change also has some effects on
> css/css-grid/grid-child-percent-basis-resize-1.html test, have you checked
> that on WebKit?
> 
> > Source/WebCore/rendering/RenderGrid.cpp:1144
> > +        // Checking the logical-height of a child isn't enough. Setting an override
> > +        // logical-height changes the definiteness, resulting in percentages to
> > +        // resolve differently.
> 
> Nit: In WebKit you don't need to wrap comments in 80-columns.

Comments have been addressed. Thank you!

[Manuel Rego Casasnovas]

Comment on attachment 418035 [details]
Patch

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

r=me with the requested changes inline.

> Source/WebCore/ChangeLog:8
> +        When setting an overide logical-height the definiteness can change causing %-height

Nit: s/overide/override/

> Source/WebCore/ChangeLog:-12
> -        * rendering/RenderGrid.cpp:
> -        (WebCore::RenderGrid::applyStretchAlignmentToChildIfNeeded):
> -

I guess this was a mistake, you shouldn't change other parts of the ChangeLog.

> Source/WebCore/rendering/RenderGrid.cpp:1142
> +        // Checking the logical-height of a child isn't enough. Setting an override ogical-height

s/ogical/logical/

> LayoutTests/TestExpectations:-3970
> -webkit.org/b/212246 imported/w3c/web-platform-tests/css/css-grid/grid-child-percent-basis-resize-1.html [ ImageOnlyFailure ]

I believe you have to reflect this in LayoutTests/ChangeLog.

Created attachment 418145 [details]
Patch

(In reply to Manuel Rego Casasnovas from comment #10)
> Comment on attachment 418035 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418035&action=review
> 
> r=me with the requested changes inline.
> 
> > Source/WebCore/ChangeLog:8
> > +        When setting an overide logical-height the definiteness can change causing %-height
> 
> Nit: s/overide/override/
> 
> > Source/WebCore/ChangeLog:-12
> > -        * rendering/RenderGrid.cpp:
> > -        (WebCore::RenderGrid::applyStretchAlignmentToChildIfNeeded):
> > -
> 
> I guess this was a mistake, you shouldn't change other parts of the
> ChangeLog.
> 
> > Source/WebCore/rendering/RenderGrid.cpp:1142
> > +        // Checking the logical-height of a child isn't enough. Setting an override ogical-height
> 
> s/ogical/logical/
> 
> > LayoutTests/TestExpectations:-3970
> > -webkit.org/b/212246 imported/w3c/web-platform-tests/css/css-grid/grid-child-percent-basis-resize-1.html [ ImageOnlyFailure ]
> 
> I believe you have to reflect this in LayoutTests/ChangeLog.

All comments have been addressed. Thanks!

[Radar WebKit Bug Importer]

<rdar://problem/73498198>

[EWS]

Committed r271745: <https://trac.webkit.org/changeset/271745>

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