Bug 220733 – [css-grid] max-height percentages are wrongly resolved for replaced grid items

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

Webkit fails in WPT test css/css-grid/grid-items/grid-img-item-percent-max-height-001.html

Chromium also has reported and fixed this issue at https://bugs.chromium.org/p/chromium/issues/detail?id=750631

Created attachment 417951 [details]
Patch

[Javier Fernandez]

Comment on attachment 417951 [details]
Patch

r=me

[Manuel Rego Casasnovas]

Comment on attachment 417951 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        https://github.com/web-platform-tests/wpt/pull/26136

So there are different tests that we're interested here, maybe we should rewrite this somehow. I'm not sure if we need so many links.

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

Nit: This should be the line after the bug link and not here.

> Source/WebCore/ChangeLog:14
> +

It's good to explain the changes in the ChangeLog, even when we have link to the Chromium commit, it's nice to explain here again what the patch is doing and why it's fixing the issue.

> Source/WebCore/ChangeLog:15
> +        Test: imported/w3c/web-platform-tests/css/css-grid/grid-items/grid-img-item-percent-max-height-001.html

Is this patch only fixing this test, or also the other ones linked before?

> Source/WebCore/rendering/RenderBox.cpp:3187
> +        if (!document().inQuirksMode())

This is specifically checking something about quirks mode, is this a behavior change or is already covered by some existent quirks mode test.

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

You can mention here, that you're importing this tests from WPT.

Nit: They should be added to the corresponding w3c-import.log file.

> LayoutTests/imported/w3c/ChangeLog:9
> +        * web-platform-tests/css/css-grid/grid-items/grid-img-item-percent-max-height-001.html: Added.

Why only this test? There are a bunch more on the links in the ChangeLog.

Is this fix only for grid, or it also affects flexbox? If so we might need a flexbox test too.

Created attachment 418044 [details]
Patch

Created attachment 418050 [details]
Patch

(In reply to Manuel Rego Casasnovas from comment #3)
> Comment on attachment 417951 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417951&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        https://github.com/web-platform-tests/wpt/pull/26136
> 
> So there are different tests that we're interested here, maybe we should
> rewrite this somehow. I'm not sure if we need so many links.
Sorry! The second link doesn't belong here. Updated.

> 
> > Source/WebCore/ChangeLog:13
> > +        Reviewed by NOBODY (OOPS!).
> 
> Nit: This should be the line after the bug link and not here.
>
Updated.

> > Source/WebCore/ChangeLog:14
> > +
> 
> It's good to explain the changes in the ChangeLog, even when we have link to
> the Chromium commit, it's nice to explain here again what the patch is doing
> and why it's fixing the issue.
> 
Updated.

> > Source/WebCore/ChangeLog:15
> > +        Test: imported/w3c/web-platform-tests/css/css-grid/grid-items/grid-img-item-percent-max-height-001.html
> 
> Is this patch only fixing this test, or also the other ones linked before?
> 
It only intends to fix this test.

> > Source/WebCore/rendering/RenderBox.cpp:3187
> > +        if (!document().inQuirksMode())
> 
> This is specifically checking something about quirks mode, is this a
> behavior change or is already covered by some existent quirks mode test.
> 
I'm not so sure. I just think that it handles differently in quirk mode and I haven't see any extra tests added in Chromium case.

> > LayoutTests/imported/w3c/ChangeLog:7
> > +
> 
> You can mention here, that you're importing this tests from WPT.
> 
Mentioned.

> Nit: They should be added to the corresponding w3c-import.log file.
> 
Only one test and it's been added.

> > LayoutTests/imported/w3c/ChangeLog:9
> > +        * web-platform-tests/css/css-grid/grid-items/grid-img-item-percent-max-height-001.html: Added.
> 
> Why only this test? There are a bunch more on the links in the ChangeLog.
> 
> Is this fix only for grid, or it also affects flexbox? If so we might need a
> flexbox test too.

It's a fix for grid only. I added some lines in ChangeLog to explain it. 

Thank you!

[Manuel Rego Casasnovas]

Comment on attachment 418050 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        if not in quirk mode.

Nit: s/quirk/quirks/

> Source/WebCore/rendering/RenderBox.cpp:3188
> +        if (!document().inQuirksMode())
> +            return false;

I'm still curious, do we need this check?

I don't remember how grid works (if any) in quirk modes.

Could you verify if this code is used and if it has any effect depending on being on quirks mode or not? Thanks.

Created attachment 418148 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 418148 [details]
Patch

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

r=me, thanks for the changes.

> Source/WebCore/rendering/RenderBox.cpp:3187
> +        if (overridingContainingBlockContentLogicalHeight() == LayoutUnit(-1))
> +            return true;
> +        return false;

Nit: It's more common to write this like:
    return overridingContainingBlockContentLogicalHeight() == LayoutUnit(-1);

In that case you won't need the braces on the if either.

Created attachment 418280 [details]
Patch

[EWS]

commit-queue failed to commit attachment 418280 [details] to WebKit repository.

[Radar WebKit Bug Importer]

<rdar://problem/73617628>

Created attachment 419012 [details]
Patch

[EWS]

Committed r272309: <https://trac.webkit.org/changeset/272309>

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