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

Status: RESOLVED WORKSFORME

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

Description

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Result of test_align_shorthand_serialization.html

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

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

nox, mats, what do you think?

Comment 3

[Mats Palmgren (inactive)]

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)

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

/me hates unstable spec...

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

Comment 5

[Mats Palmgren (inactive)]

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.

Comment 6

[Mats Palmgren (inactive)]

... 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.

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 8

[Chris Peterson [:cpeterson]]

status-firefox57=wontfix unless someone thinks this bug should block 57

Comment 9

[Chris Peterson [:cpeterson]]

Unassigning from nox because he is not currently working on Stylo.

Comment 10

[Cameron McCormack (:heycam)]

I think there's nothing left to do here now that bug 1339656 has landed.