Xidorn Quan [:xidorn] UTC+11 Reporter
	Description • 7 years ago

The layout part of subgrid has not been implemented in Gecko, and its parsing code is behind layout.css.grid-template-subgrid-value.enabled which is disabled by default.

However, Servo implements the parsing of subgrid without checking the pref, so subgrid syntax is incorrectly exposed to web in Stylo.

Since subgrid has been removed from CSS Grid L1, and the L2 spec seems to be exposing a different approach to declare, I guess the easiest solution would be simply removing all subgrid parsing (and serialization) code for now.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 1 • 7 years ago

I realized this when looking at bug 1421592 which seems to be a subgrid serialization code path.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 2 • 7 years ago

(In reply to Xidorn Quan [:xidorn] UTC-6 (less responsive Nov 5 ~ Dec 16) from comment #0)
> and the L2 spec seems to be exposing a different approach to declare,

*exploring*

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 3 • 7 years ago

canaltinova, mind having a look at this?

	Nazım Can Altınova [:canova][:canaltinova on phabricator] Assignee
	Comment 4 • 7 years ago

Hi, xidorn. Sure, I will look at this!

	Nazım Can Altınova [:canova][:canaltinova on phabricator] Assignee
	Comment 6 • 7 years ago

Servo side: https://github.com/servo/servo/pull/19464
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f0f302c60802c268a5e45dedb2b4da29a96e69e

	Emilio Cobos Álvarez (:emilio)
	Comment 7 • 7 years ago
mozreview-review

Comment on attachment 8933941 [details]
Bug 1421645 - Hide accidentally exposed subgrid behind prefs

https://reviewboard.mozilla.org/r/204872/#review210342

Nice, thanks! I guess this needs to wait for the servo thingies to land, but after that this looks fine to me!

::: layout/style/test/test_grid_container_shorthands.html:168
(Diff revision 1)
> -            "subgrid [foo] [] [a b] [c] [] [a b] [c] [] [a b] [c]" : "none",
>      },
>      {
>          // Test that the number of lines is clamped to kMaxLine = 10000.
>          specified: "subgrid [foo] repeat(999999999, [a]) / subgrid",
> -        gridTemplateColumns: isGridTemplateSubgridValueEnabled ? "subgrid" : "none",
> +        gridTemplateColumns: "none",

IMO you may as well get rid of these subgrid-specific tests, but I guess they don't hurt.

	Nazım Can Altınova [:canova][:canaltinova on phabricator] Assignee
	Comment 9 • 7 years ago

Thanks for the review! Removed the tests.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 10 • 7 years ago

I would prefer also have mats review this, or at least have him aware of this removal, since he worked on CSS Grid.

	Nazım Can Altınova [:canova][:canaltinova on phabricator] Assignee
	Comment 11 • 7 years ago

Sure, I'll r- the servo side to wait for this review then.

	Mats Palmgren (inactive)
	Comment 12 • 7 years ago
mozreview-review

Comment on attachment 8933941 [details]
Bug 1421645 - Hide accidentally exposed subgrid behind prefs

https://reviewboard.mozilla.org/r/204872/#review210550

I'm exploring ways to implement subgrid right now, and this is
the syntax I intend to use for now.  So I'm opposed to removing it.
Please put the stylo bits behind the subgrid pref instead.

No browser implements subgrid yet, so the risk of any real web sites
breaking from supporting this syntax for a little while is extremely
unlikely.

FWIW, it'll take a couple of months before I know if supporting
subgrid independently in each axis is viable or not (and if it is,
then we'll need this syntax, or something like it).

	Nazım Can Altınova [:canova][:canaltinova on phabricator] Assignee
	Comment 13 • 7 years ago

Thank you for the review Mats. I'll put the subgrids behind the pref then.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 14 • 7 years ago

(In reply to Nazım Can Altınova [:canaltinova] from comment #13)
> Thank you for the review Mats. I'll put the subgrids behind the pref then.

Sorry about that. I thought we don't need that anymore, but it seems I was wrong.

	Nazım Can Altınova [:canova][:canaltinova on phabricator] Assignee
	Comment 15 • 7 years ago

(In reply to Xidorn Quan [:xidorn] UTC-6 (less responsive Nov 5 ~ Dec 16) from comment #14)
> (In reply to Nazım Can Altınova [:canaltinova] from comment #13)
> > Thank you for the review Mats. I'll put the subgrids behind the pref then.
> 
> Sorry about that. I thought we don't need that anymore, but it seems I was
> wrong.

No problem :)

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 17 • 7 years ago
mozreview-review

Comment on attachment 8933941 [details]
Bug 1421645 - Hide accidentally exposed subgrid behind prefs

https://reviewboard.mozilla.org/r/204872/#review211158

Looks good. Thanks.

	Pulsebot
	Comment 19 • 7 years ago

Pushed by canaltinova@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a7721f527737
Hide accidentally exposed subgrid behind prefs r=emilio,xidorn

	Nazım Can Altınova [:canova][:canaltinova on phabricator] Assignee
	Comment 20 • 7 years ago

Oh, btw, servo side of the patch: https://github.com/servo/servo/pull/19499

	Raul Gurzau (:RaulG)
	Comment 21 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/a7721f527737

	Ryan VanderMeulen [:RyanVM]
	Comment 22 • 7 years ago

Is this something we should consider uplifting to Beta?

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 23 • 7 years ago

Hmmm, I guess it's probably a good idea to uplift, since subgrid is something that we don't have complete implementation. Although I'm not too worried about webcompat since no browser has shipped it and thus no website should be using it, I do think exposing some premature code path can potentially increase our risk.

So, yeah, probably we should consider uplifting it.

	Gerry Chang [:gchang]
	Comment 24 • 7 years ago

Too late for Beta58. Mark 58 won't fix.