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

RESOLVED FIXED in Firefox 61

Status

()

P3
normal
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: mats, Assigned: mats)

Tracking

({dev-doc-complete, DevAdvocacy, site-compat})

Trunk
mozilla61
dev-doc-complete, DevAdvocacy, site-compat
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [DevRel:P1] [Parity-webkit] [Parity-blink])

Attachments

(4 attachments)

(Assignee)

Description

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

Updated

2 years ago
Blocks: 1398483
(Assignee)

Comment 1

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

Updated

2 years ago
Depends on: 1398492
(Assignee)

Updated

2 years ago
Depends on: 1398537
(Assignee)

Comment 2

2 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.
Priority: -- → P3
Keywords: DevAdvocacy
Whiteboard: [DevRel:P1][Parity-webkit][parity-blink]
Whiteboard: [DevRel:P1][Parity-webkit][parity-blink] → [DevRel:P1] [Parity-webkit] [Parity-blink]
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/
(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!
Flags: needinfo?(emilio)
Mats, should we do this without waiting for bug 1398537?
Flags: needinfo?(mats)
(Assignee)

Comment 6

a year 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
Assignee: nobody → mats
Flags: needinfo?(mats)
Ok, that sounds great! Let me know if I can help somehow :)
Flags: needinfo?(emilio)
(Assignee)

Comment 12

a year 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.
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.)
Attachment #8962212 - Flags: review?(dholbert) → review?(emilio)
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.)
Attachment #8962213 - Flags: review?(dholbert) → review+
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.
Attachment #8962212 - Flags: review?(emilio) → review+
(Assignee)

Comment 17

a year 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...
Keywords: dev-doc-needed
(Assignee)

Updated

11 months ago
Depends on: 1456166

Comment 19

11 months 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
(Assignee)

Updated

11 months ago
Flags: in-testsuite+
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!
Keywords: dev-doc-needed → dev-doc-complete
status-firefox57: affected → ---
You need to log in before you can comment on or make changes to this bug.