Closed
Bug 1398482
Opened 7 years ago
Closed 6 years ago
[css-grid] Drop the "grid-" prefix from the grid-gap, grid-row-gap, and grid-column-gap properties
Categories
(Core :: CSS Parsing and Computation, enhancement, P3)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)
References
Details
(Keywords: dev-doc-complete, DevAdvocacy, site-compat, Whiteboard: [DevRel:P1] [Parity-webkit] [Parity-blink])
Attachments
(4 files)
7.73 KB,
patch
|
emilio
:
review+
|
Details | Diff | Splinter Review |
36.63 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
4.61 KB,
patch
|
Details | Diff | Splinter Review | |
8.32 KB,
patch
|
Details | Diff | Splinter Review |
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 | ||
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.
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.
Updated•7 years ago
|
Priority: -- → P3
Updated•6 years ago
|
Keywords: DevAdvocacy
Whiteboard: [DevRel:P1][Parity-webkit][parity-blink]
Updated•6 years ago
|
Whiteboard: [DevRel:P1][Parity-webkit][parity-blink] → [DevRel:P1] [Parity-webkit] [Parity-blink]
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/
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!
Updated•6 years ago
|
Flags: needinfo?(emilio)
Comment 5•6 years ago
|
||
Mats, should we do this without waiting for bug 1398537?
Flags: needinfo?(mats)
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
Assignee: nobody → mats
Flags: needinfo?(mats)
Comment 7•6 years ago
|
||
Ok, that sounds great! Let me know if I can help somehow :)
Flags: needinfo?(emilio)
Assignee | ||
Comment 8•6 years ago
|
||
Attachment #8962212 -
Flags: review?(dholbert)
Assignee | ||
Comment 9•6 years ago
|
||
Attachment #8962213 -
Flags: review?(dholbert)
Assignee | ||
Comment 10•6 years ago
|
||
Assignee | ||
Comment 11•6 years ago
|
||
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.
Assignee | ||
Comment 13•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dded947fddd5decff2023d24d3ffa932eef7bfb2
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.)
Attachment #8962212 -
Flags: review?(dholbert) → review?(emilio)
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.)
Attachment #8962213 -
Flags: review?(dholbert) → review+
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.
Attachment #8962212 -
Flags: review?(emilio) → review+
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...
Assignee | ||
Comment 18•6 years ago
|
||
https://groups.google.com/forum/#!topic/mozilla.dev.platform/InRVDzXKbkM
Updated•6 years ago
|
Keywords: dev-doc-needed
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
Assignee | ||
Updated•6 years ago
|
Flags: in-testsuite+
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
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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/
Keywords: site-compat
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!
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
status-firefox57:
affected → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•