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

RESOLVED WORKSFORME

Status

()

P3
normal
RESOLVED WORKSFORME
2 years ago
7 months ago

People

(Reporter: xidorn, Unassigned)

Tracking

(Blocks: 2 bugs)

53 Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

2 years 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

2 years 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)

Comment 2

2 years 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

2 years ago
/me hates unstable spec...

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

Updated

2 years 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

Updated

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

Updated

2 years ago
Blocks: 1339656
(Reporter)

Updated

2 years ago
Depends on: 1361531
Priority: P2 → --
(Reporter)

Comment 7

a year ago
It is a potential behavior change. If it doesn't cause any real issue, maybe it isn't that urgent. But it would be nice to avoid such change, anyway.
Blocks: 1337166
Priority: -- → P3
status-firefox57=wontfix unless someone thinks this bug should block 57
status-firefox57: --- → wontfix
Unassigning from nox because he is not currently working on Stylo.
Assignee: nox → nobody
Status: ASSIGNED → NEW
I think there's nothing left to do here now that bug 1339656 has landed.
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.