Bug 228076

Summary: REGRESSION (r277997) Images get stretched with aspect-ratio and max-width: x%
Product: WebKit Reporter: Ian Stewart <ian.c.stewart>
Component: Layout and RenderingAssignee: cathiechen <cathiechen>
Status: RESOLVED FIXED    
Severity: Major CC: bfulgham, cathiechen, changseok, esprehn+autocc, ews-watchlist, glenn, jonlee, koivisto, kondapallykalyan, pdr, rbuis, sabouhallawa, simon.fraser, svillar, webkit-bug-importer, zalan, zsun
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Mac (Intel)   
OS: macOS 11   
URL: https://gashouder.nl/partners/
See Also: https://bugs.webkit.org/show_bug.cgi?id=228872
Bug Depends on: 228734    
Bug Blocks:    
Attachments:
Description Flags
safari 14 & safari 15 side by side showing the image stretching problem
none
test-case
none
Possible WPT test
none
test-case
none
Patch
none
Patch
none
Patch none

Description Ian Stewart 2021-07-19 02:37:29 PDT
Created attachment 433781 [details]
safari 14 & safari 15 side by side showing the image stretching problem

When making an image element grow to its parent element aspect ratio seems to win from width, height => auto & max-width, -height => 100%.

This results in a stretched image in safari 15 (Technology Preview 127) while in safari 14 the image sizes correctly.

**not sure if this is a webkit bug or just a css styling mistake regarding the new css specs
Comment 1 Radar WebKit Bug Importer 2021-07-19 07:48:24 PDT
<rdar://problem/80783648>
Comment 2 Simon Fraser (smfr) 2021-07-19 11:07:13 PDT
Caused by r277997.
Comment 3 cathiechen 2021-07-20 06:13:35 PDT
Thanks for reporting this, I'll take a look today!
Comment 4 Jon Lee 2021-07-20 15:20:05 PDT
The attached png has loaded https://gashouder.nl/partners/
Comment 5 cathiechen 2021-07-23 02:01:20 PDT
Created attachment 434076 [details]
test-case
Comment 6 Rob Buis 2021-07-23 05:51:13 PDT
Created attachment 434082 [details]
Possible WPT test
Comment 7 cathiechen 2021-07-23 19:02:29 PDT
Created attachment 434155 [details]
test-case

Added a case that img uses aspect-ratio directly.
Comment 8 Jon Lee 2021-07-27 12:03:42 PDT
any update on the investigation?
Comment 9 cathiechen 2021-07-27 19:28:12 PDT
(In reply to Jon Lee from comment #8)
> any update on the investigation?

Yeah, there's some progress, but still looking for an approach.

This issue seems related to flex + img with aspect-ratio.

* In RenderFlexibleBox::setOverridingMainSizeForChild, the img's OverridingLogicalHeight is generated by img's aspect-ratio * FlexibleBox's width.

* Then we use OverridingLogicalHeight to calculate img's logical width in RenderReplaced::computeReplacedLogicalWidth.

The value of width is correct until now.

* But in RenderFlexibleBox::applyStretchAlignmentToChild which stretches child in the cross Axis, the logical width is changed to the intrinsic size because it has aspect-ratio and OverridingLogicalHeight.

Using the intrinsic size is not right, OverridingLogicalHeight is from FlexibleBox's width, not the intrinsic size.
Comment 10 cathiechen 2021-08-02 11:16:57 PDT
Created attachment 434775 [details]
Patch
Comment 11 cathiechen 2021-08-03 11:21:10 PDT
Created attachment 434842 [details]
Patch
Comment 12 cathiechen 2021-08-03 11:30:40 PDT
Hi,
I think this patch is ready for review now.
Please take a look, thanks:)

The debugging story is long, it ends up with constrainLogicalWidthInFragmentByMinMax:)

> This issue seems related to flex + img with aspect-ratio.
> 
> * In RenderFlexibleBox::setOverridingMainSizeForChild, the img's
> OverridingLogicalHeight is generated by img's aspect-ratio * FlexibleBox's
> width.
> 
> * Then we use OverridingLogicalHeight to calculate img's logical width in
> RenderReplaced::computeReplacedLogicalWidth.
> 
> The value of width is correct until now.
> 
> * But in RenderFlexibleBox::applyStretchAlignmentToChild which stretches
> child in the cross Axis, the logical width is changed to the intrinsic size
> because it has aspect-ratio and OverridingLogicalHeight.

The image get stretched because constrainLogicalWidthInFragmentByMinMax returns the intrinsic width while computing MinSize.
Comment 13 cathiechen 2021-08-04 01:41:39 PDT
Hi Antti,
Thanks for the review!
Comment 14 cathiechen 2021-08-04 01:42:49 PDT
Created attachment 434893 [details]
Patch
Comment 15 EWS 2021-08-04 03:57:16 PDT
Committed r280631 (240244@main): <https://commits.webkit.org/240244@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 434893 [details].
Comment 16 cathiechen 2021-08-04 08:40:48 PDT
Comment on attachment 434893 [details]
Patch

It's a fake warning.