Last Comment Bug 939905 - Support "flex-flow" shorthand for multi-line flexbox
: Support "flex-flow" shorthand for multi-line flexbox
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
P3 normal with 3 votes (vote)
: mozilla28
Assigned To: Daniel Holbert [:dholbert]
:
: Jet Villegas (:jet)
Mentors:
http://dev.w3.org/csswg/css-flexbox/#...
: 836881 880258 971145 (view as bug list)
Depends on: 702508
Blocks:
  Show dependency treegraph
 
Reported: 2013-11-18 10:47 PST by Daniel Holbert [:dholbert]
Modified: 2014-03-15 17:03 PDT (History)
8 users (show)
dholbert: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix v1 (9.78 KB, patch)
2013-11-18 22:07 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
fix v2 (using ParseChoice) (7.79 KB, patch)
2013-11-19 13:22 PST, Daniel Holbert [:dholbert]
cam: review+
Details | Diff | Splinter Review

Description User image Daniel Holbert [:dholbert] 2013-11-18 10:47:52 PST
Bug 702508 adds support for "flex-wrap" property.

With that, we can now support the "flex-flow" shorthand property (which sets both "flex-direction" and "flex-flow").

http://dev.w3.org/csswg/css-flexbox/#flex-flow-property
Comment 1 User image Daniel Holbert [:dholbert] 2013-11-18 10:57:26 PST
*** Bug 880258 has been marked as a duplicate of this bug. ***
Comment 2 User image Daniel Holbert [:dholbert] 2013-11-18 10:57:49 PST
*** Bug 836881 has been marked as a duplicate of this bug. ***
Comment 3 User image Daniel Holbert [:dholbert] 2013-11-18 22:07:09 PST
Created attachment 8334364 [details] [diff] [review]
fix v1

heycam, would you mind reviewing?

Link to spec is in comment 0. (and as implied by comment 0, this layers on the patches from that bug as well.)

Quoting the spec, for convenience:
> Name:	flex-flow
> Value:	<flex-direction> || <flex-wrap>

flex-direction and flex-wrap are both enum-valued properties. The "||" means they can be provided in either order, and either of them may be omitted (but at least one has to be there).
Comment 4 User image Daniel Holbert [:dholbert] 2013-11-18 22:33:25 PST
Comment on attachment 8334364 [details] [diff] [review]
fix v1

Actually, I think I might need to tweak the inherit/unset/initial handling a bit. Canceling review request for the moment.
Comment 5 User image Cameron McCormack (:heycam) 2013-11-18 22:35:41 PST
While I've begun looking at the patch, and before you put a new one up, I think it would be good to use ParseChoice to parse the two optional components, once you've got past the initial/inherit/unset checking.
Comment 6 User image Cameron McCormack (:heycam) 2013-11-18 22:36:34 PST
Actually, ParseChoice can handle the parsing of initial/inherit/unset, too.
Comment 7 User image Daniel Holbert [:dholbert] 2013-11-18 23:18:10 PST
Ah, that looks great - thanks! I'll give that a shot tomorrow. Looks like it should simplify things a good bit.
Comment 8 User image Daniel Holbert [:dholbert] 2013-11-19 13:22:20 PST
Created attachment 8334819 [details] [diff] [review]
fix v2 (using ParseChoice)
Comment 9 User image Daniel Holbert [:dholbert] 2013-11-19 14:49:58 PST
https://tbpl.mozilla.org/?tree=Try&rev=3141d4d15d1d
Comment 10 User image Cameron McCormack (:heycam) 2013-11-19 15:44:19 PST
Comment on attachment 8334819 [details] [diff] [review]
fix v2 (using ParseChoice)

Looks great!  r=me with the understanding that bug 940229 will land soon and so we don't need to include invalid_values for initial/inherit/unset explicitly.
Comment 11 User image Daniel Holbert [:dholbert] 2013-11-19 16:10:56 PST
(In reply to Cameron McCormack (:heycam) from comment #10)
> Comment on attachment 8334819 [details] [diff] [review]
> fix v2 (using ParseChoice)
> 
> Looks great!

Thanks!
> r=me with the understanding that bug 940229 will land soon and
> so we don't need to include invalid_values for initial/inherit/unset
> explicitly.

Yup. (I briefly added some invalid_values like that (which caused testfailures in my first patch), but I took it out after verifying that bug 940229 catches those issues just as effectively.)
Comment 12 User image Daniel Holbert [:dholbert] 2013-12-05 11:06:34 PST
Landed this (and the other helper-bugs for multi-line flexbox) on inbound:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/872e64c93fda
Comment 13 User image Carsten Book [:Tomcat] 2013-12-06 04:38:12 PST
https://hg.mozilla.org/mozilla-central/rev/872e64c93fda
Comment 14 User image Daniel Holbert [:dholbert] 2014-02-11 21:15:04 PST
*** Bug 971145 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.