Closed Bug 1430622 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

57 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox57 --- wontfix
firefox58 - wontfix
firefox59 - fixed

People

(Reporter: ben, Assigned: emilio)

References

Details

Attachments

(2 files)

Attached file 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.
Component: Untriaged → CSS Parsing and Computation
Product: Firefox → Core
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.
Assignee: nobody → 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
Flags: needinfo?(jfernandez)
Also, cc mats and dholbert in case they have any opinion on comment 2 re. what to do.
Summary: CSS: place-content bug → CSS: place-content allow(ed) for ambiguity
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Flags: needinfo?(jfernandez)
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.
Summary: CSS: place-content allow(ed) for ambiguity → stylo: place-content accepts fallback alignment, causing parsing ambiguity.
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.
See Also: → 1430679
Attachment #8942750 - Flags: review?(mats)
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...
Attachment #8942750 - Flags: review?(xidorn+moz) → review+
(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).
(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 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
Attachment #8942750 - Flags: review?(mats) → review+
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.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2ea467872d7f
Don't allow fallback alignment in place-content shorthand. r=xidorn,mats
https://hg.mozilla.org/mozilla-central/rev/2ea467872d7f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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
Attachment #8942750 - Flags: approval-mozilla-beta?
(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.)
(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 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.
Attachment #8942750 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
(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)
Flags: needinfo?(gchang)
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/
NI on Emilio to make sure you don't miss Javier's comment.
Flags: needinfo?(gchang) → needinfo?(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 :)
Flags: needinfo?(emilio)
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.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.