1430622 - stylo: place-content accepts fallback alignment, causing parsing ambiguity.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Benjamin De Cock]

Attachment: Test case

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.132 Safari/537.36

Steps to reproduce:

The `place-content` CSS property is supposed to be a shorthand for `align-content` and `justify-content`, but it behaves differently with the `space-evenly` value.


Actual results:

It looks like the first value of `place-content`is misinterpreted (see attached example), which makes the black square appear at the center of the page in Firefox.


Expected results:

In the attached example, the black square should appear at the bottom center of the page.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Yup, thanks for the report. This is a Stylo regression, that is, the same page with layout.css.servo.enabled=false produces the expected rendering.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

So, there's a gotcha here, which is that, as implemented in Blink / Gecko, "end space-evenly" is a valid value of align-content.

So the issue here is that the grammar is ambiguous. In particular, Stylo is interpreting the shorthand as "end space-evenly" "end space-evenly", while other engines do so as "end" "space-evenly".

So I think this is not wrong per se, but there's a spec bug here. Looking at the spec, I can no longer see the ambiguity, and I think it was fixed by [1].

So we could update to the spec, though that also has some risk because it's different from what Blink implements, at least in Chrome 63.

Javier, is there any plan to update the grammar per [1] in Blink and WebKit? If so, it may be worth taking the risk of doing it.

[1]: https://github.com/w3c/csswg-drafts/commit/21a4728f9a044cd202b094809c059295cee192f1

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Also, cc mats and dholbert in case they have any opinion on comment 2 re. what to do.

Comment 4

[Javier Fernandez]

The issue of allowing fallback alignments in the shorthand has been discussed and resolved here:

https://github.com/w3c/csswg-drafts/issues/1002#issuecomment-319634895

This is not implemented in Blink or WebKit, yet, but basically, a slash must be used to separate main and fallback alignments in the shorthand syntax. 

The spec draft hasn't been updated yet, as fat as I know; it still has the "Needs Edits" flag, indeed

It'd be good to file bugs against Blink and WebKit as well.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Ok, thanks a lot Javier!

So the issue here is simply that we're allowing fallback values in the shorthand, which is ambiguous. Will open another bug to update the parsing to the spec per https://github.com/w3c/csswg-drafts/issues/1002.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1430622: Don't allow fallback alignment in place-content shorthand.

Review commit: https://reviewboard.mozilla.org/r/213002/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/213002/

Comment 7

[Emilio Cobos Álvarez (:emilio)]

We may want to uplift this one to avoid shipping this in two releases, though maybe it's too late for 58... I still think this is low risk and should take it though.

[Tracking Requested - why for this release]: Potential Stylo web-compat regression.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8942750 [details]
Bug 1430622: Don't allow fallback alignment in place-content shorthand.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213002/diff/1-2/

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8942750 [details]
Bug 1430622: Don't allow fallback alignment in place-content shorthand.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213002/diff/2-3/

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8942750 [details]
Bug 1430622: Don't allow fallback alignment in place-content shorthand.

https://reviewboard.mozilla.org/r/213002/#review218690

The change to Rust part seems fine to me, but I'd like to leave review of the new test to mats...

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8942750 [details]
Bug 1430622: Don't allow fallback alignment in place-content shorthand.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213002/diff/3-4/

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8942750 [details]
Bug 1430622: Don't allow fallback alignment in place-content shorthand.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213002/diff/4-5/

Comment 13

