Bug 38095 - webkit should implement -moz-any selector (as -webkit-any obviously)
: webkit should implement -moz-any selector (as -webkit-any obviously)
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
: 52693
  Show dependency treegraph
 
Reported: 2010-04-25 07:31 PST by
Modified: 2011-03-23 23:53 PST (History)


Attachments
patch v1 (42.29 KB, patch)
2011-02-08 01:04 PST, Rémi Zara
no flags Review Patch | Details | Formatted Diff | Diff
Patch v2 (42.26 KB, patch)
2011-02-08 01:16 PST, Rémi Zara
no flags Review Patch | Details | Formatted Diff | Diff
Patch (39.86 KB, patch)
2011-03-17 20:11 PST, Ojan Vafai
koivisto: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-04-25 07:31:59 PST
http://dbaron.org/log/20100424-any

This is awesome and takes away one of the frequent frustrations with CSS.
------- Comment #1 From 2010-04-25 20:12:23 PST -------
Seems easy enough.  It could replace -webkit-any-link also, since that could just turn into :-webkit-any(:link, :visited).
------- Comment #2 From 2010-07-07 22:05:03 PST -------
A copy of the article was posted at http://hacks.mozilla.org/2010/05/moz-any-selector-grouping/
------- Comment #3 From 2011-02-08 01:04:02 PST -------
Created an attachment (id=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.
------- Comment #4 From 2011-02-08 01:05:45 PST -------
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.
------- Comment #5 From 2011-02-08 01:16:14 PST -------
Created an attachment (id=81612) [details]
Patch v2

Fix style.
------- Comment #6 From 2011-02-19 07:28:05 PST -------
(From update of attachment 81612 [details])
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.
------- Comment #7 From 2011-03-07 16:47:12 PST -------
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.
------- Comment #8 From 2011-03-17 20:11:48 PST -------
Created an attachment (id=86131) [details]
Patch
------- Comment #9 From 2011-03-17 22:32:45 PST -------
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.
------- Comment #10 From 2011-03-17 22:54:34 PST -------
(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 #11 From 2011-03-21 02:25:21 PST -------
(From update of attachment 86131 [details])
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)
------- Comment #12 From 2011-03-21 20:30:44 PST -------
(In reply to comment #11)
> (From update of attachment 86131 [details] [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.
------- Comment #13 From 2011-03-21 20:52:16 PST -------
(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 #14 From 2011-03-21 20:52:39 PST -------
(From update of attachment 86131 [details])
Marking for review again per the above comment.
------- Comment #15 From 2011-03-22 01:02:30 PST -------
(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 #16 From 2011-03-22 01:02:46 PST -------
(From update of attachment 86131 [details])
r=me
------- Comment #17 From 2011-03-22 18:45:02 PST -------
Committed r81742: <http://trac.webkit.org/changeset/81742>
------- Comment #18 From 2011-03-22 19:36:59 PST -------
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
------- Comment #19 From 2011-03-22 19:51:19 PST -------
Looking
------- Comment #20 From 2011-03-22 19:53:09 PST -------
Another case where V8's more specific errors are different than JSC's. Sigh. I'll add new results.
------- Comment #21 From 2011-03-22 20:11:28 PST -------
Committed r81744: <http://trac.webkit.org/changeset/81744>
------- Comment #22 From 2011-03-22 20:46:55 PST -------
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
------- Comment #23 From 2011-03-23 23:53:45 PST -------
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