1672462 - [REGRESSION] width: auto; set on svg image acts as 100% fill, not as auto image width

Status: RESOLVED FIXED

(Core :: Layout: Flexbox, defect, P2)

Description

[Dariusz Czajkowski]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:83.0) Gecko/20100101 Firefox/83.0

Steps to reproduce:

Demo: https://codepen.io/DCzajkowski/pen/pobNgzw?editors=1100

Visit this link in Chromium, Firefox ~80, and Firefox 83 beta (tested on Developer Edition, build 83.0b2 on macOS).

Actual results:

In the Firefox 83 beta (in previous versions (not sure how many back) it was behaving the same way as in Chromium) the image has smaller size to fit the available width.

To reproduce, you need: display: flex on the parent and the image inside the <img> tag to be an SVG with width: auto.

Expected results:

I think FF should set the img's width to x, where its aspect ratio is preserved based on the viewBox.

This is how it used to be in FF and this is how it is in Chromium.

I am not sure if this is a regression or is this an intended change

Comment 1

[Dariusz Czajkowski]

Update: Safari behaves the same as Chromium and as FF used to behave, so this seems as an unintended regression.

Comment 2

[Pascal Chevrel:pascalc (PTO until April 26)]

https://bugzilla.mozilla.org/show_bug.cgi?id=131653434:53.30 INFO: Last good revision: 095c8aec7254d999ce27a3f0c531ecd965f87f3b
34:53.30 INFO: First bad revision: fe228ad057f67075b8dbb39eb9cfe34cde30cad3
34:53.30 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=095c8aec7254d999ce27a3f0c531ecd965f87f3b&tochange=fe228ad057f67075b8dbb39eb9cfe34cde30cad3

Ting-Yu, this 83 regression was caused by bug 1316534, could you have a look please? Thanks

Comment 3

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Thanks for reporting this issue. This essentially the same as bug 1665532 (or bug 1670151).

The svg image in the testcase has an intrinsic ratio and definite height, but no intrinsic size. Currently, we return 0 from nsImageFrame::GetMinISize as its min-content width, and use it as the the flex item's content size suggestion.

I investigated a proper fix for this bug and found it's not trivial to make a replaced element's intrinsic size respect aspect-ratio, so it may take takes a while. If this bug occurs on real webpages, I'd suggest adding flex-shrink: 0 to the flex item or enlarging the flex container's width to prevent the item from shrinking because its main size does have the expected value from intrinsic ratio * height.

Comment 4

[Dariusz Czajkowski]

It did cause a bug on our widget that is embedded all over the Internet. Our fix was just to wrap the <img> in a plain ol' <div>. We just found it very weird no change to a code-base broke something on everyone's computer. It was the fact I got a new update of my Developer Edition Firefox a day ago that made me think a regression could be what's at play.

Comment 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Thank you for using Firefox in your dev environment. Sorry for causing the bug. It was an unfortunately consequence that fixing one bug ends up uncovering another bug.

Comment 6

[Dariusz Czajkowski]

No need to apologize! I also work with code. I know something about it 😅

Comment 7

[Karl Dubost💡 :karlcow]

Ting-Yu

do you know about Bug 1521882 ?
SVG without width/height specified has different intrinsic-sizing behavior in Chrome vs Firefox (e.g. in a float or in flexbox)

Comment 8

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1672462 - Print transferred size suggestion only if it exists.

Comment 9

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1672462 - Compute flex item's content size suggestion in inline axis via nsIFrame::ComputeSize.

Flex item's content size suggestion is the min-content size in the main
axis. Quoting from the CSS Sizing spec, section "5.1. Intrinsic Sizes",

The min-content size of a box in each axis is the size it would have
if it was a float given an auto size in that axis (and no minimum or
maximum size in that axis) and if its containing block was
zero-sized in that axis. (In other words, the minimum size it has
when sized as “shrink-to-fit”.)

However, all the frame types' GetMinISize() doesn't consider aspect
ratio (either the intrinsic aspect ratio on images or svg, or the
preferred aspect ratio set by the aspect-ratio property). That is, we
currently don't allow aspect ratio * definite block-size as part of
the equation in inline-size:min-content (bug 1646100 or bug 1670151).

Luckily, nsIFrame::ComputeSize() already takes care of this [2]. We just
need to add a flag to let ComputeSize() behave as if the flex item had
an auto inline size. The code should be similar to FloatMarginISize() in
BlockReflowInput.cpp except we are calculate the flex item's content-box
size.

[1] https://drafts.csswg.org/css-sizing-3/#intrinsic-sizes
[2] https://searchfox.org/mozilla-central/rev/dbced93f1c57942501c23d4633d01ce59d9087a1/layout/generic/nsContainerFrame.cpp#2732,2735-2736

Depends on D94801

Comment 10

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Hi Karl, Thanks for bringing bug 1521882 into my attention. This bug is slightly different from it because the SVG in this bug's testcase does specify a height, where bug 1521882 is about the size when both width and height are not specified.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/03abf875b544
Print transferred size suggestion only if it exists. r=emilio
https://hg.mozilla.org/integration/autoland/rev/9a7e5873fac5
Compute flex item's content size suggestion in inline axis via nsIFrame::ComputeSize. r=emilio

Comment 13

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/03abf875b544
https://hg.mozilla.org/mozilla-central/rev/9a7e5873fac5

Comment 14

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 9183911 [details]
Bug 1672462 - Compute flex item's content size suggestion in inline axis via nsIFrame::ComputeSize.

Beta/Release Uplift Approval Request

• User impact if declined: SVG images, which has no definite size specified on the main axis and have no intrinsic main size, can have a wrong minimum size on the main axis, resulting a overly shrinking final size.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Not risky since this is a small patch, and only affects flex items which have no definite size specified on the main axis and have no intrinsic main size.
• String changes made/needed:

Comment 15

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9183911 [details]
Bug 1672462 - Compute flex item's content size suggestion in inline axis via nsIFrame::ComputeSize.

Approved for uplift in 83 beta 8 as this is evaluated by the developer as low risk, is an 83 regression, has a webcompat impact (comment #4) and the fix is covered by new tests. Thanks.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9f4f0d4e58ac
https://hg.mozilla.org/releases/mozilla-beta/rev/9c3dc1f7ee90