983175 - Add style-system support for CSS Grid’s subgrid feature

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Simon Sapin (:SimonSapin)]

Bug 976787 added support for the grid-template-columns and grid-template-rows properties, but skipped the 'subgrid' feature.

There was a proposal to punt the feature to Level 2 of the spec, but also some disagreement. No decision has been made yet.

http://lists.w3.org/Archives/Public/www-style/2014Feb/0047.html
http://lists.w3.org/Archives/Public/www-style/2014Mar/0035.html

Still, we may want to write the style system code for subgrid. The parsing is a bit tricky, especially in the grid-template and grid shorthands (bug 981752)

Comment 1

[Simon Sapin (:SimonSapin)]

One non-obvious is how to represent this in computed values.

Currently, the computed value of the grid-template-columns and grid-template-rows properties is represented (each) as a nsStyleGridTrackList struct (defined in layout/style/nsStyleStruct.h) with the following fields:

  nsTArray<nsTArray<nsString>> mLineNameLists;
  nsTArray<nsStyleCoord> mMinTrackSizingFunctions;
  nsTArray<nsStyleCoord> mMaxTrackSizingFunctions;

This represents 'none' when all arrays are empty, or a <track-list> otherwise.

One option could be to add a boolean field:

  bool mIsSubgrid;

When that field is true, mLineNameLists is re-used but the other two arrays are empty/ignored.

One option to

Comment 2

[Simon Sapin (:SimonSapin)]

Attachment: Patch 1: Refactor

Comment 3

[Simon Sapin (:SimonSapin)]

Attachment: Patch 2: subgrid

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8396751 [details] [diff] [review]
Patch 1: Refactor

>From: Simon Sapin <[e-mail]>
>Bug 983175 part 1: Have ParseGridLineNames distinguish "OK" and "Not found".

Commit-message nit: could you make the description more concrete, e.g.:
Bug 983175 part 1: Make ParseGridLineNames distinguish between empty list "()" and the lack of a list.

r=me with that


