Closed Bug 1030952 Opened 10 years ago Closed 8 years ago

Flex items with intrinsic aspect ratio (e.g. images) which are shrunk due to max-width or flexing should have their height shrunk as well

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla49
Tracking Status
firefox48 + verified
firefox49 + verified

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(11 files, 2 obsolete files)

5.19 KB, text/html
Details
5.05 KB, text/html
Details
6.27 KB, patch
Details | Diff | Splinter Review
5.40 KB, text/html
Details
17.31 KB, image/png
Details
9.54 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
4.38 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
10.33 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
8.89 KB, patch
Details | Diff | Splinter Review
5.36 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
51.56 KB, patch
Details | Diff | Splinter Review
Attached file testcase 1
STR:
 1. Load attached testcase

EXPECTED RESULTS:
Testcase should be show a proportionally-scaled-down version of the tabzilla "mozilla" logo.

ACTUAL RESULTS:
The logo isn't shrunk vertically, so it looks squashed.


NOTES:
The testcase has a flex container with a flex item, with stretching/flexing disabled in both axes (with "align-items:flex-start" and "flex:none"). There is also a max-width constraining the width of the img element.  (Ideally, it should *also* be constraining its height, since the img has an aspect ratio.)
Attached file reference case 1
Here's a reference case, with a block container instead of a flex container. I'm pretty sure the testcase should match this rendering.

(In Chrome 37, they do match.)
Attachment #8446746 - Attachment description: test.html → testcase 1
Flags: needinfo?(dholbert)
OK, so what's going on is:
 * For flex items, we defer "min-width" / "max-width" clamping until after we've done flexing.
 * In other words, nsHTMLReflowState doesn't take min-width/max-width into account, when initializing its values for ComputedWidth() & ComputedHeight() for a flex item. It trusts that the flex container will do that (and adjust the reflow state).
 * In the flex container, we do adjust the reflow state to set the resolved width (main-size)...
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFlexContainerFrame.cpp?rev=0051c5234fe5#3208
...but we don't consider that this may have an effect on the computed height (cross-size), which we're about to measure from the reflow state and/or from an actual measuring reflow. If there's an aspect ratio involved, we likely need to override both the width *and* the height on the reflow state there.

