https://github.com/w3c/csswg-drafts/issues/595 It would be nice to have these shorthands for the Grid release. It looks fairly simple to implement.

Comment on attachment 8813931 [details] [diff] [review] Implement the place-items/self/content shorthands. Review of attachment 8813931 [details] [diff] [review]: ----------------------------------------------------------------- Partial review, from a quick American-Thanksgiving-night skim of the patch: ::: layout/style/nsCSSParser.cpp @@ +10208,5 @@ > AppendValue(aPropID, value); > return true; > } > > +// place-content: <align-content> [ <justify-content> ] ? I don't think the square-brackets serve a purpose here -- square-brackets are used for grouping, per https://drafts.csswg.org/css-values-4/#comb-one and the "group" just has one thing inside it here. I think this should just be: // place-content: <align-content> <justify-content>? (Same goes for the subsequent comment on place-self / place-items.) Or alternately, use the grammar from the spec text (which is more verbose, and doesn't quite match this grammar). @@ +10251,5 @@ > + return false; > + } > + AppendValue(eCSSProperty_align_items, first); > + nsCSSValue second; > + // Note: 'auto' is valid for justify-items, but not align-items. Hmm, the spec doesn't currently cover the bit of nuance that you're describing in this comment (and implementing just below it). Right now it says: # Name: place-content # Value: [ auto | normal | stretch | <baseline-position> | <self-position> ]{1,2} ...which is saying "auto" and "auto auto" are both fine. Link: https://drafts.csswg.org/css-align/#propdef-place-content Please file a spec issue to get clarification on what's intended here... ::: layout/style/test/property_database.js @@ +4566,5 @@ > + type: CSS_TYPE_TRUE_SHORTHAND, > + subproperties: [ "align-content", "justify-content" ], > + initial_values: [ "normal" ], > + other_values: [ "normal start", "end baseline", "end end", > + "space-between flex-end", " last baseline start", Two nits: (1) you've got a stray space between the open-quote and "last". Not a big deal, but I'm guessing it's by accident, since I'm pretty sure we have dedicated tests for testing stuff like that. (But it's fine if you want to leave it too.) (2) Is "last baseline start" really meant to be an acceptable value? On the github issue, jensimmons' recent summary of the consensus (later confirmed by fantasai) is: # place-content: <'align-content'> <'justify-content'>; # [...] # where only single-keyword values are allowed. https://github.com/w3c/csswg-drafts/issues/595#issuecomment-262584268 Notably, "last baseline" is NOT a single-keyword value. So I think this needs to be moved to "invalid_values", and we need to update the parsing logic accordingly. (Or, if you prefer, we need to convince the CSSWG to allow "last|first" as an exception to this single-keyword restriction.) (I don't immediately see that the single-keyword restriction made it into the spec text, actually -- I'm not sure if that's an oversight or if "single keyword" was meant to exclude things like fallbacks & safe|unsafe.) @@ +4567,5 @@ > + subproperties: [ "align-content", "justify-content" ], > + initial_values: [ "normal" ], > + other_values: [ "normal start", "end baseline", "end end", > + "space-between flex-end", " last baseline start", > + "space-evenly", "flex-start", "end", "left" ], nit: there's a redundant space character towards the end of this line -- between "end," and "left". @@ +4590,5 @@ > + subproperties: [ "align-self", "justify-self" ], > + initial_values: [ "auto" ], > + other_values: [ "normal start", "end first baseline", "end auto", > + "end", "right", "normal", "baseline", "start baseline", > + "left self-end", "last baseline start", "stretch" ], As above, there's a redundant space before the final value here. ::: layout/style/test/test_align_shorthand_serialization.html @@ +13,5 @@ > +<script> > + > +var initial_values = { > + alignContent: "normal", > + alignItems: "auto", This line is incorrect -- align-items does not have "auto" as its initial value. The correct initial value is "normal". https://drafts.csswg.org/css-align/#align-items-property https://dxr.mozilla.org/mozilla-central/rev/bad312aefb42982f492ad2cf36f4c6c3d698f4f7/layout/style/test/property_database.js#4497-4501

Spec: https://drafts.csswg.org/css-align/#propdef-place-content https://drafts.csswg.org/css-align/#propdef-place-items https://drafts.csswg.org/css-align/#place-self-property

> (Or, if you prefer, we need to convince the CSSWG to allow "last|first" as an exception > to this single-keyword restriction.) Yes, "last|first baseline" should definitely be allowed. I mentioned this in: https://github.com/w3c/csswg-drafts/issues/595#issuecomment-257620722 and no one objected so I think that's what they really mean. I'll file a github issue on this, and the 'auto' error...

