Closed Bug 1137984 Opened 8 years ago Closed 8 years ago
Selector All throws Syntax Error for attribute selector with missing ']'
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
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
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 ']'
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?
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).
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.
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
(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). :-)
You need to log in before you can comment on or make changes to this bug.