1421645 - stylo: subgrid is incorrectly exposed to content in stylo

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

(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*

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

canaltinova, mind having a look at this?

Comment 4

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

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

Comment 5

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1421645 - Hide accidentally exposed subgrid behind prefs

We are accidentally exposing subgrids because of the subgrid parsing on stylo.
While we are removing it from rust side, we can remove the gecko side as well.
Subgrid was pref'd off before but it's removed from CSS Grid L1 spec and it seems
we won't need that in the future.

Review commit: https://reviewboard.mozilla.org/r/204872/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204872/

Comment 6

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 8

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204872/diff/1-2/

Comment 9

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Thanks for the review! Removed the tests.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 11

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

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

Comment 12

[Mats Palmgren (inactive)]

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).

Comment 13

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

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

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

(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.

Comment 15

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

(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 :)

Comment 16

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204872/diff/2-3/

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

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

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

Looks good. Thanks.

Comment 18

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204872/diff/3-4/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a7721f527737
Hide accidentally exposed subgrid behind prefs r=emilio,xidorn

Comment 20

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

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

Comment 21

[Raul Gurzau (:RaulG)]

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider uplifting to Beta?

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 24

[Gerry Chang [:gchang]]

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