The spec (now) actually says: "The first value is assigned to align-items. The second value is assigned to justify-items; if omitted, it is copied from the first value." https://drafts.csswg.org/css-align/#place-items-property not "first keyword", so I think we're good. And the <baseline-position> links to https://drafts.csswg.org/css-align/#typedef-baseline-position "<baseline-position> = [ first | last ]? baseline" so I think it's unambiguously clear that "last|first baseline" counts as a single *value*. I see that I used "single keyword" myself in the code comment -- I'll fix that so that it says "single value" instead.

github issue for the place-items 'auto' grammar error: https://github.com/w3c/csswg-drafts/issues/763

The place-items 'auto' grammar error has now been fixed in the spec: https://github.com/w3c/csswg-drafts/commit/6f0ffd39d8297358ee454062a08401f862e52b1c

Comment on attachment 8814454 [details] [diff] [review] Implement the place-items/self/content shorthands. Review of attachment 8814454 [details] [diff] [review]: ----------------------------------------------------------------- r=me, just a few nits: ::: layout/style/Declaration.cpp @@ +1468,5 @@ > + case eCSSUnit_Initial: > + case eCSSUnit_Unset: > + return true; > + case eCSSUnit_Enumerated: > + return aValue.GetIntValue() <= NS_STYLE_JUSTIFY_SPACE_EVENLY; Could you add a comment to clarify what you're checking with this "<=" comparison here? (I think you're testing whether there's another value / <overflow-position> encoded in the high bits, but I'm not sure.) @@ +1470,5 @@ > + return true; > + case eCSSUnit_Enumerated: > + return aValue.GetIntValue() <= NS_STYLE_JUSTIFY_SPACE_EVENLY; > + default: > + return false; Is this 'default' case expected to be reachable? I suspect it's not -- maybe add: MOZ_ASSERT_UNREACHABLE("Unexpected unit for css-align property val"); or something like that here, to make that expectation clearer (& help us keep this up to date if we happen to add a new type of unit) @@ +1473,5 @@ > + default: > + return false; > + } > + }; > + // Both values must be a single value (i.e. no fallback value and no s/Both values/Each longhand value/ (This current phrasing is really easy to misinterpret as "The two values must be the same")

(In reply to Daniel Holbert [:dholbert] from comment #11) > s/Both values/Each longhand value/ > > (This current phrasing is really easy to misinterpret as "The two values > must be the same") (A bit more explanation, in case this doesn't make sense to a non-native-English-speaking ear: "Each" unambiguously implies "each of them, individually/independently" -- whereas "both" can meant that, or it can mean "both of them, together/collectively". You want the first meaning, so "Each" is a more precise way of expressing that.)

Pushed by mpalmgren@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/07ecf6f5e0ab [css-align][css-grid][css-flexbox] Implement the place-items/self/content shorthands. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/8a3c0687c948 [css-grid] Sprinkle some place-items/self/content shorthands around in the Grid reftests to increase test coverage.

Pushed by mpalmgren@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/dad211727cb3 (followup) - Add the place-content/items/self shorthands to devtools. r=bustage

https://hg.mozilla.org/mozilla-central/rev/07ecf6f5e0ab https://hg.mozilla.org/mozilla-central/rev/8a3c0687c948 https://hg.mozilla.org/mozilla-central/rev/dad211727cb3

Comment on attachment 8814454 [details] [diff] [review] Implement the place-items/self/content shorthands. Approval Request Comment [Feature/regressing bug #]: CSS Grid [User impact if declined]: none, but CSS authors have expressed a strong desire to have these shorthands so it would be nice have these available with the release of Grid [Describe test coverage new/current, TreeHerder]: have reftests, all tests pass [Risks and why]: low risk [String/UUID change made/needed]: none This is part of the CSS Grid release, please uplift to Aurora 52, thanks.

That is, we should uplift all three changesets in comment 13 + comment 14.

Julien, can you help with the Aurora approval here please?

Comment on attachment 8814454 [details] [diff] [review] Implement the place-items/self/content shorthands. part of css grid feature for aurora52

https://hg.mozilla.org/releases/mozilla-aurora/rev/d62dd8718f59 https://hg.mozilla.org/releases/mozilla-aurora/rev/32d412d71906 https://hg.mozilla.org/releases/mozilla-aurora/rev/75ff11d338b2

https://developer.mozilla.org/en-US/docs/Web/CSS/place-self https://developer.mozilla.org/en-US/docs/Web/CSS/place-items https://developer.mozilla.org/en-US/docs/Web/CSS/place-content