[css-grid] Fix min/max widths of grid affected by ancestor

• Component: CSS
• Status: REOPENED ---

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

[Javier Fernandez]

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!

[Javier Fernandez]

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!

[Javier Fernandez]

Comment on attachment 421316 [details]
Patch

r=me

[EWS]

Committed r273435: <https://commits.webkit.org/r273435>

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

[Radar WebKit Bug Importer]

<rdar://problem/74711256>

[Robert Jenner]

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 222503

Created attachment 422134 [details]
Patch

[Javier Fernandez]

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.

[Oriol Brufau]

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

[Sergio Villar Senin]

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.

[Sergio Villar Senin]

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

[Javier Fernandez]

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

[EWS]

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

[zalan]

This patch has been reverted in r281662. See https://bugs.webkit.org/show_bug.cgi?id=229586#c0 for more info.