Bug 219194

<rdar://problem/71760198>

Created attachment 419000 [details]
Patch

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!

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

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

(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

Re-opened since this is blocked by bug 221403

Created attachment 419587 [details]
Patch

Created attachment 419706 [details]
Patch

Created attachment 419839 [details]
Patch

Comment on attachment 419839 [details]
Patch

r=me

/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

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

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

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