Summary: | CSS Selectors Level 4: Add parsing for :matches | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||
Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | benjamin, commit-queue, syoichi | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Yusuke Suzuki
2014-10-02 10:49:55 PDT
ref: http://dev.w3.org/csswg/selectors4/#matches http://dev.w3.org/csswg/selectors4/#selector-list :matches(selector list) Created attachment 239133 [details]
Patch
Created attachment 239139 [details]
Patch
Comment on attachment 239139 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239139&action=review Exciting! > Source/WebCore/ChangeLog:11 > + Curretnly :not doesn't accept any functional pseudo classes, :not(:matches(...)) is rejected. Typo: Curretnly > Source/WebCore/ChangeLog:12 > + And currently, :matches(:visited, :link) is allowed in the parsing phase. That's okay, our specialized behavior can be at runtime. > Source/WebCore/css/CSSSelector.cpp:450 > + const CSSSelector* firstSubSelector = cs->selectorList()->first(); > + for (const CSSSelector* subSelector = firstSubSelector; subSelector; subSelector = CSSSelectorList::next(subSelector)) { > + if (subSelector != firstSubSelector) > + str.appendLiteral(", "); > + str.append(subSelector->selectorText()); > + } It would be a good idea to move this into a static function. The code of :nth-child(of) does more or less the same thing, both could use the same function. > LayoutTests/ChangeLog:7 > + Wow, that's some serious testing going on here! :) > LayoutTests/ChangeLog:32 > + Maybe I missed it, but I did not see any test for canonicalization of the selector when accessed through CSS OM. For example, testing that :matches(a,b,c) returns :matches(a, b, c). > LayoutTests/fast/css/parsing-css-matches-4.html:29 > + // "*", I guess you commented some for efficiency? Better remove them entirely, someone could be confused by the commented ones. > LayoutTests/fast/css/parsing-css-matches-5.html:22 > +var invalidSelectors = [ Let's add unbalanced parenthesis to this set: -:not( -:matches( -:nth-child(2n+1 of .foo Comment on attachment 239139 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239139&action=review >> Source/WebCore/css/CSSSelector.cpp:450 >> + } > > It would be a good idea to move this into a static function. The code of :nth-child(of) does more or less the same thing, both could use the same function. Yes! I've extracted it to `appendSelectorList` static function. >> LayoutTests/ChangeLog:32 >> + > > Maybe I missed it, but I did not see any test for canonicalization of the selector when accessed through CSS OM. > > For example, testing that :matches(a,b,c) returns :matches(a, b, c). Ah! Thanks for your pointing. I missed it. I'll add it :) >> LayoutTests/fast/css/parsing-css-matches-4.html:29 >> + // "*", > > I guess you commented some for efficiency? Better remove them entirely, someone could be confused by the commented ones. Ah! OK, I'll remove them. Yup. In this test, we generate the selector list composed of 3 selectors. So there are `n! where n = validSelectors.length` selectors to be tested. It takes too much time and easily exceeds the time limit. To avoid this, I reduced the number of validSelectors to 1/2. And the comment outed 1/2 are tested in the parsing-css-matches-3.html. >> LayoutTests/fast/css/parsing-css-matches-5.html:22 >> +var invalidSelectors = [ > > Let's add unbalanced parenthesis to this set: > -:not( > -:matches( > -:nth-child(2n+1 of .foo Looks very nice. I'll add them. Created attachment 239177 [details]
Patch
Comment on attachment 239139 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239139&action=review >>> LayoutTests/fast/css/parsing-css-matches-4.html:29 >>> + // "*", >> >> I guess you commented some for efficiency? Better remove them entirely, someone could be confused by the commented ones. > > Ah! OK, I'll remove them. > Yup. In this test, we generate the selector list composed of 3 selectors. > So there are `n! where n = validSelectors.length` selectors to be tested. > It takes too much time and easily exceeds the time limit. > To avoid this, I reduced the number of validSelectors to 1/2. And the comment outed 1/2 are tested in the parsing-css-matches-3.html. s/`n! where n = validSelectors.length`/`nC3 where n = validSelectors.length`/ Comment on attachment 239177 [details]
Patch
Looks good to me.
Comment on attachment 239177 [details] Patch Clearing flags on attachment: 239177 Committed r174259: <http://trac.webkit.org/changeset/174259> All reviewed patches have been landed. Closing bug. |