u+foo is not parsed as a valid selector due to the "u+f" being treated as a unicode-range-token

RESOLVED FIXED in Firefox 62

Status

()

Product:
Core
Component:
CSS Parsing and Computation
RESOLVED FIXED
Reported:
4 years ago
Modified:
a month ago

People

(Reporter: proger.xp, Assigned: emilio, Mentored)

Tracking

Version:
33 Branch
Target:
mozilla62
x86_64
All
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

demo.html
207 bytes, text/html
Details
Bug 1032034: Test that unicode range syntax isn't a primitive token.
59 bytes, text/x-review-board-request
bz
: review+
Details | Review
(Reporter)

Description

•
4 years ago 
Created attachment 8447820 [details]
demo.html

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)

Steps to reproduce:

demo.html:

<!DOCTYPE html>
<html>
  <head>
    <style>
    #nav u+a { background: yellow; }
    </style>
  </head>
  <body>
    <nav id="nav">
      <u>THE U</u>
      <a href="#">THE A</a>
    </nav>
  </body>
</html>


Actual results:

The <a> element is not styled (not yellow). Rule defined in <style> is dropped; console says:

Dangling combinator.  Ruleset ignored due to bad selector.

This can be fixed by adding a space either after u+ or before +a or both:

    #nav u+ a { background: yellow; }
    #nav u +a { background: yellow; }
    #nav u + a { background: yellow; }

Either of these 3 works.

Tested on: 33 (latest), 28, 26. All broken.


Expected results:

u+a should work the same way as u + a with or without spaces.
 (Reporter)

Updated

•
4 years ago
OS: Linux → All

Comment 1

•
4 years ago 
Reproduced on latest Nightly 33.0a1 2014-06-29 under Win 7, 64-bit. 
Please notice that the yellow background is not displayed on Chrome too.
Component: Untriaged → CSS Parsing and Computation
Product: Firefox → Core
 (Reporter)

Comment 2

•
4 years ago 
> Please notice that the yellow background is not displayed on Chrome too.
It is not indeed but this behaviour is a mystery to me. Obviously it also doesn't work on Safari. 

But it *does* work on IE 10+ - probably one of those lucky cases when Microsoft got it right.

Comment 3

•
4 years ago 
Consider how this gets tokenized by the CSS tokenizer.  I'm basing the analysis on http://dev.w3.org/csswg/css-syntax/ which, unlike CSS2, has a clearly defined tokenizer.

First we see the '#nav'.  This produces a <hash-token>.

Then we see a space.  This produces a <whitespace-token>.

Then we see a 'u'.  Looking at http://dev.w3.org/csswg/css-syntax/#consume-a-token we see:

  U+0055 LATIN CAPITAL LETTER U (U)
  U+0075 LATIN SMALL LETTER U (u)
    If the next 2 input code points are U+002B PLUS SIGN (+) followed by a hex digit or
    U+003F QUESTION MARK (?), consume the next input code point. Note: don’t consume both
    of them. Consume a unicode-range token and return it. 

In this case, the next two input code points are in fact U+002B followed by a hex digit, so this returns a <unicode-range-token>.  This token is not valid in selectors, so we seem to treat this as end of selector and claim that we had a combinator (space) followed by no more selector, hence dangling combinator.

So the error reporting is suboptimal, for sure.  It would be nice if it actually said it saw an unexpected token, and what kind.

IE does support unicode-range, so I wonder what exactly they are doing in their tokenizer, and whether their tokenizer is based on the spec at all...

That said, this is a pretty weird footgun, and I wonder whether we can adjust the spec here somehow.  Right now "#nav u+i" would work, but "#nav u+b" would not, nor would "#nav u+code" and various others!

I posted http://lists.w3.org/Archives/Public/www-style/2014Jun/0486.html about that.
Flags: needinfo?(jackalmage)
Flags: needinfo?(dbaron)
Summary: Dangling combinator. Ruleset ignored due to bad selector. → u+foo is not parsed as a valid selector due to the "u+f" being treated as a unicode-range-token

Comment 4

•
4 years ago 
And thank you for the excellent bug report!
Status: UNCONFIRMED → NEW
Ever confirmed: true
 (Reporter)

Comment 5

