If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

querySelectorAll throws SyntaxError for attribute selector with missing ']'

RESOLVED FIXED in Firefox 39

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Colin Snover, Assigned: bz)

Tracking

38 Branch
mozilla39
x86
All
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20150222232811

Steps to reproduce:

1. http://jsfiddle.net/k0wfsv3f/


Actual results:

Displays SyntaxError: An invalid or illegal string was specified


Expected results:

Displays PASS
(Reporter)

Comment 1

3 years ago
More information: https://code.google.com/p/chromium/issues/detail?id=460399#c6

Chrome 40: PASS
IE 11: PASS
Saf 8: PASS
Saf 7: FAIL
Firefox 38 (aurora): FAIL
Firefox 36 (release): FAIL

Related Web compat issue: https://github.com/webcompat/web-bugs/issues/676
(Reporter)

Comment 2

3 years ago
Sorry, I screwed up my URLs. Safari 8 also fails.
Reproduced in Nightly 39.0a1 e10s and non e10s windows.
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM
Ever confirmed: true
OS: Mac OS X → All
Product: Firefox → Core
Hmm.  I guess we do auto-close things like ":not(div" in selector parsing...

I really wish the CSS WG would get their parsing act together and actually define it, but in the meantime I guess we can probably treat EOF in an attribute selector when the ']' is expected as the same thing as ']'.

Note that this does NOT make the selector valid.  It just means the invalid-syntax error recovery setup in CSS will assume you meant to have a ']' at the end, as opposed to whatever you actually wanted.
Component: DOM → CSS Parsing and Computation
Summary: querySelectorAll throws SyntaxError for valid attribute selector → querySelectorAll throws SyntaxError for attribute selector with missing ']'
Created attachment 8571394 [details] [diff] [review]
When parsing attribute selectors, treat EOF as ']' any place ']' is allowed
Attachment #8571394 - Flags: review?(dbaron)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8571394 [details] [diff] [review]
When parsing attribute selectors, treat EOF as ']' any place ']' is allowed

r=dbaron, but maybe also add some test_parseable_via_api() calls in test_selectors.html to test both cases as well, in case we contribute test_selectors.html elsewhere...
Attachment #8571394 - Flags: review?(dbaron) → review+
I added those.

Also, there is a web platform test (selectors/attribute-selectors/attribute-case/syntax.html) that assumes that "[foo /* sanity check (invalid) */" is not a valid selector and that this means it should throw from querySelector.

Of course the test assumes that a "valid" selector will parse in a stylesheet.  But clearly this dichotomy is not correct: any selector that will throw from querySelector will also not parse in a stylesheet, but there are selectors that will work in querySelector but not parse in a stylsheet.

Simon, do you have a preferred way I should change the test here?
Flags: needinfo?(zcorpan)

Comment 8

3 years ago
As far as I can tell the test is right and the spec requires to throw SyntaxError for missing ] in querySelector.

https://dom.spec.whatwg.org/#dom-parentnode-queryselector
->
https://dom.spec.whatwg.org/#scope-match-a-selectors-string
->
http://dev.w3.org/csswg/selectors/#parse-a-selector

"Let selector be the result of parsing source against the complex_selector_list grammar. If it does not match the grammar, return failure"

It does not match the grammar (http://dev.w3.org/csswg/selectors/#grammar see attrib), so failure is returned.

->
https://dom.spec.whatwg.org/#scope-match-a-selectors-string

"If s is failure, throw a SyntaxError."
But what about the rule that all open constructs are allowed to be implicitly closed when you reach EOF (which here would be the end of the string)?
The relevant part of the stuff linked to in comment 8 is really this bit in <http://dev.w3.org/csswg/selectors/#grammar>:

  Issue 12: Rewrite this whole danged thing in terms of Syntax parsing algos instead.

As in, its current state is known to be bunk (not least because of the various self-contradictions in it, esp around EOF handling).

Comment 11

3 years ago
If auto-closing ] here is intended but not specified, you could change the test to something like

"[foo[ /* sanity check (invalid) */"
> If auto-closing ] here is intended but not specified

Oh, it's specified.  In CSS 2.1.

> "[foo[ /* sanity check (invalid) */"

I can do that, but presumably we want tests for the auto-closing behavior too, and afaict selectors/attribute-selectors/attribute-case/syntax.html simply can't test that right now, yes?  I'm ok with that if you are, but just wanted to check.

Comment 13

3 years ago
Yeah, I need to get off my ass and rewrite the parsing section in Selectors, but per Syntax, it's *impossible* to tell that a block was closed by EOF versus its literal closing character.  That information disappears by the time parsing is done; all you see is a []-block with some contents.  querySelector("[foo") is totally valid and exactly equivalent to querySelector("[foo]").

We had the inverse bug on Chrome (we dont' report an error, someone thought we should) which I just WONTFIX'd a few days ago. ^_^ https://code.google.com/p/chromium/issues/detail?id=460399
https://hg.mozilla.org/integration/mozilla-inbound/rev/0217de65664e

Comment 15

3 years ago
(In reply to Not doing reviews right now from comment #12)
> I can do that, but presumably we want tests for the auto-closing behavior
> too, and afaict selectors/attribute-selectors/attribute-case/syntax.html
> simply can't test that right now, yes?  I'm ok with that if you are, but
> just wanted to check.

Sure. Testing auto-closing should probably be a separate test. The intent of the particular test was to pick a baseline that should fail to parse even if `i` is not supported. So omitting ] was a bad choice since it's supposed to work (which I didn't realize since I did a literal reading of the "latest" spec). :-)
Flags: needinfo?(zcorpan)
OK, https://github.com/w3c/web-platform-tests/pull/1656
https://hg.mozilla.org/mozilla-central/rev/0217de65664e
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.