Bug 83438 – querySelectorAll unable to find SVG camelCase elements imported into HTML

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: DOM

[Rob Buis]

This is related to bug 46000, but covers the querySelector part of finding SVG camelCase elements.

[Rob Buis]

Correction, related to bug 46800.

[Rob Buis]

I have no intention of working on this but will try to attach a testcase this week, unless somebody beats me to it.

[Rob Buis]

*** Bug 74919 has been marked as a duplicate of this bug. ***

[Mike Bostock]

I have a test case here: http://bl.ocks.org/1322814

I would love for this bug to be fixed soon!

[Philip Rogers]

Moving this to P1; this is high priority.

[Alexandru Chiculita]

Created attachment 176588 [details]
test case

Looks like it also affects CSS styling.

[Alexandru Chiculita]

Looks like this is a problem in the CSS Grammar. It always converts the selector tag name to lower case. However, the element tag names are stored in their canonical format, so matching will never succeed. This worked just fine for HTML elements because their canonical format is always lower case.

Also noted that the problem doesn't reproduce when CSS is not parsed as part of an HTML context (ie. SVG documents). The bug might also affect attribute names selectors for attribute names that have both lower case and upper case letters. 

Tested FF and IE10 and they both parse case-insensitive for SVG and case-sensitive for HTML.
Opera parses case-insensitive for HTML too, but getElementsByTagName will still be case-sensitive.

[Alexandru Chiculita]

Created attachment 177299 [details]
Patch V1

[Alexandru Chiculita]

Comment on attachment 177299 [details]
Patch V1

Removing review flags for now. I want to make sure the patch is not adding any performance regressions.

[Allan Sandfeld Jensen]

Comment on attachment 177299 [details]
Patch V1

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

