Closed Bug 790903 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: kennyluck, Assigned: dholbert)

References

(Blocks 1 open bug, )

Details

Attachments

(2 files)

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
Oh, I forgot to mention that Chrome Canary supports '-webkit-flex: 1 1 0' too.
Assignee: nobody → yuguozhou
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!
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.
Blocks: 783409
No longer blocks: css3-flexbox
Depends on: 696253
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
(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 ;)
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.
(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.
Sounds good to me -- thanks for the update!
Hi Kenny & yuguozhou -- are there any updates here, and/or anything I can help with on this?
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
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.
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. :)
(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.
OK, taking. :)
Assignee: yuguozhou → dholbert
Status: NEW → ASSIGNED
Whiteboard: [good first bug][mentor=kennyluck][lang=c++]
Attached patch fix v1Splinter Review
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.
Attachment #674151 - Flags: review?(dbaron)
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/dholbert@mozilla.com-7cbbdb0cca9e
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/dholbert@mozilla.com-7cbbdb0cca9e
Comment on attachment 674151 [details] [diff] [review]
fix v1

r=dbaron
Attachment #674151 - Flags: review?(dbaron) → review+
https://hg.mozilla.org/mozilla-central/rev/2a8a4b8ed382
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.