Closed Bug 1363971 Opened 8 years ago Closed 7 years ago

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

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

53 Branch
enhancement

Tracking

()

RESOLVED WORKSFORME
Tracking Status
firefox57 --- wontfix

People

(Reporter: xidorn, Unassigned)

References

()

Details

Attachments

(1 file)

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.
This is the result of test_align_shorthand_serialization.html I'm seeing.
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)
/me hates unstable spec... So probably we need to fix it for both Stylo and Gecko.
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: nobody → nox
Status: NEW → ASSIGNED
Blocks: 1339656
Depends on: 1361531
Priority: P2 → --
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
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
Closed: 7 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: