Bug 219194 – the nested grid container which has replaced item with 'max-height' has wrong width(0px).

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

[Hyunjune Kim]

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.

[Radar WebKit Bug Importer]

<rdar://problem/71760198>

Created attachment 419000 [details]
Patch

[Javier Fernandez]

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.

Created attachment 419142 [details]
Patch

(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!

[EWS]

Committed r272338: <https://trac.webkit.org/changeset/272338>

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

[Aakash Jain]

(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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 221403

Created attachment 419587 [details]
Patch

Created attachment 419706 [details]
Patch

Created attachment 419839 [details]
Patch

[Javier Fernandez]

Comment on attachment 419839 [details]
Patch

r=me

[EWS]

/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).

Created attachment 419892 [details]
Patch

[EWS]

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

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

[Sergio Villar Senin]

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.

[Javier Fernandez]

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.