Closed Bug 1464091 Opened 6 years ago Closed 6 years ago

CSS Media Features not case-insensitive

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Version:
57 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- fixed
firefox60 --- wontfix
firefox61 --- fixed
firefox62 --- fixed

People

(Reporter: go.shoemake, Assigned: mozbugz)

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

Bug 1464091 - Ignore case in media feature names inside media query expressions -
59 bytes, text/x-review-board-request
heycam
: review+
RyanVM
: approval-mozilla-beta+
Details
Bug 1464091 - Test ascii-case-insensitivity of media query expressions -
59 bytes, text/x-review-board-request
heycam
: review+
Details
1464091-esr60-1.patch (Ignore case in media feature names inside media query expressions)
2.46 KB, patch
mozbugz
: review+
RyanVM
: approval-mozilla-esr60+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180516032328

Steps to reproduce:

Opened a site with a media query with a media feature which contains an uppercase letter.

+ Example: data:text/html,<STYLE>@media(Width){HTML{Background:Blue}}</STYLE>


Actual results:

The media query was ignored.


Expected results:

The media query should have been treated in a case-insensitive manner; ie as though it were all lowercase.

+ https://www.w3.org/TR/css3-mediaqueries/#syntax states: “CSS style sheets are generally case-insensitive, and this is also the case for media queries.”
+ Expected results for example: The same as if data:text/html,<STYLE>@media(width{HTML{Background:Blue}}</STYLE> had been loaded instead (note the lowercase w)
+ This also is the behaviour of Chrome and Safari
Moving to CSS Parsing and Computation.
Component: Untriaged → CSS Parsing and Computation
Product: Firefox → Core
This is a regression from Stylo.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Version: 60 Branch → 57 Branch
Priority: -- → P3
We just need to lower-case the feature name before atomizing it, in Expression::parse in servo/components/style/gecko/media_queries.rs.  Gerald, do you want to take a look at this one?
Flags: needinfo?(gsquelart)
Assignee: nobody → gsquelart
Flags: needinfo?(gsquelart)
Try with new WPTest only:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e4bf3424edb8d60a9045adbf9ad1beb13d11e0b
Notice the failures in Wr1's, as expected.

Try with the fix:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=da2c64acf03522d6bcc2b12844cbe41123e70d2d
No more Wr1 failures.


Cameron, a few questions please, as you review:
- I modified an ancient test, is that appropriate (it was testing '@media' alone, so it seemed relevant), or should I create a separate test?
- Is to_lowercase() efficient enough? I was thinking that maybe there could be an Atom::lowercase_from() function instead that changes the text as it copies(?) it -- worth considering?
- And of course please forward the review to other people as appropriate.
Flags: needinfo?(cam)
Fwiw (since I happened to take a look at the patch and Cam may be more busy / backlogged): to_lowercase is not (technically) the right thing to do here, though I don't think it's observable, since I don't think we have any unicode media feature and it'll just fail to parse.

It should be ASCII lowercase, in theory. We have `string_as_ascii_lowercase`, to avoid the allocation if the string was already ASCII lowercase... Though Atom::ascii_lowercase_from or something of the sort may be even better!

Thanks for working on this! :)
Thank you Emilio for the string_as_ascii_lowercase suggestion, I see it was used in another media_queries.rs file.
Exactly what the rules are for where tests for specific versions of CSS specs should live I'm not sure.  But it looks like tests in css/CSS2/ should only cover features that are in CSS 2, and so since media features like width and orientation were only introduced in Media Queries Level 3, they shouldn't go in there.  For other specs, there is no division into directories by version, so adding the new test in css/mediaqueries/ makes sense to me.
Flags: needinfo?(cam)
Comment on attachment 8982357 [details]
Bug 1464091 - Ignore case in media feature names inside media query expressions -

https://reviewboard.mozilla.org/r/248314/#review254576
Attachment #8982357 - Flags: review?(cam) → review+
Comment on attachment 8982358 [details]
Bug 1464091 - Test case-insensitivity of media query expressions -

https://reviewboard.mozilla.org/r/248316/#review254578

