1015474 - Update min-width:auto/min-height:auto support to match updated flexbox spec language

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

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

Comment 1

[Daniel Holbert [:dholbert]]

(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.)

Comment 2

[Daniel Holbert [:dholbert]]

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.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: wip

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)

Comment 4

[Daniel Holbert [:dholbert]]

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.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: part 1, sub-patch 1: Remove special case that greedily resolves "min-width:auto" in reflow state

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)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: sub-patch 2: Make FlexItem detect when it has a min-[width|height]:auto that needs resolving

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.)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: sub-patch 3: Actually resolve min-width:auto values

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: sub-patch 3 v2: Actually resolve min-width:auto values (and consider ratio when resolving flex-basis:auto)

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 9

[Mats Palmgren (inactive)]

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.)

Comment 10

[Mats Palmgren (inactive)]

> 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 11

[Mats Palmgren (inactive)]

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?

Comment 12

[Daniel Holbert [:dholbert]]

(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. :)

Comment 13

[Mats Palmgren (inactive)]

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)

Comment 14

[Daniel Holbert [:dholbert]]

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.

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: part 1, sub-patch 2 v2: Make FlexItem detect when it has a min-[width|height]:auto that needs resolving (r=mats)

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

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: part 1, sub-patch 3 v3: Actually resolve min-width:auto values (and consider ratio when resolving flex-basis:auto) (r=mats)

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

Comment 17

[Daniel Holbert [:dholbert]]

(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

Comment 18

[Daniel Holbert [:dholbert]]

(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.

Comment 19

[Daniel Holbert [:dholbert]]

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.

Comment 20

[Daniel Holbert [:dholbert]]

(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
)

Comment 21

[Daniel Holbert [:dholbert]]

Attachment: new reftests rollup patch

(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.

Comment 22

[Daniel Holbert [:dholbert]]

Attachment: part 2: add reftests for "min-width:auto" behavior

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

Comment 23

[Daniel Holbert [:dholbert]]

Attachment: part 3: add reftests for "min-height:auto" behavior

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

Comment 24

[Daniel Holbert [:dholbert]]

Attachment: part 4: add reftest for "min-width:auto" being disabled by "overflow-x"

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

Comment 25

[Daniel Holbert [:dholbert]]

Attachment: part 5: add reftests for other "min-[width|height]:auto"/"overflow-[x|y]" interactions

...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.

Comment 26

[Daniel Holbert [:dholbert]]

Attachment: part 6: remove tests specifically about old min-[width|height]:auto behavior

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.

Comment 27

[Daniel Holbert [:dholbert]]

Attachment: part 7: set min-width:0 / min-height:0 in flex-flow testcases

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.

Comment 28

[Daniel Holbert [:dholbert]]

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

Comment 29

[Daniel Holbert [:dholbert]]

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

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 31

[Kevin Grandon :kgrandon]

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.

Comment 32

[Jean-Yves Perrier [:teoli]]

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

Comment 33

[Daniel Holbert [:dholbert]]

"main-size" did get removed.

"min-width:auto" / "min-height:auto" stayed & there are no plans to remove it.