Support type selector for camel-cased SVG elements in HTML.

Support type selector for camel-cased SVG elements in HTML.

In HTML documents, type selectors are case-insensitive for HTML elements,
but case sensitive for SVG elements [1]. We store the lower case version
of the type selector. For SVG elements, the tag name is normalized to
the camel-case version during parsing. That means we would never match
SVG tag names containing upper-case letters in HTML documents.

This CL matches SVG tag names case-insensitively in HTML documents. That
is not correct according to spec, but at least makes it possible to use
type selectors for SVG elements in HTML documents.

Since the tag name for SVG elements are still stored camelCased, we need
to lower the localName() before using it collecting tag rules from rule
sets or collecting tag name hashes for the the bloom filter.

[1] https://html.spec.whatwg.org/multipage/scripting.html#selectors

BUG=237435
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195038

[rune, 2015-04-22 14:41:19]

I think I'm ready for review now.

Camel-cased elements (like foreignObject) and attributes from SVG did not work
with type selectors in HTML.

There is the same problem for camel-cased attribute selectors. I haven't fixed
that yet.

With the latest patch set, I don't see perf-regressions for blink_perf.css, at
least.

[esprehn, 2015-04-22 15:57:57]

I'd really like to avoid adding more flags and this complexity. Especially
needing to create this map, we should just do std::lower_bound over a sorted
list of the names. It's always the same list. 

Do other browsers do this?

https://codereview.chromium.org/1099963003/diff/60001/Source/core/css/Selecto...
File Source/core/css/SelectorFilter.cpp (right):

https://codereview.chromium.org/1099963003/diff/60001/Source/core/css/Selecto...
Source/core/css/SelectorFilter.cpp:123: if (!selector.tagIsCamelCase() &&
selector.tagQName().localName() != starAtom)
This doesn't make sense. How does being camel case have any impact on if you
match the star?

[esprehn, 2015-04-22 16:18:02]

esprehn@chromium.org changed reviewers:
+ timloh@chromium.org

[esprehn, 2015-04-22 16:18:58]

FYI, I'm on vacation until Monday. timloh@ want to take this?

[rune, 2015-04-22 21:28:02]

On 2015/04/22 15:57:57, esprehn wrote:
> I'd really like to avoid adding more flags and this complexity. Especially
> needing to create this map, we should just do std::lower_bound over a sorted
> list of the names. It's always the same list. 
> 
> Do other browsers do this?

Gecko passes all tests in foreign-object-case-sensitivity.html

WebKit has http://trac.webkit.org/changeset/179132, I don't have a build to test
with.

[rune, 2015-04-22 21:48:00]

On 2015/04/22 15:57:57, esprehn wrote:
> I'd really like to avoid adding more flags and this complexity. Especially
> needing to create this map, we should just do std::lower_bound over a sorted
> list of the names. It's always the same list. 

Isn't that going to be too slow for selector matching? An alternative is to
store both the camel-case and the lower-case strings in the selector, which is
what the WebKit patch does, including a separate match type for it.

[rune, 2015-04-22 21:48:24]

https://codereview.chromium.org/1099963003/diff/60001/Source/core/css/Selecto...
File Source/core/css/SelectorFilter.cpp (right):

https://codereview.chromium.org/1099963003/diff/60001/Source/core/css/Selecto...
Source/core/css/SelectorFilter.cpp:123: if (!selector.tagIsCamelCase() &&
selector.tagQName().localName() != starAtom)
On 2015/04/22 15:57:57, esprehn wrote:
> This doesn't make sense. How does being camel case have any impact on if you
> match the star? 

starAtom is not added to the bloom filter for any elements, so selectors with
starAtom should not cause rejection. <foreignobject> should match foreignObject
type selector, so foreignObject selectors should not be rejected if
foreignobject is pushed into the filter. As the description says, the
alternative is to push both foreignobject and foreignObject for <foreignobject>,
which would add complexity to the push part instead.

[rune, 2015-04-29 10:43:07]

ping

[esprehn, 2015-04-30 01:04:42]

On 2015/04/29 at 10:43:07, rune wrote:
> ping

I talked to tab about this, why not just treat SVG case insensitively always?

[rune, 2015-04-30 11:20:39]

On 2015/04/30 01:04:42, esprehn wrote:
> On 2015/04/29 at 10:43:07, rune wrote:
> > ping
> 
> I talked to tab about this, why not just treat SVG case insensitively always?

I don't know the rationale for the way it's spec'ed. Hixie would know, I guess.
I found some very long posts on the whatwg list, but not able to find a
definitive answer. What's really weird is the inconsistency between
case-insensitivity of tag names in the source and the case-sensitivity of the
selector against the camelCased form the tags names are transformed into.

[esprehn, 2015-05-01 05:47:02]

On 2015/04/30 at 11:20:39, rune wrote:
> On 2015/04/30 01:04:42, esprehn wrote:
> > On 2015/04/29 at 10:43:07, rune wrote:
> > > ping
> > 
> > I talked to tab about this, why not just treat SVG case insensitively
always?
> 
> I don't know the rationale for the way it's spec'ed. Hixie would know, I
guess. I found some very long posts on the whatwg list, but not able to find a
definitive answer. What's really weird is the inconsistency between
case-insensitivity of tag names in the source and the case-sensitivity of the
selector against the camelCased form the tags names are transformed into.

