696253 - Add support for parsing/computing properties for CSS Flexbox

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

To simplify bug 666041 (flexbox) and reduce the number of patches that'll need to be attached/reviewed/discussed there, I'm splitting its implementation into several logically-separate bugs. (this being the first, possibly more to come)

This bug will track parsing & computing the values of the newly-added flexbox properties.  (but not having layout actually react to them)

Those new properties are:
  flex-align
  flex-flow
  flex-line-pack
  flex-order
  flex-pack
Reference: http://dev.w3.org/csswg/css3-flexbox/#property

Note: Patches attached here will be disabled by default, via #ifdef MOZ_FLEXBOX (a build flag defined in the first patch on bug 666041)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: patch 1: parse/compute flex-align, flex-line-pack, flex-pack

This patch adds support for parsing / computing value for the CSS properties flex-align, flex-line-pack, and flex-pack.  (I did these three together since they're simple and nearly the same.)

This patch stacks on top of patch 3 from bug 666041. (That patch defines NS_DISPLAY_FLEXBOX, which I use here in nsStyleDisplay::CalcDifference).

This also implicitly requires patch 1 from bug 666041 (which creates the MOZ_FLEXBOX build flag, disabled by default).

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: patch 1 v2: parse/compute flex-align, flex-line-pack, flex-pack

(forgot to qref and missed one whitespace fix in nsCSSProps.cpp (missing newlines); fixed now)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: patch 2: parse/compute flex-order

This patch parses the flex-order property
http://dev.w3.org/csswg/css3-flexbox/#flex-order0

There's one thing I'm unsure of in this patch -- the eStyleAnimType.
I currently have eStyleAnimType_None, which is what we use for -moz-box-ordinal-group, but that would prevent us from doing CSS transitions.

I could use eStyleAnimType_Coord, but that would require me to use a nsStyleCoord to store this property, and that seems like overkill for this property (which just takes integer values -- unlike e.g. z-index, which is nearly the same except that it also accepts the value 'auto').

I have an XXXdholbert comment about possibly creating eStyleAnimType_Int, but maybe I should just use nsStyleCoord / eStyleAnimType_Coord?

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: patch to enable these properties

(In reply to Daniel Holbert [:dholbert] from comment #0)
> Note: Patches attached here will be disabled by default, via #ifdef MOZ_FLEXBOX

Just to be clear, here's the patch that would be used to enable these properties via the MOZ_FLEXBOX build flag, and simultaneously uncomment the relevant non-#ifdeffable chunks of IDL and JS.

Note that this patch won't land as part of this bug, since we wouldn't actually want to make these properties visible to web content until we've got layout support for them.  I'm just posting it here to show how these properties would get enabled.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: patch to enable these properties

(sorry, posted before merging in the configure.in change. fixed now.)

Comment 6

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

Comment on attachment 568799 [details] [diff] [review]
patch 1 v2: parse/compute flex-align, flex-line-pack, flex-pack

r=dbaron, assuming you've run the mochitests with the property_database.js bits uncommented and they pass

Comment 7

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

(In reply to Daniel Holbert [:dholbert] from comment #3)
> Created attachment 568809 [details] [diff] [review] [diff] [details] [review]
> patch 2: parse/compute flex-order
> 
> This patch parses the flex-order property
> http://dev.w3.org/csswg/css3-flexbox/#flex-order0

Though it makes it take <integer> rather than <number> (floats).  I'm not sure if the spec author really meant <number>, though -- I think it's worth asking.


> There's one thing I'm unsure of in this patch -- the eStyleAnimType.
> I currently have eStyleAnimType_None, which is what we use for
> -moz-box-ordinal-group, but that would prevent us from doing CSS transitions.
> 
> I could use eStyleAnimType_Coord, but that would require me to use a
> nsStyleCoord to store this property, and that seems like overkill for this
> property (which just takes integer values -- unlike e.g. z-index, which is
> nearly the same except that it also accepts the value 'auto').
> 
> I have an XXXdholbert comment about possibly creating eStyleAnimType_Int,
> but maybe I should just use nsStyleCoord / eStyleAnimType_Coord?

I wouldn't use nsStyleCoord.  Either create an eStyleAnimType_Int32 or just use eStyleAnimType_Custom.  Probably the latter, since this is the only property of its type; if there's later another we can create a type for it.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to David Baron [:dbaron] from comment #6)
> Comment on attachment 568799 [details] [diff] [review] [diff] [details] [review]
> patch 1 v2: parse/compute flex-align, flex-line-pack, flex-pack
> 
> r=dbaron, assuming you've run the mochitests with the property_database.js
> bits uncommented and they pass

Yup, I have. Thanks!

(In reply to David Baron [:dbaron] from comment #7)
> Though it makes it take <integer> rather than <number> (floats).  I'm not
> sure if the spec author really meant <number>, though -- I think it's worth
> asking.

Oh, good point -- I hadn't noticed that.

It looks like the choice of <number> was deliberate, based on this message to www-style regarding an accidental (reverted) number-->integer change that happened at one point:
http://lists.w3.org/Archives/Public/www-style/2011Aug/0636.html

Comment 9

[Daniel Holbert [:dholbert]]

I brought up the flex-order <number> vs <integer> topic on www-style:
  http://lists.w3.org/Archives/Public/www-style/2011Oct/0829.html
and Tab also brought it up in today's CSS meeting.

The upshot of the meeting is:
   (1) being consistent with z-index is very important.
   (2) it'd be really nice if we could change z-index to take <number>
   (3) However, the csswg is hesitant to change z-index to take <number> because
       existing content could break/change. (due to float precision issues & also
       invalid z-index declarations that would suddenly become valid & affect rendering)

Hence, the flexbox spec is being changed to make flex-order take <integer>, bringing it into line with z-index. (matching what my patch here already does)

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: patch 2 v2: parse/compute flex-order

This patch makes flex-order use "eStyleAnimType_Custom" as suggested above (and adds a chunk to nsStyleAnimation.cpp in the Custom clause to pull out the value).

Now that we can transition this property, I've also added a test to test_transitions_per_property.html.  (I actually genericized the existing "test_zindex_transition" function to be purely about integers.  I removed its 'auto' chunk, since that already gets tested for 'z-index' by its other test method, "test_pos_integer_or_auto_transition".)

The only thing I'm not sure about in this patch is the nsStyleDisplay::CalcDiffererence hint.  Basically, we want to invalidate our geometry and that of our parent, but I'm not sure what the correct (mix of) style hints would be for that.  (I think it might be nsChangeHint_ClearAncestorIntrinsics, per my XXXdholbert comment there, but I'm not sure.)  So, advice on that would be much appreciated.

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: patch 3 v1: parse/compute flex-flow (without multiline suffixes)

This patch adds support for parsing/computing the "flex-flow" property.

It doesn't add support for the optional "wrap" / "wrap-reverse" suffixes yet, since those are for triggering multiline flexbox, and I don't intend to support that in the initial flexbox impl.  (that'll be in a followup bug)

With that simplification, flex-flow behaves as a simple single-keyword-valued property, just like e.g. flex-align, so that's how this patch implements it.

Relevant chunk of spec, for reference:
  http://dev.w3.org/csswg/css3-flexbox/#flex-flow0

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 568799 [details] [diff] [review]
patch 1 v2: parse/compute flex-align, flex-line-pack, flex-pack

Note that "flex-line-pack" is only relevant for multi-line flexboxes, as mentioned in the note at the end of http://dev.w3.org/csswg/css3-flexbox/#flex-line-pack0

Perhaps I should split off the "flex-line-pack" chunk of patch 1 here and move it to bug 702508 (which will add multi-line flexbox support), in case authors happen to use flex-line-pack as a feature-detection mechanism for multi-line flexbox support.

Comment 13

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

Comment on attachment 571221 [details] [diff] [review]
patch 2 v2: parse/compute flex-order

Maybe put a comment above #define NS_STYLE_FLEX_ORDER_INITIAL saying it's the initial value rather than a named constant?  It's a reasonable thing to do... but it's atypical.

Assuming the tests pass with the pseudo-commented stuff uncommented, r=dbaron

(Also, if you end up landing it uncommented, you'll need to bump the nsIDOMCSS2Properties IID.)

And in general, we shouldn't enable properties until we have layout support for them.  That might make landing this stuff fun.  (If we come up with a way to fix bug 698072, fixing the tests to allow ifdefing is relatively straightforward with the XUL preprocessor.)

And do test if the change hint you have is sufficient once you have the layout patch.

Comment 14

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

Comment on attachment 574208 [details] [diff] [review]
patch 3 v1: parse/compute flex-flow (without multiline suffixes)

This seems reasonable except that this property, in the current editor's draft, has now been turned into a shorthand.  So you should probably implement it that way... though your existing tests in property_database.js ought to be fine, except for the type line.

Given that you're only supporting half anyway, it might actually make more sense to just support flex-direction for now.

You could practically turn this into a patch for flex-direction with some search-replace.  That might make more sense than trying to support flex-flow as well.

(Also, all the comments for the previous patch apply here too.)

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to David Baron [:dbaron] from comment #14)
> Comment on attachment 574208 [details] [diff] [review] [diff] [details] [review]
> patch 3 v1: parse/compute flex-flow (without multiline suffixes)
> 
> This seems reasonable except that this property, in the current editor's
> draft, has now been turned into a shorthand.
[...]
> Given that you're only supporting half anyway, it might actually make more
> sense to just support flex-direction for now.

Makes sense -- thanks!

Just to be sure I don't lose it, here's the www-style post with the proposed spec change (not quite into the ED yet):  http://lists.w3.org/Archives/Public/www-style/2011Dec/0218.html

Comment 16

[Jesse Ruderman]

In attachment 568818 [details] [diff] [review], I think you forgot to rev the IDL's UUID.

Comment 17

[Daniel Holbert [:dholbert]]

Right you are, thanks!  I'll fix that in the updated patches (coming today I think).

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: patch 1 v3: parse/compute flex-pack

Here's an updated version of patch 1.

Notable changes since previous version:
 - This patch just adds support for 'flex-pack' now. (not the other two properties from previous "patch 1" versions here, because 'flex-line-pack' is for multiline & will come with bug 702508, and I'm thinking 'flex-align' may come as a followup to the main flexbox support patches as well)
 - The computed values of these properties now live in nsStylePosition instead of nsStyleDisplay, per IRL discussion w/ dbaron today
 - The spec has hadded one additional flex-pack value -- "distribute" -- which behaves like justify but with an additional 1/2-portion of packing-space on each edge. (This patch adds "distribute" to nsCSSKeywordList.h, since this is our first usage of that string as a CSS keyword.)

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: patch 2 v3: parse/compute flex-order

Updated version of patch 2.

Pretty similar to the previous version (which has r=dbaron). The differences are:
 - Moved to nsStylePosition instead of nsStyleDisplay
 - A few minor comment / test tweaks
 - Changes in contextual code from updates to patch 1 (per prev. comment)

Comment 20

[Daniel Holbert [:dholbert]]

(In reply to David Baron [:dbaron] from comment #13)
> Comment on attachment 571221 [details] [diff] [review]
> Maybe put a comment above #define NS_STYLE_FLEX_ORDER_INITIAL saying it's
> the initial value rather than a named constant?  It's a reasonable thing to
> do... but it's atypical.

Sorry, missed this comment -- I'll do this & repost patch 2.

Comment 21

[Daniel Holbert [:dholbert]]

Attachment: patch 2 v4: parse/compute flex-order

All right, here's patch 2 again, now with an added comment to address the feedback quoted above about NS_STYLE_FLEX_ORDER_INITIAL.

Comment 22

[Daniel Holbert [:dholbert]]

Attachment: patch 3 v2: parse/compute flex-direction

Here's an updated version of patch 3.

Differences from previous version (which was labeled 'flex-flow', & which you gave r- to in part because 'flex-flow' had just at that point just been split into 2 properties in the spec):
 - this is now for "flex-direction" component instead of the multipart "flex-flow" property
 - moved to nsStylePosition
 - contextual code changes from other patches

Comment 23

[Daniel Holbert [:dholbert]]

Attachment: patch 4 v1: parse/compute flex

This patch parses/computes the "flex" property, whose current spec is here:
 http://dev.w3.org/csswg/css3-flexbox/#flexibility

Note: this doesn't add the code for transitions/animations to work yet, but it does add a placeholder chunk in nsStyleAnimation::ExtractComputedValue with an XXX comment, and it simply makes that method fail right now (so that we effectively treat it as a non-transitionable property).

I'd initially tried to implement transitions using the existing nsStyleAnimation support for nsCSSValueTriplet, but that ended up being bad because the third ("Z") value in a nsCSSValueTriplet gets special treatment[1].  So, I need to do this using an array-backed value, which should be pretty straightforward, but I'm deferring on that for now in the interests of getting this landed.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleAnimation.cpp?rev=36c73ec83bb3&mark=1867-1869#1867

Comment 24

[Daniel Holbert [:dholbert]]

Attachment: patch to enable these properties, v2

Here's an updated version of the patch to enable these properties.

(revving the IID now, per comment 16 & parenthesized chunk of comment 13)

If you layer this on top of bug 666041's "patch 1" (which I just landed on m-i) and patches 1-4 here, then it'll produce a build that parses / computes all of these properties (but layout won't actually respond to them yet -- that will come on bug 666041).

I verified that the layout/style mochitests all pass both before & after this "enable these properties" patch is applied.

Comment 25

[Daniel Holbert [:dholbert]]

Comment on attachment 606810 [details] [diff] [review]
patch 2 v4: parse/compute flex-order

FWIW: note that fantasai has proposed renaming "flex-order" to "box-order", in this thread:
  http://lists.w3.org/Archives/Public/www-style/2012Feb/0741.html
  http://lists.w3.org/Archives/Public/www-style/2012Mar/0473.html

(I don't have strong feelings either way, & happily it doesn't affect the mechanics of this "flex-order" patch at all. If the change happens, we can easily switch using a search-and-replace on the code added by this patch.)

Comment 26

[Daniel Holbert [:dholbert]]

Attachment: patch 5 v1: code for transitions/animations on 'flex' property

(In reply to Daniel Holbert [:dholbert] from comment #23)
> Note: this doesn't add the code for transitions/animations to work yet, but
> it does add a placeholder chunk in nsStyleAnimation::ExtractComputedValue

This followup patch adds the nsStyleAnimation code for transitions on simple 'flex' values (with an absolute length value in the preferred size component).

Other preferred sizes which are hypothetically supportable but not supported in this patch:
  - calc(Npx) (easy to support)
  - "auto" for the preferred size (which translates to the computed value of the width or height property, depending on our flexbox's axis)
  - percentages (and calc w/ percentages) -- they're tricky in the same way as "auto", because we'd have to evaluate them against the computed value of 'width' or 'height' on our flexbox (if it's even got a fixed width or height).
  - intrinsic width keywords (though we should support those in simpler properties first, per bug 731886)

Comment 27

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

Comment on attachment 606776 [details] [diff] [review]
patch 1 v3: parse/compute flex-pack

Don't bother with the #ifdef in nsCSSKeywordList.h; it seems a
little dangerous to have #ifdefs in that list.

r=dbaron with that

Comment 28

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

Comment on attachment 606810 [details] [diff] [review]
patch 2 v4: parse/compute flex-order

r=dbaron

Comment 29

[Daniel Holbert [:dholbert]]

Comment on attachment 606922 [details] [diff] [review]
patch 4 v1: parse/compute flex

(Note that there's a discussion on www-style about splitting the "flex" property into 3 component properties (and converting 'flex' to a shorthand).
  http://lists.w3.org/Archives/Public/www-style/2012Apr/0713.html

Hence, switching r?dbaron to feedback?dbaron, since this patch will likely change after that's resolved)

Comment 30

[Daniel Holbert [:dholbert]]

Attachment: patch 1 v4: parse/compute 'justify-content' (formerly 'flex-pack')

Updating the 'flex-pack' patch to use the new property-name, 'justify-content', and to rename the old values 'justify' and 'distribute' to their new names, 'space-between' and 'space-around'.

Carrying forward r+, per IRL chat w/ dbaron, since this is just a string-replacement on the existing patch.

Comment 31

[Daniel Holbert [:dholbert]]

Attachment: patch 2 v5: parse/compute 'order' (formerly 'flex-order')

Similarly: renaming 'flex-order' to 'order'. (and rebasing on top of the updated patch 1)

Carrying forward r=dbaron.

Comment 32

[Daniel Holbert [:dholbert]]

Attachment: patch 3 v3: parse/compute 'flex-direction'

and here's an updated version of the patch for 'flex-direction' (just rebased on top of the new versions of patches 1 & 2)

This one still needs review.

Comment 33

[Daniel Holbert [:dholbert]]

Attachment: patch 4 v2: parse/compute 'align-items'

Here's the patch for the new "align-items" property (formerly named "flex-align").  Just a straightforward enum-valued property, like flex-direction & justify-content.

Comment 34

[Daniel Holbert [:dholbert]]

Attachment: patch 5 v1: parse/compute 'align-self'

Here's the patch for the new "align-self" property (formerly named "flex-item-align").

This is an enum-valued property, which takes the same values as 'align-items', plus one tricky value -- 'auto' -- which computes to the parent's computed "align-items" value.  (It's kind of like "inherit", but we inherit a different property.)  This is the initial value of the property, too.

I discussed this with bz a bit, and (at his suggestion) I ended up implementing this using a special enum value for 'auto', which we can then replace with the parent's "align-items" value wherever we actually care about it:  in layout code (over on bug 666041), in getComputedStyle (::DoGetAlignSelf()), and in inheritance (e.g. if an elem has 'align-self: inherit', its parent has 'align-self: auto', and its grandparent has 'align-items: baseline', then our elem should end up with computed 'align-self: baseline'.  That is -- it should inherit the computed value of 'auto', rather than 'auto' itself.)

Comment 35

[Daniel Holbert [:dholbert]]

Attachment: patch 6 v1: parse/compute 'flex-grow' & 'flex-shrink'

Comment 36

[Daniel Holbert [:dholbert]]

Comment on attachment 628976 [details] [diff] [review]
patch 6 v1: parse/compute 'flex-grow' & 'flex-shrink'

[gah, accidentally hit Enter key while typing patch name & posted it before filling out comment field]

This patch adds the 'flex-grow' and 'flex-shrink' properties. I'm bundling them into one patch because they have the exact same behavior -- they're non-negative float-valued properties which default to 1.0, and which have special transition behavior for transitions to/from "0". (in nsStyleAnimation.cpp)

Comment 37

[Daniel Holbert [:dholbert]]

Attachment: patch 7 v1: parse/compute 'flex-basis'

Here's the patch for flex-basis, without the magic for "auto" (making it compute to the computed value of width | height IFF we're a flex item). Hence, not requesting review on this one yet.

Comment 38

[Daniel Holbert [:dholbert]]

Attachment: patch 8 v1: parse/compute 'flex' shorthand

Here's the patch for the 'flex' shorthand.
  http://dev.w3.org/csswg/css3-flexbox/#flex
  http://dev.w3.org/csswg/css3-flexbox/#flex-examples

Comment 39

[Daniel Holbert [:dholbert]]

Attachment: patch 8 v2: parse/compute 'flex' shorthand

(sorry, forgot to qfold the property-database.js chunk into my 'flex' shorthand patch. added that now.)

Comment 40

[Daniel Holbert [:dholbert]]

Attachment: patch 1 v5: parse/compute 'justify-content' (formerly 'flex-pack') [r=dbaron]

(note: the Flexbox ED has recently changed s/start/flex-start/ and s/end/flex-end/ as values for the "justify-content", "align-items", and "align-self" properties. I've made those changes in my patch queue, but I won't re-post the entire stack here, to avoid needless patch-spam.)

Comment 41

[Daniel Holbert [:dholbert]]

(er -- I accidentally attached a new version of patch 1 with the trivial change discussed in comment 40. I obsoleted it now -- feel free to ignore it. :))

Comment 42

[Daniel Holbert [:dholbert]]

Comment on attachment 629994 [details] [diff] [review]
patch 1 v5: parse/compute 'justify-content' (formerly 'flex-pack') [r=dbaron]

(In reply to Daniel Holbert [:dholbert] from comment #40)
> (note: the Flexbox ED has recently changed s/start/flex-start/ and
> s/end/flex-end/ as values for the "justify-content", "align-items", and
> "align-self" properties. I've made those changes in my patch queue, but I
> won't re-post the entire stack here, to avoid needless patch-spam.)

OK -- a few more things changed in flexbox properties since comment 40, so I'm re-posting the stack of patches here. These are up-to-date as of the CSS Flexbox WD released yesterday:
 http://www.w3.org/TR/2012/WD-css3-flexbox-20120612/

(Nothing has changed in this first "justify-content" patch since I accidentally left it attached in comment 40, so I'm just un-hiding that updated patch-version and carrying forward r=dbaron)

Comment 43

[Daniel Holbert [:dholbert]]

Attachment: patch 2 v5a: parse/compute 'order' (formerly 'flex-order')

(just bitrot-fixes in this patch, from e.g. s/end/flex-end/ in contextual code)

Comment 44

[Daniel Holbert [:dholbert]]

Attachment: patch 3 v3a: parse/compute 'flex-direction'

(just fixes for bitrot in contextual code here, too)

Comment 45

[Daniel Holbert [:dholbert]]

Attachment: patch 4 v3: parse/compute 'align-items'

Updated "align-items" patch so that the property now takes "flex-start"/"flex-end" instead of "start"/"end", per the spec change mentioned in comment 40.

Comment 46

[Daniel Holbert [:dholbert]]

Attachment: patch 5 v2: parse/compute 'align-self'

(same here, for "align-self")

Comment 47

[Daniel Holbert [:dholbert]]

Attachment: patch 6 v2: parse/compute 'flex-grow' & 'flex-shrink'

Updated to make flex-grow default to 0. (plus contextual-code bitrot fix)

Comment 48

[Daniel Holbert [:dholbert]]

Attachment: patch 7 v2: parse/compute 'flex-basis'

...and here's a bitrot-fixed & ready-to-review version of the patch for flex-basis.

(In the time since I posted the previous WIP version on comment 37, the CSSWG decided that the magic I hinted at there isn't actually needed -- now, "flex-basis:auto" just computes to "auto", instead of magically computing to the height|width computed value.  Now layout, rather than the CSS cascade, will look up the computed height|width and use that as appropriate.)

Comment 49

[Daniel Holbert [:dholbert]]

Attachment: patch 8 v3: parse/compute 'flex' shorthand

...and here's an updated version of the flex shorthand patch.

(Diffs. vs version of this patch: cleaned up the parsing code a bit (incl. some spec updates) and added a mochitest "test_flexbox_flex_shorthand.html" that tries a bunch of shorthand values and verifies that they produce the expected computed values for flex-grow, flex-shrink, & flex-basis.)

Comment 50

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

Comment on attachment 632859 [details] [diff] [review]
patch 2 v5a: parse/compute 'order' (formerly 'flex-order')

I wonder a little bit whether we should be doing this prefixed or unprefixed, but I guess prefixed is ok.

I think you could also at least swap the order of the two MOZ_FLEXBOX bits in nsStylePosition::CalcDifference.  (NeedReflow is a bit within the ReflowFrame set-of-bits.)

r=dbaron

Comment 51

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

Comment on attachment 632861 [details] [diff] [review]
patch 3 v3a: parse/compute 'flex-direction'

I think it's better not to bother with ifdefs in nsCSSKeywordList.h

r=dbaron

Comment 52

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

Comment on attachment 632862 [details] [diff] [review]
patch 4 v3: parse/compute 'align-items'

r=dbaron

Comment 53

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

Comment on attachment 632868 [details] [diff] [review]
patch 6 v2: parse/compute 'flex-grow' & 'flex-shrink'

This doesn't quite get the animation behavior right, since animations from 0 to 0 are valid animations.  (With CSS animations, you can tell -- an animation that says it should animate from 0 to 0 should do something.)  There should also be tests for the not-0 animation behavior.

Alternatively, we could try to get the spec changed to say that animations involving 0 values are not animatable.  That's fine too.

But either way you need tests for the animation behavior.


Otherwise this looks fine.

r=dbaron conditional on getting the spec changed; otherwise I want to take a look again at the revised animation code

Comment 54

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

Comment on attachment 633275 [details] [diff] [review]
patch 7 v2: parse/compute 'flex-basis'

r=dbaron, though we might want to go back to the nsDOMComputedStyle comment at some point.

Comment 55

[Daniel Holbert [:dholbert]]

(In reply to David Baron [:dbaron] from comment #53)
> Comment on attachment 632868 [details] [diff] [review]
> patch 6 v2: parse/compute 'flex-grow' & 'flex-shrink'
> 
> This doesn't quite get the animation behavior right, since animations from 0
> to 0 are valid animations.  (With CSS animations, you can tell -- an
> animation that says it should animate from 0 to 0 should do something.) 
> There should also be tests for the not-0 animation behavior.
> 
> Alternatively, we could try to get the spec changed to say that animations
> involving 0 values are not animatable.  That's fine too.

IIUC, that's what the spec already says:

  # 7.3.1. The ‘flex-grow’ property
  [...]
  # Animatable: 	yes, except between ‘0’ and other values 

(same for flex-shrink)

Perhaps I'm misunderstanding you, though?

Comment 56

[Daniel Holbert [:dholbert]]

Oh, sorry, disregard comment 55 -- I missed the "from 0 to 0"  (I inserted an "and" into that sentence when I read it)

Comment 57

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

Comment on attachment 632864 [details] [diff] [review]
patch 5 v2: parse/compute 'align-self'

I *think* you're better off adding an additional keyword table rather than using eCSSUnit_Auto in the specified style (nsCSSValue) representation and VARIANT_AUTO for the parser.  But I could be wrong there.  (I think we mostly tend to avoid using VARIANT_AUTO / VARIANT_NORMAL / VARIANT_NONE for properties that take other keywords too.)  But it might be better the way it is too -- if it looks like it's going to make things more complicated, don't do it.


I think the code in nsRuleNode that handles the special inheritance behavior should actually handle the inherit value and make |SetDiscrete| an else.  That's the normal pattern in nsRuleNode and it's less confusing if it doesn't vary.  (See, say, font-weight or text-align.)

You should have a comment explaining in a little more detail why you put 'auto' in nsStylePosition even though it's not a computed value:  it's because it's the initial value, and if you computed it every time you wouldn't be able to store any nsStylePosition in the rule tree, ever.

You should probably also document that your dependency on the parent style context is safe by asserting that canStoreInRuleTree is already false (which it should be because you're handling an 'inherit' value).

r=dbaron with those things fixed, though depending on the changes it might make sense for me to take another look at the patch

Comment 58

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

Comment on attachment 634142 [details] [diff] [review]
patch 8 v3: parse/compute 'flex' shorthand


CSSParserImpl::ParseFlex


>+    // 'inherit' and 'initial' must be alone
>+    if (!ExpectEndProperty()) {
>+      return false;
>+    }

You don't need this check.  The caller will look for extra garbage at the end.
Just remove these 4 lines.

>+    // 'none' must be alone
>+    if (!ExpectEndProperty()) {
>+      return false;
>+    }

Same here.

>+    NS_ABORT_IF_FALSE(eCSSUnit_Inherit == tmpVal.GetUnit() ||
>+                      eCSSUnit_Initial == tmpVal.GetUnit(),
>+                      "We only looked for inherit & initial, but we "
>+                      "succeeded by getting something else...?");

I'd also skip this, just because we may at some point add another
value like 'inherit' and 'initial' for which this would work fine.

>+  // To keep track of what components we've parsed:
>+  bool gotFlexGrow = false;
>+  bool gotFlexShrink = false;
>+  bool gotFlexBasis = false;

I think everything from here to the end of the function should be
restructured.  There's a bunch of unneeded complexity here, and I think
it doesn't quite match the spec (e.g., in allowing flex-grow and
flex-shrink to be separated).

In particular, I think you should take the approach of:

 * look for a valid flex-basis value, and if you find one
   make it the flex-basis value and remember that you found one

 * look for a valid flex-grow value, and if you found one:
   * record it, and remember that you found it
   * look for a valid flex-shrink value
     * if found, record it

 * if you found neither flex-basis nor flex-grow, fail

 * if you did not find flex-basis earlier, look again for flex-basis, and
   record it

Also, your ParseFlexShorthandComponent could be simplified into
a single call to ParseNonNegativeVariant by |ing VARIANT_NUMBER
into the second mask.  (ParseVariant intentionally does the right
thing for unitless 0 there.)  But I think the above should lead to
changing that structure.

(At some point in the future, I'd like to make ParseSingleValueProperty
not accept initial/inherit for anything, so that you could use
it more easily in cases like this... though this case is special because
of the need to not accept unitless zero for flex-basis.)


I'd like to have another look at this after the parser changes.

Comment 59

[Daniel Holbert [:dholbert]]

Attachment: patch 6 v3: parse/compute 'flex-grow' & 'flex-shrink'

(In reply to David Baron [:dbaron] from comment #53)
> Comment on attachment 632868 [details] [diff] [review]
> patch 6 v2: parse/compute 'flex-grow' & 'flex-shrink'
> 
> This doesn't quite get the animation behavior right, since animations from 0
> to 0 are valid animations.  (With CSS animations, you can tell

Thanks for catching that!  I addressed this by making flex-grow & flex-shrink standard "eStyleAnimType_float" properties (instead of _Custom), so they'll now unconditionally be extracted and stored in a float-typed nsStyleAnimation::Value.  Then, I added a special case to AddWeighted and ComputeDistance to check for those properties and fail on animations when exactly one of the endpoints is 0.

> There should also be tests for the not-0 animation behavior.

I've added a mochitest to test CSS animations on flex-grow & flex-shrink, with animations...
 from 0 to 1 (shouldn't work)
 from 1 to 0 (shouldn't work)
 from 0 to 0 (should work)
 from 2 to 3 (should work)

That mochitest passes after I enable MOZ_FLEXBOX and uncomment the corresponding chunks of JS / IDL.  (I verified that the 0-to-0 animation fails with the previous patch-revision, too.)

> r=dbaron conditional on getting the spec changed; otherwise I want to take a
> look again at the revised animation code

Thanks -- re-requesting review, since I went the change-the-code route.

Comment 60

[Daniel Holbert [:dholbert]]

Attachment: patch 8 v4: parse/compute 'flex' shorthand

(In reply to David Baron [:dbaron] from comment #58)
> You don't need this check.  The caller will look for extra garbage at the
> end.

Thanks! Done. (I could've sworn I needed that at some point...)

> I'd also skip this, just because we may at some point add another
> value like 'inherit' and 'initial' for which this would work fine.

Good point -- removed that assertion.

> I think everything from here to the end of the function should be
> restructured.  There's a bunch of unneeded complexity here

Yeah -- that complexity is largely because of the "unitless zero isn't allowed as a flex-basis" requirement, and so I was trying to pre-screen each component to see if it smelled like a number before letting us parse it as a length.

> I think
> it doesn't quite match the spec (e.g., in allowing flex-grow and
> flex-shrink to be separated).

(FWIW, the old patch did at least match the spec on that point -- it'd reject separated flex-grow & flex-shrink values (e.g. "flex: 2 50px 3"), though in a somewhat non-obvious way.  Whenever it got a flexBasis and had previously parsed flexGrow, it would assume that we were done. So, any separated flex-shrink value would be still remaining un-parsed & would be treated as garbage at the end & make us fail.)

> In particular, I think you should take the approach of:
> 
>  * look for a valid flex-basis value, and if you find one
>    make it the flex-basis value

It's slightly trickier than that, because we have to refuse to accept unitless 0 as a flex-basis, and there's no direct way to do that except by screening the first component to see if it's a number.

But with your suggested |'ing VARIANT_NUMBER tweak, I can parse the first component as a potential flex-basis and flex-grow *simultaneously* (rather than pre-screening it), which simplifies things a bit.  (That's probably what you had in mind.)

So with that tweak, I've restructured the code along the lines of your suggested logic -- it's still a bit complex, but I think it's an improvement.  Here's an overview of the strategy (quoting a comment from the new patch):
  // a) Parse the first component as either flex-basis or flex-grow.
  // b) If it wasn't flex-grow, parse the _next_ component as flex-grow.
  // c) Now we've just parsed flex-grow -- so try parsing the next thing as
  //    flex-shrink.
  // d) Finally: If we didn't get flex-basis at the beginning, try to parse
  //    it now, at the end.

Thanks again!

Comment 61

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

Comment on attachment 638127 [details] [diff] [review]
patch 6 v3: parse/compute 'flex-grow' & 'flex-shrink'

>+      if ((aProperty == eCSSProperty_flex_grow ||
>+          aProperty == eCSSProperty_flex_shrink) &&
>+          (aValue1.GetFloatValue() == 0.0f ||
>+           aValue2.GetFloatValue() == 0.0f) &&
>+          aValue1.GetFloatValue() != aValue2.GetFloatValue()) {

The second line above needs one more space of indent.

Also a little annoying that you reduced the amount of context in your diff since the last patch (made diffing diffs harder; also not enough context).

r=dbaron

Comment 62

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

(In reply to David Baron [:dbaron] from comment #61)
> Comment on attachment 638127 [details] [diff] [review]
> patch 6 v3: parse/compute 'flex-grow' & 'flex-shrink'
> 
> >+      if ((aProperty == eCSSProperty_flex_grow ||
> >+          aProperty == eCSSProperty_flex_shrink) &&
> >+          (aValue1.GetFloatValue() == 0.0f ||
> >+           aValue2.GetFloatValue() == 0.0f) &&
> >+          aValue1.GetFloatValue() != aValue2.GetFloatValue()) {
> 
> The second line above needs one more space of indent.

(happens twice)

Comment 63

[Daniel Holbert [:dholbert]]

(In reply to David Baron [:dbaron] from comment #61)
> The second line above needs one more space of indent.

Fixed in patch queue (both instances).  Thanks!

> Also a little annoying that you reduced the amount of context in your diff
> since the last patch (made diffing diffs harder; also not enough context).

Oops! Sorry for any pain that caused.  I did that to minimize the qpush-pain after making tweaks, since these patches share lots of contextual lines.  I should've qref'd with increased lines before reposting, though -- I'll do that for the updated align-self patch before reposting it (in a minute).

Comment 64

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

Comment on attachment 638160 [details] [diff] [review]
patch 8 v4: parse/compute 'flex' shorthand

>+    // d) Finally: If we didn't get flex-basis at the beginning, try to parse
>+    //    it now, at the end.
>+    if (!wasFirstComponentFlexBasis &&
>+        ParseNonNegativeVariant(tmpVal, variantMask,
>+                                nsCSSProps::kWidthKTable)) {
>+      if (tmpVal.GetUnit() == eCSSUnit_Number) {
>+        // This is where we reject "0 0 0" (which the spec says is invalid,
>+        // because we must reject unitless 0 as a flex-basis value).
>+        return false;
>+      }

You should add an explicit comment here that not passing VARIANT_NUMBER
wouldn't do this correctly, since then we'd parse '0' as a length.



Also, please add comments to the nsCSSPropList.h entries for all
three of flex-basis, flex-grow, and flex-shrink (near their VARIANT_*
lines) saying that the shorthand parsing for 'flex' has its own code
and doesn't depend on the longhand parsing.

r=dbaron with that

Comment 65

[Daniel Holbert [:dholbert]]

(In reply to David Baron [:dbaron] from comment #64)
> You should add an explicit comment here that not passing VARIANT_NUMBER
> wouldn't do this correctly, since then we'd parse '0' as a length.

Added comment in patch-queue:
https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/69ff99f7d425#l1.114
(ignore the rest of that cset -- it's just a -U3 --> -U8 lines-of-context increase)

> Also, please add comments to the nsCSSPropList.h entries for all
> three of flex-basis, flex-grow, and flex-shrink (near their VARIANT_*
> lines) saying that the shorthand parsing for 'flex' has its own code
> and doesn't depend on the longhand parsing.

Added comments in patch-queue:
https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/c742d7f95b88#l1.31

> r=dbaron with that

Thanks!

Comment 66

[Daniel Holbert [:dholbert]]

Attachment: patch 5 v3: parse/compute 'align-self'

(In reply to David Baron [:dbaron] from comment #57)
> I *think* you're better off adding an additional keyword table rather than
> using eCSSUnit_Auto  [...] But it might be better the way it
> is too -- if it looks like it's going to make things more complicated, don't
> do it.

Thanks -- I agree that things are simpler with that change (or at least, not significantly more complicated), so I went ahead and changed this.

> I think the code in nsRuleNode that handles the special inheritance behavior
> should actually handle the inherit value and make |SetDiscrete| an else. 
> That's the normal pattern in nsRuleNode and it's less confusing if it
> doesn't vary.  (See, say, font-weight or text-align.)

Cool -- fixed here.

> You should have a comment explaining in a little more detail why you put
> 'auto' in nsStylePosition even though it's not a computed value: it's
> because it's the initial value, and if you computed it every time you
> wouldn't be able to store any nsStylePosition in the rule tree, ever.

Added a comment explaining that, in ComputeStylePosition() 

> You should probably also document that your dependency on the parent style
> context is safe by asserting that canStoreInRuleTree is already false

(I'm assuming you meant for this assertion to happen next to the existing aContext->GetParent() assertion, after we've detected that pos and parentPos are distinct -- let me know if that's not the case.)

So -- AFAICT, we don't necessarily know that canStoreInRuleTree is always false there.  I tried adding an assertion, and it fails.  See below...

> (which
> it should be because you're handling an 'inherit' value).

So, I think you'd be right about that if nsStylePosition used COMPUTE_START_INHERITED, which sets canStoreInRuleTree when it sees inherited stuff, here:
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsRuleNode.cpp?mark=2245-2245,2268-2271#2245
...but we're instead using COMPUTE_START_RESET, which AFAICT does *not* directly set canStoreInRuleTree to false.  It does, however, set "parentdata_" to our actual parent computed-style data (parentPos in this case) if we've got an "inherit" value in play, so we *can* reason based on parentPos. (as my patch does)

(AFAICT, with COMPUTE_START_RESET, canStoreInRuleTree only gets set to false when we call SetDiscrete() for an "inherit" value (or we do the property-specific equivalent of that.))

Comment 67

[Daniel Holbert [:dholbert]]

(I also added a mochitest to the align-self patch, to test the special behavior of "align-self: auto", especially as it interacts with inheritance.  When I said in the previous bug-comment "I tried adding an assertion, and it fails" -- it fails on this mochitest when we have "align-self:inherit" on a node and  "align-self:auto" on its parent -- e.g. in the first chunk of testNodeThatHasGrandparent().)

Comment 68

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

Comment on attachment 638571 [details] [diff] [review]
patch 5 v3: parse/compute 'align-self'

r=dbaron

Comment 69

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

Comment on attachment 638571 [details] [diff] [review]
patch 5 v3: parse/compute 'align-self'


+    // NOTE: This needs to match what SetDiscrete does for eCSSUnit_Inherit:

I don't think this comment is needed.


+  } else
+    SetDiscrete(*aRuleData->ValueForAlignSelf(),
+                pos->mAlignSelf, canStoreInRuleTree,
+                SETDSC_ENUMERATED,
+                parentPos->mAlignSelf, // (unused -- we handled inherit above)
+                NS_STYLE_ALIGN_SELF_AUTO, // initial == auto
+                NS_STYLE_ALIGN_SELF_AUTO, // auto == auto
+                0, 0, 0);

You don't need the auto entry here; just use 0 for it as well.

Also, please brace the call to SetDiscrete

r=dbaron with that

Comment 70

[Daniel Holbert [:dholbert]]

(In reply to David Baron [:dbaron] from comment #69)
> +    // NOTE: This needs to match what SetDiscrete does for eCSSUnit_Inherit:
> 
> I don't think this comment is needed.

Fixed.

> You don't need the auto entry here; just use 0 for it as well.

Ah right -- missed that when changing "auto" to an enumerated value. Fixed.

> Also, please brace the call to SetDiscrete

Fixed.

Above fixes are in this patch-queue commit:
https://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/rev/7ac01e6eacdf

Comment 71

[Daniel Holbert [:dholbert]]

Landed patch 1 - patch 8:
https://hg.mozilla.org/integration/mozilla-inbound/rev/15b88a02abe2
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b82cae00864
https://hg.mozilla.org/integration/mozilla-inbound/rev/499c380b84f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f84743f59a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/08dc24b4a21c
https://hg.mozilla.org/integration/mozilla-inbound/rev/10d1162cd4e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/57126745d462
https://hg.mozilla.org/integration/mozilla-inbound/rev/a112868224d1

This includes the review comments from comment 50 (in 0b82cae00864) & comment 51 (in 0b82cae00864) -- AFAIK, those are the only fixes that I didn't explicitly respond to with a comment here, since they were minor.

Comment 72

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/15b88a02abe2
https://hg.mozilla.org/mozilla-central/rev/0b82cae00864
https://hg.mozilla.org/mozilla-central/rev/499c380b84f8
https://hg.mozilla.org/mozilla-central/rev/7f84743f59a1
https://hg.mozilla.org/mozilla-central/rev/08dc24b4a21c
https://hg.mozilla.org/mozilla-central/rev/10d1162cd4e1
https://hg.mozilla.org/mozilla-central/rev/57126745d462
https://hg.mozilla.org/mozilla-central/rev/a112868224d1

Comment 73

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #71)
> This includes the review comments from comment 50 (in 0b82cae00864) &
> comment 51 (in 0b82cae00864)

(er, copy-paste-fail -- pasted the same cset twice. comment 51 was addressed in 499c380b84f8 )

Comment 74

[Daniel Holbert [:dholbert]]

Attachment: patch 9: trivial s/_TEST_FILES/MOCHITEST_FILES/

Comment 75

[Daniel Holbert [:dholbert]]

(Patch 9 is a followup to accommodate changes from  bug 370750, which renamed _TEST_FILES to MOCHITEST_FILES globally in mochitest Makefiles. It landed after this bug's patches, but it missed the "_TEST_FILES +=" chunk added in this bug's patches.)

Comment 76

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

Comment on attachment 641149 [details] [diff] [review]
patch 9: trivial s/_TEST_FILES/MOCHITEST_FILES/

r=dbaron

Comment 77

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b65b8500bf89

Comment 78

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b65b8500bf89

Comment 79

[Jean-Yves Perrier [:teoli]]

I don't think we should document this as supported before we have the layout part (I let dev-doc-needed to keep track of this). It would only confuse Web devs that would think it can already be used.

Comment 80

[Daniel Holbert [:dholbert]]

Agreed. (It's also disabled in builds right now, for the same reason.)

Comment 81

[Jean-Yves Perrier [:teoli]]

Flexbox are documented: see https://bugzilla.mozilla.org/show_bug.cgi?id=666041#c132