Bug 234564 - [css-flexbox] Incorrect height of flex items with aspect-ratio whenever the cross axis intrinsic size is larger than the viewport
Summary: [css-flexbox] Incorrect height of flex items with aspect-ratio whenever the c...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Major
Assignee: Sergio Villar Senin
URL:
Keywords: InRadar
Depends on: 235102
Blocks:
  Show dependency treegraph
 
Reported: 2021-12-21 08:21 PST by Pedro Paulo
Modified: 2022-01-13 05:04 PST (History)
20 users (show)

See Also:


Attachments
Minimal HTML file which reproduces the bug (183 bytes, text/html)
2021-12-21 08:21 PST, Pedro Paulo
no flags Details
Patch (7.58 KB, patch)
2021-12-28 03:44 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (9.52 KB, patch)
2022-01-10 02:46 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (10.66 KB, patch)
2022-01-12 09:18 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (10.15 KB, patch)
2022-01-13 00:47 PST, Sergio Villar Senin
rego: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Paulo 2021-12-21 08:21:01 PST
Created attachment 447717 [details]
Minimal HTML file which reproduces the bug

Hey! Hello everyone!

I was testing one of my websites on GNOME Web and found out that there was a major bug on the <img> tag when it is a child of a column-flexed flex element. The image gets the correct width, but the height does not follow the aspect ratio as it should, pushing the original image height instead, which generates problems for alignment. It can be fixed by putting the actual height you want, but that doesn't work on some cases (like on my website) where the width of the parent is dynamic, and it is non-trivial to calculate the correct height there without cluttering the website with Javascript resizes, which is also not recommended. It works fine on both Firefox and Chromium-based browsers, so it seems to be a Webkit glitch. I later tested the issue on a friend's iPhone on Safari and the glitch happened there too.

## Minimal example:
```html
<html>
	<body>
		<div style="display: flex;flex-direction: column;">
			<img src="https://upload.wikimedia.org/wikipedia/commons/8/8f/Artix_Linux_Logo.svg">
		</div>
	</body>
</html>
```

Tested on Epiphany (GNOME Web) 41.0 Flatpak (latest on Flathub) and on a friend's iPhone (don't know which version, but probably up to date).
Comment 1 Sergio Villar Senin 2021-12-22 00:36:56 PST
(In reply to Pedro Paulo from comment #0)
> Created attachment 447717 [details]
> Minimal HTML file which reproduces the bug
> 
> Hey! Hello everyone!
> 
> I was testing one of my websites on GNOME Web and found out that there was a
> major bug on the <img> tag when it is a child of a column-flexed flex
> element. The image gets the correct width, but the height does not follow
> the aspect ratio as it should, pushing the original image height instead,
> which generates problems for alignment. It can be fixed by putting the
> actual height you want, but that doesn't work on some cases (like on my
> website) where the width of the parent is dynamic, and it is non-trivial to
> calculate the correct height there without cluttering the website with
> Javascript resizes, which is also not recommended. It works fine on both
> Firefox and Chromium-based browsers, so it seems to be a Webkit glitch. I
> later tested the issue on a friend's iPhone on Safari and the glitch
> happened there too.
> 
> ## Minimal example:
> ```html
> <html>
> 	<body>
> 		<div style="display: flex;flex-direction: column;">
> 			<img
> src="https://upload.wikimedia.org/wikipedia/commons/8/8f/Artix_Linux_Logo.
> svg">
> 		</div>
> 	</body>
> </html>
> ```
> 
> Tested on Epiphany (GNOME Web) 41.0 Flatpak (latest on Flathub) and on a
> friend's iPhone (don't know which version, but probably up to date).

Thanks for the report.

Please, avoid things like "major bug" or the like in the bug title because it does not help triagging. It'd be much useful to try to describe the bug, like "aspect ratio of SVG not working on column flexboxes" or whatever you like.

