1398482 - [css-grid] Drop the "grid-" prefix from the grid-gap, grid-row-gap, and grid-column-gap properties

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Mats Palmgren (inactive)]

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

Comment 1

[Mats Palmgren (inactive)]

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.

Comment 2

[Mats Palmgren (inactive)]

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.

Comment 3

[Manuel Rego Casasnovas]

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/

Comment 4

[Emilio Cobos Álvarez (:emilio)]

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

Comment 5

[Emilio Cobos Álvarez (:emilio)]

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

Comment 6

[Mats Palmgren (inactive)]

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

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

Comment 8

[Mats Palmgren (inactive)]

Attachment: 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)

Comment 9

[Mats Palmgren (inactive)]

Attachment: 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)

Comment 10

[Mats Palmgren (inactive)]

Attachment: 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)

Comment 11

[Mats Palmgren (inactive)]

Attachment: 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 resul

Comment 12

[Mats Palmgren (inactive)]

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.

Comment 13

[Mats Palmgren (inactive)]

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

Comment 14

[Daniel Holbert [:dholbert]]

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

Comment 15

[Daniel Holbert [:dholbert]]

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

Comment 16

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 17

[Mats Palmgren (inactive)]

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

Comment 18

[Mats Palmgren (inactive)]

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

Comment 20

[Eliza Balazs [:ebalazs_]]

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

Comment 21

[Kohei]

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/

Comment 22

[Chris Mills [:cmills]]

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!