Mats Palmgren (inactive) Assignee
	Description • 8 years ago

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.

	Mats Palmgren (inactive) Assignee
	Comment 3 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=de8117b787f03ace92289055ba30b413f427e952&exclusion_profile=false

	Daniel Holbert [:dholbert]
	Comment 4 • 8 years ago

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

	yisibl
	Comment 5 • 8 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 6 • 8 years ago

> (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...

	Mats Palmgren (inactive) Assignee
	Comment 7 • 8 years ago

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.

	Mats Palmgren (inactive) Assignee
	Comment 8 • 8 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 10 • 8 years ago

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

	Daniel Holbert [:dholbert]
	Comment 11 • 8 years ago

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")

	Daniel Holbert [:dholbert]
	Comment 12 • 8 years ago

(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.)

	Pulsebot
	Comment 13 • 8 years ago

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.

	Pulsebot
	Comment 14 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 15 • 8 years ago
bugherder

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

	Mats Palmgren (inactive) Assignee
	Comment 16 • 8 years ago

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.

	Mats Palmgren (inactive) Assignee
	Comment 17 • 8 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 18 • 8 years ago

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

	Julien Cristau [:jcristau]
	Comment 19 • 8 years ago

Comment on attachment 8814454 [details] [diff] [review]
Implement the place-items/self/content shorthands.

part of css grid feature for aurora52

	Carsten Book [:Tomcat]
	Comment 20 • 8 years ago
bugherder uplift

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

	Jean-Yves Perrier [:teoli]
	Comment 21 • 7 years ago

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