Bug 89682 – Throw SYNTAX_ERR instead of NAMESPACE_ERR from querySelector(All)

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: DOM

[Pablo Flouret]

The spec now says:

[[
If the group of selectors include namespace prefixes that need to be resolved, the implementation must raise a SYNTAX_ERR exception
]]

http://dev.w3.org/2006/webapi/selectors-api/#resolving-namespaces

Thread here: http://lists.w3.org/Archives/Public/public-webapps/2012AprJun/1225.html

Mozilla already changed it: https://bugzilla.mozilla.org/show_bug.cgi?id=766798

[Pablo Flouret]

It seems i linked to the email with the decision, the first email in the thread shows the problem: http://lists.w3.org/Archives/Public/public-webapps/2012AprJun/1207.html

Basically, in some cases where both SyntaxError and NamespaceErr applied, webkit found the namespace error first, whereas other browsers found the syntax error first, leading to different exceptions being thrown.

[Pablo Flouret]

Created attachment 148855 [details]
Patch

[Pablo Flouret]

(In reply to comment #2)
> Created an attachment (id=148855) [details]
> Patch

I modified Element::webkitMatchesSelector as well, let me know if it's better to keep that one as is.

[Pablo Flouret]

Created attachment 148884 [details]
Rebased patch

[Erik Arvidsson]

Comment on attachment 148884 [details]
Rebased patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148884&action=review

Looks good to me but I'm not a reviewer.

> Source/WebCore/ChangeLog:14
> +        [[
> +        If the group of selectors include namespace prefixes that need to be
> +        resolved, the implementation must raise a SYNTAX_ERR exception
> +        ]]

I'm not sure we should use trac wiki markup here?

[Ryosuke Niwa]

Comment on attachment 148884 [details]
Rebased patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148884&action=review

>> Source/WebCore/ChangeLog:14
>> +        ]]
> 
> I'm not sure we should use trac wiki markup here?

Yeah, we don't normally do that.

> Source/WebCore/ChangeLog:20
> +        Thread here:
> +        http://lists.w3.org/Archives/Public/public-webapps/2012AprJun/1225.html
> +

Are you sure the spec is stable enough for us to change the behavior?
I would not want us end up flip-flopping between different behaviors.

[Pablo Flouret]

(In reply to comment #6)
> (From update of attachment 148884 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148884&action=review
> 
> >> Source/WebCore/ChangeLog:14
> >> +        ]]
> > 
> > I'm not sure we should use trac wiki markup here?
> 
> Yeah, we don't normally do that.

It's sometimes used to refer to spec excerpts, i didn't realize it was wiki markup as well. I can use quotes if it'll bring trouble.

> > Source/WebCore/ChangeLog:20
> > +        Thread here:
> > +        http://lists.w3.org/Archives/Public/public-webapps/2012AprJun/1225.html
> > +
> 
> Are you sure the spec is stable enough for us to change the behavior?
> I would not want us end up flip-flopping between different behaviors.

Mozilla made the change already, and multiple Opera people are the ones that suggested this change.

Lachy also argues that the distinction is probably not very useful anymore (and that at least Sizzle doesn't seem to care about it):
http://lists.w3.org/Archives/Public/public-webapps/2012AprJun/1215.html

The spec is going to last call soon, and moving straight to PR if nothing comes up, if i understood correctly. I don't think it's very urgent, so it's probably ok to wait for a while before landing if you prefer.

[Eric Seidel (no email)]

Comment on attachment 148884 [details]
Rebased patch

Looks like reviewers are askign for another round.

[Pablo Flouret]

The spec moved to PR a while ago. http://lists.w3.org/Archives/Public/public-webapps/2012OctDec/0682.html

[Anne van Kesteren]

Document::selectorQueryForString() appears to handle this correctly.