984711 - Add back "min-width:auto" / "min-height:auto" for flex items

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Per issue 19 mentioned in
 http://lists.w3.org/Archives/Public/www-style/2014Mar/0350.html
we'll likely need to add back the "auto" value as the default for min-width / min-height, with special behavior on flex items.

(Backstory: We previously had an older specced version of this behavior in the original patch-stack that landed in bug 666041. Then, the spec changed to remove this "auto" value & its behavior, which I addressed in bug 848539. Now, per email above, the spec editors are recommending adding an updated version of the behavior back.)

See analysis/proposal of the new behavior here:
 http://lists.w3.org/Archives/Public/www-style/2013Jul/0456.html
(which is also linked from the www-style post at the top of this comment)

Comment 1

[Daniel Holbert [:dholbert]]

The CSSWG has now resolved to add back the "auto" value, so this is now official.

http://lists.w3.org/Archives/Public/www-style/2014Mar/0463.html

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: script to generate patches to revert bug 848539's patches

Here's the script I'm using to generate a patch queue of backout patches for bug 848539, to re-introduce our old "min-[width|height]:auto" support (removed in that bug).

Comment 3

[Daniel Holbert [:dholbert]]

My plan here is:

 a) Post clean patches generated by the script in comment 2. (These patches will be clean backouts *of the backouts* that removed min-{width|height}:auto support, based off of m-c at the time that we removed this feature.)

 b) Post my/ rebased-to-current-m-c versions of those patches, for review. (Much of this rebasing is trivial, but some of it required manual rewriting, due to code-changes & refactoring.)

 c) SEPARATELY: Modify our implementation to match the spec's updated behavior for this feature in a different bug -- bug 1015474.

Hopefully this will make things easy to review -- the rebased patches can just be diffed against the clean backout patches, generally.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: mechanical part 1: backout afe27d80eeef (reverting test tweaks)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: mechanical part 2: backout e7a9e30409eb (adding back "auto" support in parser & as initial value, just computing to 0 for now)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: mechanical part 3: backout f8b5090d02e5 (add general support for nsStylePosition::mMinWidth having eStyleUnit_Auto)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: mechanical part 4: backout 5db313632268 (add general support for nsStylePosition::mMinHeight having eStyleUnit_Auto)

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: mechanical part 5: backout 7a289f49170f (adding back nsFlexContainerFrame code for handling min-height:auto)

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: mechanical part 6: backout 1eae876d6c3a (adding back special-handling for min-width:auto in reflow state & computed style)

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: mechanical part 7: backout edd98255e81d (adding back mochitest for min-[size]:auto)

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: unbitrotted part 1: reverting test tweaks

Here's the unbitrotted version of part 1. There are only minor changes w.r.t. the mechanical version, and the changes are all in lines of context, so I'm not bothering with review on this one.

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: unbitrotted part 2: adding back "auto" support in parser & as initial value, just computing to 0 for now

Here's the unbitrotted version of part 2. Similar to part 1, the only changes vs. the mechanical-backout are in contextual lines, plus some indentation changes that've happened in property_database.js, so I don't think review is necessary.

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8431740 [details] [diff] [review]
unbitrotted part 1: reverting test tweaks

(As a convention on this bug, I'm setting "feedback+" on the patches that I don't think need additional review.)

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8431746 [details] [diff] [review]
unbitrotted part 2: adding back "auto" support in parser & as initial value, just computing to 0 for now

(As a convention on this bug, I'm setting "feedback+" on the patches that I don't think need additional review.)

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: unbitrotted part 3: add general support for nsStylePosition::mMinWidth having eStyleUnit_Auto