As mentioned, let's put these in a separate test under css/mediaqueries/.
Attachment #8982358 - Flags: review?(cam) → review-
(In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
> Fwiw (since I happened to take a look at the patch and Cam may be more busy
> / backlogged): to_lowercase is not (technically) the right thing to do here,
> though I don't think it's observable, since I don't think we have any
> unicode media feature and it'll just fail to parse.

There's a detectable difference here even if all the features are ASCII.  For example, should "HEİGHT" (with a dot on the capital I) be a valid media feature?  The capital dotted I (İ) should lowercase to a standard ASCII lowercase i.
(In reply to David Baron :dbaron: ⌚UTC-7 from comment #14)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
> > Fwiw (since I happened to take a look at the patch and Cam may be more busy
> > / backlogged): to_lowercase is not (technically) the right thing to do here,
> > though I don't think it's observable, since I don't think we have any
> > unicode media feature and it'll just fail to parse.
> 
> There's a detectable difference here even if all the features are ASCII. 
> For example, should "HEİGHT" (with a dot on the capital I) be a valid media
> feature?  The capital dotted I (İ) should lowercase to a standard ASCII
> lowercase i.

Just to clarify:
- Assuming "media features" should be like CSS keywords [1], then only the [a-z] and [A-Z] ranges should compare equally, non-ascii letters should not be modified. So The capital dotted I (İ) should NOT lowercase to a standard ASCII lowercase i.
- Emilio's suggestion to use string_as_ascii_lowercase is correct.
- Emilio and I were only wrong to say it's not detectable, because of non-ascii characters like capital dotted I, which could be lowercased to an ascii 'i'.

Should I add tests to check this ascii-only case-insensitiveness? E.g., to verify that "HEİGHT" is not accepted as media feature?


[1] https://drafts.csswg.org/css-values/#keywords
(In reply to Gerald Squelart [:gerald] from comment #15)
> Should I add tests to check this ascii-only case-insensitiveness? E.g., to
> verify that "HEİGHT" is not accepted as media feature?

Sounds like a good idea.
And it would be great if such a test were in web-platform-tests -- so good to put it there if you can.
Down the rabbit hole I went, trying to prove that "HEİGHT" (with the dotted 'I') would have triggered the non-ascii lowercasing code in my first patch.

Instead I found out that Rust's to_lowercase() transforms 'İ' (Latin capital letter I with dot above) into 'i̇' (Latin small letter i + combining dot above)!
That's because Rust [1] is using unicode.org tables [2] with this spec; interestingly another unicode.org document suggests that the lowercase should just be 'i' (no extra dot) [3] -- I'll see if I can find more info and/or raise an issue.

So anyway, even using (non-ascii) to_lowercase() in our code would not have been found with this test.

I think this test is still valuable to have in WPT (in case some other code converts the dotted 'I' into a normal 'i') so I will publish it anyway. I'll see if I can find some other strange letters that could trip non-ascii lowercasing.


[1] https://doc.rust-lang.org/std/primitive.char.html#method.to_lowercase
[2] ftp://ftp.unicode.org/Public/UNIDATA/UnicodeData.txt and ftp://ftp.unicode.org/Public/UNIDATA/SpecialCasing.txt
[3] http://www.unicode.org/charts/PDF/U0100.pdf
Attachment #8982358 - Attachment is obsolete: true
Comment on attachment 8984009 [details]
Bug 1464091 - Test ascii-case-insensitivity of media query expressions -

https://reviewboard.mozilla.org/r/249880/#review256798

::: testing/web-platform/tests/css/mediaqueries/mq-case-insensitive-001.html:6
(Diff revision 1)
> +  <link rel="author" title="Gerald Squelart" href="mailto:gerald@mozilla.com"/>
> +  <link rel="help" href="http://www.w3.org/TR/CSS21/syndata.html#characters"/>
> +  <link rel="match" href="../reference/ref-filled-green-100px-square.xht">

Nit: be consistent about the "/".  (It's not needed for self-closing elements like <link> in text/html.)

::: testing/web-platform/tests/css/mediaqueries/mq-case-insensitive-001.html:34
(Diff revision 1)
> +
> +  /* In some languages Non-ASCII 'İ' (Latin capital I with dot above) may be
> +     lowercased to ASCII 'i'; This would make "heİght" compare the same as
> +     "height", which would be incorrect. */
> +  @media all and (heİght) {
> +   div { background-color: orange; }

Probably better use red here, given the text in the <p>.
Attachment #8984009 - Flags: review?(cam) → review+
Comment on attachment 8984009 [details]
Bug 1464091 - Test ascii-case-insensitivity of media query expressions -

https://reviewboard.mozilla.org/r/249880/#review256798

> Nit: be consistent about the "/".  (It's not needed for self-closing elements like <link> in text/html.)

Well spotted! That was because I had copied the two '/' lines from an xhtml test. :-)

> Probably better use red here, given the text in the <p>.

I wanted to be able to distinguish which way the test was going wrong, but it's true it could be confusing.
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8a3088e2b467
Ignore case in media feature names inside media query expressions - r=heycam
https://hg.mozilla.org/integration/autoland/rev/32a24b5f542d
Test ascii-case-insensitivity of media query expressions - r=heycam
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11468 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
https://hg.mozilla.org/mozilla-central/rev/8a3088e2b467
https://hg.mozilla.org/mozilla-central/rev/32a24b5f542d
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Seems like a pretty low-risk webcompat patch to backport? Did you want to nominate this for Beta/ESR60 approval?
status-firefox60: --- → wontfix
status-firefox61: --- → affected
status-firefox-esr52: --- → unaffected
status-firefox-esr60: --- → affected
Flags: needinfo?(gsquelart)
Flags: in-testsuite+
Comment on attachment 8982357 [details]
Bug 1464091 - Ignore case in media feature names inside media query expressions -

Thank you Ryan for the suggestion.

Approval Request Comment
[Feature/Bug causing the regression]: @media expressions
[User impact if declined]: Non-lowercase @media expressions would be ignored; probably a very tiny number of websites affected?
[Is this code covered by automated tests?]: Yes, WPT in patch 2.
[Has the fix been verified in Nightly?]: Through WPT, no other formal verification.
[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 only ascii-lowercasing feature names to compare their with our hard-coded lowercase names.
[String changes made/needed]: None.
Attachment #8982357 - Flags: approval-mozilla-beta?
Trivial rebase of the 1st patch to esr60; 2nd patch applies as-is.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: No-risk change for webcompat. Naively I would think it's very unlikely to happen or even matter much, but I don't have actual data; so it's not critical to backport, but would be nice to have if easy enough.
User impact if declined: Some websites may not be shown as intended by their @media expressions.
Fix Landed on Version: 62
Risk to taking this patch (and alternatives if risky): I think no risks (ascii-lowercasing media feature names).
String or UUID changes made by this patch:  None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Flags: needinfo?(gsquelart)
Attachment #8985240 - Flags: review+
Attachment #8985240 - Flags: approval-mozilla-esr60?
Comment on attachment 8982357 [details]
Bug 1464091 - Ignore case in media feature names inside media query expressions -

Stylo webcompat fix that makes us better adhere to the specs. Approved for 61.0b14 and ESR 60.1.
Attachment #8982357 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8985240 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: