Summary: | [css-grid] Fix min/max widths of grid affected by ancestor | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | zsun | ||||||||||||||||||||||
Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||
Status: | REOPENED --- | ||||||||||||||||||||||||
Severity: | Normal | CC: | changseok, commit-queue, esprehn+autocc, ews-watchlist, glenn, jenner, jfernandez, kondapallykalyan, obrufau, pdr, rego, svillar, webkit-bug-importer, zalan | ||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=229586 | ||||||||||||||||||||||||
Bug Depends on: | 222503 | ||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||
Attachments: |
|
Description
zsun
2021-02-18 05:25:34 PST
Created attachment 420823 [details]
Patch
Created attachment 420837 [details]
Patch
Comment on attachment 420837 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420837&action=review > Source/WebCore/ChangeLog:9 > + Before update logical-width, for element that needs prefrredWidth recalcution, s/prefrredWidth/preferredWidth > Source/WebCore/rendering/RenderBox.cpp:2485 > + if (cb->needsPreferredWidthsRecalculation() && !box->preferredLogicalWidthsDirty()) { I supposed box->preferredLogicalWidthsDirty() is false for sure, otherwise we should have entered in the branch above, right ? Perhaps you meant "cb->" here ? Just wondering. > Source/WebCore/rendering/RenderBox.cpp:2498 > + // Laying out this object means that its containing block is also being laid out. This object is special, in that its min/max widths depend on Nit, I think it'd be clearer if we put the comment just above the if statement. Created attachment 421305 [details]
Patch
(In reply to Javier Fernandez from comment #3) > Comment on attachment 420837 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=420837&action=review > > > Source/WebCore/ChangeLog:9 > > + Before update logical-width, for element that needs prefrredWidth recalcution, > > s/prefrredWidth/preferredWidth > > > Source/WebCore/rendering/RenderBox.cpp:2485 > > + if (cb->needsPreferredWidthsRecalculation() && !box->preferredLogicalWidthsDirty()) { > > I supposed box->preferredLogicalWidthsDirty() is false for sure, otherwise > we should have entered in the branch above, right ? > > Perhaps you meant "cb->" here ? Just wondering. > > > Source/WebCore/rendering/RenderBox.cpp:2498 > > + // Laying out this object means that its containing block is also being laid out. This object is special, in that its min/max widths depend on > > Nit, I think it'd be clearer if we put the comment just above the if > statement. Comments have addressed. Thank you! Comment on attachment 421305 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421305&action=review > Source/WebCore/rendering/RenderBox.cpp:2479 > + // If the preferred widths are already dirty at this point (during layout), it actually means that we never need to calculate them, since that should I think we can move the comments above the if clause, and remove the brackets. > Source/WebCore/rendering/RenderBox.cpp:2485 > + // If our containing block also has min/max widths that are affected by the ancestry, we have already dealt with this object as well. Avoid Ditto > Source/WebCore/rendering/RenderBox.cpp:2496 > + if (shouldRecalculateMinMaxWidthsAffectedByAncestor(this)) { Why not using a composed conditional clause instead two nested if ? > Source/WebCore/rendering/RenderBox.cpp:2497 > + // Laying out this object means that its containing block is also being laid out. This object is special, in that its min/max widths depend on We could move the comments here as well, as I suggested before. Created attachment 421316 [details]
Patch
(In reply to Javier Fernandez from comment #6) > Comment on attachment 421305 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=421305&action=review > > > Source/WebCore/rendering/RenderBox.cpp:2479 > > + // If the preferred widths are already dirty at this point (during layout), it actually means that we never need to calculate them, since that should > > I think we can move the comments above the if clause, and remove the > brackets. > > > Source/WebCore/rendering/RenderBox.cpp:2485 > > + // If our containing block also has min/max widths that are affected by the ancestry, we have already dealt with this object as well. Avoid > > Ditto > > > Source/WebCore/rendering/RenderBox.cpp:2496 > > + if (shouldRecalculateMinMaxWidthsAffectedByAncestor(this)) { > > Why not using a composed conditional clause instead two nested if ? > > > Source/WebCore/rendering/RenderBox.cpp:2497 > > + // Laying out this object means that its containing block is also being laid out. This object is special, in that its min/max widths depend on > > We could move the comments here as well, as I suggested before. All review comments have been addressed. Thank you! Comment on attachment 421316 [details]
Patch
r=me
Committed r273435: <https://commits.webkit.org/r273435> All reviewed patches have been landed. Closing bug and clearing flags on attachment 421316 [details]. It appears this test may have broken another test: imported/w3c/web-platform-tests/css/css-flexbox/percentage-max-height-003.html Reported at bug: https://bugs.webkit.org/show_bug.cgi?id=222485 Re-opened since this is blocked by bug 222503 Created attachment 422134 [details]
Patch
Comment on attachment 422134 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422134&action=review > Source/WebCore/ChangeLog:8 > + It's a reland of r273435, which got reverted because it broken nit: s/broken/broke > Source/WebCore/rendering/RenderBox.cpp:2499 > + if (needsPreferredWidthsRecalculation() && shouldRecalculateMinMaxWidthsAffectedByAncestor(this) && parent()->preferredLogicalWidthsDirty()) { I think we should have a check here for parent(); I believe it's not guaranteed that it's not null. Comment on attachment 422134 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422134&action=review >> Source/WebCore/rendering/RenderBox.cpp:2499 >> + if (needsPreferredWidthsRecalculation() && shouldRecalculateMinMaxWidthsAffectedByAncestor(this) && parent()->preferredLogicalWidthsDirty()) { > > I think we should have a check here for parent(); I believe it's not guaranteed that it's not null. Why does WebKit need parent()->preferredLogicalWidthsDirty() if Chromium doesn't have it? And maybe the condition should be moved into shouldRecalculateMinMaxWidthsAffectedByAncestor() ? Created attachment 422253 [details]
Patch
Comment on attachment 422253 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422253&action=review What I don't exactly understand is which cases fix this and why the setPreferredLogicalWidthsDirty() is not invoked in the specific subclass RenderFlexibleBox, RenderGrid... etc > Source/WebCore/rendering/RenderBox.cpp:2490 > + } You can write this in a more compact way using some C++ fancyness if (const RenderBox* cb = box->containingBlock(); !cb->preferredLogicalWidthsDirty()) return false; Also I guess this function is only meant to be used from updateLogicalWidth(). If that's indeed the case it'd be better to move it to a lambda inside that method. Comment on attachment 422253 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422253&action=review What I don't exactly understand is which cases fix this and why the setPreferredLogicalWidthsDirty() is not invoked in the specific subclass RenderFlexibleBox, RenderGrid... etc > Source/WebCore/rendering/RenderBox.cpp:2490 > + } You can write this in a more compact way using some C++ fancyness if (const RenderBox* cb = box->containingBlock(); !cb->preferredLogicalWidthsDirty()) return false; Also I guess this function is only meant to be used from updateLogicalWidth(). If that's indeed the case it'd be better to move it to a lambda inside that method. Created attachment 425002 [details]
Patch
Created attachment 425089 [details]
Patch
Comment on attachment 425089 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425089&action=review r=me > Source/WebCore/rendering/RenderBlock.cpp:-639 > - Please, remove these empty lines Created attachment 425395 [details]
Patch
Created attachment 425506 [details]
Patch
Committed r275754 (236332@main): <https://commits.webkit.org/236332@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425506 [details]. This patch has been reverted in r281662. See https://bugs.webkit.org/show_bug.cgi?id=229586#c0 for more info. |