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
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.
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!
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!
<rdar://problem/73498198>
Committed r271745: <https://trac.webkit.org/changeset/271745> All reviewed patches have been landed. Closing bug and clearing flags on attachment 418145 [details].