The question I have is, did it used to work at some point? I just basically want to know whether it's a recent regression (there were recent changes) or something that has been around for a while.
Comment 2 Pedro Paulo 2021-12-22 17:35:55 PST
(In reply to Sergio Villar Senin from comment #1)
> (In reply to Pedro Paulo from comment #0)
> > Created attachment 447717 [details]
> > Minimal HTML file which reproduces the bug
> > 
> > Hey! Hello everyone!
> > 
> > I was testing one of my websites on GNOME Web and found out that there was a
> > major bug on the <img> tag when it is a child of a column-flexed flex
> > element. The image gets the correct width, but the height does not follow
> > the aspect ratio as it should, pushing the original image height instead,
> > which generates problems for alignment. It can be fixed by putting the
> > actual height you want, but that doesn't work on some cases (like on my
> > website) where the width of the parent is dynamic, and it is non-trivial to
> > calculate the correct height there without cluttering the website with
> > Javascript resizes, which is also not recommended. It works fine on both
> > Firefox and Chromium-based browsers, so it seems to be a Webkit glitch. I
> > later tested the issue on a friend's iPhone on Safari and the glitch
> > happened there too.
> > 
> > ## Minimal example:
> > ```html
> > <html>
> > 	<body>
> > 		<div style="display: flex;flex-direction: column;">
> > 			<img
> > src="https://upload.wikimedia.org/wikipedia/commons/8/8f/Artix_Linux_Logo.
> > svg">
> > 		</div>
> > 	</body>
> > </html>
> > ```
> > 
> > Tested on Epiphany (GNOME Web) 41.0 Flatpak (latest on Flathub) and on a
> > friend's iPhone (don't know which version, but probably up to date).
> 
> Thanks for the report.
> 
> Please, avoid things like "major bug" or the like in the bug title because
> it does not help triagging. It'd be much useful to try to describe the bug,
> like "aspect ratio of SVG not working on column flexboxes" or whatever you
> like.
> 
> The question I have is, did it used to work at some point? I just basically
> want to know whether it's a recent regression (there were recent changes) or
> something that has been around for a while.

Oh! Sorry about that. My mistake.

I encountered it while I was testing my website recently, but I found someone relating a similar issue back in 2019 or something, but I cannot find it here. Oh, and it seems to also happen with regular rasterized (non-SVG, such as jpeg and png files) too, but they get stretched instead.
Comment 3 Sergio Villar Senin 2021-12-28 03:44:04 PST
Created attachment 448041 [details]
Patch
Comment 4 Sergio Villar Senin 2021-12-28 03:45:32 PST
Note that I'm going to include the test I uploaded for review to the WPT repo mentioned in the "See Also" field.
Comment 5 Radar WebKit Bug Importer 2021-12-28 08:21:15 PST
<rdar://problem/86958389>
Comment 6 Sergio Villar Senin 2022-01-10 02:46:23 PST
Created attachment 448729 [details]
Patch
Comment 7 Sergio Villar Senin 2022-01-12 09:18:16 PST
Created attachment 448952 [details]
Patch
Comment 8 Manuel Rego Casasnovas 2022-01-12 13:32:39 PST
Comment on attachment 448952 [details]
Patch

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

> Source/WebCore/rendering/RenderFlexibleBox.cpp:996
> +bool RenderFlexibleBox::childCrossSizeShouldUseContainerCrossSize(const RenderBox& child, SizeType sizeType) const

Why are you adding the sizeType as argument, if it's always the default one?
Comment 9 Sergio Villar Senin 2022-01-13 00:44:37 PST
Comment on attachment 448952 [details]
Patch

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

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:996
>> +bool RenderFlexibleBox::childCrossSizeShouldUseContainerCrossSize(const RenderBox& child, SizeType sizeType) const
> 
> Why are you adding the sizeType as argument, if it's always the default one?

Ups, a leftover from a previous version of the patch. I'll remove it. Good catch!
Comment 10 Sergio Villar Senin 2022-01-13 00:44:39 PST
Comment on attachment 448952 [details]
Patch

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

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:996
>> +bool RenderFlexibleBox::childCrossSizeShouldUseContainerCrossSize(const RenderBox& child, SizeType sizeType) const
> 
> Why are you adding the sizeType as argument, if it's always the default one?

Ups, a leftover from a previous version of the patch. I'll remove it. Good catch!
Comment 11 Sergio Villar Senin 2022-01-13 00:47:05 PST
Created attachment 449032 [details]
Patch
Comment 12 Manuel Rego Casasnovas 2022-01-13 01:03:21 PST
Comment on attachment 449032 [details]
Patch

r=me
Comment 13 Sergio Villar Senin 2022-01-13 05:04:13 PST
Committed r287976 (246005@trunk): <https://commits.webkit.org/246005@trunk>