1221677 - [css-grid] Put the 'subgrid' support behind a pref, disabled by default

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

The 'subgrid' feature is at-risk:
https://drafts.csswg.org/css-grid/

Since we don't intend to support it in the initial release
we should put the style system support we have behind a pref.

Comment 1

[Tobias Schneider [:tobytailor]]

Attachment: Patch 1: Pref out support for 'subgrid'.

Comment 2

[Mats Palmgren (inactive)]

Comment on attachment 8686855 [details] [diff] [review]
Patch 1: Pref out support for 'subgrid'.

This looks great, thanks!

A couple of nits:

>layout/base/nsLayoutUtils.cpp
>+nsLayoutUtils::IsGridTemplateSubgridValueEnabled()
>+{
>+  static bool sGridTemplateSubgridValueEnabled;
>+  static bool sTGridTemplateSubgridValueEnabledPrefCached = false;

The first T in sTGrid... looks like a typo?  Please remove that T
so we follow the established naming scheme in surrounding code.

>layout/style/nsCSSParser.cpp
>     if (ident->LowerCaseEqualsLiteral("subgrid")) {
>+      if (!nsLayoutUtils::IsGridTemplateSubgridValueEnabled()) {
>+        return false;
>+      }

I think it would be helpful to add an explicit error message in these
places that explains to the author that we don't support 'subgrid'
in this version.  Something like:
  REPORT_UNEXPECTED(PESubgridNotSupported);
with the associated message string:
"The 'subgrid' keyword of CSS Grid isn't supported in this version."
or something like that.


>layout/style/test/property_database.js
>+  if (IsCSSPropertyPrefEnabled("layout.css.grid-template-subgrid-value.enabled")) {
>+    gCSSProperties["grid-template-columns"].other_values.push(
> ...
>+    gCSSProperties["grid-template-columns"].invalid_values.push(
> ...

Could you move the above two sections to just after
the "gCSSProperties["grid-template-columns"] = ..." section?
That way we will also include these into 'grid-template-rows'
which follows and copies the values off -columns:
http://mxr.mozilla.org/mozilla-central/source/layout/style/test/property_database.js#5864


>+    gCSSProperties["grid-templaye"].other_values.push(
> ...

Typo: s/templaye/template/

>+    gCSSProperties["grid-templaye"].invalid_values.push(

Again.

And move these two to just after the "gCSSProperties["grid-template"] ="
section please.

Comment 3

[Mats Palmgren (inactive)]

I think it would be good idea to submit two separate Try jobs for testing:
one with the patch as is, and one with the pref set to true.
(in both cases, "-u mochitests,reftest" should be sufficient I think)

Comment 4

[Tobias Schneider [:tobytailor]]

Attachment: Patch 1 (v2): Pref out support for 'subgrid'.

Comment 5

[Tobias Schneider [:tobytailor]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0aade5384ef with pref set to false (default)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb4e6aae14b5 with pref set to true

Comment 6

[Mats Palmgren (inactive)]

Comment on attachment 8687392 [details] [diff] [review]
Patch 1 (v2): Pref out support for 'subgrid'.

Thanks, this look good to me.  r=mats

The Try jobs doesn't look good though.  You we're probably just unlucky
and used a bad revision as parent.  Might be worth to rebase and resubmit?

Comment 7

[Tobias Schneider [:tobytailor]]

Yeah, seams like the Try jobs look way better after a rebase:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e1bbc74a271
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8358695f56f3

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/026f4fbcf931

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/026f4fbcf931