1522898 - Chrome stretches flexbox to child img's height, Firefox scales the image down w/o aspect ratio

Status: VERIFIED FIXED

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

Description

[Thomas Wisniewski [:twisniewski]]

Attachment: test.html

In the attached test-case, Firefox is sizing the img tag down to its flexbox parent's intrinsic height, while Chrome is stretching the flexbox to match the img's height of 32px.

This is causing a similar cosmetic issue on Dropbox's web UI, as per https://webcompat.com/issues/24856

Comment 1

[Daniel Holbert [:dholbert]]

Thanks for the report & the reduced testcase! I'll take a look. This definitely looks weird (seems like we're subtracting the padding from the intrinsic width, when computing the final height via the aspect ratio,, or something...)

Comment 2

[Daniel Holbert [:dholbert]]

mozregression gave me...
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8c3fd523d75bd30f691ca2d6cfdad18d576392a1&tochange=2b7c421063ad7e30b6491d62ed8480ca333b628a
...which suggests this is a regression from bug 1030952, BTW (which makes sense).

(That was back in the Firefox 48 timeframe, so all supported Firefox versions are affected.)

Comment 3

[Daniel Holbert [:dholbert]]

This is an issue with the "imposed main size" codepaths, e.g. here:
https://searchfox.org/mozilla-central/rev/465dbfe030dfec7756b9b523029e90d48dd5ecce/layout/generic/nsFrame.cpp#5658

We use that codepath to tell flex items what their resolved main size is, so that e.g. an item whose width has been flexed can use the post-flexing value to compute its correct desired height.

The problem here is that we're using it to store the "imposed" content-box size, but then the code further down treats it as if it were the actual "width" value (and subtracts out borderpadding to account for 'box-sizing:border-box').

So, we need to be careful to account for that, somehow (either setting a larger value as the "imposed" main size, or opting out of the border-box calculations in this using-an-imposed-main-size scenario).

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: Bug 1522898: Account for box-sizing (add border & padding) when setting a main-size property value override on a flex item (which it uses for aspect ratio calculations).

For elements that have box-sizing:border-box specified, the aspect ratio
calculation code subtracts out border & padding from any specified property
values.

So, when we create a fake "override" specified property value for a flex item
whose main size has been resolved, we need to add in the border and padding to
account for the fact that they're going to be subtracted out later.

Comment 5

[Daniel Holbert [:dholbert]]

Patch posted, which fixes the issue locally. (I still need to add an automated test, but I verified that it makes us match other browsers on the attached testcase and on another local variant using <canvas>.)

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf9669675c17bbb22cf189a33d9b8045cbae4187

Comment 6

[Daniel Holbert [:dholbert]]

[gentle ping, in case the phabricator review-request mail got lost or this slipped past your radar. Patch is hopefully pretty straightforward; let me know if it's not clear, and/or if you don't have cycles and would prefer that I find another reviewer.]

Comment 7

[Mats Palmgren (inactive)]

Sorry for the late review. Yeah, I tend to miss some phabricator requests since they don't show up in the bugzilla dashboard.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1c3a7347d224
Account for box-sizing (add border & padding) when setting a main-size property value override on a flex item (which it uses for aspect ratio calculations). r=mats

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1c3a7347d224

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

WPT merge in https://github.com/web-platform-tests/wpt/pull/15272

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Is this something which requires backport consideration or can it ride the trains?

Comment 12

[Daniel Holbert [:dholbert]]

Sure. It's not a recent regression (we've been shipping with it for a while before noticing it), but it's a pretty small/safe change and a nice correctness/webcompat win.

/me fills out the uplift request forms.

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 9039279 [details]
Bug 1522898: Account for box-sizing (add border & padding) when setting a main-size property value override on a flex item (which it uses for aspect ratio calculations).

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 1030952

User impact if declined

Potentially broken layout in some cases (e.g. images squished in a way that doesn't preserve their aspect ratio) -- see screenshot on https://webcompat.com/issues/24856

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)

It's a very targeted fix that's a pretty clear step in the direction of correctness & compatibility.

String changes made/needed

None

Comment 14

[Daniel Holbert [:dholbert]]

To be clear, it'd also be fine if this were to just ride the trains.

The motivation for the uplift request is just that it'd be an "easy" small-risk-small-reward webcompat win.

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9039279 [details]
Bug 1522898: Account for box-sizing (add border & padding) when setting a main-size property value override on a flex item (which it uses for aspect ratio calculations).

Webcompat fix, adds tests. What's not to love!
OK for beta 8.

Comment 16

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2af591523036

Comment 17

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this issue using Fx 66.0a1 (2019-01-25), on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 67.0a1 (2019-02-15) and Fx 66.0b8, on Windows 10 x64, macOS 10.13 and Ubuntu 16.06 LTS.