[Mats Palmgren (inactive)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
> So I think this is not wrong per se, but there's a spec bug here. Looking at
> the spec, I can no longer see the ambiguity, and I think it was fixed by [1].
> [1]:
> https://github.com/w3c/csswg-drafts/commit/21a4728f9a044cd202b094809c059295cee192f1

No, explicit fallback alignment was remove here:
https://github.com/w3c/csswg-drafts/commit/c38cac45421c55ccb6c70cd9e49a04fc36e6b19c#diff-77b45f4d92eb4bee76c4242f1752be4c
which changed the meaning of 'place-content: end space-evenly' (from one value
with a fallback to two separate values).  Our parsing is correct per the spec
before that change (which is when we implemented these shorthands).

Comment 14

[Emilio Cobos Álvarez (:emilio)]

(In reply to Mats Palmgren (:mats) from comment #13)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
> > So I think this is not wrong per se, but there's a spec bug here. Looking at
> > the spec, I can no longer see the ambiguity, and I think it was fixed by [1].
> > [1]:
> > https://github.com/w3c/csswg-drafts/commit/21a4728f9a044cd202b094809c059295cee192f1
> 
> No, explicit fallback alignment was remove here:
> https://github.com/w3c/csswg-drafts/commit/
> c38cac45421c55ccb6c70cd9e49a04fc36e6b19c#diff-
> 77b45f4d92eb4bee76c4242f1752be4c
> which changed the meaning of 'place-content: end space-evenly' (from one
> value
> with a fallback to two separate values).  Our parsing is correct per the spec
> before that change (which is when we implemented these shorthands).

Yeah, I realised of this in comment 5, thus the patch :)

Comment 15

[Mats Palmgren (inactive)]

Comment on attachment 8942750 [details]
Bug 1430622: Don't allow fallback alignment in place-content shorthand.

https://reviewboard.mozilla.org/r/213002/#review218866

LGTM, r=mats

Comment 16

[Mats Palmgren (inactive)]

Might be worth filing a follow-up bug on implementing the fallback alignment
syntax that the CSSWG resolved on?  (it seems rather straight-forward so
I guess someone could start working on that if they want to)

I filed bug 1430817 to update the set of (non-fallback) values we accept
for these properties, since it's not up-to-date with the latest spec.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2ea467872d7f
Don't allow fallback alignment in place-content shorthand. r=xidorn,mats

Comment 18

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/2ea467872d7f

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8942750 [details]
Bug 1430622: Don't allow fallback alignment in place-content shorthand.

I know it's late in the cycle, but let's try to get it into beta. The patch is super-low risk.

Approval Request Comment
[Feature/Bug causing the regression]: stylo
[User impact if declined]: broken webpages, potentially
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]:  no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: just avoids parsing some extra values of the properties in a shorthand, which causes ambiguity.
[String changes made/needed]: none

Comment 20

[Mats Palmgren (inactive)]

(Now that I think about it, using a reftest to test shorthands is kinda sub-optimal.
It's better to test that the corresponding longhands get the expected values, like
we do in layout/style/test/test_grid_item_shorthands.html etc.)

Comment 21

[Emilio Cobos Álvarez (:emilio)]

(In reply to Mats Palmgren (:mats) from comment #20)
> (Now that I think about it, using a reftest to test shorthands is kinda
> sub-optimal.
> It's better to test that the corresponding longhands get the expected
> values, like
> we do in layout/style/test/test_grid_item_shorthands.html etc.)

Thought about that, but given the amount of WPT tests that this fixes, and that the shorthand syntax is in flux, I didn't worry much about it.

Comment 22

[Gerry Chang [:gchang]]

Comment on attachment 8942750 [details]
Bug 1430622: Don't allow fallback alignment in place-content shorthand.

This is very late for 58 and the patch seems huge to me. Let's let it ride the train on 59.

Comment 23

[Emilio Cobos Álvarez (:emilio)]

(In reply to Gerry Chang [:gchang] from comment #22)
> Comment on attachment 8942750 [details]
> Bug 1430622: Don't allow fallback alignment in place-content shorthand.
> 
> This is very late for 58 and the patch seems huge to me. Let's let it ride
> the train on 59.

Most of the patch is removing expectations for new tests that now pass and adding tests. Given that, worth reconsidering? (no problem if not, I just thought it was worth pointing that out)

Comment 24

[Javier Fernandez]

It'd be awesome if you could update or add the required Web Platform Tests at http://w3c-test.org/css/css-align/content-distribution/

Comment 25

[Sylvestre Ledru [:Sylvestre]]

NI on Emilio to make sure you don't miss Javier's comment.

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Yup, I read it, I don't plan to update to the spec syntax in https://github.com/w3c/csswg-drafts/issues/1002 until the edits are made, but I think we're on the same page :)

Comment 27

[Chris Mills [:cmills]]

Adding dev-doc-needed, as it sounds like the docs might need to be updated. The property needs to be entered into the MDN properties datastore anyway.