Closed Bug 1341802 Opened 7 years ago Closed 7 years ago

stylo: implement grid property glue

Categories

(Core :: CSS Parsing and Computation, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: bzbarsky, Assigned: waffles)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Waffles, you're working on this right? How much left is there to do here?
Assignee: nobody → wafflespeanut
Flags: needinfo?(wafflespeanut)
Priority: P2 → P1
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.
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 :)
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
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
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.
Only grid and grid-template shorthands remain now.
Once https://github.com/servo/servo/pull/17021/ lands, we'll have all grid properties (only a few bugs will remain).
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.
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).
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.
https://hg.mozilla.org/mozilla-central/rev/db3e0c6cab27
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Pushed by canaltinova@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fdcb00d8e0c4
stylo: Don't skip a passing test in grid 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.
Pushed by canaltinova@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a6a0f1b4a69d
stylo: Enable the forgotten disabled grid test r=me
Target Milestone: mozilla55 → mozilla56
You need to log in before you can comment on or make changes to this bug.