Mats Palmgren (inactive) Assignee
	Description • 7 years ago

We should drop the "grid-" prefix from the grid-gap, grid-row-gap,
and grid-column-gap properties, and keep the old names as aliases
for web-compat.

https://logs.csswg.org/irc.w3.org/css/2017-08-04/#e847329
"Resolved: column-gap and row-gap apply to flex, grid, and multicol"

https://github.com/w3c/csswg-drafts/issues/1696

These are now defined in the Box Alignment spec:
https://www.w3.org/TR/css-align-3/#gaps

with specific Grid text at:
https://drafts.csswg.org/css-grid/#gutters

(I haven't checked if there are any functional changes besides
the renaming.  I'll file separate bugs for flexbox and multicol.)

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

I filed bug 1398483 for flexbox layout.

We already support 'column-gap' in multi-column layout and css-align
currently says "'row-gap' does not currently apply" for multicol.

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

I noticed that we don't support <percentage> for the existing
'column-gap' (multicol) property:
http://searchfox.org/mozilla-central/rev/d29c832536839b60a9ee5b512eeb934274b879b0/layout/style/nsCSSPropList.h#1513
but we do for 'grid-column-gap':
http://searchfox.org/mozilla-central/rev/d29c832536839b60a9ee5b512eeb934274b879b0/layout/style/nsCSSPropList.h#2183
so I filed bug 1398537 to add that for multicol.

	Manuel Rego Casasnovas
	Comment 3 • 6 years ago

JFYI, this has just been implemented in Blink and WebKit.

Some WPT tests created for those patches that might be useful for you too:
http://w3c-test.org/css/css-align/gaps/

	Emilio Cobos Álvarez (:emilio)
	Comment 4 • 6 years ago

(In reply to Manuel Rego Casasnovas from comment #3)
> JFYI, this has just been implemented in Blink and WebKit.
> 
> Some WPT tests created for those patches that might be useful for you too:
> http://w3c-test.org/css/css-align/gaps/

Thanks!

	Emilio Cobos Álvarez (:emilio)
	Comment 5 • 6 years ago

Mats, should we do this without waiting for bug 1398537?

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

I have patches for both.  I was mostly just waiting for the CSSWG
to make its mind up about percentages ([1][2] etc) but I think we
should just do what the spec currently says, i.e. resolve
calc(Npx + M%) as Npx when the percentage basis is indefinite.
I'll fix some of that here, but mostly in bug 1434478.

[1] https://github.com/w3c/csswg-drafts/issues/2297
[2] https://github.com/w3c/csswg-drafts/issues/1132

	Emilio Cobos Álvarez (:emilio)
	Comment 7 • 6 years ago

Ok, that sounds great! Let me know if I can help somehow :)

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

Comment on attachment 8962213 [details] [diff] [review]
part 2 - [css-grid][css-flexbox][css-multicol] Add 'row-gap' and 'gap' properties; make 'grid-[column|row]-gap' and 'grid-gap' alias the respective unprefixed properties (Gecko part)

A couple of notes:
I'm moving mColumnGap from the multicol-specific values in
nsStyleColumn to nsStylePosition because I think it makes more
sense now that it's shared with grid/flexbox.

BackComputedIntrinsicSize will be removed (or rewritten) in
bug 1434478 so it doesn't matter what it looks like here as
long as it pass tests.

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dded947fddd5decff2023d24d3ffa932eef7bfb2

	Daniel Holbert [:dholbert]
	Comment 14 • 6 years ago

Comment on attachment 8962212 [details] [diff] [review]
part 1 - [css-grid][css-flexbox][css-multicol] Add 'row-gap' and 'gap' properties; make 'grid-[column|row]-gap' and 'grid-gap' alias the respective unprefixed properties (Stylo part)

Review of attachment 8962212 [details] [diff] [review]:
-----------------------------------------------------------------

As in bug 1398537, I'd like to have emilio (or xidorn) review the rust parts --> punting review on part 1. One thing I noticed, though:

::: servo/components/style/properties/longhand/position.mako.rs
@@ +361,5 @@
> +${helpers.predefined_type("row-gap",
> +                          "length::NonNegativeLengthOrPercentageOrNormal",
> +                          "Either::Second(Normal)",
> +                          alias="grid-row-gap",
> +                          servo_pref="layout.columns.enabled",

Is there a reason we're tying "row-gap" to layout.columns.enabled?  The spec says it "does not currently apply" in multicol containers.

(Also, do we even have to tie column-gap to that pref, a few lines further up? I can see why we might've done so in the past when it was a multicol-only property, but now that it's defined for several layout modes [including flex which IIRC servo has some support for], it seems odd to make it conditional on multicol support.)

	Daniel Holbert [:dholbert]
	Comment 15 • 6 years ago

Comment on attachment 8962213 [details] [diff] [review]
part 2 - [css-grid][css-flexbox][css-multicol] Add 'row-gap' and 'gap' properties; make 'grid-[column|row]-gap' and 'grid-gap' alias the respective unprefixed properties (Gecko part)

Review of attachment 8962213 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits:

::: layout/style/nsCSSPropList.h
@@ +1464,5 @@
>          CSS_PROPERTY_VALUE_NONNEGATIVE,
>      "",
>      VARIANT_HLP | VARIANT_NORMAL | VARIANT_CALC,
>      nullptr,
> +    offsetof(nsStylePosition, mColumnGap),

There's one more change you need here, for this nsCSSPropList.h "column-gap" boilerplate:  it's not shown in the patch context, but this is still inside of a CSS_PROP_COLUMN(...) macro, and you need to change that to CSS_PROP_POSITION(...)  (s/COLUMN/POSITION/)

::: layout/style/test/property_database.js
@@ -1711,5 @@
> -    type: CSS_TYPE_LONGHAND,
> -    initial_values: [ "normal" ],
> -    other_values: [ "2px", "1em", "4em", "3%", "calc(3%)", "calc(1em - 3%)",
> -      "calc(2px)",
> -      "calc(-2px)",

Looks like this column-gap "other_values" list has a few entries that we won't be testing anymore (i.e. other_values entries that don't have counterparts in the new place where this property is defined here).

Could you restore some of these values (by extending the new place where this list lives)? In particular, here are two quirky values that were in this list that I think would be worth testing:
 calc(-2px)
 calc(3*25px + 5em)

(And I guess might as well add those ^^ to the row-gap other_values list, too.)

	Emilio Cobos Álvarez (:emilio)
	Comment 16 • 6 years ago

Comment on attachment 8962212 [details] [diff] [review]
part 1 - [css-grid][css-flexbox][css-multicol] Add 'row-gap' and 'gap' properties; make 'grid-[column|row]-gap' and 'grid-gap' alias the respective unprefixed properties (Stylo part)

Review of attachment 8962212 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, thank you!

I was going to ask whether this needed to handle the new values in nsGridContainerFrame, but I see that's taken care of in the other patch.

::: servo/components/style/properties/longhand/position.mako.rs
@@ +361,5 @@
> +${helpers.predefined_type("row-gap",
> +                          "length::NonNegativeLengthOrPercentageOrNormal",
> +                          "Either::Second(Normal)",
> +                          alias="grid-row-gap",
> +                          servo_pref="layout.columns.enabled",

I think it's fine to leave it as is, I don't think Servo's flex support supports column-gap.

Maybe :SimonSapin or :mbrubeck have an opinion on it, but changing it after the fact is not a big deal. I'll file an issue once this lands.

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

It occurred to me I should probably send an intent-to-ship
for this although its mostly just unprefixing existing
properties.  I'll send a summary of the changes here and in
bug 1398537 just in case...

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

https://groups.google.com/forum/#!topic/mozilla.dev.platform/InRVDzXKbkM

	Pulsebot
	Comment 19 • 6 years ago

Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef9fa58539c0
part 1 - [css-grid][css-flexbox][css-multicol] Add 'row-gap' and 'gap' properties; make 'grid-[column|row]-gap' and 'grid-gap' alias the respective unprefixed properties (Stylo part).  r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/8478901f3de6
part 2 - [css-grid][css-flexbox][css-multicol] Add 'row-gap' and 'gap' properties; make 'grid-[column|row]-gap' and 'grid-gap' alias the respective unprefixed properties (Gecko part).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed064619187c
part 3 - [css-grid][css-flexbox][css-multicol] Add 'row-gap' and 'gap' properties; make 'grid-[column|row]-gap' and 'grid-gap' alias the respective unprefixed properties (automated devtools update).
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab2b4fed2183
part 4 - [css-grid][css-flexbox][css-multicol] Add 'row-gap' and 'gap' properties; make 'grid-[column|row]-gap' and 'grid-gap' alias the respective unprefixed properties (automated update of WPT results).
https://hg.mozilla.org/integration/mozilla-inbound/rev/a398b01cc697
part 5 - Update devtools autocompletion expectations.  r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/d759ec86eb38
part 6 - Update devtools with the renamed longhands.  r=me

	Eliza Balazs [:ebalazs_]
	Comment 20 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/ef9fa58539c0
https://hg.mozilla.org/mozilla-central/rev/8478901f3de6
https://hg.mozilla.org/mozilla-central/rev/ed064619187c
https://hg.mozilla.org/mozilla-central/rev/ab2b4fed2183
https://hg.mozilla.org/mozilla-central/rev/a398b01cc697
https://hg.mozilla.org/mozilla-central/rev/d759ec86eb38

	Kohei Yoshino [:kohei]
	Comment 21 • 6 years ago

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/grid-gap-grid-row-gap-and-grid-column-gap-properties-have-been-unprefixed/

	Chris Mills [:cmills]
	Comment 22 • 6 years ago

The docs have been updated here, thanks to some amazing work from our ace contributor and CSS secret weapon, Rachel Andrew ;-)

See the links in the related rel note, at https://developer.mozilla.org/en-US/Firefox/Releases/61#CSS

Let me know if you think this needs anything else. Thanks!