Closed Bug 1032922 Opened 10 years ago Closed 10 years ago

Rename "flex-basis:auto" to "main-size", while preserving "flex:auto" shorthand value

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox34 --- disabled

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 2 open bugs, )

Details

(Keywords: dev-doc-complete, Whiteboard: [this was ultimately backed out in bug 1093316 due to a spec change])

Attachments

(4 files, 3 obsolete files)

Per http://lists.w3.org/Archives/Public/www-style/2014Jul/0008.html , flex-basis's keyword "auto" is being renamed to "main-size".

Also, the "flex" shorthand will now take the keyword "auto", which will expand to "1 1 main-size".

This is already reflected in the current flexbox ED -- change here:
 https://dvcs.w3.org/hg/csswg/diff/bd6e4c8effaf/css-flexbox/Overview.bs
Note: We'll need to switch all usages of e.g. "flex: 0 0 auto" to "flex: 0 0 main-size" in testcases and in CSS that's used in firefox's UI, or else they'll stop working.

MXR search for all such strings:
 http://mxr.mozilla.org/mozilla-central/search?string=flex%3A.*auto&regexp=1

(mostly tests, and a few instances of "flex: auto" which won't need changing, but also a few instances of the full shorthand syntax in e.g. devtools & themes code.)

MXR search for all "flex-basis: auto" usages (which will also need fixing):
 http://mxr.mozilla.org/mozilla-central/search?string=flex-basis.*auto&regexp=1
(fortunately, just code-comments and one testcase for this one.)
Patches coming up here soon.

FWIW, I'm planning to break this into (at least) the following parts -- parts (1) and (2) are non-functional, and while (3) will actually change behavior to fix this bug:

(1) Add code for the new "flex"-shorthand special-case, for "flex:auto".  (This special-case will initially expand to "flex: 1 1 auto", though a later patch will change that to "1 1 main-size".)

(2) Make flex-basis use its own dedicated nsCSSProps keyword table, instead of co-opting the keyword table for "width" like it does in current mozilla-central. For now, the new keyword table will be exactly the same as the one for width. (The next patch will extend it.) Also: add a mochitest to ensure that these keyword tables stay in sync, by verifying that "flex-basis" accepts all of the valid "width" & "height" values listed in property_database.js.

(3) Add & use the new "main-size" keyword. In particular: update the flex:auto expansion that we created in (1); add the "main-size" keyword to the flex-basis keyword table that we created in (2); change the initial value for flex-basis in nsRuleNode.cpp; change the code that currently checks for "auto" to check for "main-size" instead; and update in-tree CSS (in testcases & UI) as-necessary.

(Also: I may want to post to dev.gaia around when this lands, since it's possible this will cause breakage there. Though I think this is less likely to break stuff than bug 1015474 (min-size:auto).)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
This patch ("part 1") adds special-case parsing for "flex:auto". (As noted in comment 2, this special-case isn't technically necessary yet, but it will become necessary once we add the "main-size" keyword.)
Attachment #8468305 - Flags: review?(cam)
As described in comment 2, this part ("part 2") gives flex-basis its own dedicated keyword table, instead of having it co-opt the one for "width".

(Up until now, they accepted the exact same values, so sharing a table was appropriate.  Now they'll accept the same values, plus flex-basis will accept one more -- 'main-size' -- which will be added to its new table in the next patch after this one.)

And to make sure this flex-basis keyword table doesn't get overlooked when we add new keywords for "width" (or "height") in the future, I'm including a mochitest to check that "flex-basis" accepts all of property_database.js's valid values for width & height.
Attachment #8468308 - Flags: review?(cam)
Comment on attachment 8468305 [details] [diff] [review]
part 0: add special-case for "flex: auto"

Review of attachment 8468305 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSParser.cpp
@@ +7421,5 @@
> +    AppendValue(eCSSProperty_flex_shrink, nsCSSValue(1.0f, eCSSUnit_Number));
> +    AppendValue(eCSSProperty_flex_basis, nsCSSValue(eCSSUnit_Auto));
> +    return true;
> +  }
> +

I was going to suggest considering merging these two and parsing with (VARIANT_NONE | VARIANT_AUTO) and having a local variable set to 0.0f or 1.0f based on that, to avoid duplicating the AppendValue calls (which you probably already considered).  But on reflection it's clearer as it is.
Attachment #8468305 - Flags: review?(cam) → review+
Comment on attachment 8468308 [details] [diff] [review]
part 1: give flex-basis its own dedicated keyword table

Review of attachment 8468308 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSParser.cpp
@@ +7456,5 @@
>    // 'flex-grow' value (a number), *not* as a 'flex-basis' value (a length).
>    // Conveniently, that's the behavior this combined variant-mask gives us --
>    // it'll treat unitless 0 as a number. The flexbox spec requires this:
>    // "a unitless zero that is not already preceded by two flex factors must be
>    //  interpreted as a flex factor.

There's a missing closing quote here you could fix while you're around here.

::: layout/style/nsCSSProps.cpp
@@ +1131,5 @@
> +  eCSSKeyword__moz_fit_content, NS_STYLE_WIDTH_FIT_CONTENT,
> +  eCSSKeyword__moz_available,   NS_STYLE_WIDTH_AVAILABLE,
> +  eCSSKeyword_UNKNOWN,-1
> +};
> +

I am *slightly* worried about mixing style constants defined (and named) for one property with those that are specific to this table.  Someone might introduce a new width constant later, at the end of the NS_STYLE_WIDTH_ block in nsStyleConsts.h, with the same value as the to-come-in-part-3 NS_STYLE_FLEX_BASIS_MAIN_CONTENT.

How about we define NS_STYLE_FLEX_BASIS_MAX_CONTENT etc. and use those here?

Or otherwise, add a comment (in part 3) in nsStyleConsts.h above the NS_STYLE_WIDTH_ block to remind people to adjust the value of NS_STYLE_FLEX_BASIS_MAIN_CONTENT.

::: layout/style/test/test_flexbox_flex_basis_values.html
@@ +46,5 @@
> +function main()
> +{
> +  let decl = document.getElementById("testnode").style;
> +
> +  let props = [ "width", "height" ];

Do you have a reason to believe the spec will add some keywords to 'height' that will be usable in 'flex-basis' too?  I'm wondering whether we should be checking the 'height' values given the spec only lists <'width'> in the property definition line.
Oh, I just saw this in the spec:

  It accepts the same values as the width and height property.

so I guess that is the intention.
Comment on attachment 8468308 [details] [diff] [review]
part 1: give flex-basis its own dedicated keyword table

r=me with at least the second request from comment 6 addressed.
Attachment #8468308 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #6)
> I am *slightly* worried about mixing style constants [...]
> 
> How about we define NS_STYLE_FLEX_BASIS_MAX_CONTENT etc. and use those here?
> 
> Or otherwise, add a comment (in part 3) in nsStyleConsts.h above the
> NS_STYLE_WIDTH_ block to remind people to adjust the value of
> NS_STYLE_FLEX_BASIS_MAIN_CONTENT.

Good point! I agree, and I'm already doing this latter option (in my local version of part 3) -- I'm actually defining NS_STYLE_FLEX_BASIS_MAIN_CONTENT in the same chunk of nsStyleConsts.h as the width keywords, with a comment explaining why it's there (because it's very important that it not collide with a new width keyword's enum-value).

> Do you have a reason to believe the spec will add some keywords to 'height'
> that will be usable in 'flex-basis' too?

I'm not sure whether/when "height" will get new keywords, but if it does, I'm fairly sure they need to be valid for "flex-basis".  flex-basis sort of takes the place of 'width'/'height' for a flex item, so it needs to be as expressive as they are.

I think the spec just uses <'width'> in the property-definition to be concise, since there's currently nothing that's valid for <'height'> that isn't also valid for <'width'>.  But it's very careful to be direction-independent, so I'm fairly sure that if there *were* a new <'height'>-specific keyword, we'd need to support it.  So, the test is trying to be generic & future-proof.

> I'm wondering whether we should be
> checking the 'height' values given the spec only lists <'width'> in the
> property definition line.

I don't think there's any harm -- if any height-specific keyword does appear, we'll want to support it.  In the meantime, it's at worst a tiny bit of overkill, and perhaps a bit misleading/confusing when compared against the spec (as you noted) -- I'll add a comment to the test to explain, to make this clearer.  Sound good?

Thanks for the reviews!
(In reply to Cameron McCormack (:heycam) from comment #7)
> Oh, I just saw this in the spec:
> 
>   It accepts the same values as the width and height property.
> 
> so I guess that is the intention.

Ah, just saw this. Cool -- I'll just add a clarifying comment to the test then.
Comment on attachment 8468305 [details] [diff] [review]
part 0: add special-case for "flex: auto"

Sorry, I just realized that Part 1 is currently kinda-broken. We can't actually parse "flex:auto" via an early-return like the patch does right now -- that breaks the parsing of expressions like...
      flex: auto 5;
 (flex-basis-^) (^--flex-grow)
...because we'll see the "auto" and immediately succeed, and then we subsequently see the "5" and treat it as trailing junk and reject the value.

(Fortunately, we have a mochitest (test_flexbox_flex_shorthand.html) that checks that "auto 5" is a valid value, and it caught this problem. I just needed to run it. :) Sorry for not testing thoroughly before requesting review.  New patch up soon.)
Attachment #8468305 - Attachment is obsolete: true
Attachment #8468305 - Attachment description: part 1: add special-case for "flex: auto" → part 0: add special-case for "flex: auto"
Attachment #8468308 - Attachment description: part 2: give flex-basis its own dedicated keyword table → part 1: give flex-basis its own dedicated keyword table
In light of comment 11, I'm just dropping the old "part 1" (for the flex:auto special-case) completely, and I'll add the special-case parsing for "flex:auto" in the main patch here that does the s/flex-basis:auto/flex-basis:main-size/ renaming.

So, I'm renumbering the parts so that the keyword-table patch (which is still useful) is now part 1. The main patch which does the rename (not posted yet) will be part 2.
Here's the updated keyword-table patch, with a comment about 'height' added to the test (per end of comment 9), and with one more s/kWidthKTable/kFlexBasisKTable/ that I was missing in the last patch-version, in nsComputedDOMStyle.cpp. (This caused some mochitests to fail.)

(I grepped for remaining kWidthKTable usages to make sure I didn't miss any others, and I confirmed that all the remaining usages are for 'width', 'min-width', and 'max-width'.)

Carrying forward r+.
Attachment #8468308 - Attachment is obsolete: true
Attachment #8469065 - Flags: review+
Attachment #8469065 - Attachment description: part 1 v2 [r=heycam] → part 1 v2: give flex-basis its own dedicated keyword table [r=heycam]
This patch introduces "flex-basis:main-size" (which is replacing "flex-basis:auto", so that now "flex-basis:auto" will actually mean the same thing as "width:auto", instead of meaning "look at width").

Most of this is keyword-introduction & renaming, but here are some notes on the less-obvious things here:
 - nsLayoutUtils.cpp & nsFrame.cpp: This is the code where we swap in "flex-basis" in place of "width" or "height" (depending on if we're vertical or horizontal) when computing the size of a flex item.  (producing a nscoord value which we use as the "flex base size" in the flex layout algorithm)  In current trunk, we swap in "flex-basis", unless it's got the value "auto" [which up until now has meant "clone the main-size property"].  This patch now checks for "main-size" instead of "auto", though I've refactored out some of the logic into a nsStyleUtil helper-function, due to some quirks about handling width-specific keywords when we're vertical.  (See the comments I've added in nsStyleUtil.cpp|h for more.)

- nsFlexContainerFrame.cpp: the first comment being tweaked here is just about situations in which we might have to measure the auto-height of a flex-item before we can do flex layout -- we now have one more way that can happen, so I'm extending that comment a bit.

 - nsCSSParser.cpp: Here, I'm adding the special-case for "flex:auto" --> "1 1 main-size", as well as doing s/auto/main-size/ in the existing "flex:none" expansion.

 - nsRuleNode.cpp: The SetCoord() call for flex-basis is getting a little more complex.  Since the initial value of "flex-basis" is no longer "auto", we can't do everything in a single SetCoord() call anymore (using the SETCOORD_INITIAL_AUTO flag). We could hypothetically add a SETCOORD_INITIAL_MAIN_SIZE flag, but that'd be overkill since it'd be just for this property. So, I'm letting "initial" & "unset" just make SetCoord() fail (return false), and then I'm handling them explicitly.  (This is exactly what we do for "vertical-align", too -- I used its chunk in nsRuleNode.cpp as a reference.)
Attachment #8469080 - Flags: review?(cam)
This patch fixes up unit tests in light of the s/auto/main-size/ renaming.

This is mostly changes like "flex: 1 1 auto" --> "flex: 1 1 main-size", and a few cases of "flex-basis:auto" that need renaming to "flex-basis:main-size".

(Note that content with "flex:auto" doesn't need to change, since that shorthand's semantic meaning isn't changing.  This was an explicit goal in this renaming, as noted in the www-style post in comment 1, in the sentence beginning "To avoid breaking content -- it's why the "flex:auto" special-case is being introduced.)

(I'm naming this "2b" because I indent to fold this into patch 2a when landing, so as not to create an intermediate point in the repo history where tests fail. I'm just splitting it out to make review easier.)
Attachment #8469083 - Flags: review?(cam)
...and this patch (part 2c) does the renaming in frontend CSS files.

I intend to merge this with parts 2a and 2b when landing, so as not to create a point in mozilla-central history with broken browser-UI.

I sanity-checked that I'd caught all of the CSS that needs a rename by running the following commands in my $SRCDIR, with these patches all applied:

(1) grep -r "flex-basis:.*auto" *
The remaining instances caught by this command (all comments) really do mean "auto", not "main-size". (They're talking about auto in the sense of "width:auto" or "height:auto", not in the sense of "look at my width or height".)  So they're good.

(2) grep -r "flex:.*auto" *
The remaining instances caught by this command are all "flex: auto", which doesn't need to change, as noted in the middle of comment 15.

So I think I've caught all the CSS that needs this renaming in mozilla-central.
Attachment #8469086 - Flags: review?(cam)
(oops, previous version still had some straggling "auto" values in gCSSProperties["flex"].initial_values, in property_database.js.  Fixed 'em here, so that test_value_computation.html passes now.)

(Maybe the property_database.js parts belong in part 2b instead of 2a; I'm not going to worry about it, since it's all getting merged together for landing.)
Attachment #8469080 - Attachment is obsolete: true
Attachment #8469080 - Flags: review?(cam)
Attachment #8469091 - Flags: review?(cam)
Blocks: 1050431
Comment on attachment 8469091 [details] [diff] [review]
part 2a, v2: rename "flex-basis:auto" to "main-size", but add special case for "flex: auto"

Review of attachment 8469091 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsStyleUtil.h
@@ +124,5 @@
> +   * encounter them, we fall back to behaving as if we had flex-basis's initial
> +   * value.
> +   */
> +  static bool IsFlexBasisMainSize(const nsStyleCoord& aFlexBasis,
> +                                  bool aIsMainAxisHorizontal);

Newline after this one.
Attachment #8469091 - Flags: review?(cam) → review+
Attachment #8469083 - Flags: review?(cam) → review+
Comment on attachment 8469086 [details] [diff] [review]
part 2c: s/auto/main-size/ in non-test CSS

Review of attachment 8469086 [details] [diff] [review]:
-----------------------------------------------------------------

Maybe worth filing a followup to replace |flex: {0 0,1 1} main-size| with |flex: {none,auto}|?
Attachment #8469086 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #20)
> Maybe worth filing a followup to replace |flex: {0 0,1 1} main-size| with
> |flex: {none,auto}|?

I'd probably prefer not do do a mass-switch of "1 1 main-size" --> "auto", because IMHO "1 1 main-size" is clearer. (particularly when combined with other flex items with different flexibilities).  It's non-obvious what "flex:auto" means -- I think it's largely there for backwards-compatibility.

However, "flex:none" is pretty clear (and at least as human-readable as "0 0 main-size"), so I agree that that one probably merits a followup to simplify things & use the shorter shorthand.
(In reply to Daniel Holbert [:dholbert] from comment #21)
> However, "flex:none" is pretty clear (and at least as human-readable as "0 0
> main-size"), so I agree that that one probably merits a followup to simplify
> things & use the shorter shorthand.

Filed bug 1050654.
Thanks for the reviews! Added the newline from comment 19 (good catch), folded patches 2[a,b,c] together, and landed:

 part 1:   https://hg.mozilla.org/integration/mozilla-inbound/rev/af2a4fb980ad
 part 2*:  https://hg.mozilla.org/integration/mozilla-inbound/rev/aece7f9f944c
Flags: in-testsuite+
(In reply to Daniel Holbert [:dholbert] from comment #2)
> (Also: I may want to post to dev.gaia around when this lands, since it's
> possible this will cause breakage there. Though I think this is less likely
> to break stuff than bug 1015474 (min-size:auto).)

I didn't bother posting, because this only affects a few lines of CSS in gaia (only in the video app), and even there it probably doesn't technically break anything, and I've already got a pull request up in bug 1050431 to fix that CSS just in case.
https://hg.mozilla.org/mozilla-central/rev/af2a4fb980ad
https://hg.mozilla.org/mozilla-central/rev/aece7f9f944c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Possible this patch broke the search bar width on www.google.com ?

1. Open Google.com
2. Search for anything to get returns
3. Note that now the search box at the top is very short.
Blocks: 1051511
Yes -- thanks very much for the heads up. I filed bug 1051511 to cover this issue -- I suspect the site's CSS needs some tweaking to accommodate the spec change, though I need to investigate a bit more to be sure. (In the meantime, the search box isn't unusably-skinny, so I don't think we need to back out or anything at this point.)
Depends on: 1052182
Blocks: 1057047
Depends on: 1057162
No longer blocks: 1051511
No longer blocks: 1057047
I spun off bug 1057162 to track regressions in web content that are caused by this keyword-renaming.

This will let us separate tech evang bugs (where the website just needs to rename the keyword to be compatible with the updated spec) vs. any actual regressions caused by this bug's commits.  Tech evang bugs should block bug 1057162; other regressions should block this bug here.
Blocks: 1093316
As discussed in bug 1093316, this feature "flex-basis:main-size" is still under active discussion at the CSSWG, so I've backed it out on beta.  (approval granted on bug 1093316; backout patches posted & landed there)

I'm setting "status-firefox34:disabled" on this bug, to indicate that this feature *was* on Firefox 34, but has been disabled (via backout) there.
Swapping ddc -> ddn to be sure I revisit this very soon.
Thanks! (Sorry, I should have thought of doing that.)
Blocks: 1097860
Blocks: 1097984
I dropped the chunk about "flex-basis: main-size" from
  https://developer.mozilla.org/en-US/Firefox/Releases/34/Site_Compatibility
and moved it to the Firefox 35 site compat page:
  https://developer.mozilla.org/en-US/Firefox/Releases/35/Site_Compatibility

I also updated our articles on "flex" & "flex-basis" to associate this rename with Firefox 35 instead of 34:
 https://developer.mozilla.org/en-US/docs/Web/CSS/flex
 https://developer.mozilla.org/en-US/docs/Web/CSS/flex-basis

Not sure if there's more dev-doc-needed beyond that; leaving the keyword for now.
Blocks: 1105111
Following up on comment 33 -- now that the "main-size" keyword has been officially removed from the spec (and backed out on all branches, in bug 1093316), we should update those articles to remove all mention of "main-size".  (It never shipped [or will ship] in an official firefox release, since we backed it out everywhere before Firefox 34 hit release.)

needinfo=me to remind myself to do this at some point [might not get to it right away], but if someone else gets to it before me, thanks!
Flags: needinfo?(dholbert)
I've updated the following pages to remove any hint of main-size:
https://developer.mozilla.org/en-US/docs/Web/CSS/flex
https://developer.mozilla.org/en-US/docs/Web/CSS/flex-basis
and
https://developer.mozilla.org/en-US/Firefox/Releases/34
https://developer.mozilla.org/en-US/Firefox/Releases/35

I assumed this back-out also reverted:
"The value used for the longhand {{cssxref("flex-basis")}} when omitted in the shorthand is now  0%, and the one for {{cssxref("flex-grow")}} is now 1, both different than their initial value." 
Is this correct?
Toggled dev-doc-complete so that it stopped reappearing in our dashboard. If I wasn't correct just toggle it to dev-doc-needed again.
(In reply to Jean-Yves Perrier [:teoli] from comment #35)
> I assumed this back-out also reverted:
> "The value used for the longhand {{cssxref("flex-basis")}} when omitted in
> the shorthand is now  0%, and the one for {{cssxref("flex-grow")}} is now 1,
> both different than their initial value." 
> Is this correct?

That text is still true -- the "flex" shorthand behavior, when values are omitted, remains unchanged.  I think the MDN page for "flex" is currently correct on this (it says "Defaults to 0% when omitted" and Defaults to 1 when omitted"), but it may be worth adding [back?] a chunk to specifically mention that these are different from the properties' initial values.
Flags: needinfo?(dholbert)
Whiteboard: [this was ultimately backed out in bug 1093316 due to a spec change]
Blocks: 1180272
No longer blocks: 1180272
You need to log in before you can comment on or make changes to this bug.