In bug 984711, I'm re-introducing "min-width:auto" / "min-height:auto" support, as it existed when that feature was removed from the flexbox spec. (Basically, it computes to "min-content" on flex items, and it computes to 0 on everything else. Pretty simple.) I'm filing *this* bug here to cover updating our implementation to match what the spec *currently* says about this feature, now that it's been added back to the spec with a bit more nuance. Nowadays, min-width:auto / min-height:auto is specced as-follows: # auto # On a flex item whose overflow is visible, this keyword # specifies as the minimum size the smaller of: # # * the computed width (height), if that value is definite, # * the computed max-width (max-height), if that value is definite, # * if the item has no intrinsic aspect ratio, its min-content size, # * if the item has an intrinsic aspect ratio, the width (height) # calculated from the aspect ratio and any definite size constraints # in the opposite dimension. # # Otherwise, this keyword computes to 0 http://dev.w3.org/csswg/css-flexbox/#min-size-auto

(In reply to Daniel Holbert [:dholbert] from comment #0) > # * the computed width (height), if that value is definite, In the ED, this line has been changed to: * the used flex-basis, if the computed flex-basis was 'auto' so that we only have to consider 'width'/'height' if they're being used as the source for flex-basis. (The line was subsequently further changed with s/auto/main-size/, per bug 1032922, but that's not relevant to this bug.)

Note: we're also supposed to consider the aspect ratio when resolving 'flex-basis:auto' (aka 'main-size'), too. Quoting the current flexbox ED: # If the flex item has ... # an intrinsic aspect ratio, # a flex basis of main-size, and # a definite cross size # then the flex base size is calculated from its inner # cross size and the flex item’s intrinsic aspect ratio. http://dev.w3.org/csswg/css-flexbox/#algo-main-item ...though we don't currently implement that. I'm going to implement that as part of this bug, because... (1) we use the same code to implement auto-height computation for flex-basis:auto and min-height:auto (as of bug 984711's patches) (2) we're now needing to make that code consider the intrinsic ratio for one of those quantities (min-[height|width], per comment 0), so we might as well start considering it for the other quantity as well (flex-basis), since we're really supposed to.

Here's the patch as it currently stands. Not requesting review quite yet, since I need to write a few more tests and possibly shuffle things around a bit. This layers on top of the patches in bug 1037177. (which layer on top of the patches in bug 984711)

Patch stack coming up. I'm splitting this up into sub-patches solely for review purposes -- I intend to fold the patches here together when landing, since the intermediate states are not useful for testing.

Currently, with the patches for bug 984711 (part 6 in particular), we resolve "min-width:auto" in the reflow state, since in the previous incarnation of this feature, it was simply an alias for "min-content". But now that min-width:auto is more complicated to resolve, I'm removing this special-case in the reflow state, and making us resolve "min-width:auto" at the same place where we resolve "min-height:auto", in nsFlexContainer.cpp. So, this patch just removes this special-case from nsHTMLReflowState & copies the existing comment for min-height:auto there. (with some adjustments to make the comment a bit shorter)

This patch adds a flag & some functionality to FlexItem for detecting when we have a min-size:auto value that needs resolving. (The next & final patch will actually react to this & resolve the value.)

