1730351 - Scaling an SVG inside a nested flexbox keeps original size in the flow

Status: RESOLVED FIXED

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

Description

[Leon]

Attachment: SVGs were shrinked visually, but the used space wasn't.

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:92.0) Gecko/20100101 Firefox/92.0

Steps to reproduce:

The page I'm currently developing will contain a list of players, each shown with their profile picture. The picture will either be what the user provided or a placeholder SVG. The whole list will be a flexbox which is itself part of another flexbox.

See this fiddle: https://jsfiddle.net/Humorhose/7jfqwzd2/28/

The avatars are scaled down when the page's width is decreased. This works fine with the regular images, but not with the SVGs: Firefox does scale them, but the width they use up doesn't. You can reproduce this by opening the fiddle above and changing the output's width. The SVGs will have lots of blank space to the right and below them when you decrease the width, or overlap if you increase it. When the page is refreshed, everything will look as expected until you change the width again. The issue goes away when the outer flex is removed, but I need that on my site.
It's also a little weird on Firefox for Android: Open https://jsfiddle.net/Humorhose/7jfqwzd2/28/show and keep switching between portrait and landscape mode. At least on my device, it works most of the time, but sometimes the bug does show up.

Everything works as expected in Chrome, so I think this might be a bug in Firefox.

Desktop: Firefox 91.0.2 (64 bit) on Arch Linux
Mobile: Firefox for Android 91.4.0 on a OnePlus 7T Pro

Here is the link to my post on the support forums: https://support.mozilla.org/en-US/questions/1188220

Actual results:

When an SVG inside a nested flex is scaled up or down at runtime, it is scaled visually, but the space it consumes in the flow remains the same.

Expected results:

I expect the space consumed to be scaled as well.

Comment 1

[Leon]

Attachment: SVGs were enlarged visually, but the used space wasn't.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::Layout: Flexbox' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Test-case from reporter.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Yeah, this seems like an incremental layout bug. Mozregression says:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=29bdbbe89264b91d1bd4205c45536c9222a8d6ee&tochange=257f2c96cef502a1d674df56c8e39d76d8ed4d89

So I suspect this is a regression from bug 1383650. The person that sent the patch hasn't been active lately so I'll try to poke at it when time allows.

Thanks for the report!

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Changing severity to S3 because of it's a very long-standing regression.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: More reduced test-case.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1730351 - Don't use NS_IsHintSubset unnecessarily in some places. r=#layout-reviewers

This should cause no behavior change, but is a bit cleaner when one of the
arguments to IsHintSubset is just one bit.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1730351 - Fix reflow root handling in presence of inherited changes in FrameNeedsReflow. r=#layout-reviewers

The issue here is that we don't clear the cached intrinsic size of the
flex item, but this reproduces without flex at all.

The main issue is that we choose whether to clear the intrinsic sizes
from a reflow root (the outer svg in this case) based on whether the
reflow changes size or position:

https://searchfox.org/mozilla-central/rev/3fa5cc437a4937c621ea068ba5dc246f75831633/layout/base/RestyleManager.cpp#1224-1229

This is ~fine, except the nsChangeHint_ReflowChangesSizeOfPosition hint
can be "inherited" (and thus be cleared if already subsumed by a
parent):

https://searchfox.org/mozilla-central/rev/3fa5cc437a4937c621ea068ba5dc246f75831633/layout/base/nsChangeHint.h#465-469

This is ~fine, as we'll already start the reflow further up the tree (so
we don't need to start go past the reflow root), but we still need to
clear the ancestor intrinsics. We still get to StyleChangeReflow with
the ClearAncestorIntrinsics hint. We could pass that information down,
but the information is really already in via the IntrinsicChange.

I think it's just not correct to stop clearing intrinsic sizes if the
target is a reflow root and we get a TreeChange/StyleChange, regardless
of whether it changes size/position. It should also be a few less
instructions, though not that it matters.

Depends on D126812

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e1e27cddabe
Don't use NS_IsHintSubset unnecessarily in some places. r=layout-reviewers,jfkthame
https://hg.mozilla.org/integration/autoland/rev/edb7b0ca847b
Fix reflow root handling in presence of inherited changes in FrameNeedsReflow. r=layout-reviewers,jfkthame

Comment 10

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/3e1e27cddabe
https://hg.mozilla.org/mozilla-central/rev/edb7b0ca847b

Comment 11

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/31055 for changes under testing/web-platform/tests

Comment 12

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot