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
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To: Dave Hyatt
:
Depends on:
Blocks: 52693
  Show dependency treegraph
 
Reported: 2010-04-25 07:31 PDT by Ojan Vafai
Modified: 2011-03-23 23:53 PDT (History)
19 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2010-04-25 07:31:59 PDT
http://dbaron.org/log/20100424-any

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

Fix style.
Comment 6 Kent Tamura 2011-02-19 07:28:05 PST
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.
Comment 7 Trevor Downs 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 Ojan Vafai 2011-03-17 20:11:48 PDT
Created attachment 86131 [details]
Patch
Comment 9 WebKit Review Bot 2011-03-17 22:32:45 PDT
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 Ojan Vafai 2011-03-17 22:54:34 PDT
(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 Antti Koivisto 2011-03-21 02:25:21 PDT
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)
Comment 12 Ojan Vafai 2011-03-21 20:30:44 PDT
(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.
Comment 13 Ojan Vafai 2011-03-21 20:52:16 PDT
(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 Ojan Vafai 2011-03-21 20:52:39 PDT
Comment on attachment 86131 [details]
Patch

Marking for review again per the above comment.
Comment 15 Antti Koivisto 2011-03-22 01:02:30 PDT
(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 Antti Koivisto 2011-03-22 01:02:46 PDT
Comment on attachment 86131 [details]
Patch

r=me
Comment 17 Ojan Vafai 2011-03-22 18:45:02 PDT
Committed r81742: <http://trac.webkit.org/changeset/81742>
Comment 19 Ojan Vafai 2011-03-22 19:51:19 PDT
Looking
Comment 20 Ojan Vafai 2011-03-22 19:53:09 PDT
Another case where V8's more specific errors are different than JSC's. Sigh. I'll add new results.
Comment 21 Ojan Vafai 2011-03-22 20:11:28 PDT
Committed r81744: <http://trac.webkit.org/changeset/81744>
Comment 22 WebKit Review Bot 2011-03-22 20:46:55 PDT
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 Masataka Yakura 2011-03-23 23:53:45 PDT
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