Closed
Bug 544834
Opened 15 years ago
Closed 15 years ago
implement :-moz-any() for selector grouping (OR) between combinators
Categories
(Core :: CSS Parsing and Computation, defect, P4)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a5
People
(Reporter: dbaron, Assigned: dbaron)
References
Details
(Keywords: dev-doc-complete)
Attachments
(3 files, 2 obsolete files)
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
I have a patch to implement a :-moz-any() pseudo-class that allows selector grouping between combinators. For example:
ul > li, ol > li, dir > li, menu > li
could be combined into:
:-moz-any(ul, ol, dir, menu) > li
I expect this will speed up some existing selectors in our user-agent stylesheets, and could be even more useful when implementing some things that are in HTML5.
Assignee | ||
Comment 1•15 years ago
|
||
This has better error handling for pseudo-class argument parsing; I couldn't think of any existing cases where it changes behavior, but patch 2 introduces some.
Attachment #425761 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 2•15 years ago
|
||
Attachment #425762 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 3•15 years ago
|
||
Attachment #425763 -
Flags: review?(bzbarsky)
Comment 4•15 years ago
|
||
Comment on attachment 425761 [details] [diff] [review]
patch 1: cleanup pseudo-class argument parsing
r=bzbarsky
Attachment #425761 -
Flags: review?(bzbarsky) → review+
Comment 5•15 years ago
|
||
Comment on attachment 425763 [details] [diff] [review]
patch 3: use :-moz-any() in html.css
r=bzbarsky; you can use this orthe xul|videocontrols styles too, right?
Attachment #425763 -
Flags: review?(bzbarsky) → review+
Comment 6•15 years ago
|
||
And forms.css can use this stuff a bit for the .anonymous-div rules.
Do we want blizzard to blog this and dev-doc-needed it?
Comment 7•15 years ago
|
||
Comment on attachment 425762 [details] [diff] [review]
patch 2: implement :-moz-any()
AddRule needs to look at :-moz-any pseudos, I think. Without that there will be dynamic change bugs here where attr, class, id, state selectors inside :-moz-any will not respond properly to DOM changes. r- due to that; please make that AddRule change and add tests for that code?
Having to add aStateMask to all those callback functions is kinda annoying. I wonder whether we should just go with a switch() after all instead of an array of function pointers. Certainly the more arguments the functions have the more attractive the switch() is performance-wise, and the switch() version is likely easier to maintain....
Maybe a followup bug (filed on me if you want) to make that change?
Attachment #425762 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 8•15 years ago
|
||
Here's a version of patch 2 that I believe addresses your previous comments.
It still doesn't fix specificity. However, I think that's a decent amount of work, and my current inclination is to get the patch in (and maybe even blog about it) and get a bug filed on getting the right specificity (which probably means more dynamic specificity computation).
Attachment #425762 -
Attachment is obsolete: true
Attachment #435004 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 9•15 years ago
|
||
It turns out that patch stopped working at some point, I think due to zwol's selector parsing refactoring. This fixes it by adding an extra check in ParseSelectorGroup.
Attachment #435004 -
Attachment is obsolete: true
Attachment #437152 -
Flags: review?(bzbarsky)
Attachment #435004 -
Flags: review?(bzbarsky)
Comment 10•15 years ago
|
||
David, you don't have a sane interdiff of that last patch against what I reviewed before by chance?
Comment 11•15 years ago
|
||
Comment on attachment 437152 [details] [diff] [review]
patch 2: implement :-moz-any()
> + nsTArray<nsCSSSelector*>* stateArray = &aCascade->mStateSelectors;
> + nsTArray<nsCSSSelector*>* classArray = &aCascade->mClassSelectors;
> + nsTArray<nsCSSSelector*>* idArray = &aCascade->mIDSelectors;
Those locals are used once each, so not sure they're really needed.
r=bzbarsky with that fixed.
Attachment #437152 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 12•15 years ago
|
||
I filed bug 561154 for doing the right thing about specificity.
I have the patches ready to push in my queue; doesn't seem like there's much hope of actually pushing them this week.
Assignee | ||
Comment 13•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/008d2f6c9d27
http://hg.mozilla.org/mozilla-central/rev/6efa2358f849
http://hg.mozilla.org/mozilla-central/rev/8e552e1afa49
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•15 years ago
|
Priority: -- → P4
Target Milestone: --- → mozilla1.9.3a5
Comment 14•15 years ago
|
||
(In reply to comment #0)
> I have a patch to implement a :-moz-any() pseudo-class that allows selector
> grouping between combinators. For example:
> ul > li, ol > li, dir > li, menu > li
> could be combined into:
> :-moz-any(ul, ol, dir, menu) > li
Would a > :-moz-any(x,y) perform better or worse than a > x, a > y?
Comment 15•15 years ago
|
||
Comment on attachment 425763 [details] [diff] [review]
patch 3: use :-moz-any() in html.css
> /* 2 deep unordered lists use a circle */
>-ol ul, ul ul, menu ul, dir ul,
>-ol menu, ul menu, menu menu, dir menu,
>-ol dir, ul dir, menu dir, dir dir {
>+:-moz-any(ol, ul, menu, dir) ul,
>+:-moz-any(ol, ul, menu, dir) menu,
>+:-moz-any(ol, ul, menu, dir) dir {
> list-style-type: circle;
> }
Dao, does this answer your question?
Comment 16•15 years ago
|
||
I understand that this is better:
:-moz-any(x,y) > a
But I'm asking about:
a > :-moz-any(x,y)
Updated•15 years ago
|
Keywords: dev-doc-needed
Comment 17•15 years ago
|
||
> Would a > :-moz-any(x,y) perform better or worse than a > x, a > y?
Almost certainly worse. See >https://developer.mozilla.org/en/Writing_Efficient_CSS>: the former is in bucket 4, while the latter two are in bucket 3.
Assignee | ||
Comment 18•15 years ago
|
||
Yes, worse, for now, at least (though it could probably be changed). Right now we don't attempt to use :-moz-any() selectors when determining buckets for the RuleHash. We could, but it would probably double the size of the implementation.
Comment 19•15 years ago
|
||
Should this be added to https://developer.mozilla.org/en/Upcoming_Firefox_features_for_developers?
Assignee | ||
Comment 20•15 years ago
|
||
(In reply to comment #19)
> Should this be added to
> https://developer.mozilla.org/en/Upcoming_Firefox_features_for_developers?
Yes, once developer.mozilla.org is working again.
Also more info at http://dbaron.org/log/20100424-any
Comment 21•15 years ago
|
||
We should consider having a hacks post about this too.
Comment 22•15 years ago
|
||
(In reply to comment #21)
> We should consider having a hacks post about this too.
Definitely. I was thinking about cross posting dbaron's post (with some more context).
David, is it ok for you?
(In reply to comment #20)
> (In reply to comment #19)
> > Should this be added to
> > https://developer.mozilla.org/en/Upcoming_Firefox_features_for_developers?
Done.
Comment 23•15 years ago
|
||
Please mention in all material that this will be extremely valuable in HTML5 when it comes to sectioning and headings.
Differentiating between headings based on how deep they are inside sectioning elements is the missing key for those elements to take off in a usable fashion.
h1
-moz-any(section, article, aside, nav) h1 = h2
-moz-any(section, article, etc) -moz-any(section, article, etc) h1 = h3
Without a selector like this one, the variations are endless (or 4 to the power of the depth minus 1).
Assignee | ||
Comment 24•15 years ago
|
||
(In reply to comment #22)
> (In reply to comment #21)
> > We should consider having a hacks post about this too.
>
> Definitely. I was thinking about cross posting dbaron's post (with some more
> context).
>
> David, is it ok for you?
Sure, it's ok with me.
Comment 25•14 years ago
|
||
Added document here; please review and let me know if there are any issues. It's not as clear as I like because my CSS skills are not über-l337.
https://developer.mozilla.org/en/CSS/:-moz-any
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•