Bug 219194 - the nested grid container which has replaced item with 'max-height' has wrong width(0px).
Summary: the nested grid container which has replaced item with 'max-height' has wrong...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 221403
Blocks:
  Show dependency treegraph
 
Reported: 2020-11-20 00:44 PST by Hyunjune Kim
Modified: 2021-02-12 01:15 PST (History)
19 users (show)

See Also:


Attachments
grid.html (365 bytes, text/html)
2020-11-20 00:44 PST, Hyunjune Kim
no flags Details
Patch (9.35 KB, patch)
2021-02-02 07:59 PST, zsun
no flags Details | Formatted Diff | Diff
Patch (9.31 KB, patch)
2021-02-03 08:46 PST, zsun
no flags Details | Formatted Diff | Diff
Patch (10.24 KB, patch)
2021-02-08 07:05 PST, zsun
no flags Details | Formatted Diff | Diff
Patch (10.57 KB, patch)
2021-02-09 05:51 PST, zsun
no flags Details | Formatted Diff | Diff
Patch (10.57 KB, patch)
2021-02-10 06:24 PST, zsun
no flags Details | Formatted Diff | Diff
Patch (10.65 KB, patch)
2021-02-10 12:43 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 Hyunjune Kim 2020-11-20 00:44:44 PST
Created attachment 414658 [details]
grid.html

Chromium Issue : https://bugs.chromium.org/p/chromium/issues/detail?id=1149143

What steps will reproduce the problem?
(1) Run attached file named 'grid.html'.
(2) Turn on the inspector
(3) The width of nested container is zero.

What is the expected result?
The width of nested container is '100px'

What happens instead?
The width of nested container is zero.
Comment 1 Radar WebKit Bug Importer 2020-11-27 00:45:15 PST
<rdar://problem/71760198>
Comment 2 zsun 2021-02-02 07:59:14 PST
Created attachment 419000 [details]
Patch
Comment 3 Javier Fernandez 2021-02-02 12:37:28 PST
Comment on attachment 419000 [details]
Patch

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

r=me

The failures doesn't seem related to this change, but please, try to verify it before landing; especially the flexbox test.

> Source/WebCore/rendering/RenderReplaced.cpp:797
> +    // If the height is a percentage and the width is auto, then the

We usually avoid wrapping comments, unless they are too long.
Comment 4 zsun 2021-02-03 08:46:51 PST
Created attachment 419142 [details]
Patch
Comment 5 zsun 2021-02-03 08:49:31 PST
(In reply to Javier Fernandez from comment #3)
> Comment on attachment 419000 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419000&action=review
> 
> r=me
> 
> The failures doesn't seem related to this change, but please, try to verify
> it before landing; especially the flexbox test.
> 
The failed flexbox test doesn't seem caused by this patch. In my local run, it failed for master branch as well. 

> > Source/WebCore/rendering/RenderReplaced.cpp:797
> > +    // If the height is a percentage and the width is auto, then the
> 
> We usually avoid wrapping comments, unless they are too long.

Addressed. Thank you!
Comment 6 EWS 2021-02-03 13:22:35 PST
Committed r272338: <https://trac.webkit.org/changeset/272338>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419142 [details].
Comment 7 Aakash Jain 2021-02-04 04:46:13 PST
(In reply to EWS from comment #6)
> Committed r272338: <https://trac.webkit.org/changeset/272338>
This seems to have broken imported/w3c/web-platform-tests/css/css-sizing/percentage-height-in-flexbox.html on ios. EWS also indicated that failure in https://ews-build.webkit.org/#/builders/51/builds/5936, should have waited for EWS to finish before cq+ing the patch.

https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fcss%2Fcss-sizing%2Fpercentage-height-in-flexbox.html
Comment 8 WebKit Commit Bot 2021-02-04 04:47:15 PST
Re-opened since this is blocked by bug 221403
Comment 9 zsun 2021-02-08 07:05:38 PST
Created attachment 419587 [details]
Patch
Comment 10 zsun 2021-02-09 05:51:18 PST
Created attachment 419706 [details]
Patch
Comment 11 zsun 2021-02-10 06:24:44 PST
Created attachment 419839 [details]
Patch
Comment 12 Javier Fernandez 2021-02-10 12:19:42 PST
Comment on attachment 419839 [details]
Patch

r=me
Comment 13 EWS 2021-02-10 12:34:56 PST
/Volumes/Data/worker/Commit-Queue/build/LayoutTests/imported/w3c/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
Comment 14 zsun 2021-02-10 12:43:45 PST
Created attachment 419892 [details]
Patch
Comment 15 EWS 2021-02-11 00:44:59 PST
Committed r272711: <https://commits.webkit.org/r272711>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419892 [details].
Comment 16 Sergio Villar Senin 2021-02-11 01:14:48 PST
Comment on attachment 419892 [details]
Patch

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

> Source/WebCore/rendering/RenderReplaced.cpp:798
> +    return hasRelativeLogicalHeight() && style().logicalWidth().isAuto();

I know I'm already too late here but shouldn't we check that the replaced element does indeed have an aspect ratio? Being a replaced element does not imply that it has an aspect ratio.
Comment 17 Javier Fernandez 2021-02-12 01:15:29 PST
Comment on attachment 419892 [details]
Patch

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

>> Source/WebCore/rendering/RenderReplaced.cpp:798
>> +    return hasRelativeLogicalHeight() && style().logicalWidth().isAuto();
> 
> I know I'm already too late here but shouldn't we check that the replaced element does indeed have an aspect ratio? Being a replaced element does not imply that it has an aspect ratio.

I think this change affects to the second part of this statement of the Grid Spec, related to the natural-size:

https://drafts.csswg.org/css-grid-1/#grid-item-sizing
"If the grid item has no preferred aspect ratio, and no natural size in the relevant axis (if it is a replaced element), the grid item is sized as for align-self: stretch."

There is a CSS WG issue precisely about this: https://github.com/w3c/csswg-drafts/issues/5713

Maybe we could consider also the case when the replaced item has a preferred aspect-ratio, but I think grid still needs more work to properly support this properly.