Bug 38095

Seems easy enough. It could replace -webkit-any-link also, since that could just turn into :-webkit-any(:link, :visited).

A copy of the article was posted at http://hacks.mozilla.org/2010/05/moz-any-selector-grouping/

Created attachment 81611 [details] patch v1 Here is a patch. Specificity is handled as gecko (that of a pseudo class), and should be fixed in a followup bug. No other selectors removed or rules changed in .css files. Had to move the inline CSSSelectorList::next() to the .cpp file to handle the introduced dependency of CSSSelector on CSSSelectorList. Not sure that's right... Finally, I had trouble testing for :hover. It works when manually testing though. Pointers welcome for that.

Attachment 81611 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/css/CSSSelectorList.cpp:28: You should add a blank line after implementation file's own header. [build/include_order] [4] Source/WebCore/css/CSSSelector.cpp:688: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/css/CSSStyleSelector.cpp:2370: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 3 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 81612 [details] Patch v2 Fix style.

Comment on attachment 81612 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=81612&action=review I'm not familiar with CSS parser and selector. However I set r- because of a failing test. > Source/WebCore/css/CSSParser.h:176 > + Vector<OwnPtr<CSSSelectorList> >* createFloatingSelectorList(); > + PassOwnPtr<Vector<OwnPtr<CSSSelectorList> > > sinkFloatingSelectorList(Vector<OwnPtr<CSSSelectorList> >*); We had better have a typedef for Vector<OwnPtr<CSSSelectorList>>. It would make the code simpler. > Source/WebCore/css/CSSStyleSelector.cpp:2368 > + if (simpleSelectorList->size() !=1) Need a space before 1. > LayoutTests/fast/css/pseudo-any-expected.txt:80 > +TEST COMPLETE > +FAIL document.defaultView.getComputedStyle(elhover, null).getPropertyValue('color') should be rgb(0, 128, 0). Was rgb(255, 0, 0). There is a test after "TEST COMPLETE" and the test is failing. They're bad. We should use async script test. Please look at some tests with "jsTestIsAsync = true". > LayoutTests/fast/css/pseudo-any.html:110 > +shouldBe("document.defaultView.getComputedStyle(el, null).getPropertyValue('color')", "'rgb(0, 128, 0)'"); We had better introduce a function like getComputedColor(element). > LayoutTests/fast/css/pseudo-any.html:167 > +if (window.layoutTestController) { > + elhover = document.getElementById("span_5"); > + shouldBe("document.defaultView.getComputedStyle(elhover, null).getPropertyValue('color')", "'rgb(255, 0, 0)'"); > + var targetElem = document.getElementById("p_5"); > + eventSender.mouseMoveTo(targetElem.offsetLeft + 1, targetElem.offsetTop + 1); > + window.setTimeout(function() { > + shouldBe("document.defaultView.getComputedStyle(elhover, null).getPropertyValue('color')", "'rgb(0, 128, 0)'"); > + window.layoutTestController.notifyDone(); > + },50); Indentation looks not good.

I'd like to see the :any() selector extended to properties as well. For example, I'd like to simplify the following code: > div { > border: 1px solid black; > -webkit-border-radius: 1em; > -moz-border-radius: 1em; > -o-border-radius: 1em; > -ms-border-radius: 1em; > border-radius: 1em; > } To something like this: > div { > border: 1px solid black; > :any(-webkit-, -moz-, -o-, -ms-, '')border-radius: 1em; > } or > div { > border: 1px solid black; > [-webkit-, -moz-, -o-, -ms-, '']border-radius: 1em; > } The last argument here is an empty string to make it just `border-radius`. Thus one line can stand in for the half dozen lines of css needed for compatibility of new features, and would simplify backwards compatibility as well.

Created attachment 86131 [details] Patch

Attachment 86131 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/css/CSSSelector.h:32: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #9) > Source/WebCore/css/CSSSelector.h:32: Code inside a namespace should not be indented. [whitespace/indent] [4] I'm matching the code around it.

Comment on attachment 86131 [details] Patch Looks generally good. You will need to modify RuleSet::collectFeatures() and pals (in CSSStyleSelector.cpp) to scan the content of the :any selector lists too. Otherwise you get bugs at least with style sharing. r- for that. As you note, you should (probably in a separate patch) eliminate CSSSelector::m_simpleSelector in favor of using m_selectorList. Matching of these won't be very well optimized as neither the ancestor identifier filter or fastCheckSelector can be used with them. If they get popular we need to think of ways to optimize them. (personally I don't think typedeffing stuff like Vector<OwnPtr<CSSParserSelector> > is always helpful. It tends to make code more opaque)

