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

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks 1 bug)

Trunk
mozilla34
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(17 attachments, 1 obsolete attachment)

258 bytes, text/plain
Details
35.63 KB, patch
Details | Diff | Splinter Review
4.93 KB, patch
Details | Diff | Splinter Review
13.51 KB, patch
Details | Diff | Splinter Review
10.44 KB, patch
Details | Diff | Splinter Review
4.67 KB, patch
Details | Diff | Splinter Review
3.56 KB, patch
Details | Diff | Splinter Review
8.28 KB, patch
Details | Diff | Splinter Review
35.07 KB, patch
dholbert
: feedback+
Details | Diff | Splinter Review
5.55 KB, patch
dholbert
: feedback+
Details | Diff | Splinter Review
12.82 KB, patch
dholbert
: feedback+
Details | Diff | Splinter Review
8.80 KB, patch
dholbert
: feedback+
Details | Diff | Splinter Review
3.89 KB, patch
dholbert
: feedback+
Details | Diff | Splinter Review
8.57 KB, patch
dholbert
: feedback+
Details | Diff | Splinter Review
5.62 KB, patch
dholbert
: feedback+
Details | Diff | Splinter Review
1.38 KB, patch
dholbert
: feedback+
Details | Diff | Splinter Review
6.83 KB, patch
mats
: review+
Details | Diff | Splinter Review
Assignee

Description

5 years ago
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)
Assignee

Comment 1

5 years ago
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
Assignee

Comment 2

5 years ago
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).
Assignee

Updated

5 years ago
Blocks: 1015474
Assignee

Comment 3

5 years ago
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.
Assignee

Updated

5 years ago
Summary: Add back "min-width:auto" / "min-height:auto" for flex items, with updated behavior → Add back "min-width:auto" / "min-height:auto" for flex items
Assignee

Updated

5 years ago
Depends on: 1015482
Assignee

Updated

5 years ago
Attachment #8431687 - Attachment description: mechanical part 3: backout f8b5090d02e5 (adding back style-system handling) → mechanical part 3: backout f8b5090d02e5 (adding back style-system handling for min-width:auto)
Assignee

Updated

5 years ago
Attachment #8431692 - Attachment description: mechanical part 5: backout 7a289f49170f (adding back flexbox frame class code for min-height:auto) → mechanical part 5: backout 7a289f49170f (adding back nsFlexContainerFrame code for handling min-height:auto)
Assignee

Updated

5 years ago
Attachment #8431684 - Attachment description: mechanical part 1: backout afe27d80eeef (test tweaks) → mechanical part 1: backout afe27d80eeef (reverting test tweaks)
Assignee

Comment 11

5 years ago
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.
Assignee

Updated

5 years ago
Attachment #8431686 - Attachment description: mechanical part 2: backout e7a9e30409eb (adding back css parser support) → mechanical part 2: backout e7a9e30409eb (adding "auto" support in parser & as initial value, just computing to 0 for now)
Assignee

Updated

5 years ago
Attachment #8431686 - Attachment description: mechanical part 2: backout e7a9e30409eb (adding "auto" support in parser & as initial value, just computing to 0 for now) → mechanical part 2: backout e7a9e30409eb (adding back "auto" support in parser & as initial value, just computing to 0 for now)
Assignee

Comment 12

5 years ago
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.
Assignee

Comment 13

5 years ago
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.)
Attachment #8431740 - Flags: feedback+
Assignee

Comment 14

5 years ago
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.)
Attachment #8431746 - Flags: feedback+
Assignee

Updated

5 years ago
Attachment #8431687 - Attachment description: mechanical part 3: backout f8b5090d02e5 (adding back style-system handling for min-width:auto) → mechanical part 3: backout f8b5090d02e5 (add general support for nsStylePosition::mMinWidth having eStyleUnit_Auto)
Assignee

Updated

5 years ago
Attachment #8431690 - Attachment description: mechanical part 4: backout 5db313632268 (adding back style-system handling for min-height:auto) → mechanical part 4: backout 5db313632268 (add general support for nsStylePosition::mMinHeight having eStyleUnit_Auto)
Assignee

Comment 15

5 years ago
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.
Attachment #8431845 - Flags: feedback+
Assignee

Comment 16

5 years ago
(This patch has the same categories of unbitrotting w.r.t. its mechanical version as described in previous comment.)
Attachment #8431860 - Flags: feedback+
Assignee

Comment 17

5 years ago
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.)
Attachment #8435558 - Flags: review?(matspal)
Assignee

Comment 18

5 years ago
(Not requesting review on part 6; just trivial unbitrotting diffs as described in comment 15.)
Attachment #8435936 - Flags: feedback+
Assignee

Comment 19

5 years ago
(unbitrotted mochitest patch to use mochitest.ini instead of Makefile.in.)
Attachment #8435962 - Flags: feedback+
Assignee

Comment 20

5 years ago
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.
Attachment #8435969 - Flags: feedback+
Assignee

Comment 21

5 years ago
...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.
Attachment #8435978 - Flags: feedback+
Assignee

Updated

5 years ago
Attachment #8435978 - Attachment description: 984711-part9-rename-mochitest.patch → part 9: Rename file_flexbox_min_size_auto.html to test_flexbox_min_size_auto.html
Assignee

Updated

5 years ago
Depends on: 1021913
Assignee

Comment 22

5 years ago
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.
Attachment #8435558 - Attachment is obsolete: true
Attachment #8435558 - Flags: review?(matspal)
Attachment #8436082 - Flags: review?(matspal)
Comment on attachment 8436082 [details] [diff] [review]
unbitrotted part 5, v2: adding back nsFlexContainerFrame code for handling min-height:auto

r=mats
Attachment #8436082 - Flags: review?(matspal) → review+
Assignee

Comment 25

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/dc309b3a6da7
https://hg.mozilla.org/mozilla-central/rev/106c96d0abd1
https://hg.mozilla.org/mozilla-central/rev/4d5d939010e5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Assignee

Comment 28

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

Updated

5 years ago
Flags: in-testsuite+

Updated

5 years ago
Depends on: 1055155
Assignee

Updated

5 years ago
No longer depends on: 1055155
Assignee

Updated

5 years ago
Depends on: 1067136
Assignee

Updated

5 years ago
Duplicate of this bug: 1078431

Updated

5 years ago
Depends on: 1105865
No longer depends on: 1105865

Updated

4 years ago
Depends on: 1165988

Updated

3 years ago
Depends on: 1247212
Assignee

Updated

3 years ago
No longer depends on: 1247212
You need to log in before you can comment on or make changes to this bug.