Bug 220733 - [css-grid] max-height percentages are wrongly resolved for replaced grid items
Summary: [css-grid] max-height percentages are wrongly resolved for replaced grid items
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-01-19 08:41 PST by zsun
Modified: 2021-02-03 06:05 PST (History)
12 users (show)

See Also:


Attachments
Patch (5.99 KB, patch)
2021-01-20 01:06 PST, zsun
no flags Details | Formatted Diff | Diff
Patch (8.60 KB, patch)
2021-01-21 08:45 PST, zsun
no flags Details | Formatted Diff | Diff
Patch (6.56 KB, patch)
2021-01-21 09:30 PST, zsun
no flags Details | Formatted Diff | Diff
Patch (6.60 KB, patch)
2021-01-22 07:46 PST, zsun
no flags Details | Formatted Diff | Diff
Patch (6.54 KB, patch)
2021-01-25 05:44 PST, zsun
no flags Details | Formatted Diff | Diff
Patch (6.60 KB, patch)
2021-02-02 09:40 PST, zsun
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zsun 2021-01-19 08:41:18 PST
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
Comment 1 zsun 2021-01-20 01:06:25 PST
Created attachment 417951 [details]
Patch
Comment 2 Javier Fernandez 2021-01-20 09:13:50 PST
Comment on attachment 417951 [details]
Patch

r=me
Comment 3 Manuel Rego Casasnovas 2021-01-21 00:25:37 PST
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.
Comment 4 zsun 2021-01-21 08:45:19 PST
Created attachment 418044 [details]
Patch
Comment 5 zsun 2021-01-21 09:30:09 PST
Created attachment 418050 [details]
Patch
Comment 6 zsun 2021-01-21 09:37:12 PST
(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!
Comment 7 Manuel Rego Casasnovas 2021-01-22 00:58:16 PST
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.
Comment 8 zsun 2021-01-22 07:46:15 PST
Created attachment 418148 [details]
Patch
Comment 9 Manuel Rego Casasnovas 2021-01-25 03:32:32 PST
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.
Comment 10 zsun 2021-01-25 05:44:48 PST
Created attachment 418280 [details]
Patch
Comment 11 EWS 2021-01-25 06:16:38 PST
commit-queue failed to commit attachment 418280 [details] to WebKit repository.
Comment 12 Radar WebKit Bug Importer 2021-01-26 08:42:15 PST
<rdar://problem/73617628>
Comment 13 zsun 2021-02-02 09:40:41 PST
Created attachment 419012 [details]
Patch
Comment 14 EWS 2021-02-03 06:05:03 PST
Committed r272309: <https://trac.webkit.org/changeset/272309>

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