(In reply to comment #11) > (From update of attachment 86131 [details]) > Looks generally good. Thanks for the quick review! > You will need to modify RuleSet::collectFeatures() and pals (in CSSStyleSelector.cpp) to scan the content of the :any selector lists too. Otherwise you get bugs at least with style sharing. r- for that. Will fix. > As you note, you should (probably in a separate patch) eliminate CSSSelector::m_simpleSelector in favor of using m_selectorList. Yup. Plan to do this immediately after landing this. > Matching of these won't be very well optimized as neither the ancestor identifier filter or fastCheckSelector can be used with them. If they get popular we need to think of ways to optimize them. I'd like to figure this out in a followup patch. Ideally :any should be no slower than the long-form version. > (personally I don't think typedeffing stuff like Vector<OwnPtr<CSSParserSelector> > is always helpful. It tends to make code more opaque) I agree.

(In reply to comment #12) > > You will need to modify RuleSet::collectFeatures() and pals (in CSSStyleSelector.cpp) to scan the content of the :any selector lists too. Otherwise you get bugs at least with style sharing. r- for that. > > Will fix. Responded too quickly. I think the current patch does this. I do have at least one case testing style sharing in fast/css/pseudo-any.html (line 125). RuleSet::collectFeatures calls collectFeaturesFromList, which in turn calls collectFeaturesFromSelector, which is modified in this patch to handle :any selectors. The part I'm not totally sure about is whether I need to modify RuleSet::addRule to handle :any selectors. Right now they're get inserted into m_universalRules. So, collectFeaturesFromList does get called on them. What do you think? I guess this is the bit that needs figuring out to make fastCheckSelector work?

Comment on attachment 86131 [details] Patch Marking for review again per the above comment.

(In reply to comment #13) > Responded too quickly. I think the current patch does this. I do have at least one case testing style sharing in fast/css/pseudo-any.html (line 125). RuleSet::collectFeatures calls collectFeaturesFromList, which in turn calls collectFeaturesFromSelector, which is modified in this patch to handle :any selectors. Ah, yes, you do. I totally didn't see that. > The part I'm not totally sure about is whether I need to modify RuleSet::addRule to handle :any selectors. Right now they're get inserted into m_universalRules. So, collectFeaturesFromList does get called on them. What do you think? I guess this is the bit that needs figuring out to make fastCheckSelector work? Getting inserted to m_universal rules list is bad for performance as they get compared against every element in the document. They should likely be inserted multiple times (if the topmost selector is :any) to all the correct more specific lists. However, it is probably better to leave optimization for future patches.

Comment on attachment 86131 [details] Patch r=me

Committed r81742: <http://trac.webkit.org/changeset/81742>

The unknown-pseudo test fails on most of the Chrome bots. Can you take a look? http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&group=%40ToT%20-%20chromium.org&tests=fast%2Fdom%2FSelectorAPI%2Funknown-pseudo.html

Looking

Another case where V8's more specific errors are different than JSC's. Sigh. I'll add new results.

Committed r81744: <http://trac.webkit.org/changeset/81744>

http://trac.webkit.org/changeset/81742 might have broken SnowLeopard Intel Release (WebKit2 Tests) The following tests are not passing: fast/css/pseudo-any.html

There's now an Editor's Draft of Selectors 4, introducing the :matches() pseudo-class which gives the same functionality as :any(). http://dev.w3.org/csswg/selectors4/#matches