[css-flexbox] WebKit doesn't preserve aspect ratio when computing cross size of flexed images in auto-height flex container

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Daniel Holbert]

STR:
(1) Load https://bug1362789.bmoattachments.org/attachment.cgi?id=8866580

EXPECTED RESULT:
Image should be a large square. (700px wide and tall)

ACTUAL RESULT:
Image is stretched to be rectangular, destroying aspect ratio -- image is 700px wide but only 300px tall.


Chrome has the same problem, which is filed as https://bugs.chromium.org/p/chromium/issues/detail?id=721123 .

EdgeHTML and Firefox give EXPECTED RESULTS.

The flexbox spec section 9.4 "Cross Size Determination" says the following about how to determine cross sizes (the height in this testcase):
  # Determine the hypothetical cross size of each item
  # by performing layout with the used main size
  # and the available space
https://drafts.csswg.org/css-flexbox-1/#cross-sizing

Here, the "used main size" is the stretched size (700px), and "performing layout" (for an image with auto height) should involve stretching its height accordingly.

The resulting hypothetical main size is used to determine the size of the flex line, which in turn is used as the cross size for all of the line's cross-axis-stretched items (including the image). So, the image should end up 700px tall here.

Note: This is kinda similar to bug 199583 and bug 169974, but it's not a dupe of either one, I think, because:
- bug 199583 is about min-size resolution & images that are forced to shrink below their normal content size [and Chrome gets that one right whereas it gets this bug here wrong]
- bug 169974 is about the opposite sort of influence, where a specified cross size is intended to influence the resolved main size.

[Radar WebKit Bug Importer]

<rdar://problem/61288094>

[Carlos Alberto Lopez Perez]

On bug 191460 I'm trying to import the WPT css-flexbox tests into WebKit and also de-deplicate the old versions of the flexbox tests we have, in favor of the new versions from WPT.

The WebKit layout test css3/flexbox/flexitem-stretch-image.html is moved to WPT as imported/w3c/web-platform-tests/css/css-flexbox/flexitem-stretch-image.html but this new version of the test causes failures due to this bug.

Check https://github.com/web-platform-tests/wpt/commit/557efa4d00 (its the diff between what we had as layout test vs the new version of the WPT test)

[Jen Simmons]

This is a key bug to fix so that Authors can use Flexbox as intended. At the moment, no one can use Flexbox in a DOM structure where images are flex children.

[Jen Simmons]

Here's a test: https://labs.jensimmons.com/2017/02-008.html

All the circles should be round.

[Sergio Villar Senin]

(In reply to Jen Simmons from comment #4)
> Here's a test: https://labs.jensimmons.com/2017/02-008.html
> 
> All the circles should be round.

I've a patch for this issue. I'll upload it soon

[Sergio Villar Senin]

Created attachment 414685 [details]
Patch

[Darin Adler]

Comment on attachment 414685 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Aspect ratio was not preserved in the cross axis because WebKit was stretching the items (as they're auto sized) whitout considering

Spelling error here in the word "without".

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1555
> +    // Aspect ratio is properly handled by RenderReplaced during layout.
> +    if (child.isRenderReplaced() && child.hasAspectRatio())
> +        return false;

This is frustratingly specific; I worry about a line of code like this that affects only a single regression test. Are there other things we should be testing?

> LayoutTests/ChangeLog:9
> +        * css3/flexbox/flexitem.html: Updated expectations. Aspect ratio must be preserved.

Do we need to keep this test around?

When our tests are duplicates of tests in Web Platform Tests, it would be good for the WebKit project to delete the older WebKit tests, unless they still have separate value. I presume in many cases the WPT test is a descendant of the test that began as a separate WebKit test. It’s better for long term maintainability to cut down the duplication when we can as we notice it.

[Sergio Villar Senin]

Comment on attachment 414685 [details]
Patch

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

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1555
>> +        return false;
> 
> This is frustratingly specific; I worry about a line of code like this that affects only a single regression test. Are there other things we should be testing?

It's a single regression test indeed, but it's a very common use case for using images as flex items. Anyway I've filled https://github.com/web-platform-tests/wpt/pull/26617 with a new test. I'll import it before landing this patch.

>> LayoutTests/ChangeLog:9
>> +        * css3/flexbox/flexitem.html: Updated expectations. Aspect ratio must be preserved.
> 
> Do we need to keep this test around?
> 
> When our tests are duplicates of tests in Web Platform Tests, it would be good for the WebKit project to delete the older WebKit tests, unless they still have separate value. I presume in many cases the WPT test is a descendant of the test that began as a separate WebKit test. It’s better for long term maintainability to cut down the duplication when we can as we notice it.

We have to keep it indeed because it's testing some code paths that are not tested by any current WPT test. Actually EWS reported some test failures in the first version of this patch that were triggered by this specific test.

That said, I think it's worth thinking about moving this test to WPT in  a follow up.

[Sergio Villar Senin]

*** Bug 199583 has been marked as a duplicate of this bug. ***

[Sergio Villar Senin]

(In reply to Sergio Villar Senin from comment #8)
> Comment on attachment 414685 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=414685&action=review
> 
> >> Source/WebCore/rendering/RenderFlexibleBox.cpp:1555
> >> +        return false;
> > 
> > This is frustratingly specific; I worry about a line of code like this that affects only a single regression test. Are there other things we should be testing?
> 
> It's a single regression test indeed, but it's a very common use case for
> using images as flex items. Anyway I've filled
> https://github.com/web-platform-tests/wpt/pull/26617 with a new test. I'll
> import it before landing this patch.

The patch landed with the new test. I'll proceed to import several aspect ratio tests from the WPT repository once the SVN lockdown is over and then I'll land this patch.

[Sergio Villar Senin]

Created attachment 415112 [details]
Patch for landing

After importing more tests we're passing 3 more tests plus the WebKit test with incorrect expectations

[Sergio Villar Senin]

Committed r270288: <https://trac.webkit.org/changeset/270288>