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)
Core
Layout
Tracking
()
VERIFIED
FIXED
mozilla49
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+
lizzard
:
approval-mozilla-aurora+
|
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 |
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.)
Assignee | ||
Comment 1•10 years ago
|
||
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.)
Assignee | ||
Updated•10 years ago
|
Attachment #8446746 -
Attachment description: test.html → testcase 1
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(dholbert)
Assignee | ||
Comment 2•10 years ago
|
||
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)
Assignee | ||
Comment 3•10 years ago
|
||
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.
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
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.
Comment 6•10 years ago
|
||
(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.
Assignee | ||
Comment 8•9 years ago
|
||
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.)
Assignee | ||
Comment 9•9 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
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
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(dholbert)
Comment 11•8 years ago
|
||
Opera 12.16 can maintain the image intrinsic aspect ratio.
Comment 12•8 years ago
|
||
Chrome can used this to fixed it. .imageItem { min-width: 0; max-width: 100%; }
Assignee | ||
Comment 13•8 years ago
|
||
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
Assignee | ||
Comment 14•8 years ago
|
||
Got a report of this bug on Twitter: http://codepen.io/raphaelgoetter/pen/dMvdre
Assignee | ||
Comment 16•8 years ago
|
||
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)
Assignee | ||
Comment 17•8 years ago
|
||
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)
Assignee | ||
Comment 18•8 years ago
|
||
(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)
Updated•8 years ago
|
Attachment #8746660 -
Flags: review?(mats) → review+
Comment 19•8 years ago
|
||
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+
Assignee | ||
Comment 20•8 years ago
|
||
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)
Assignee | ||
Comment 21•8 years ago
|
||
Assignee | ||
Comment 22•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
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
Assignee | ||
Comment 23•8 years ago
|
||
Assignee | ||
Comment 24•8 years ago
|
||
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)
Assignee | ||
Comment 25•8 years ago
|
||
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 26•8 years ago
|
||
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+
Assignee | ||
Comment 27•8 years ago
|
||
Fixed that locally -- thanks!
Comment 28•8 years ago
|
||
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+
Assignee | ||
Comment 29•8 years ago
|
||
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!
Assignee | ||
Comment 30•8 years ago
|
||
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.
Assignee | ||
Comment 31•8 years ago
|
||
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=40ffed8ae200
Assignee | ||
Comment 32•8 years ago
|
||
(I added the "explicit" keyword to my RAII class's constructor (in part 3), to address the static-analysis error in the Try run.)
Comment 33•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9799a1b6d30 https://hg.mozilla.org/integration/mozilla-inbound/rev/69dd11091fd1 https://hg.mozilla.org/integration/mozilla-inbound/rev/f73c4127855c https://hg.mozilla.org/integration/mozilla-inbound/rev/3e962c342017 https://hg.mozilla.org/integration/mozilla-inbound/rev/3f3dc83fdd26
Comment 34•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce3098d73e26 (backout) https://hg.mozilla.org/integration/mozilla-inbound/rev/55690a010030
Assignee | ||
Comment 35•8 years ago
|
||
(I backed out & re-landed the final patch to fix the commit message (mistakenly labeled it "part 4" when it's really "part 5").)
Comment 36•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e9799a1b6d30 https://hg.mozilla.org/mozilla-central/rev/69dd11091fd1 https://hg.mozilla.org/mozilla-central/rev/f73c4127855c https://hg.mozilla.org/mozilla-central/rev/3e962c342017 https://hg.mozilla.org/mozilla-central/rev/55690a010030
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee | ||
Comment 37•8 years ago
|
||
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 38•8 years ago
|
||
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+
Comment 39•8 years ago
|
||
Tracking for 48+ and adding a request for QE to verify this (in aurora 48 please)
status-firefox48:
--- → affected
tracking-firefox48:
--- → +
tracking-firefox49:
--- → +
Flags: qe-verify+
Assignee | ||
Comment 40•8 years ago
|
||
Backported comment 36's commits to Aurora (i.e. everything that landed, with the mistake/backout dance noted in comment 35 excised from history): https://hg.mozilla.org/releases/mozilla-aurora/rev/39e8e0eebc51 https://hg.mozilla.org/releases/mozilla-aurora/rev/76e3ffd2a6d6 https://hg.mozilla.org/releases/mozilla-aurora/rev/cbf0c9d7ee9d https://hg.mozilla.org/releases/mozilla-aurora/rev/ecb12e83403b https://hg.mozilla.org/releases/mozilla-aurora/rev/4757576ac6a9
Assignee | ||
Comment 41•8 years ago
|
||
> 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.)
Comment 42•8 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•