(I'm assuming a horizontal flexbox above; for vertical flexbox, "width" & "height" are the other way around, of course.)
Flags: needinfo?(dholbert)
I'm pretty sure we have another bug open on images not maintaining their intrinsic aspect ratio when they flexed in the main axis.  This bug has the same root issue as that bug.
Attached patch fix v1Splinter Review
Attached patch just lets us take min|max-width|height into consideration earlier for inflexible flex items, as a special-case. (Not 100% sure we want to take this, since it adds a little bit of complexity, but it is a common enough special case that it might be worth it.)

This fixes the bug for testcases with "flex:none", at least (but we'll still need to fix the code linked in comment 2, to fix this even when items are flexible).

Seth, you should be able to work on top of this for now, to proceed with bug 1029285, in any case.
(In reply to Daniel Holbert [:dholbert] from comment #5)
> Seth, you should be able to work on top of this for now, to proceed with bug
> 1029285, in any case.

Thanks, I'll give that a shot.
The relevant spec text is below:
(Note: single-digit numbers "6" and "7" here are sequential steps of the flex layout algorithm. The decimal number "9.4" is just a spec section number -- the algorithm is broken up into several sections.)

  #     6. Resolve the flexible lengths of all the flex items to
  #        find their used main size (see section 9.7.).
  #
  # 9.4. Cross Size Determination
  #     7. Determine the hypothetical cross size of each item by
  #        performing layout with the used main size and the
  #        available space, treating auto as fit-content. 
  #     [...]
  #    11. Determine the used cross size of each flex item.
  #        If [...special cases that don't apply here...].
  #        Otherwise, the used cross size is the item’s
  #        hypothetical cross size.
http://dev.w3.org/csswg/css-flexbox-1/#algo-flex

So basically, in the testcase here & the one on dupe bug 1147532, the spec is clear that we should be using the *used width* (post-flexing/clamping) when doing layout to compute the image's "hypothetical height".

(I think the analysis of the code brokenness in comment 3 is still pretty much correct, too.)
Here's a second testcase, with the image shrinking due to "flex-shrink" & competition for space with an inflexible spacer element, instead of being clamped by "max-width".

Per the spec text in previous comment, the spec requires the image in this testcase to also scale nicely (since the "used main size" is small here, and should produce a correspondingly-small "used cross size")

My attached "fix v1" does not address this testcase. Also, while Chrome renders "testcase 1" correctly, it fails on this "testcase 2" in the same way that we do.
Summary: Flex items with intrinsic aspect ratio & max-width should have their height clamped as well → Flex items with intrinsic aspect ratio (e.g. images) & max-width should have their height clamped as well
Flags: needinfo?(dholbert)
Attached image Opera-12.16.png
Opera 12.16 can maintain the image intrinsic aspect ratio.
Chrome can used this to fixed it.

.imageItem {
  min-width: 0;
  max-width: 100%;
}
Yeah, adding "max-width:100%" converts testcase 2 into something more like testcase 1, which the attached patch would fix and which works in Chrome, as discussed in comment 9.
Summary: Flex items with intrinsic aspect ratio (e.g. images) & max-width should have their height clamped as well → Flex items with intrinsic aspect ratio (e.g. images) which are shrunk due to max-width or flexing should have their height shrunk as well
Got a report of this bug on Twitter: http://codepen.io/raphaelgoetter/pen/dMvdre
Blocks: 1264210
OK, so the high-level idea for this bug's fix is:
 (a) We need to bypass some optimizations that assume (in some cases) that the our initial measurement of the cross size is still valid.  These optimizations aren't valid for elements with an aspect ratio.

 (b) When we measure the cross size (after we've resolved the main size by flexing), we need to make sure the relevant layout code (several levels removed from nsFlexContainerFrame) is actually using the *resolved main size* in its aspect-ratio-calculation math, instead of e.g. the computed "width" (or "height") from StylePosition().

Patches coming up, which fall under these two umbrellas.
Flags: needinfo?(dholbert)
This patch is idempotent -- it just adds the aspect ratio to the list of things that we store on the (temporary) FlexItem structure, so we can check for it a bit later on without having to look it up again.

Note: In the "strut" version of the FlexItem construtor (for visibility:collapse elements), we don't need to store the aspect ratio -- so I just initialize it using the default nsSize constructor (which sets it to 0,0).  (These collapsed flex items simply exist as a zero-main-size "strut".  They simply impose a cross size on their flex line, and have no other effect.  So, there's no need to look up & save a meaningful aspect ratio for them.)
Attachment #8746660 - Flags: review?(mats)
(This patch basically achieves "(a)" from comment 16.)

Key things here:
 - This adjusts CanMainSizeInfluenceCrossSize() to reflect the reality that, for something with an aspect ratio, a change to one dimension *does* influence the other dimension. (This new check happens *after* we've checked if the item is stretched, because stretched items do not respect their aspect ratio. It's also after the mIsStrut check, because we don't save (or need) useful aspect ratios for struts, as noted in comment 17.)

 - As a result of this CanMainSizeInfluenceCrossSize change, we'll now be able to hit SizeItemInCrossAxis() in a way that we couldn't previously -- for flex items [with aspect ratios] in a vertically-oriented flexbox.  So, this patch relaxes a MOZ_ASSERT_UNREACHABLE check there, and tweaks the comment alongside it, and removes another related XXXdholbert comment about code being unreachable.  (Note that what we do in the formerly-MOZ_ASSERT_UNREACHABLE early-return -- reading the cross-size off of the nsHTMLReflowState -- is still the correct thing to do, for elements with an aspect ratio. At least, it will be correct thing to do, after the next patch here.)  This (SizeItemInCrossAxis) all still needs to be made more vertical-writing-mode friendly (probably in bug 1267462), but for now this is just a minor tweak.
Attachment #8746696 - Flags: review?(mats)
Attachment #8746660 - Flags: review?(mats) → review+
Comment on attachment 8746696 [details] [diff] [review]
part 2: Change flex layout to recognize that an aspect ratio allows main-size to influence cross-size

r=mats
Attachment #8746696 - Flags: review?(mats) → review+
This part adds a new frame property, which lets us communicate the resolved main size from the flex container down to the aspect-ratio calculations, so that we can be using the correct value in those calculations.

These aspect ratio calculations happen via this backtrace:
 - nsFlexContainerFrame wants to measure the frame's cross size.
 - It creates a nsHTMLReflowState (called "childReflowState" in contextual code in this patch).
 - That nsHTMLReflowState constructor calls InitConstraints, which calls ComputeSize, which (if the frame has an intrinsic ratio) calls nsLayoutUtils::ComputeSizeWithIntrinsicDimensions.
 - ComputeSizeWithIntrinsicDimensions does a bunch of math & clamping with the computed "width" & "height" properties and their min/max versions (from StylePosition()), which produces ComputedWidth() and ComputedHeight() sizes on the reflow state.
 - This intrinsic-dimension math is only useful if it's working with the actual resolved main size.  So, we need to communicate that down to the ComputeSizeWithIntrinsicDimensions call.
 - The easiest way to cleanly communicate that information from the outside of this callstack to the inside is using a frame property that we can check for in nsLayoutUtils. This shouldn't cause any delay for the common case (non flex items), because it happens inside of an "if (isFlexItem)" check.

Note also that the end of the nsLayoutUtils.cpp chunk here is just reindentation. I'll post a "diff -w" version to make that clearer.
Attachment #8746722 - Flags: review?(mats)
Sorry, the previous version of "part 3" had a silly "else -> else-if" change in nsLayoutUtils that was unnecessary and useless (left over from an earlier version of the patch that had a few extra checks there).

Dropped that change in this version.
Attachment #8746722 - Attachment is obsolete: true
Attachment #8746723 - Attachment is obsolete: true
Attachment #8746722 - Flags: review?(mats)
Attachment #8746724 - Flags: review?(mats)
Attachment #8746724 - Attachment description: part 3 v2. → part 3 v2: Add a frame property to allow flex container to impose a different main-size on a flex item for aspect ratio calculations
This is the final code change here, to make us set the correct cross size in the reflow state for the flex item's final reflow (after we've determined its correct main-size, cross size, and position).

For this part, we *could* use the same RAII property-table-tweak strategy that I introduced in part 3 -- but since we already know the main-size & cross-size (unlike part 3), it's cheaper to just directly stomp on those values in the reflow state.  So, that's what I do here. (See code-comment in the patch for more detail.)

(This patch splits up some existing code for handling stretched items, so that we can share it.  The old code stomps on the cross size for the stretched item, and it also sets a flag to tag the item as having a relative height [effectively saying "stretch is kind of like height:100%"].  We want to share the first piece of code -- stomping on the cross size -- but not the second piece.)
Attachment #8746731 - Flags: review?(mats)
Note that our behavior doesn't really change until all 4 parts have been applied -- hence, I'm tweaking test manifests in part 4 to reflect updated behavior.  Notes on those:
 * Of the two tests that are marked as failing due to duplicate bug 1041019, one becomes fixed, and one remains broken due to bug 1055354. (a min-width:auto + intrinsic-ratio spec change that we need to fix).
 * There's a crashtest involving a gigantic <video> element that now ends up having a different size (correctly) due to the changes here, which means it trips over 1 fewer assertion on my machine (2 instead of 3).  So I've reduced the lower bound on its assertion count from 3 to 2, to keep things green.  (We may be able to reduce its upper bound, too, but I'm hesitant to do that, since I don't know how much to lower it by, and Android tends to reflow tests multiple times & hence trip each assertion multiple times.)
Comment on attachment 8746724 [details] [diff] [review]
part 3 v2: Add a frame property to allow flex container to impose a different main-size on a flex item for aspect ratio calculations

> class MOZ_RAII AutoFlexItemMainSizeOverride final {

nit: { on a separate line please
Attachment #8746724 - Flags: review?(mats) → review+
Fixed that locally -- thanks!
Comment on attachment 8746731 [details] [diff] [review]
part 4: For flex items with an aspect ratio, stomp on reflow state's main size *and cross size* in final reflow

>+  if (aItem.IsStretched()) {
>+    // If this item's height is stretched, it's a relative height.
>+    aItem.Frame()->AddStateBits(NS_FRAME_CONTAINS_RELATIVE_BSIZE);

Hmm, is it intentional that you now set this bit also when
aAxisTracker.IsCrossAxisHorizontal() is true?
Seems odd that the comment is the same.
Attachment #8746731 - Flags: review?(mats) → review+
Oops, you're right -- I didn't mean to change that piece of logic.

I've now added a "!aAxisTracker.IsCrossAxisHorizontal()" to that if-check, to restore the old behavior there.

Thanks for the reviews!
Attached patch part 5: reftestsSplinter Review
Included are some comprehensive reftests for flex items with aspect ratios (using an <img>), with a variety of constraints in each dimension.

Tests 001 and 002 use flex items that are inflexible & unstretched.
Tests 003 and 004 use flex items that are stretched (in the cross axis).
Tests 005 and 006 use flex items that are flexible (in the main axis).

Odd tests have "flex-direction:row" (horizontal); even tests have "flex-direction:column" (vertical). Aside from that, each pair of adjacent tests are identical.
(I added the "explicit" keyword to my RAII class's constructor (in part 3), to address the static-analysis error in the Try run.)
(I backed out & re-landed the final patch to fix the commit message (mistakenly labeled it "part 4" when it's really "part 5").)
Depends on: 1180107
Comment on attachment 8746660 [details] [diff] [review]
part 1: Save each flex item's intrinsic ratio during reflow, for easy reuse

Approval Request Comment (Requesting approval to uplift all 5 patches here)

[Feature/regressing bug #]:
 * CSS Flexbox (web layout feature), as well as web compatibility. (This makes our flex layout implementation match other browsers' implementations more closely, which is a web compat win in and of itself; and it also gets closer to being able to let CSS webkit-prefix support ride the trains without causing any known regressions -- that's bug 1259345.)

[User impact if declined]:
 * Potentially broken layout (though, not a regression) on some sites that use CSS flexbox.
 * AND, either:
  - If we let webkit prefix support ride the trains in Firefox 48 (which I'd like to do in bug 1259345 as noted above) *without* backporting this bug's patches, it'll potentially break layout on some sites by stretching/skewing some images (which will manifest as a regression), on some sites that partly-rely on webkit prefixes.  (We've only seen one instance of this so far -- bug 1264210).
  - OR: if we punt on letting webkit prefix support ride the trains, then Firefox for Android users will have to wait another 6 weeks before the mobile web goes from largely-broken to largely-working (see http://www.otsukare.info/2016/01/04/webkit-resolved-fixed )

[Describe test coverage new/current, TreeHerder]: Comprehensive tests included, and our flexbox code is already pretty well tested with automated tests.  Additionally, this will be getting Nightly testing starting tomorrow morning (Saturday 4/30) and I anticipate that it will have had a few days of sanity-check testing before it's uplifted.

[Risks and why]: This bug is changing our flexbox layout algorithm a bit, so it's always possible that such a change could cause unforeseen layout breakage or performance issues. HOWEVER: the fix is targeted enough that I think this risk is low (there are 5 patches but they're all pretty small). And we're early enough in the Aurora release cycle that I'm confident we could either address any hypothetical breakage or backout as-needed, if this bug were to cause unforeseen issues.  (It's also possible that any new hypothetical breakage would be less severe than the webcompat issues that are being fixed here.) So, I think this risk is outweighed by the benefits.

[String/UUID change made/needed]: None
Attachment #8746660 - Flags: approval-mozilla-aurora?
Comment on attachment 8746660 [details] [diff] [review]
part 1: Save each flex item's intrinsic ratio during reflow, for easy reuse

I'm persuaded, it's early aurora, refactor away. 
Do we have any sort of test plan to determine whether it's ready to ship or not?
KWierso, please uplift the stuff from 1180107 first or this will break things
Attachment #8746660 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tracking for 48+ and adding a request for QE to verify this (in aurora 48 please)
Flags: qe-verify+
> Do we have any sort of test plan to determine whether it's ready to ship or not?

I assume this is referring to the webkit prefix support (the broader thing that we're still making a call about ready-to-ship-or-not).  That would be covered by bug 1259345 -- test plan discussion should happen over there.

(Briefly: that feature's been default-enabled on Nightly & Aurora [blocked from beta] since version 46, so it's been getting testing on those channels for a few months now. The rough plan is to let it ship to beta once we've fixed all known/reported regressions, and see if any other issues get reported/discovered, and either fix them or punt for a release. If we need a more serious test plan than that, we can figure one out in bug 1259345.)
I verified this issue on latest Nightly 49.0a1 and latest Aurora 48.0a2 using: 
- Windows 10 x64
- Mac OS X 10.11
- Ubuntu 14.04 x86.
Status: RESOLVED → VERIFIED
Depends on: 1362789
Depends on: 1522898
Blocks: 1661847
You need to log in before you can comment on or make changes to this bug.