•
4 years ago 
Thanks Boris, I would have never guessed that it expects to see a Unicode sequence. Shouldn't it be U+... (case-sensitive)? At any rate this is quite a gotcha, I would expect the parser to ignore possible Unicode reference as soon as it fails to recognize it (it doesn't fail on u+zzz so why it does on u+a?). That's probably what IE does, reasonably.

Curious to see W3C's reaction.
 (Reporter)

Comment 6

•
4 years ago 
And by the way, forgot to mention that I ran into this after minifying some stylesheets so I guess many if not all existing CSS minifiers are broken due to this behaviour as they always remove spaces around selector symbols.

Comment 7

•
4 years ago 
> I would have never guessed that it expects to see a Unicode sequence.

It actually expects a sequence or range.  Something like u+123-678, but the part after '-' is optional.

> Shouldn't it be U+... (case-sensitive)? 

CSS syntax is uniformly case-insensitive, so both U+ and u+ are allowed.

> it doesn't fail on u+zzz so why it does on u+a

Because 'a' is a valid hex digit and 'z' is not.
 (Reporter)

Comment 8

•
4 years ago 
> Because 'a' is a valid hex digit and 'z' is not.
That was my point though now I have checked the spec and found that it starts unicode-range-token after u+<hex digit> and not just 'u+'. Well, Firefox and Chrome do all right by the spec but it's still lousy IMO.

Comment 9

•
4 years ago 
Right, we agree the current spec situation is not OK.  ;)

Comment 10

•
4 years ago 
The working group discussed this in today's teleconference and decided to change things so unicode-range is not a special token, but instead gets parsed from the appropriate set of tokens.

As a shorter-term fix, I think we could probably just switch to context-sensitive tokenization (i.e., calling a different scanner method when parsing the unicode-range descriptor) for now, at least until the spec for the new way stabilizes.
Flags: needinfo?(dbaron)

Comment 11

•
4 years ago 
http://dev.w3.org/csswg/css-syntax/#urange-syntax has draft rules, though likely to move to css-fonts.

I think we should implement this by:

 * changing nsCSSToken to have an nsDependentString for its textual representation (I think we can do this, though we'd need to check)
   * as followup to this, getting rid of the complexity of nsCSSToken::AppendToString, which I think is only currently used in error messages (need to check that too -- if not it might be a problem)
 * then using nsCSSToken::AppendToString to implement what is described in the spec, and simultaneously remove the eCSSToken_URange token, and the mInteger2 from nsCSSToken.

Comment 12

•
a month ago 
Does stylo do this correctly?  (i.e., can we close this?)
Flags: needinfo?(simon.sapin)

Comment 13

•
a month ago 
Yes. rust-cssparser’s tokenizer does not have a unicode-range token (anymore), and instead provides a type and method to parse <unicode-range> based on other tokens. This matches what is not specified in CSS Syntax Level 3:

* https://drafts.csswg.org/css-syntax/#tokenizer-algorithms
* https://drafts.csswg.org/css-syntax/#urange

I’ve verified that the yellow background does show up in Firefox 60. (It presumably does since 57.)
Status: NEW → RESOLVED
Last Resolved: a month ago
Flags: needinfo?(simon.sapin)
Flags: needinfo?(jackalmage)
Resolution: --- → FIXED

Comment 14

•
a month ago 
> what is not specified

Typo: what is *now* specified

Comment 15

•
a month ago 
We should really make sure we have a testcase for this...
 (Assignee)

Comment 16

•
a month ago 
Agreed.
Assignee: nobody → emilio
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment hidden (mozreview-request)

Comment 18

•
a month ago
mozreview-review 
Comment on attachment 8974782 [details]
Bug 1032034: Test that unicode range syntax isn't a primitive token.

https://reviewboard.mozilla.org/r/243184/#review249026

Thank you!
Attachment #8974782 - Flags: review?(bzbarsky) → review+

Comment 19

•
a month ago 
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/85d3e5f02273
Test that unicode range syntax isn't a primitive token. r=bz

Comment 20

•
a month ago 
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10960 for changes under testing/web-platform/tests

Comment 21

•
a month ago 
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 22

•
a month ago
bugherder 
https://hg.mozilla.org/mozilla-central/rev/85d3e5f02273
Status: REOPENED → RESOLVED
Last Resolved: a month ago → a month ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62

Comment 23

•
a month ago 
Upstream PR merged
You need to log in before you can comment on or make changes to this bug.