790903 - Accept unitless 0 as <'flex-basis'> in 'flex' shorthand

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Kang-Hao (Kenny) Lu [:kennyluck]]

Looking at the source code (sorry but I haven't compiled Gecko for a while now), I noticed that it doesn't accept things like 'flex: 1 1 0', while the spec has recently changed[1] (notice the wording around "preceded by both flex ratios") to say that should be valid. (See also [2] for a request to make this clearer)

I also think 'flex: 1 1 0' should be valid because otherwise it sort of violates the common invariant in CSS that you can compose values of longhand properties to a shorthand.

The parsing function is at [3], and it has pretty detailed comments.

[1] https://dvcs.w3.org/hg/csswg/diff/0101972a0eb9/css3-flexbox/Overview.src.html
[2] http://lists.w3.org/Archives/Public/www-style/2012Sep/thread#msg216
[3] http://hg.mozilla.org/mozilla-central/file/f36b93c70d05/layout/style/nsCSSParser.cpp#l5127

Comment 1

[Kang-Hao (Kenny) Lu [:kennyluck]]

Attachment: tests for unitless 0 in 'flex' shorthand

Oh, I forgot to mention that Chrome Canary supports '-webkit-flex: 1 1 0' too.

Comment 2

[Daniel Holbert [:dholbert]]

Yup, I was actually about to file this bug myself in the next day or so :) I was discussing this with fantasai offline to see whether that change was actually intended or not.  (since the commit message for the dvcs.w3.org cset in comment 0 is "Rearrangements of prose", and didn't sound like it intended to change meaning.)

Anyway, the upshot is that the change was intentional (and the previous text was more directed at authors than at implementors), and we do need this change.


So -- we need to change this code...
https://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSParser.cpp?rev=d16c4404e8c4&mark=5208-5223#5208
...to remove the "== eCSSUnit_Number" clause, and to pass a different variantMask -- without VARIANT_NUMBER.

In fact, we probably want to do something like:
 uint32_t flexBasisVariantMask =
   (nsCSSProps::ParserVariant(eCSSProperty_flex_basis) & ~(VARIANT_INHERIT));
 uint32_t flexBasisOrFactorVariantMask = flexBasisVariantMask | VARIANT_NUMBER;

and then use the latter one in place of |variantMask| everywhere except for this last clause, where we'll now use the former one. (since we don't want to parse unitless 0 as a number)

