Bug 228076 - REGRESSION (r277997) Images get stretched with aspect-ratio and max-width: x%
Summary: REGRESSION (r277997) Images get stretched with aspect-ratio and max-width: x%
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: Safari Technology Preview
Hardware: Mac (Intel) macOS 11
: P2 Major
Assignee: cathiechen
URL: https://gashouder.nl/partners/
Keywords: InRadar
Depends on: 228734
Blocks:
  Show dependency treegraph
 
Reported: 2021-07-19 02:37 PDT by Ian Stewart
Modified: 2021-08-06 14:08 PDT (History)
17 users (show)

See Also:


Attachments
safari 14 & safari 15 side by side showing the image stretching problem (1.52 MB, image/png)
2021-07-19 02:37 PDT, Ian Stewart
no flags Details
test-case (393 bytes, text/html)
2021-07-23 02:01 PDT, cathiechen
no flags Details
Possible WPT test (606 bytes, text/html)
2021-07-23 05:51 PDT, Rob Buis
no flags Details
test-case (572 bytes, text/html)
2021-07-23 19:02 PDT, cathiechen
no flags Details
Patch (7.22 KB, patch)
2021-08-02 11:16 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (8.08 KB, patch)
2021-08-03 11:21 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (8.08 KB, patch)
2021-08-04 01:42 PDT, cathiechen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.