Just caught a bug in sub-patch 3 -- I was converting to float() too late (after the integer-division) in this line: >+ conversionFactor = float(aIntrinsicRatio.width / aIntrinsicRatio.height); which produces conversionFactor = 0.0f for any cases where the numerator is smaller than the denominator. (Hadn't initially caught that, because my main ratio testcase uses a square image, with 1:1 ratio. I caught it when adjusting another testcase with a (rectangular-intrinsic-ratio) <canvas>.) Rather than adjusting the float math, I'm switching to use MULDIV like most of nsLayoutUtils.cpp does for working with intrinsic ratios. (I'd initially been referencing a different section of nsLayoutUtils.cpp that uses float math when working with ratios; I filed bug 1039796 on making that use MULDIV as well.)

Comment on attachment 8457350 [details] [diff] [review] sub-patch 2: Make FlexItem detect when it has a min-[width|height]:auto that needs resolving r=mats, but I have a few nits on the use of "min-size". > layout/generic/nsFlexContainerFrame.cpp >+ // Indicates whether we need to resolve an 'auto' value for the main-axis >+ // min-size property. >+ bool NeedsMinSizeAutoResolution() const Using the phrase "min-size property" might be confusing for the casual reader since there is no CSS property named 'min-size'. I think it's fine to use "MinSize" in the name though, since it's not referring to a property per se. So in the above comment I think we should write it out as "min-width/height property" or "min-[width|height] property", whichever you prefer. >+ // Helper called by constructor, to set mNeedsMinSizeAutoResolution: >+ void CheckForMinSizeAuto(const nsHTMLReflowState& aFlexItemReflowState, s/by constructor/by the constructor/ maybe? >+ // Does this item need the flex-item-specific min-size:auto behavior (for the >+ // main-axis min-size property)? i.e. does it have 'overflow:visible' and >+ // 'min-[width|height]:auto'? >+ bool mNeedsMinSizeAutoResolution; Ditto for "min-size" + I think the comment should be briefer, how about: // Does this item need to resolve a min-width/height:auto (for the main-axis). The code for NeedsMinSizeAutoResolution() can document details like 'overflow:visible' if needed. >+ // mNeedsMinSizeAutoResolution initialized in CheckForMinSizeAuto() s/initialized/is initialized/ >+// Check if we need to resolve "min-[main-size]:auto", and set a flag if so. >+void >+FlexItem::CheckForMinSizeAuto(const nsHTMLReflowState& aFlexItemReflowState, I think you can just drop the comment above. The method is documented in the header file, and the code is documented inline. >+ // We only need to consider this for the *main-axis* min-size property: >+ const nsStyleCoord& minSize = GET_MAIN_COMPONENT(aAxisTracker, >+ pos->mMinWidth, >+ pos->mMinHeight); I think you can just remove the "min-size property" part at the end. It's obvious from the code what happens. >+ // We'll need special behavior for "min-[main-size]:auto" iff: I think you should write out "min-width/height for the main-axis" here >+ // (a) we actually have "auto" in our min-main-size property, and ... and make that shorter, e.g. // (a) its property value is "auto", and >+ // (b) we have "overflow:visible" (i.e. both "overflow-x" and "overflow-y" >+ // have a computed value of "visible") I think you can skip the first part, i.e. just write: // (b) both "overflow-x" and "overflow-y" have a computed value of "visible" (It's not an important detail that "overflow:visible" implies that.) >+ // XXXdholbert Maybe we should only check the "overflow" sub-property in the >+ // direction of our main axis? (Right now, the flexbox spec just says >+ // 'overflow', which implies both sub-properties.) >+ mNeedsMinSizeAutoResolution = >+ (minSize.GetUnit() == eStyleUnit_Auto && >+ disp->mOverflowX == NS_STYLE_OVERFLOW_VISIBLE && >+ disp->mOverflowY == NS_STYLE_OVERFLOW_VISIBLE); Fwiw, I find it rather odd that 'visible' is treated differently than 'auto' that doesn't trigger a scrollbar. I think other specs defines "overflow:auto" to have the same layout as "overflow:visible" as long as there is no overflowing content. IOW, you might want to check for scrollbars here instead. (This might be a Flexbox spec issue to consider.)

> a few nits on the use of "min-size". To be clear: I think other uses of the term "min-size" in code comments are fine as long as they refer to some nscoord value. It's just explicit references to the CSS properties min-width/height that I think should be written out as such.

Comment on attachment 8457631 [details] [diff] [review] sub-patch 3 v2: Actually resolve min-width:auto values (and consider ratio when resolving flex-basis:auto) >+ // Helper to set the resolved value of main-min-size:auto. >+ // (Should only be used if NeedsMinSizeAutoResolution() returns true.) > void UpdateMainMinSize(nscoord aNewMinSize) Change "main-min-size:auto" to use the phrase you decide on from part 2. >+ // Resolve "flex-basis:auto" and/or "min-[width/height]:auto" ... Ditto. >+// should be set IFF the caller indends to resolve the main min-size.) s/indends/intends/ >+static nscoord >+GetCrossSizeToUseWithRatio(FlexItem& aFlexItem, No need for "Get" in the name. Also, would "const FlexItem&" work? >+// XXX This macro shamelessly stolen from nsLayoutUtils.cpp. >+// (Maybe it should be exposed via a nsLayoutUtils method?) >+#define MULDIV(a,b,c) (nscoord(int64_t(a) * int64_t(b) / int64_t(c))) Perhaps move it to nsCoord.h and make a NS_COORD_IS_FLOAT version too? (in that case we should probably add "nscoord" somewhere in the name). >+static nscoord >+ResolveMainSizeFromAspectRatio(nscoord aCrossSize, >+ const nsSize& aIntrinsicRatio, >+ const FlexboxAxisTracker& aAxisTracker) Is "Resolve" needed here? MainSizeFromAspectRatio is shorter. >+static nscoord >+PartiallyResolveAutoMinSize(FlexItem& aFlexItem, >+ const nsHTMLReflowState& aItemReflowState, >+ const nsSize& aIntrinsicRatio, >+ const FlexboxAxisTracker& aAxisTracker) Would "const FlexItem&" work? (I do get the "Resolve" bit here though) >+ nscoord crossSizeToUseWithRatio = >+ GetCrossSizeToUseWithRatio(aFlexItem, aItemReflowState, >+ true, aAxisTracker); It's unclear what 'true' means here. Perhaps introduce a local 'bool useMinSizeIfCrossSizeIsIndefinite = true' and pass that? >+ nscoord crossSizeToUseWithRatio = >+ GetCrossSizeToUseWithRatio(aFlexItem, aItemReflowState, >+ false, aAxisTracker); Ditto for 'false'. >+ if (NS_STYLE_FLEX_WRAP_NOWRAP == >+ flexContainerRS->mStylePosition->mFlexWrap) { Fits on one line? >+ // we're resolving min-size:auto or flex-basis:auto. "min-width/height:auto". >+ MeasureFlexItemContentHeight(aPresContext, aFlexItem, >+ forceVerticalResizeForMeasuringReflow, >+ *aItemReflowState.parentReflowState); Last param could be "*flexContainerRS" instead?

(In reply to Mats Palmgren (:mats) from comment #9) > Using the phrase "min-size property" might be confusing for the casual > reader since there is no CSS property named 'min-size'. Ah, right. The spec uses the terms "main-size property" and "main-axis min-size property", to be generic -- that's why I have those terms stuck in my head. :) But you're right that it's misleading/confusing in isolation, out of the context of the actual spec (which is better about defining things). Anyway, I've now fixed this to use "min-[width|height] property" more consistently, in the places you mentioned. > Fwiw, I find it rather odd that 'visible' is treated differently than > 'auto' that doesn't trigger a scrollbar. I think other specs defines > "overflow:auto" to have the same layout as "overflow:visible" as long > as there is no overflowing content. Fortunately, we'll get the behavior that you're wanting for free -- namely, the layout will be the same for overflow:auto vs. visible as long as there is no overflowing content (i.e. as long as there are no scrollbars). Justification: (a) A flex item's min-[width|height] property *only* affects layout when the algorithm tries to make the flex item smaller than this property's value. (b) On a flex item, min-[width|height]:auto will resolve to, at most, the item's min-content size. (c) So, the special min-[width|height]:auto behavior only affects layout when the flex layout algorithm tries to make the item smaller than its min-content size. (d) In an "overflow:auto" flex item, *scrollbars would appear* if the item were to be made smaller than its min-content size. (e) So, if scrollbars haven't appeared, then that means that we didn't try to make the flex item smaller than its min-content size, which (per c) means the special min-width:auto behavior isn't affecting layout. Hopefully that makes sense. :)

OK, that makes sense. Then I agree with your comment there that we should only check the property that corresponds to the main-size. Otherwise, you might get different layout by just toggling the overflow sub-property in the cross direction, right? (assuming no overflowing content)

Ah, right. I think this scenario would trigger that problem: <div style="display:flex;"> <div>wiiiiiiiiiiiiiiiiiiiiiiiiiiiide text text text</div> <!-- some other flex item with a large flex-basis (maybe a paragraph of text) which will end up meaning that we'll be trying to shrink our flex items. --> </div> The first flex item *should* have a min-width that's the length of the word "wii..ide", from min-width:auto. (So it will refuse to shrink to less than that.) If we apply "overflow-y: auto", then it will instead have a min-width of 0, and it'll shrink such that "wii...iide" will now overflow off of its right side. No scrollbars will be shown, as there's no vertical overflow. I'll email www-style to clarify this, but yeah, I think we only should be checking the main-axis 'overflow' sub-property.

Here's sub-patch 2 again, w/ review comments (comment 9 & comment 13) addressed.

sub-patch 3, with notes from comment 11 addressed.

(In reply to Daniel Holbert [:dholbert] from comment #14) > I'll email www-style to clarify this, but yeah, I think we only should be > checking the main-axis 'overflow' sub-property. http://lists.w3.org/Archives/Public/www-style/2014Jul/0317.html

(In reply to Mats Palmgren (:mats) from comment #11) > >+// XXX This macro shamelessly stolen from nsLayoutUtils.cpp. > >+// (Maybe it should be exposed via a nsLayoutUtils method?) > >+#define MULDIV(a,b,c) (nscoord(int64_t(a) * int64_t(b) / int64_t(c))) > > Perhaps move it to nsCoord.h and make a NS_COORD_IS_FLOAT version too? > (in that case we should probably add "nscoord" somewhere in the name). Oh -- meant to reply to this. I'm spinning this off as bug 1040582.

Also, just to update The Plan here: * I've got a patch with new reftests that I'm going to finish off and post for review here shortly. -- (These tests include one scenario with intrinsic ratios that doesn't quite work correctly, which I'll be marking as 'fails' and filing a followup bug for.) * I've got another few patches which tweak existing tests to adjust to the new behavior. I won't bother with review on those. * I intending to land all of this (with bug 984711 and bug 1037177) *after* the merge/version-bump on Monday, since these bugs are shaking things up a bit, and I don't want to do that right before a merge.

(Following up on comment 17: Tab agrees & has updated the spec, RE overflow vs. overflow-x/overflow-y: http://lists.w3.org/Archives/Public/www-style/2014Jul/0319.html https://dvcs.w3.org/hg/csswg/rev/c4cc9750acd5 )

(In reply to Daniel Holbert [:dholbert] from comment #19) > Also, just to update The Plan here: > * I've got a patch with new reftests that I'm going to finish off and post > for review here shortly. Here's a roll-up patch w/ the reftests that I'm adding in this bug. (Locally, I've actually split these new tests into several patches, to take advantage of 'hg cp', and I'll be posting the individual patches after this one. I'm just requesting review on the roll-up patch, to avoid spamming review-requests, but if you prefer, feel free to instead review the individual parts. Not sure whether it's easier to skim the roll-up vs. the individual patches.) (I'm also requesting review from dbaron instead of mats, since mats is on vacation.) The tests are as follows (testing "min-width:auto" in a horizontal flex container): * flexbox-min-width-auto-001.html: test min-width:auto resolving to each of the possibilities listed at http://dev.w3.org/csswg/css-flexbox/#min-size-auto except for the aspect-ratio-dependent one. * flexbox-min-width-auto-002.html: test the aspect-ratio-dependent possibilities (using 'height', 'min-height', and 'height+max-height' as the constraint in the other dimension) * flexbox-min-width-auto-003.html: test that 'overflow-x:[non-visible]' neuters min-width:auto * flexbox-min-width-auto-004.html: test that 'overflow-y:[non-visible]' also neuters min-width:auto (indirectly, since overflow-y influences overflow-x) * ...and then there are equivalent "min-height-auto" versions of all of those, with a vertical flex container and widths/heights & overflow-x/overflow-y all reversed.

This patch adds flexbox-min-width-auto-001.html and flexbox-min-width-auto-002[abc].html, and their reference cases.

This patch uses "hg cp" to make vertical-flexbox copies of the tests in the previous patch.

This patch adds flexbox-min-width-auto-003.html & its reference (for "overflow-x" disabling "min-width:auto").

...and this last patch adds the other "min-[width|height]:auto"/"overflow-[x|y]" interaction tests, as hg cp's of flexbox-min-width-auto-003.html.

This patch removes two groups of tests that are invalid, as of this bug: - flexbox-minSize-*-1, which were general tests for the old min-content-only "min-width:auto" behavior. These tests' assumptions are no longer correct, and they're obsoleted by the tests added in parts 2, 3, and 4 here. - flexbox-basic-*-2*, which were tests for particular elements (canvas, textarea, etc.), specifically checking that the "width" property does *not* influence "min-width:auto" on all of these elements. (and similar for "height"/"min-height:auto"). This assumption is no longer valid, since now those properties do influence min-width:auto/min-height:auto, per comment 0. (Technically via "flex-basis" -- the spec language has been finessed a bit since comment 0.) Since testing that now-invalid assumption was the sole purpose of these tests, I'm dropping them.

This final patch adds "min-width:0" & "min-height:0" to the flex-flow-* reftests. Those reftests exercise all of the "flex-flow" values, in a grid, and in some sections of the grid (depending on the amount of shrinking & the exact fonts used), the textual labels (1, 2, 3, 4) can force their flex items to expand via establishing a nontrivial value for "min-width:auto" or "min-height:auto", and then that affects the test's rendering. We don't want that to happen; these numeric labels are *just* labels, so we don't really care if they overflow a bit. So, this patch adds "min-width:0"/"min-height:0" to let them overflow when necessary, instead of letting them influencing the flex item sizing.

As of "part 7", I believe all reftests pass, with this bug & its dependencies checked in. Full try run: https://tbpl.mozilla.org/?tree=Try&rev=c3071440fb1b

Landed all patches (parts 1 through 7): https://hg.mozilla.org/integration/mozilla-inbound/rev/f5b98d327bde https://hg.mozilla.org/integration/mozilla-inbound/rev/dad1229b5cfe https://hg.mozilla.org/integration/mozilla-inbound/rev/d82a0354fdcd https://hg.mozilla.org/integration/mozilla-inbound/rev/51999ceb00ae https://hg.mozilla.org/integration/mozilla-inbound/rev/48cf2fca495b https://hg.mozilla.org/integration/mozilla-inbound/rev/2a194f050406 https://hg.mozilla.org/integration/mozilla-inbound/rev/f7c111168a77

https://hg.mozilla.org/mozilla-central/rev/f5b98d327bde https://hg.mozilla.org/mozilla-central/rev/dad1229b5cfe https://hg.mozilla.org/mozilla-central/rev/d82a0354fdcd https://hg.mozilla.org/mozilla-central/rev/51999ceb00ae https://hg.mozilla.org/mozilla-central/rev/48cf2fca495b https://hg.mozilla.org/mozilla-central/rev/2a194f050406 https://hg.mozilla.org/mozilla-central/rev/f7c111168a77

This is causing one of our gaia integration tests (currently hidden on TBPL unfortunately) to fail. I'm currently investigating to see if this is causing any real regressions or not.

Daniel, I'm a bit lost with the removal of main-size: did this part sticked? (I think so)

"main-size" did get removed. "min-width:auto" / "min-height:auto" stayed & there are no plans to remove it.