stylo: implement grid property glue

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
5 months ago
4 days ago

People

(Reporter: bz, Assigned: waffles)

Tracking

(Blocks: 4 bugs)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed, firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

5 months ago
Pretty much all the reftests in layout/reftests/css-grid are marked failing.
Priority: -- → P2
This is because most of the grid properties aren't yet implemented, but this is underway, see:

https://github.com/servo/servo/pull/15628
https://github.com/servo/servo/pull/16081
Summary: stylo: need to make grid support actually work → stylo: implement grid property glue
https://github.com/servo/servo/issues/15307 is the metabug, though that may only be for parsing / serialization.
Blocks: 1243581
Blocks: 1356087
Waffles, you're working on this right? How much left is there to do here?
Assignee: nobody → wafflespeanut
Flags: needinfo?(wafflespeanut)
Priority: P2 → P1
(Assignee)

Comment 4

3 months ago
All the longhands other than `grid-template-areas` have been implemented ( grid-template-{rows,columns} will land soon - https://github.com/servo/servo/pull/16067 ). Though the metabug lists only parsing/serialization, I made sure that the gecko glue was also implemented along the way :)

None of the shorthands have been implemented yet.
Flags: needinfo?(wafflespeanut)
Great - thanks for the update! Let me know if you get blocked on anything, the grid stuff is pretty high priority.
Comment hidden (mozreview-request)
We needed to update the test expectations after https://github.com/servo/servo/pull/16443. But I need to wait for someone to push it since I can't do it myself :)

Comment 8

3 months ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/337f30ecca23
stylo: Update reftest expectations for grid-{row,column} and grid-area. r=test-annotations-update

Comment 9

3 months ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/5f4fc7d22293
stylo: Update reftest expectations for grid-{row,column} and grid-area. r=test-annotations-fix
https://hg.mozilla.org/mozilla-central/rev/337f30ecca23
https://hg.mozilla.org/mozilla-central/rev/5f4fc7d22293
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Updated

3 months ago
Depends on: 1362843
It seems to me grid support for stylo is still far from complete. Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
:waffles has a PR open that I need to review and carry, then there will be very few things remaining.
(Assignee)

Comment 13

2 months ago
Only grid and grid-template shorthands remain now.
Blocks: 1339656
status-firefox54: affected → ---
(Assignee)

Comment 14

2 months ago
Once https://github.com/servo/servo/pull/17021/ lands, we'll have all grid properties (only a few bugs will remain).
(Assignee)

Comment 15

2 months ago
We still need to decide whether we should support subgrid grammar (which is based on old spec and it's prefs-gated in gecko), because many failures are on subgrid stuff.
(In reply to Ravi Shankar [:waffles] from comment #15)
> We still need to decide whether we should support subgrid grammar (which is
> based on old spec and it's prefs-gated in gecko), because many failures are
> on subgrid stuff.

How much work is it to mimic Gecko here (including the pref-gating)? In general that should be the default, because it's the lowest risk.
(Assignee)

Comment 17

2 months ago
Not much, really. I could add it straightaway. Just wondering whether we should implement something that doesn't even exist in the spec anymore :)
(In reply to Ravi Shankar [:waffles] from comment #17)
> Not much, really. I could add it straightaway. Just wondering whether we
> should implement something that doesn't even exist in the spec anymore :)

We should, so please do!
This preference is off by default in current Firefox. Do we really need to add that cruft to Servo?
(In reply to Ravi Shankar [:waffles] from comment #17)
> Just wondering whether we should implement something that doesn't even exist in the spec anymore :)

It's still specced, but it's just in a different level of the spec:
  https://drafts.csswg.org/css-grid-2/#subgrids
(It was moved per CSSWG resolution noted here: https://github.com/w3c/csswg-drafts/issues/958#issuecomment-286813071 )

Authors & spec editors are very adamant about "subgrid" as a feature, but the specifics may change as CSS Grid Level 2 is fleshed out.

Mats can comment more about timeline for implementing it in layout (my impression is that it's not on his front-burner yet & may not be for a little while).  So IMO it'd be OK to deprioritize "subgrid" parsing support in Stylo for the moment, and behave as if this pref simply doesn't exist (and tag the corresponding tests as "fails-if(stylo)" or whatever).

Comment 21

22 days ago
Pushed by canaltinova@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/db3e0c6cab27
Update test expectations after servo/servo#17268 r=me
These four tests:
* test_grid_computed_values.html [4]
* test_grid_container_shorthands.html [65]
* test_grid_item_shorthands.html [23]
* test_grid_shorthand_serialization.html [28]
are currently disabled because of bug 1339656.

We can probably try enabling them and see if they passes.

Any idea what is still going wrong in test_value_storage.html?
Yes, I guess we can try to re-enable them.

I was looking at all the failures and trying to reduce them. This amount of passing test seemed reasonable to land with subgrid. I still need to figure out the remaining failures. The failures in test_value_storage.html seems to be related to idempotency.

Comment 24

21 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/db3e0c6cab27
Status: REOPENED → RESOLVED
Last Resolved: 3 months ago21 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED

Comment 25

20 days ago
Pushed by canaltinova@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/89c5475d5b0f
Update test expectations for servo/servo#17616 r=me

Comment 26

19 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/89c5475d5b0f

Comment 27

18 days ago
Pushed by canaltinova@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/02ada7b63615
Update test expectations for servo/servo#17630 r=me

Comment 28

18 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/02ada7b63615

Comment 29

15 days ago
Pushed by canaltinova@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fdcb00d8e0c4
stylo: Don't skip a passing test in grid r=me

Comment 30

14 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fdcb00d8e0c4

Comment 31

13 days ago
Pushed by canaltinova@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e0e0e469b648
Update test expectations for servo/servo#17692 r=me

Comment 32

12 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e0e0e469b648

Comment 33

11 days ago
Pushed by canaltinova@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e84711b4665d
Update test expectations after servo/servo#17737 r=me
Wow, all failures of grid in test_value_storage.html has gone. I guess we can try reenabling the other grid tests mentioned in comment 22 now.
Yes, I'm trying that right now but I found some failing tests on these(and a crash). I'll fix these bugs and enable them.
https://hg.mozilla.org/mozilla-central/rev/e84711b4665d

Comment 37

5 days ago
Pushed by canaltinova@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e0ee89ef9aa9
Enable disabled grid tests after servo/servo#17776 r=me
https://hg.mozilla.org/mozilla-central/rev/e0ee89ef9aa9
You need to log in before you can comment on or make changes to this bug.