Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(624)

Issue 1099963003: Support type selector for camel-cased SVG elements in HTML. (Closed)

Created:
5 years, 6 months ago by rune
Modified:
5 years, 5 months ago
Reviewers:
pdr., esprehn, Timothy Loh
CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-dom_chromium.org, dglazkov+blink, eae+blinkwatch, ed+blinkwatch_opera.com, rwlbuis, sof, Tab Atkins
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Use map lookup instead of lower() method. #

Patch Set 3 : Use an enum for tag casing and fix an introduced implicit type selector bug #

Patch Set 4 : Fix performance regression. tagMatches() became too big to be inlined on Linux. #

Total comments: 2

Patch Set 5 : Always case-insensitive in HTML documents #

Patch Set 6 : Added case-sensitive test #

Patch Set 7 : Rebased #

Patch Set 8 : Rebase issue. querySelector didn't use tagMatches anymore. #

Total comments: 2

Patch Set 9 : Removed stray include #

Total comments: 4

Patch Set 10 : Addressed review issues. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+211 lines, -5 lines) Patch
A LayoutTests/fast/css/foreignObject-bloom-filter.html View 1 chunk +11 lines, -0 lines 0 comments Download
A LayoutTests/fast/css/foreignObject-bloom-filter-expected.txt View 1 chunk +10 lines, -0 lines 0 comments Download
A LayoutTests/fast/svg/foreign-object-case-sensitivity.html View 1 2 3 4 1 chunk +77 lines, -0 lines 0 comments Download
A LayoutTests/fast/svg/foreign-object-case-sensitivity-expected.txt View 1 2 3 4 1 chunk +55 lines, -0 lines 0 comments Download
A LayoutTests/svg/css/type-selector.svg View 1 2 3 4 5 1 chunk +18 lines, -0 lines 0 comments Download
A LayoutTests/svg/css/type-selector-expected.txt View 1 2 3 4 5 1 chunk +3 lines, -0 lines 0 comments Download
M Source/core/css/ElementRuleCollector.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M Source/core/css/SelectorChecker.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +10 lines, -2 lines 0 comments Download
M Source/core/css/SelectorFilter.cpp View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -1 line 0 comments Download
M Source/core/dom/Element.h View 1 2 3 4 5 6 7 8 9 2 chunks +8 lines, -0 lines 0 comments Download
M Source/core/dom/SelectorQuery.cpp View 1 2 3 4 5 6 7 8 9 2 chunks +16 lines, -1 line 0 comments Download

Messages

Total messages: 22 (2 generated)
rune
I think I'm ready for review now. Camel-cased elements (like foreignObject) and attributes from SVG ...
5 years, 6 months ago (2015-04-22 14:41:19 UTC) #1
esprehn
I'd really like to avoid adding more flags and this complexity. Especially needing to create ...
5 years, 6 months ago (2015-04-22 15:57:57 UTC) #2
esprehn
FYI, I'm on vacation until Monday. timloh@ want to take this?
5 years, 6 months ago (2015-04-22 16:18:58 UTC) #4
rune
On 2015/04/22 15:57:57, esprehn wrote: > I'd really like to avoid adding more flags and ...
5 years, 6 months ago (2015-04-22 21:28:02 UTC) #5
rune
On 2015/04/22 15:57:57, esprehn wrote: > I'd really like to avoid adding more flags and ...
5 years, 6 months ago (2015-04-22 21:48:00 UTC) #6
rune
https://codereview.chromium.org/1099963003/diff/60001/Source/core/css/SelectorFilter.cpp File Source/core/css/SelectorFilter.cpp (right): https://codereview.chromium.org/1099963003/diff/60001/Source/core/css/SelectorFilter.cpp#newcode123 Source/core/css/SelectorFilter.cpp:123: if (!selector.tagIsCamelCase() && selector.tagQName().localName() != starAtom) On 2015/04/22 15:57:57, ...
5 years, 6 months ago (2015-04-22 21:48:24 UTC) #7
rune
ping
5 years, 6 months ago (2015-04-29 10:43:07 UTC) #8
esprehn
On 2015/04/29 at 10:43:07, rune wrote: > ping I talked to tab about this, why ...
5 years, 6 months ago (2015-04-30 01:04:42 UTC) #9
rune
On 2015/04/30 01:04:42, esprehn wrote: > On 2015/04/29 at 10:43:07, rune wrote: > > ping ...
5 years, 6 months ago (2015-04-30 11:20:39 UTC) #10
esprehn
On 2015/04/30 at 11:20:39, rune wrote: > On 2015/04/30 01:04:42, esprehn wrote: > > On ...
5 years, 6 months ago (2015-05-01 05:47:02 UTC) #11
rune
On 2015/05/01 05:47:02, esprehn wrote: > On 2015/04/30 at 11:20:39, rune wrote: > > On ...
5 years, 5 months ago (2015-05-05 09:39:31 UTC) #12
rune
https://codereview.chromium.org/1099963003/diff/140001/Source/core/css/SelectorChecker.h File Source/core/css/SelectorChecker.h (right): https://codereview.chromium.org/1099963003/diff/140001/Source/core/css/SelectorChecker.h#newcode32 Source/core/css/SelectorChecker.h:32: #include "core/dom/Document.h" Another rebase-leftover.
5 years, 5 months ago (2015-05-05 13:36:48 UTC) #13
rune
https://codereview.chromium.org/1099963003/diff/140001/Source/core/css/SelectorChecker.h File Source/core/css/SelectorChecker.h (right): https://codereview.chromium.org/1099963003/diff/140001/Source/core/css/SelectorChecker.h#newcode32 Source/core/css/SelectorChecker.h:32: #include "core/dom/Document.h" On 2015/05/05 13:36:48, rune wrote: > Another ...
5 years, 5 months ago (2015-05-05 13:38:00 UTC) #14
esprehn
This looks totally reasonable to me, but your patch description needs an update. https://codereview.chromium.org/1099963003/diff/160001/Source/core/css/ElementRuleCollector.cpp File ...
5 years, 5 months ago (2015-05-07 02:21:27 UTC) #15
rune
https://codereview.chromium.org/1099963003/diff/160001/Source/core/css/ElementRuleCollector.cpp File Source/core/css/ElementRuleCollector.cpp (right): https://codereview.chromium.org/1099963003/diff/160001/Source/core/css/ElementRuleCollector.cpp#newcode170 Source/core/css/ElementRuleCollector.cpp:170: if (!element.isHTMLElement() && element.document().isHTMLDocument()) On 2015/05/07 02:21:27, esprehn wrote: ...
5 years, 5 months ago (2015-05-07 07:59:48 UTC) #16
rune
On 2015/05/07 02:21:27, esprehn wrote: > This looks totally reasonable to me, but your patch ...
5 years, 5 months ago (2015-05-07 08:01:09 UTC) #17
esprehn
lgtm
5 years, 5 months ago (2015-05-07 08:20:28 UTC) #18
esprehn
On 2015/05/07 at 08:01:09, rune wrote: > On 2015/05/07 02:21:27, esprehn wrote: > > This ...
5 years, 5 months ago (2015-05-07 08:20:45 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1099963003/180001
5 years, 5 months ago (2015-05-07 08:21:44 UTC) #21
commit-bot: I haz the power
5 years, 5 months ago (2015-05-07 09:50:03 UTC) #22
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195038

Powered by Google App Engine
This is Rietveld 408576698