(Note: we probably should rename nsParsingStatus to something like CSSParsingStatus, since the "ns" prefix is kind of a vestigial thing that we're trying to move away from. That's a trivial search-and-replace, though, and I or someone else can get to it later, if you don't get to it. Maybe file a bug for that, though?)

Comment 5

[Simon Sapin (:SimonSapin)]

Attachment: Patch 0: Rename nsParsingStatus to CSSParseResult.

> (Note: we probably should rename nsParsingStatus to something like
> CSSParsingStatus, since the "ns" prefix is kind of a vestigial thing that
> we're trying to move away from. That's a trivial search-and-replace, though,
> and I or someone else can get to it later, if you don't get to it. Maybe
> file a bug for that, though?)

Doing this first to get it out of the way.

Comment 6

[Simon Sapin (:SimonSapin)]

Attachment: Patch 1 v2: Refactor. r=dholbert

Rebase on top of patch 0 and expend commit message per review comment.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8397404 [details] [diff] [review]
Patch 0: Rename nsParsingStatus to CSSParseResult.

Punting review on the enum name to dbaron.  (Looks good to me, FWIW.)

Comment 8

[Simon Sapin (:SimonSapin)]

Attachment: Patch 2 v2: subgrid

Rebase on top of patch 0.

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8397407 [details] [diff] [review]
Patch 2 v2: subgrid

Looks good! Just a few nits; nothing functional.

>From: Simon Sapin <[e-mail]>
>Bug 983175 part 2: Add style system support for 'subgrid'

Add " keyword in grid-template* properties" to the end of the commit message there.

>diff --git a/layout/style/nsCSSParser.cpp b/layout/style/nsCSSParser.cpp
>+// Assuming a 'subgrid' keyword was already consumed, parse <line-name-list>?
>+bool
>+CSSParserImpl::ParseSubgridLineNameList(nsCSSValue& aValue)

Maybe name this "ParseOptionalSubgridLineNameList()" (or even "ParseOptionalLineNameListAfterSubgrid()"), to make it clearer that the parsed thing here is *optional*.

That will make it more obvious that in e.g. this chunk from ParseGridTemplate...
      if (!ParseSubgridLineNameList(value)) {
        return false;
      }
...we aren't actually *requiring* a <line-name-list>.

>@@ -7275,18 +7310,41 @@ CSSParserImpl::ParseGridTemplate()
>-  // TODO (bug 983175): add parsing for subgrid
>-  // as part of <'grid-template-columns'>
>+  nsSubstring* ident = NextIdent();
>+  if (ident) {
>+    if (ident->LowerCaseEqualsLiteral("subgrid")) {
>+      if (!ParseSubgridLineNameList(value)) {
>+        return false;
>+      }
>+      AppendValue(eCSSProperty_grid_template_columns, value);
>+      if (ExpectSymbol('/', true)) {
>+        return ParseGridTemplateAfterSlash(/* aColumnsIsTrackList = */ false);
>+      }

Add a comment towards the beginning here, saying that the "subgrid" keyword can appear by itself (per the shorthand syntax) *or* it could be the start of a <'grid-template-columns'> / <'grid-template-rows'> expression.

(Since superficially, the property's value definition makes it look like "subgrid" can only appear on its own, since it only gets called out on its own.)

(I guess this technically applies to "none", too. Maybe add a comment for that, too? Or a single comment covering both keywords?)

> bool
> CSSParserImpl::ParseGridTemplateAfterSlash(bool aColumnsIsTrackList)
> {
>+  if (ParseVariant(rowsValue, VARIANT_NONE, nullptr)) {
>+    // <'grid-template-columns'> / <'grid-template-rows'>
>+    AppendValue(eCSSProperty_grid_template_rows, rowsValue);
>+    nsCSSValue areasValue(eCSSUnit_None);  // implied
>+    AppendValue(eCSSProperty_grid_template_areas, areasValue);
>+    return true;

The <'grid-template-columns'> / <'grid-template-rows'> comment there is a bit confusing on its own. (but it's useful once I realize why it's there / what it's saying)

This would be clearer if you extended the comment above this function to say something along the lines of:

/*
 * NOTE: This parses the portion after the slash, for *one* of the
 * following types of expressions:
 * -  <'grid-template-columns'> / <'grid-template-rows'>
 * -  [ <track-list> / ]?  [ <line-names>? <string> <track-size>? <line-names>? ]+
 *
 * We don't know which type of expression we've got until we've parsed the
 * second half, since the pre-slash part is ambiguous. The various return
 * clauses below are labeled with the type of expression they're completing.)
 */

r=me with the above.

Comment 10

[Simon Sapin (:SimonSapin)]

Attachment: Patch 2 v3: subgrid. r=dholbert

Try run with patches 0, 1v2, 2v3:
https://tbpl.mozilla.org/?tree=Try&rev=f4376c0a983e

Comment 11

[Simon Sapin (:SimonSapin)]

Try failures seem to be intermittent/known. http://logbot.glob.com.au/?c=mozilla%23ateam#c771282

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Patch 2 doesn't apply. Please rebase. Also, dholbert, can you please formally r+ patch 1? Right now, there's only an "r= with..." comment for it.

Comment 13

[Simon Sapin (:SimonSapin)]

Attachment: Patch 2 v3: subgrid. r=dholbert

Looks like I uploaded the wrong file in attachment 8397648 [details] [diff] [review], sorry about that.

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8397406 [details] [diff] [review]
Patch 1 v2: Refactor. r=dholbert

r=me (sorry for not setting the flag in comment 4)

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/acba6fd6fb2c
https://hg.mozilla.org/integration/mozilla-inbound/rev/63d2474f84f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/038c28096bfe

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/acba6fd6fb2c
https://hg.mozilla.org/mozilla-central/rev/63d2474f84f8
https://hg.mozilla.org/mozilla-central/rev/038c28096bfe

Comment 17

[Jen Simmons [:jensimmons]]

As authors are digging into how to use Grid, as industry leaders are writing their books about Grid, it seems like punting Subgrids is a bad idea. Even if including it means delaying Grid for months. There's not consensus yet, but a brewing question... Here's Eric Meyer's investigation, with usecases: http://meyerweb.com/eric/thoughts/2016/01/15/subgrids-considered-essential/

(I'm reopening this ticket because of it's name. Is it better to leave it closed, since specific work was finished on this ticket, and open a new one?)

Comment 18

[Daniel Holbert [:dholbert]]

It's best to keep an individual bug focused on a single chunk of work, landing at a single point in time -- otherwise, backouts & regression-tracking gets pretty complicated/unmanageable.  (though dummy "tracking bugs" are a notable exception)

Given that this bug already had some work land 2 years ago, we shouldn't reopen & pile on any additional work here. Instead, I've filed a new bug -- bug 1240834 -- on getting layout support for subgrid (as opposed to the css parsing support which happened in this bug here).  (I thought we already had such a bug, but it seems we do not.)

(I've also transferred the "DevAdvocacy" keyword over there. I suspect it's not useful to leave it on this bug here, for work that was done 2 years ago.)

Comment 19

[Jen Simmons [:jensimmons]]

Yes. Thanks so much, Daniel.