stylo: Different handling of place-{content,items,self} shorthand

ASSIGNED
Assigned to

Status

()

Core
CSS Parsing and Computation
P2
normal
ASSIGNED
3 months ago
2 months ago

People

(Reporter: xidorn, Assigned: nox)

Tracking

(Depends on: 1 bug, Blocks: 3 bugs)

53 Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

3 months ago
Now place-{content,items,self} shorthands have been implemented in Stylo, but there are still some test failures, specifically layout/style/test/test_align_shorthand_serialization.html

After looking at the test and the spec, I'm very confused. It seems Servo, Gecko, and the spec are doing three different things which are incompatible with each other.

For example, according to the css-align spec, for justify-content, <overflow-position> (safe / unsafe) should always be before <content-position>, however, both Servo and Gecko happily accept something like 'justify-content: start safe'.

According to the spec, place-content is just an ordered combine of align-content and justify-content, which means whenever both longhands have value, shorthand should be serialized. Servo somehow follows so, but it cannot always parse the serialization it produces, e.g. 'normal start safe', while Gecko seems to refuse serializing when one contains <overflow-position>.

It is not clear to me what should we do with this. Given we've shipped all these properties, I guess disabling the test and make it not block stylo is not a choice for us.

An extra note that, the test test_align_shorthand_serialization is currently disabled for stylo because our failure pattern file cannot handle failures from testharness. You would need to manually remove 'skip-if = stylo' line in mochitest.ini to run it with stylo.
(Reporter)

Comment 1

3 months ago
Created attachment 8866649 [details]
Result of test_align_shorthand_serialization.html

This is the result of test_align_shorthand_serialization.html I'm seeing.
(Reporter)

Updated

3 months ago
(Reporter)

Comment 2

3 months ago
nox, mats, what do you think?
Flags: needinfo?(nox)
Flags: needinfo?(mats)
Right, this is a very recent spec change, bug 1361531 is filed to fix it in Gecko.
(FYI, bug 1105570 is a meta bug for css-align)
Flags: needinfo?(mats)
(Reporter)

Comment 4

3 months ago
/me hates unstable spec...

So probably we need to fix it for both Stylo and Gecko.
(Assignee)

Updated

2 months ago
Flags: needinfo?(nox)
Bug 1361531 has patches for both Stylo and Gecko for the unsafe/safe ordering.

I think the other reported issue here is invalid and the spec is wrong.
The consensus in https://github.com/w3c/csswg-drafts/issues/595 is that only
a single keyword is allowed for each of the align/justify sub-values in
the shorthand, to avoid ambiguities.  (with an exception for "first/last baseline"
which counts as a single keyword in this context)

@fantasai later filed a new issue (https://github.com/w3c/csswg-drafts/issues/1002)
with a proposal to extend/change the syntax to allow for more complex values,
but that's far from settled.  I don't think we should change Gecko before we have
consensus on what the new syntax should be.
... invalid for Gecko that is.  If Stylo serialize complex values into the shorthand
then I consider that a bug in Stylo.  It might be worth waiting until the new csswg
issue is resolved before fixing it though.
Priority: -- → P2
(Assignee)

Updated

2 months ago
Assignee: nobody → nox
Status: NEW → ASSIGNED
(Reporter)

Updated

2 months ago
Blocks: 1339656
(Reporter)

Updated

2 months ago
Depends on: 1361531
You need to log in before you can comment on or make changes to this bug.