1418152 - stylo: Attribute selector including whitespace is ignored

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Volker E.]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171112125346

Steps to reproduce:

In Firefox 57 a CSS attribute selector including whitespace is ignored, as example `[ data-attr ] { … };` will not be rendered.
Coming from https://phabricator.wikimedia.org/T180138 – the LESS compiler's minification saved us from bigger negative impact.
This is a regression from Firefox 56 and below, the whitespace shouldn't result in the selector being ignored.



Actual results:

Simplified testcase:
https://codepen.io/Volker_E/full/vWWXVp/




Expected results:

Expected result:
foo div should feature green background
bar div should feature blue background

Comment 1

[Alice0775 White]

[Tracking Requested - why for this release]: Breaks CSS, it means widely affected.

Expected result should be:
foo div should feature blue background
bar div should feature green background



Setting layout.css.servo.enabled = false fixes the problem

Comment 2

[Cameron McCormack (:heycam)]

Thanks Volker and Alice.  I think we just need an `input.skip_whitespace()` call at the start of parse_attribute_selector.

Comment 3

[Alice0775 White]

regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5f721a664bf64fed99184a866b60c24a6afcb3a0&tochange=62cebea1d1578461a423a9ca7848706455db9ea5

Comment 4

[Cameron McCormack (:heycam)]

https://github.com/servo/servo/pull/19263

Comment 5

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/autoland/rev/c6ff3a13495eb7143cf2f7ce80db081662686073

Comment 6

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/mozilla-central/rev/c6ff3a13495e

Comment 7

[Cameron McCormack (:heycam)]

Attachment: patch for beta uplift

Approval Request Comment
[Feature/Bug causing the regression]: Stylo
[User impact if declined]: some CSS selectors will fail to parse and cause page rendering/usability problems
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[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?]: it's a very small CSS parser change
[String changes made/needed]: none

Comment 8

[Cameron McCormack (:heycam)]

Attachment: patch for release uplift

Approval Request Comment
(see comment 7)

This might be worth taking as a ride-along if we do a 57 dot release.

Comment 11

[Cameron McCormack (:heycam)]

RESOLVED FIXED since the Servo patch made its way over to mozilla-central.

Comment 12

[Gerry Chang [:gchang]]

Comment on attachment 8929942 [details] [diff] [review]
patch for beta uplift

Fix a stylo issue. Beta58+.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8929946 [details] [diff] [review]
patch for release uplift

Taking it as ride along for 57 to fix a regression.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

https://hg.mozilla.org/releases/mozilla-release/rev/7d0ce703edbb1ee38358ecfc5578281543570521

Comment 15

[Andrei Vaida [:avaida]]

(In reply to Cameron McCormack (:heycam) from comment #7)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]: no

Has automated coverage, does not need manual testing - per Cameron.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

For posterity, here's the link to the Beta uplift:
https://hg.mozilla.org/releases/mozilla-beta/rev/65eaccbc3cdd

Comment 17

[ejsanders]

Are there any public stats for 57.0.1 installs? We have hacks in place to work around this bug, but would like to know when 57.0.0 usage has dropped enough for us to revert these, but the UA string only reports "57.0"

Ed S
Wikimedia

Comment 18

[Cameron McCormack (:heycam)]

(In reply to ejsanders from comment #17)
> Are there any public stats for 57.0.1 installs? We have hacks in place to
> work around this bug, but would like to know when 57.0.0 usage has dropped
> enough for us to revert these, but the UA string only reports "57.0"

Chris, do you know the answer to this?

Comment 19

[Chris Peterson [:cpeterson]]

(In reply to ejsanders from comment #17)
> Are there any public stats for 57.0.1 installs? We have hacks in place to
> work around this bug, but would like to know when 57.0.0 usage has dropped
> enough for us to revert these, but the UA string only reports "57.0"

The following dashboards (aka "Mission Control") show the usage hours for different Firefox versions, so you can track how quickly people update. 57.0.1 usage is still low and there will always be some stragglers stuck on 57.0.0 for whatever reason. Note that there is now a 57.0.2 dot release.

https://data-missioncontrol.dev.mozaws.net/#/release/windows/main_crashes
https://data-missioncontrol.dev.mozaws.net/#/release/mac/main_crashes
https://data-missioncontrol.dev.mozaws.net/#/release/linux/main_crashes

You can also distinguish dot releases in JavaScript using navigator.buildID, which is a build timestamp. Unfortunately, that won't help you if you need to check on the server side to determine which CSS file to serve. The Mission Control dashboards show the buildIDs for each release:

57.0.0 = 20171112125346
57.0.1 = 20171128222554
57.0.2 = 20171206182557

Note that there are some Linux distro builds with buildIDs that are > the Mac and Windows buildIDs, so it would be safer to check for <= 57.0.1's buildID instead of == or > 57.0.0's buildID.