Bug 38095 – webkit should implement -moz-any selector (as -webkit-any obviously)

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Ojan Vafai]

http://dbaron.org/log/20100424-any

This is awesome and takes away one of the frequent frustrations with CSS.

[Dave Hyatt]

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

[Trevor Downs]

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

[Rémi Zara]

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.

[WebKit Review Bot]

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.

[Rémi Zara]

Created attachment 81612 [details]
Patch v2

Fix style.

[Kent Tamura]

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.

[Trevor Downs]

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.

[Ojan Vafai]

Created attachment 86131 [details]
Patch

[WebKit Review Bot]

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.

[Ojan Vafai]

(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.

[Antti Koivisto]

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)

[Ojan Vafai]

(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.

[Ojan Vafai]

(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?

[Ojan Vafai]

Comment on attachment 86131 [details]
Patch

Marking for review again per the above comment.

[Antti Koivisto]

(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.

[Antti Koivisto]

Comment on attachment 86131 [details]
Patch

r=me

[Ojan Vafai]

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

[James Simonsen]

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

[Ojan Vafai]

Looking

[Ojan Vafai]

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

[Ojan Vafai]

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

[WebKit Review Bot]

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

[Masataka Yakura]

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