(We'll need to update the comment in that chunk, too.)

I'm happy to do the above, but it sounds like Kenny's on top of it, which is fine by me too!

Comment 3

[Daniel Holbert [:dholbert]]

Oh -- we also need to update the "-moz-flex" chunk in
 https://mxr.mozilla.org/mozilla-central/source/layout/style/test/property_database.js#909
to move "0 0 0" from the invalid list to the valid list.

Comment 4

[Daniel Holbert [:dholbert]]

Note also that this code is disabled (#ifdef'ed off) by default, so you'll need to do a bit extra to actually test that your changes compile correctly.

At a minimum, you'll want to start out by running the following commands in your mozilla-central directory (I'm assuming that you have a fresh up-to-date clone, with no patch queue (no .hg/patches directory)):
> hg clone https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches .hg/patches
> hg qgoto flexbox-enable.patch

That will enable everything that you'll need to build & test all of the flexbox code that has already landed (support for parsing the properties, basically -- which is all that matters for the purpose of this bug).

If you *also* want to see the flexbox _layout_ (instead of just parsing), you'll want to instead do:
> hg qgoto flexbox-marker.patch
which will apply a few more patches.  (You shouldn't need that for the purpose of this bug, though.)

If the above commands don't work or don't make sense to you, you should probably skim this page (at least "Introduction" and "How to use MQ for Mozilla development"):
 https://developer.mozilla.org/en-US/docs/Mercurial_Queues

Comment 5

[Kang-Hao (Kenny) Lu [:kennyluck]]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> I'm happy to do the above, but it sounds like Kenny's on top of it, which is
> fine by me too!

I appreciate that. Folks in the Mozilla Taiwan community recently sent me the contact information of 30 people who are interested in hands-on coding and it is more than challenging for me to create 30 mentored bugs (in fact, it's just crazy. lol) ...

I hope you guys don't accuse me of blocking Firefox's progress though ;)

Comment 6

[Daniel Holbert [:dholbert]]

yuguozhou: Do you know how soon you'll get to this? (Hopefully it shouldn't take too much time -- see comment 2 & comment 3 for an overview of what'd be required, and see comment 4 for how to get your code to actually be built.)

If you think you can knock this out in the next week or so, that's great! If not, I might steal this bug from you and just fix it myself, because I'm hoping to get flexbox out soon and I'd like for us to be correct on this bug when it's enabled. :)

Either way is fine by me -- I just want to be sure this bug doesn't drift off the radar.

Comment 7

[Kang-Hao (Kenny) Lu [:kennyluck]]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> If you think you can knock this out in the next week or so, that's great! 

We'd like to ask for a week of extension (or at least 3 days) from now, as Yuguo has read the comments and the relevant section of the spec (and is now trying hard to build the code ;). 

> If not, I might steal this bug from you and just fix it myself, because I'm
> hoping to get flexbox out soon and I'd like for us to be correct on this bug
> when it's enabled. :)

Yeah.

Comment 8

[Daniel Holbert [:dholbert]]

Sounds good to me -- thanks for the update!

Comment 9

[Daniel Holbert [:dholbert]]

Hi Kenny & yuguozhou -- are there any updates here, and/or anything I can help with on this?

Comment 10

[Kang-Hao (Kenny) Lu [:kennyluck]]

I can't solve Yuguo's problem in building Firefox. Since a week is past already I guess you might want to take over it unless Yuguo wants some extension.

By the way, what he runs into is this: http://pastebin.mozilla.org/1856302

Comment 11

[Daniel Holbert [:dholbert]]

Another week would be fine.  Have him jump into #developers (e.g. by visiting http://irc.lc/mozilla/developers/ ) and see if he someone can help him sort that out.

Comment 12

[Daniel Holbert [:dholbert]]

Yuguo, did you end up being able to compile?

If you've got a patch in-progress, let me know -- otherwise, I think I'd like to steal this bug, and we'll have Kenny find another good-first-bug for you. :)

Comment 13

[Kang-Hao (Kenny) Lu [:kennyluck]]

(In reply to Daniel Holbert [:dholbert] from comment #12)
> Yuguo, did you end up being able to compile?

I can't speak for him but probably no.
 
> If you've got a patch in-progress, let me know -- otherwise, I think I'd
> like to steal this bug, and we'll have Kenny find another good-first-bug for
> you. :)

Please don't hesitate to do so. I think I was too aggressive about the good-first-bug idea and I feel somewhat sorry about it.

Comment 14

[Daniel Holbert [:dholbert]]

OK, taking. :)

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Here's the fix.

Basically, the only change is it makes us no longer use VARIANT_NUMBER when specifically parsing a flex-basis in the final position, because we want to accept 0 as a length there.  See comments in the patch for more.

Comment 16

[Mozilla RelEng Bot]

Try run for 7cbbdb0cca9e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=7cbbdb0cca9e
Results (out of 22 total builds):
    success: 21
    warnings: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-7cbbdb0cca9e

Comment 17

[Mozilla RelEng Bot]

Try run for 7cbbdb0cca9e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=7cbbdb0cca9e
Results (out of 23 total builds):
    success: 22
    warnings: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-7cbbdb0cca9e

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 674151 [details] [diff] [review]
fix v1

r=dbaron

Comment 19

[Daniel Holbert [:dholbert]]

Green try run, aside from a few known-intermittent-oranges:
  https://tbpl.mozilla.org/?tree=Try&rev=f08b1abd0d3d

Landed:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/2a8a4b8ed382

Comment 20

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/2a8a4b8ed382