1137984 - querySelectorAll throws SyntaxError for attribute selector with missing ']'

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Colin S]

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

Comment 1

[Colin S]

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 2

[Colin S]

Sorry, I screwed up my URLs. Safari 8 also fails.

Comment 3

[Petruta Horea [:phorea], Desktop Test Engineering]

Reproduced in Nightly 39.0a1 e10s and non e10s windows.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: When parsing attribute selectors, treat EOF as ']' any place ']' is allowed

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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...

Comment 7

[Boris Zbarsky [:bzbarsky]]

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?

Comment 8

[Simon Pieters [:zcorpan]]

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

[Cameron McCormack (:heycam)]

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)?

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

[Simon Pieters [:zcorpan]]

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

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

> 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

[Tab Atkins Jr.]

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

Comment 14

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0217de65664e

Comment 15

[Simon Pieters [:zcorpan]]

(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). :-)

Comment 16

[Boris Zbarsky [:bzbarsky]]

OK, https://github.com/w3c/web-platform-tests/pull/1656

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0217de65664e