I talked to Tab about this more today, I think the agreement is to just make
selectors always match everything case insensitively. So we'd change
matchesTagName to have a check for isHTMLElement() and do what it does now, and
another case where it does a case insensitive compare of the localName (and
namespace?)

[rune, 2015-05-05 09:39:31]

On 2015/05/01 05:47:02, esprehn wrote:
> On 2015/04/30 at 11:20:39, rune wrote:
> > On 2015/04/30 01:04:42, esprehn wrote:
> > > On 2015/04/29 at 10:43:07, rune wrote:
> > > > ping
> > > 
> > > I talked to tab about this, why not just treat SVG case insensitively
> always?
> > 
> > I don't know the rationale for the way it's spec'ed. Hixie would know, I
> guess. I found some very long posts on the whatwg list, but not able to find a
> definitive answer. What's really weird is the inconsistency between
> case-insensitivity of tag names in the source and the case-sensitivity of the
> selector against the camelCased form the tags names are transformed into.
> 
> I talked to Tab about this more today, I think the agreement is to just make
> selectors always match everything case insensitively. So we'd change
> matchesTagName to have a check for isHTMLElement() and do what it does now,
and
> another case where it does a case insensitive compare of the localName (and
> namespace?)

I've uploaded new patch sets and modified the description with notes in both
description and tests about the spec violation.

I've run blink_perf.css which looks ok locally.

[rune, 2015-05-05 13:36:48]

https://codereview.chromium.org/1099963003/diff/140001/Source/core/css/Select...
File Source/core/css/SelectorChecker.h (right):

https://codereview.chromium.org/1099963003/diff/140001/Source/core/css/Select...
Source/core/css/SelectorChecker.h:32: #include "core/dom/Document.h"
Another rebase-leftover.

[rune, 2015-05-05 13:38:00]

https://codereview.chromium.org/1099963003/diff/140001/Source/core/css/Select...
File Source/core/css/SelectorChecker.h (right):

https://codereview.chromium.org/1099963003/diff/140001/Source/core/css/Select...
Source/core/css/SelectorChecker.h:32: #include "core/dom/Document.h"
On 2015/05/05 13:36:48, rune wrote:
> Another rebase-leftover.

Done.

[esprehn, 2015-05-07 02:21:27]

This looks totally reasonable to me, but your patch description needs an update.

https://codereview.chromium.org/1099963003/diff/160001/Source/core/css/Elemen...
File Source/core/css/ElementRuleCollector.cpp (right):

https://codereview.chromium.org/1099963003/diff/160001/Source/core/css/Elemen...
Source/core/css/ElementRuleCollector.cpp:170: if (!element.isHTMLElement() &&
element.document().isHTMLDocument())
You do this in two places, should we add a tagNameForSelectorMatching() method?
It's not much code so we can leave this too.

https://codereview.chromium.org/1099963003/diff/160001/Source/core/dom/Select...
File Source/core/dom/SelectorQuery.cpp (right):

https://codereview.chromium.org/1099963003/diff/160001/Source/core/dom/Select...
Source/core/dom/SelectorQuery.cpp:178: inline bool tagNameMatches(const
QualifiedName& tagName, const Element& element)
Can you add a comment for what this is about? (here and in the other place.

Also maybe matchesTagName so the functions are consistent.

[rune, 2015-05-07 07:59:48]

https://codereview.chromium.org/1099963003/diff/160001/Source/core/css/Elemen...
File Source/core/css/ElementRuleCollector.cpp (right):

https://codereview.chromium.org/1099963003/diff/160001/Source/core/css/Elemen...
Source/core/css/ElementRuleCollector.cpp:170: if (!element.isHTMLElement() &&
element.document().isHTMLDocument())
On 2015/05/07 02:21:27, esprehn wrote:
> You do this in two places, should we add a tagNameForSelectorMatching()
method?
> It's not much code so we can leave this too.

Done.

Named it localNameForSelectorMatching as that is more accurate.

https://codereview.chromium.org/1099963003/diff/160001/Source/core/dom/Select...
File Source/core/dom/SelectorQuery.cpp (right):

https://codereview.chromium.org/1099963003/diff/160001/Source/core/dom/Select...
Source/core/dom/SelectorQuery.cpp:178: inline bool tagNameMatches(const
QualifiedName& tagName, const Element& element)
On 2015/05/07 02:21:27, esprehn wrote:
> Can you add a comment for what this is about? (here and in the other place.

Done.

> Also maybe matchesTagName so the functions are consistent.

Done.

[rune, 2015-05-07 08:01:09]

On 2015/05/07 02:21:27, esprehn wrote:
> This looks totally reasonable to me, but your patch description needs an
update.

I already did that. Anything wrong or missing?

[esprehn, 2015-05-07 08:20:28]

lgtm

[esprehn, 2015-05-07 08:20:45]

On 2015/05/07 at 08:01:09, rune wrote:
> On 2015/05/07 02:21:27, esprehn wrote:
> > This looks totally reasonable to me, but your patch description needs an
update.
> 
> I already did that. Anything wrong or missing?

Nope I read it wrong.

[rune, 2015-05-07 08:21:22]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2015-05-07 08:21:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1099963003/180001

[commit-bot: I haz the power, 2015-05-07 09:50:03]

Committed patchset #10 (id:180001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195038