The following WPT tests failed in Safari css/css-grid/grid-items/grid-items-percentage-paddings-002.html css/css-grid/grid-items/grid-items-percentage-paddings-vertical-lr-002.html css/css-grid/grid-items/grid-items-percentage-paddings-vertical-rl-002.htm
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].
<rdar://problem/74711256>
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.
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.