Closed
Bug 1137984
Opened 8 years ago
Closed 8 years ago
querySelectorAll throws SyntaxError for attribute selector with missing ']'
Categories
(Core :: CSS Parsing and Computation, defect)
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: mozbugzilla2021, Assigned: bzbarsky)
Details
Attachments
(1 file)
5.51 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
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
Comment 3•8 years ago
|
||
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
![]() |
Assignee | |
Comment 4•8 years ago
|
||
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 | |
Comment 5•8 years ago
|
||
Attachment #8571394 -
Flags: review?(dbaron)
![]() |
Assignee | |
Updated•8 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment 6•8 years ago
|
||
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+
![]() |
Assignee | |
Comment 7•8 years ago
|
||
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•8 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."
Comment 9•8 years ago
|
||
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)?
![]() |
Assignee | |
Comment 10•8 years ago
|
||
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•8 years ago
|
||
If auto-closing ] here is intended but not specified, you could change the test to something like "[foo[ /* sanity check (invalid) */"
![]() |
Assignee | |
Comment 12•8 years ago
|
||
> 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•8 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
![]() |
Assignee | |
Comment 14•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0217de65664e
Comment 15•8 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)
![]() |
Assignee | |
Comment 16•8 years ago
|
||
OK, https://github.com/w3c/web-platform-tests/pull/1656
https://hg.mozilla.org/mozilla-central/rev/0217de65664e
Status: ASSIGNED → RESOLVED
Closed: 8 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.
Description
•