> Source/WebCore/css/SelectorChecker.h:200
> +#if ENABLE(SVG)
> +        if (!element->document()->isHTMLDocument() || element->isSVGElement()) {
> +#else
> +        if (!element->document()->isHTMLDocument()) {
> +#endif

Shouldn't this work for all XML content embeddable in HTML? At least also MathML. Would it be possible to instead apply it to elements that has a non-default namespace?

[Alexandru Chiculita]

(In reply to comment #10)
> (From update of attachment 177299 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177299&action=review
> 
> > Source/WebCore/css/SelectorChecker.h:200
> > +#if ENABLE(SVG)
> > +        if (!element->document()->isHTMLDocument() || element->isSVGElement()) {
> > +#else
> > +        if (!element->document()->isHTMLDocument()) {
> > +#endif
> 
> Shouldn't this work for all XML content embeddable in HTML? At least also MathML. Would it be possible to instead apply it to elements that has a non-default namespace?

You're right. However, all the elements in an HTML page were matched using case-insensitive comparison. Chancing that for MathML and/or SVG would for sure break content that was tested to work only in WebKit.

For that reason, I think my initial patch is wrong anyway, even though it was aligning the behavior for SVG with other browsers. 

I will post a new patch in which I will just preserve the current behavior: case-insensitive matching for all elements in HTML documents. The patch will only fix the mixed-case tag name that only happens for some SVG elements.

[Alexandru Chiculita]

Created attachment 177795 [details]
Patch V2

[WebKit Review Bot]

Comment on attachment 177795 [details]
Patch V2

Attachment 177795 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15138750

New failing tests:
css3/selectors3/xml/css3-modsel-181.xml
css3/selectors3/xhtml/css3-modsel-181.xml

[Alexandru Chiculita]

Created attachment 177850 [details]
Patch V3

Re-added the change in StyleResolver::checkSelector that got lost during a previous rebase. This should fix the two failing tests.

[Antti Koivisto]

Comment on attachment 177850 [details]
Patch V3

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

> Source/WebCore/css/SelectorChecker.h:208
> -    const AtomicString& localName = selector->tag().localName();
> -    if (localName != starAtom && localName != element->localName())
> -        return false;
> +    const QualifiedName& selectorTag = selector->tag();
> +    if (selectorTag.localName() != starAtom) {
> +        if (element->document()->isHTMLDocument()) {
> +            // All elements inside HTML documents will use case-insensitive matching.
> +            // That applies even to SVG and MathML elements embedded in HTML. Other browsers
> +            // match SVG using case-sensitive comparison, but WebKit has always 
> +            // supported case-insensitive matching and we need to preserve that behavior
> +            // for backwards compatibility reasons.
> +            if (selectorTag.localNameUpper() != element->tagQName().localNameUpper())
> +                return false;
> +        } else {
> +            // XML based documents (XHTML and SVG) need to use case sensitive matching.
> +            if (selectorTag.localName() != element->tagQName().localName())
> +                return false;
> +        }
> +    }

The code path here is very hot and this seems certain to regress performance. I think you need to find more efficient ways to handle these rare edge cases.

[Alexandru Chiculita]

(In reply to comment #15)
> (From update of attachment 177850 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177850&action=review
> 
> > Source/WebCore/css/SelectorChecker.h:208
> > -    const AtomicString& localName = selector->tag().localName();
> > -    if (localName != starAtom && localName != element->localName())
> > -        return false;
> > +    const QualifiedName& selectorTag = selector->tag();
> > +    if (selectorTag.localName() != starAtom) {
> > +        if (element->document()->isHTMLDocument()) {
> > +            // All elements inside HTML documents will use case-insensitive matching.
> > +            // That applies even to SVG and MathML elements embedded in HTML. Other browsers
> > +            // match SVG using case-sensitive comparison, but WebKit has always 
> > +            // supported case-insensitive matching and we need to preserve that behavior
> > +            // for backwards compatibility reasons.
> > +            if (selectorTag.localNameUpper() != element->tagQName().localNameUpper())
> > +                return false;
> > +        } else {
> > +            // XML based documents (XHTML and SVG) need to use case sensitive matching.
> > +            if (selectorTag.localName() != element->tagQName().localName())
> > +                return false;
> > +        }
> > +    }
> 
> The code path here is very hot and this seems certain to regress performance. I think you need to find more efficient ways to handle these rare edge cases.

I've included some performance tests in the patch and according to my tests it should have no performance regressions. I will post some results.

[Alexandru Chiculita]

Created attachment 177878 [details]
performance results

Test results of running the performance tests added in this patch.

[Allan Sandfeld Jensen]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 177299 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=177299&action=review
> > 
> > > Source/WebCore/css/SelectorChecker.h:200
> > > +#if ENABLE(SVG)
> > > +        if (!element->document()->isHTMLDocument() || element->isSVGElement()) {
> > > +#else
> > > +        if (!element->document()->isHTMLDocument()) {
> > > +#endif
> > 
> > Shouldn't this work for all XML content embeddable in HTML? At least also MathML. Would it be possible to instead apply it to elements that has a non-default namespace?
> 
> You're right. However, all the elements in an HTML page were matched using case-insensitive comparison. Chancing that for MathML and/or SVG would for sure break content that was tested to work only in WebKit.
> 
Do you have any examples of that, or reason to believe that would happen in common cases? I could imagine all upper-case, but I find it unlikely camel-case tags would be common. Unless this is a problem in important or common cases, shouldn't we still fix it?

[Antti Koivisto]

Comment on attachment 177850 [details]
Patch V3

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

> Source/WebCore/css/SelectorChecker.h:200
> +            // All elements inside HTML documents will use case-insensitive matching.
> +            // That applies even to SVG and MathML elements embedded in HTML. Other browsers
> +            // match SVG using case-sensitive comparison, but WebKit has always 
> +            // supported case-insensitive matching and we need to preserve that behavior
> +            // for backwards compatibility reasons.

It seems to me that we should just match what other browsers do. Is this specced somewhere?

[Philip Rogers]

Progress on this bug seems to have stalled but we were so close! @Alexandru, is there any chance of reviving this?

[Alexandru Chiculita]

(In reply to comment #20)
> Progress on this bug seems to have stalled but we were so close! @Alexandru, is there any chance of reviving this?

Sorry about that, I will get back to this starting today.

[Philip Rogers]

(In reply to comment #21)
> (In reply to comment #20)
> > Progress on this bug seems to have stalled but we were so close! @Alexandru, is there any chance of reviving this?
> 
> Sorry about that, I will get back to this starting today.

The results on TypeNameSelectors look like a show-stopper, unfortunately :/. As Antti mentioned, SelectorChecker::tagMatches(...) is going to be piping hot on perf tests. An idea: there are relatively few mixed-case SVG tags. Could you do a single static hash lookup before the matching loop to see if the tag is one of the mixed-case SVG tags?

[Alexandru Chiculita]

Created attachment 182846 [details]
testcase-svg

Added a better test case to show the differences visually.

[Alexandru Chiculita]

Created attachment 182857 [details]
testcase-svg

Used linearGradient instead of foreignObject to make it work on all browsers.

[Alexandru Chiculita]

Created attachment 183324 [details]
Patch V4

This patch makes the testcase-svg pass. It's work in progress, no changelogs, no new tests. Just saving my work so far.

[Alexandru Chiculita]

(In reply to comment #25)
> Created an attachment (id=183324) [details]
> Patch V4
> 
> This patch makes the testcase-svg pass. It's work in progress, no changelogs, no new tests. Just saving my work so far.

My previous approach will not work anymore as there's been some changes in the CSSSelector.

Another approach would be to have a new m_match type in CSSSelector that is only going to be created for mixed-case tag selectors in HTML documents. It will use the RareData structure to store the qualified name with lower case + the mixed-case version. I could reuse the m_attribute and m_argument for that reason.

How does that sound? It should keep common lower case selectors as fast as before and will only impact performance on tags with mixed-case letters.

It seems like fields of the RareData structure could be collected in an union, to save some space. That would be a different patch, and will have to come before the fix for this bug. Let me know if that's a good idea or not and I could work on that too.

[Alexandru Chiculita]

Created attachment 185327 [details]
Patch V5

[Build Bot]

Comment on attachment 185327 [details]
Patch V5

Attachment 185327 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16198367

New failing tests:
fast/css/css-selector-text.html
fast/css/css-set-selector-text.html

[Alexandru Chiculita]

(In reply to comment #28)
> (From update of attachment 185327 [details])
> Attachment 185327 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://queues.webkit.org/results/16198367
> 
> New failing tests:
> fast/css/css-selector-text.html
> fast/css/css-set-selector-text.html

I'm not sure if I need to update the tests or fix the implementation. The issue is that CSSParser::updateSpecifiersWithElementName was not propagating the tagIsForNamespaceRule on the last "custom element" case.

The two tests fail with similar assets like the following one:

FAIL parseThenSerializeRule('::-webkit-file-upload-button { }') should be *::-webkit-file-upload-button { }. Was ::-webkit-file-upload-button { }.

It seems like the new implementation does the right thing, as in it preserves the selector as it was parsed. However, I don't know if the "*" was forced to get a "canonical form" instead.

I will revert that fix in this patch, as the change is orthogonal to the curent bug.

[Alexandru Chiculita]

Created attachment 185355 [details]
Patch V6

Fixed the two failing tests.

[Antti Koivisto]

Comment on attachment 185355 [details]
Patch V6

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

> Source/WebCore/ChangeLog:14
> +        However, non-HTML elements embedded in HTML need to follow the XML rule instead. They need to be matched using case-sensitive comparison.
> +        That's aligned with the behavior of FF and IE10. However, because the CSS parser already lower-cased the tag name, the matching never
> +        works for elements like "foreignObject" or "textPath", as their canonical form has both lower-case and upper-case letters.

It may be aligned with FF and IE10 but is it correct? It is still not clear to me that these complications make sense. Shouldn't everything within a document type match the same?

> Source/WebCore/css/CSSGrammar.y.in:1215
> +            AtomicString mixedCaseTagName;
> +            if (parser->m_context.isHTMLDocument && !$2.isLowerCase()) {
> +                mixedCaseTagName = $2;
> +                $2.lower();
> +            }
> +            parser->updateSpecifiersWithElementName($1, $2, mixedCaseTagName, $$);

So if someone writes selectors in capital letters DIV { foo: bar } he will end up with rare datas. That seems bad. Can this be limited to known SVG names only or something?

> Source/WebCore/css/SelectorChecker.cpp:203
> +        case CSSSelector::MixedCaseTag:
> +            if (!fastCheckSingleSelector<checkMixedCaseTagValue>(selector, element, topChildOrSubselector, topChildOrSubselectorMatchElement))
> +                return false;
> +            break;

Please don't add this to the fast path. These are supposed to be rare (they are rare in data after all!) and making fast path bigger can make it less fast.

[Alexandru Chiculita]

(In reply to comment #31)
> (From update of attachment 185355 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185355&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        However, non-HTML elements embedded in HTML need to follow the XML rule instead. They need to be matched using case-sensitive comparison.
> > +        That's aligned with the behavior of FF and IE10. However, because the CSS parser already lower-cased the tag name, the matching never
> > +        works for elements like "foreignObject" or "textPath", as their canonical form has both lower-case and upper-case letters.
> 
> It may be aligned with FF and IE10 but is it correct? It is still not clear to me that these complications make sense. Shouldn't everything within a document type match the same?
> 
The specification is not clear about the right behavior, so I thought we already decided that the right approach is to see what other browsers do.

> > Source/WebCore/css/CSSGrammar.y.in:1215
> > +            AtomicString mixedCaseTagName;
> > +            if (parser->m_context.isHTMLDocument && !$2.isLowerCase()) {
> > +                mixedCaseTagName = $2;
> > +                $2.lower();
> > +            }
> > +            parser->updateSpecifiersWithElementName($1, $2, mixedCaseTagName, $$);
> 
> So if someone writes selectors in capital letters DIV { foo: bar } he will end up with rare datas. That seems bad. Can this be limited to known SVG names only or something?
I can limit it to only the right SVG names.
 
> > Source/WebCore/css/SelectorChecker.cpp:203
> > +        case CSSSelector::MixedCaseTag:
> > +            if (!fastCheckSingleSelector<checkMixedCaseTagValue>(selector, element, topChildOrSubselector, topChildOrSubselectorMatchElement))
> > +                return false;
> > +            break;
> 
> Please don't add this to the fast path. These are supposed to be rare (they are rare in data after all!) and making fast path bigger can make it less fast.
My tests showed that it didn't have any regression, but if we limit just to SVG elements, it makes sense to be removed from the fast path.

[Jason Davies]

Is there any chance we could get this resolved soon?  We get asked about this for D3 quite often:

https://github.com/mbostock/d3/issues/1235
https://github.com/mbostock/d3/issues/2020

Thanks!

[Dirk Schulze]

(In reply to comment #33)
> Is there any chance we could get this resolved soon?  We get asked about this for D3 quite often:
> 
> https://github.com/mbostock/d3/issues/1235
> https://github.com/mbostock/d3/issues/2020
> 
> Thanks!

It is indeed asked for quite often. Not just from D3. I am not sure if Alex has the time to continue this patch. Antti, could you take a look at the patch again and see how much work would need to be done?

[Benjamin Poulain]

Follow up with your tests in https://bugs.webkit.org/show_bug.cgi?id=140930

*** This bug has been marked as a duplicate of bug 140878 ***

This thread is confusing.  It appears to be saying that it is a duplicate of a bug that has been fixed.  That's wrong. After 3 years, this bug is not fixed.

[Jason Davies]

It seems to work as expected e.g. see this test case: http://bl.ocks.org/mbostock/1322814

If something isn't working as expected, I'd advise posting further details and perhaps opening a new issue.

Still broken in Webkit and blink http://bl.ocks.org/cool-Blue/14f575ae6945769aca39

[Jason Davies]

(In reply to comment #38)
> Still broken in Webkit and blink
> http://bl.ocks.org/cool-Blue/14f575ae6945769aca39

You should test using WebKit nightly (I just tested with r185399).  The expected behaviour is that camelCase selectors should return non-empty lists.

As for all-lowercase selectors, perhaps this is a matter for discussion, but since SVG is case-sensitive I'm guessing they should continue returning an empty list if they don't match the case exactly.

Since this is working as expected in the nightly WebKit, I would expect it will eventually become part of an official release (e.g. Safari).

Related, a similar (now resolved) bug in Firefox: https://bugs.webkit.org/show_bug.cgi?id=46800

As for Chrome, which no longer uses WebKit, I've tested in Chrome Canary (45.0.2428.0) and the test case works as expected, though Chrome appears to perform a case-insensitive match now.  I believe this change was made here: https://codereview.chromium.org/1099963003/ - arguably I think the WebKit/Firefox exact case match approach is better.

To summarise, I believe the bug is fixed, but hasn't made it to a stable release yet.

(In reply to comment #39)
> (In reply to comment #38)
> > Still broken in Webkit and blink
> > http://bl.ocks.org/cool-Blue/14f575ae6945769aca39
> 
> You should test using WebKit nightly (I just tested with r185399).  The
> expected behaviour is that camelCase selectors should return non-empty lists.
> 
> As for all-lowercase selectors, perhaps this is a matter for discussion, but
> since SVG is case-sensitive I'm guessing they should continue returning an
> empty list if they don't match the case exactly.
> 
> Since this is working as expected in the nightly WebKit, I would expect it
> will eventually become part of an official release (e.g. Safari).
> 
> Related, a similar (now resolved) bug in Firefox:
> https://bugs.webkit.org/show_bug.cgi?id=46800
> 
> As for Chrome, which no longer uses WebKit, I've tested in Chrome Canary
> (45.0.2428.0) and the test case works as expected, though Chrome appears to
> perform a case-insensitive match now.  I believe this change was made here:
> https://codereview.chromium.org/1099963003/ - arguably I think the
> WebKit/Firefox exact case match approach is better.
> 
> To summarise, I believe the bug is fixed, but hasn't made it to a stable
> release yet.

OK, thanks for taking the time to advise on that.
If camelCase returns a non-empty selection then it's fine I would suggest.  As long as the successful selection coincides with with the successful rendering I don't really care.  But anyway, the standard is clear on camelCase, so, even better.  
Currently lowercase returns non-zero selection by the way, but doesn't render.

[Jason Davies]

(In reply to comment #40)  
> Currently lowercase returns non-zero selection by the way, but doesn't
> render.

No, it works as expected and returns an empty selection for all-lowercase (WebKit nightly).  As I mentioned, I believe this is the correct behaviour, and I'm not sure why Chromium has implemented case-insensitive matching.

Anyway, let's reserve further discussion about case-sensitivity for a future Chromium/Blink bug report, to avoid spamming the good people subscribed to this (resolved) bug.