As with earlier parts, the changes here (w.r.t. the mechanical version) are trivial unbitrotting. They all fall into one of the following categories:
 - changes in contextual code.
 - s/mComputedMinWidth/ComputedMinWidth()/ in nsHTMLReflowState (per smontagu's changes to that file).
 - Removal of a crashtests.list assertion-count tweak (i.e. the tweak is present in the mechanical patch, but that patch-hunk is gone in this version), because the assertion in question (and the formerly-tweaked asserts() annotation) was completely removed in bug 943448.

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: unbitrotted part 4: add general support for nsStylePosition::mMinHeight having eStyleUnit_Auto

(This patch has the same categories of unbitrotting w.r.t. its mechanical version as described in previous comment.)

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: unbitrotted part 5: adding back nsFlexContainerFrame code for handling min-height:auto

I'm requesting review on this part, because the underlying code has changed a bit, which makes this rebased patch less of a trivial backout than the other parts.

OVERVIEW OF THIS PATCH:
So, we already compute the intrinsic height of flex items when resolving a flex base size for "flex-basis:auto;height:auto".  (flex-basis:auto means "use at the main-size property, e.g. 'height' in a vertical flexbox; and if 'height' is auto, then we have to figure out what our auto-height is.)

This patch makes us do that same computation to resolve 'min-height:auto', as well.

RE the code-changes that make this version different from the mechanical-backout-version (attachment 8431692 [details] [diff] [review]): So, we used to do this auto-height-computation right before we created a FlexItem -- that's where the mechanical-backout-patch expects to find this code. But, in current mozilla-central, we do the auto-height-computation *after* the FlexItem has been created, in 'ResolveStretchedCrossSize'[1].  So, this unbitrotted patch just targets the new code instead of the old code.

[1] (See bug bug 903880 comment 14 for why we deferred auto-height computation to after FlexItem construction, if you're curious.)

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: unbitrotted part 6: adding back special-handling for min-width:auto in reflow state & computed style

(Not requesting review on part 6; just trivial unbitrotting diffs as described in comment 15.)

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: unbitrotted part 7: adding back mochitest for min-[size]:auto

(unbitrotted mochitest patch to use mochitest.ini instead of Makefile.in.)

Comment 20

[Daniel Holbert [:dholbert]]

Attachment: part 8: Remove pref-setting wrapper for min-[size]:auto mochitest

The mochitest that's being added back in part 7 will need the same simplifications that we've applied to other flexbox mochitests (per bug 936100 comment 5), now that the flexbox pref has been removed.

Here's the first such tweak (which would have been covered in mozilla-central changeset a4ff3c828a73, if this were in the tree when that changeset landed). This removes the no-longer-necessary pref-setting wrapper file.

Comment 21

[Daniel Holbert [:dholbert]]

Attachment: part 9: Rename file_flexbox_min_size_auto.html to test_flexbox_min_size_auto.html

...and this part renames the file mochitest from file_flexbox_min_size_auto.html to test_flexbox_min_size_auto.html, now that the obsolete wrapper has been removed.

(This would have been covered in mozilla-central changeset 598570ccfca5, if this were in the tree when that changeset landed.)

This is the last part of this bug.

Comment 22

[Daniel Holbert [:dholbert]]

Attachment: unbitrotted part 5, v2: adding back nsFlexContainerFrame code for handling min-height:auto

Fixing one issue found in part 5 -- basically softening the assertion that I'm adding in UpdateMainMinSize().

The idea behind the assertion is: when we resolve "min-height:auto", we expect that (up until this point) it's been treated as 0, since flexbox layout is the only place where it does something special.

However, themed widgets impose additional min-sizing-restrictions, which could give us a nonzero min-width value even before we've resolved "min-width:auto". So, this updated patch includes a special case in the assertion to allow those cases. It also lets us bypass the guts of the function when it's not actually increasing our min-width, as a (minor) optimization.

Comment 23

[Mats Palmgren (inactive)]

Comment on attachment 8436082 [details] [diff] [review]
unbitrotted part 5, v2: adding back nsFlexContainerFrame code for handling min-height:auto

r=mats

Comment 24

[Daniel Holbert [:dholbert]]

Landed all patches (parts 1 through 9):
https://hg.mozilla.org/integration/mozilla-inbound/rev/93019f345da9
https://hg.mozilla.org/integration/mozilla-inbound/rev/5988d4a17c59
https://hg.mozilla.org/integration/mozilla-inbound/rev/e776a7beb9ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc15aa692206
https://hg.mozilla.org/integration/mozilla-inbound/rev/24c2f03786ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2c96bbed41d
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8697fee9328
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc309b3a6da7
https://hg.mozilla.org/integration/mozilla-inbound/rev/106c96d0abd1

Comment 25

[Daniel Holbert [:dholbert]]

This had a nice Try run:
  https://tbpl.mozilla.org/?tree=Try&rev=c3071440fb1b
...and yet it burned on a few platforms when landed, due to some inscrutable build failure with CSS2Properties.webidl:
  https://tbpl.mozilla.org/php/getParsedLog.php?id=44358044&tree=Mozilla-Inbound

edmorley is pretty sure it just needs a clobber, and I tend to agree, though I'm not 100% sure why / which part is causing the need -- maybe "part 2" here? (which touched nsCSSPropList.h)

In any case, edmorley triggered clobbers and I pushed a tweak to CLOBBER to make sure this gets clobbered on merges as well:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/4d5d939010e5

Comment 26

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

https://hg.mozilla.org/mozilla-central/rev/dc309b3a6da7
https://hg.mozilla.org/mozilla-central/rev/106c96d0abd1
https://hg.mozilla.org/mozilla-central/rev/4d5d939010e5

Comment 27

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

https://hg.mozilla.org/mozilla-central/rev/93019f345da9
https://hg.mozilla.org/mozilla-central/rev/5988d4a17c59
https://hg.mozilla.org/mozilla-central/rev/e776a7beb9ea
https://hg.mozilla.org/mozilla-central/rev/fc15aa692206
https://hg.mozilla.org/mozilla-central/rev/24c2f03786ac
https://hg.mozilla.org/mozilla-central/rev/b2c96bbed41d
https://hg.mozilla.org/mozilla-central/rev/d8697fee9328

Comment 28

[Daniel Holbert [:dholbert]]

Transferring dev-doc-needed keyword to bug 1015474 (where this restored functionality is brought up-to-date w/ current spec text).