Implement support for the :hover user action pseudo-class on pseudo-elements

RESOLVED FIXED in mozilla28

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
4 years ago
10 months ago

People

(Reporter: jwatt, Assigned: heycam)

Tracking

({dev-doc-complete})

Trunk
mozilla28
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DocArea=CSS][qa-], URL)

Attachments

(15 attachments, 5 obsolete attachments)

6.89 KB, patch
Details | Diff | Splinter Review
7.56 KB, patch
Details | Diff | Splinter Review
7.26 KB, patch
Details | Diff | Splinter Review
5.74 KB, patch
Details | Diff | Splinter Review
23.86 KB, patch
Details | Diff | Splinter Review
1.53 KB, patch
Details | Diff | Splinter Review
4.79 KB, patch
Details | Diff | Splinter Review
1.21 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
6.75 KB, patch
Details | Diff | Splinter Review
2.35 KB, patch
Details | Diff | Splinter Review
5.75 KB, patch
Details | Diff | Splinter Review
2.62 KB, patch
Details | Diff | Splinter Review
10.30 KB, patch
Details | Diff | Splinter Review
1.68 KB, patch
Details | Diff | Splinter Review
11.50 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
The User Action Pseudo-classes section of CSS Selectors level 4 says "The :hover pseudo-class can apply to any pseudo-element".

http://dev.w3.org/csswg/selectors4/#useraction-pseudos

We should implement that.
(Reporter)

Comment 1

4 years ago
bz: that's not one of the easy and simple parts of the parser
bz: furthermore, it requires modifying the selector representation a bit, and of course matching....
bz: well, the problem is that :hover is a pseudo-class but :before is a pseudo-element
bz: So parsing is annoying
bz: I think the right place to start is by deciding what the resulting selector object will look like
bz: and the matching code
bz: and then we figure out how to make the parser output that object
The result should then hopefully extend in a reasonable way to being able to do http://dev.w3.org/csswg/css-overflow/#style-in-fragments
(Reporter)

Updated

4 years ago
Blocks: 926419
One more difficult thing about this is handling pseudo-classes after pseudo-elements where the pseudo-element does not correspond to a whole, anonymous content node.  For example, p::before:hover (generated content) and p::first-line:hover (pseudo not matching a whole element) won't be straightforward, since :hover state is stored on content nodes.  But to handle p::-moz-number-spin-up:hover should be easy.  Is it OK just to handle things like the latter in this bug?
So that might correspond to supporting pseudo-elements for anonymous boxes only.
Keywords: dev-doc-needed
The approach I've taken in my patch queue is to allow :hover and :state after certain pseudo-elements, and to store those in the mPseudoClassList of the nsCSSSelector that represents the pseudo-element.  Then, we store that nsCSSSelector object, and not its mNext, in the RuleCascadeData hashtables.  In ContentEnumFunc, if we start with an nsCSSSelector for a pseudo-element, then we do that matching up front against mPseudoElement on the rule processor data, and then pass in its mNext into SelectorMatches to match against the real element.  SelectorMatchesTree still doesn't traverse a pseudo-element "operator".

So I don't think this readily extends to handling the CSS Overflow fragments thing, like:

  p::fragment(1) span

We're going to have to handle that differently by traversing up from the span to the fragment pseudo-element during SelectorMatchesTree.  The model of having separate ResolvePseudoElementStyle/ResolveStyleFor methods doesn't really apply once we allow combinators after the pseudo-element.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Going to assume comment 1 means bz is happy taking the reviews here. :)
Created attachment 823142 [details] [diff] [review]
Part 1: Parse selectors with user action pseudo-classes after pseudo-elements.
Attachment #823142 - Flags: review?(bzbarsky)
Created attachment 823144 [details] [diff] [review]
Part 2: Use a different operator to represent the element -> pseudo-element relationship in selectors.
Attachment #823144 - Flags: review?(bzbarsky)
Created attachment 823145 [details] [diff] [review]
Part 3: Add a flag to represent whether a pseudo-element supports user action pseudo-classes (currently :hover and :active).
Attachment #823145 - Flags: review?(bzbarsky)
Created attachment 823146 [details] [diff] [review]
Part 4: Pass in anonymous content nodes when restyling any treelike pseudo-elements.
Attachment #823146 - Flags: review?(bzbarsky)
Created attachment 823147 [details] [diff] [review]
Part 5: Store pseudo-element nsCSSSelectors directly in hashtables.
Attachment #823147 - Flags: review?(bzbarsky)
Created attachment 823148 [details] [diff] [review]
Part 6: Split out user action pseudo-class matching from SelectorMatches.
Attachment #823148 - Flags: review?(bzbarsky)
Created attachment 823149 [details] [diff] [review]
Part 7: Add nsStyleSet::HasStateDependentStyle and nsCSSRuleProcessor:HasStateDependentStyle overrides that work on pseudo-elements.
Attachment #823149 - Flags: review?(bzbarsky)
Created attachment 823150 [details] [diff] [review]
Part 8: Look at user action pseudo-classes on pseudo-elements during selector matching.
Attachment #823150 - Flags: review?(bzbarsky)
Created attachment 823151 [details] [diff] [review]
Part 9: Restyle pseudo-elements when their content state changes.
Attachment #823151 - Flags: review?(bzbarsky)
Created attachment 823152 [details] [diff] [review]
Part 10: Don't capture the <input> element when dragging a range control's thumb.
Attachment #823152 - Flags: review?(jwatt)
Created attachment 823153 [details] [diff] [review]
Part 11: Test.
Attachment #823153 - Flags: review?(bzbarsky)
Green try run: https://tbpl.mozilla.org/?tree=Try&rev=7dc20c601c27
(Reporter)

Comment 19

4 years ago
Comment on attachment 823152 [details] [diff] [review]
Part 10: Don't capture the <input> element when dragging a range control's thumb.

Can you add a commend just before this line along the lines of:

// Don't use CAPTURE_RETARGETTOELEMENT - that breaks pseudo-class styling of the thumb
Attachment #823152 - Flags: review?(jwatt) → review+
Cameron, I'm sorry for the terrible lag here.  I'm hoping to get to this in the next few days.
No problem; I won't be in a position to revise and land the patches until next week.
Comment on attachment 823142 [details] [diff] [review]
Part 1: Parse selectors with user action pseudo-classes after pseudo-elements.

What makes sure that "::foo::bar" doesn't get parsed with this patch?  Or for that matter "::foo:invalid"?  Or is that coming later?
Comment on attachment 823144 [details] [diff] [review]
Part 2: Use a different operator to represent the element -> pseudo-element relationship in selectors.

>+      if (pseudoElement &&
>+          (pseudoElementType != nsCSSPseudoElements::ePseudo_AnonBox)) {

That check should be in part 1, right?

>+        aList->mWeight += selector->CalcWeight();

Doesn't this end up doing the CalcWeight() twice now?  Once here, and once at the end of the method...
Attachment #823144 - Flags: review?(bzbarsky) → review-
Comment on attachment 823145 [details] [diff] [review]
Part 3: Add a flag to represent whether a pseudo-element supports user action pseudo-classes (currently :hover and :active).

r=me
Attachment #823145 - Flags: review?(bzbarsky) → review+
Comment on attachment 823146 [details] [diff] [review]
Part 4: Pass in anonymous content nodes when restyling any treelike pseudo-elements.

This seems to assume that PseudoElementSupportsStyleAttribute implies PseudoElementSupportsUserActionState, right?

What guarantees that?  At the very least, we should assert it somewhere early in startup for all our pseudo-elements....  Or we should check both conditions.

r=me with that fixed.
Attachment #823146 - Flags: review?(bzbarsky) → review+
Comment on attachment 823147 [details] [diff] [review]
Part 5: Store pseudo-element nsCSSSelectors directly in hashtables.

Doesn't the code in RuleValue::CollectAncestorHashes also need changing here (in particular because NS_IS_ANCESTOR_OPERATOR is false for ':')?  Seems like we should be able to write a testcase that catches that...

Also, in this setup we'll end up passing pseudo-element selectors to AddSelector, so they'll potentially end up in mStateSelectors, but then HasStateDependentStyle() will call SelectorMatches on the pseudo-element selector, no?
Attachment #823147 - Flags: review?(bzbarsky) → review-
Comment on attachment 823148 [details] [diff] [review]
Part 6: Split out user action pseudo-class matching from SelectorMatches.

>+StateSelectorMatches(Element* aElement,

Please add a precondition that aStatesToCheck is not empty.

>+    return false;
>+  } else {

As long as we're moving this code, nix the else after return?

r=me
Attachment #823148 - Flags: review?(bzbarsky) → review+
> but then HasStateDependentStyle() will call SelectorMatches on the pseudo-element selector

Ah, part 7 fixes that, ok.
Comment on attachment 823149 [details] [diff] [review]
Part 7: Add nsStyleSet::HasStateDependentStyle and nsCSSRuleProcessor:HasStateDependentStyle overrides that work on pseudo-elements.

r=me
Attachment #823149 - Flags: review?(bzbarsky) → review+
Comment on attachment 823150 [details] [diff] [review]
Part 8: Look at user action pseudo-classes on pseudo-elements during selector matching.

r=me
Attachment #823150 - Flags: review?(bzbarsky) → review+
Comment on attachment 823151 [details] [diff] [review]
Part 9: Restyle pseudo-elements when their content state changes.

Do you not need to stop at binding parent boundaries?  What if a native anon element itself has native anon stuff it causes to happen (e.g. someone stick an <input type="color"> inside the native anon content for some other element)?

How does restyling the ancestor cause the pseudo to be restyled exactly?  In particular, how does it cause that to happen if the ancestor just gets an eRestyle_Self?  Do you need to force eRestyle_Subtree here?
Attachment #823151 - Flags: review?(bzbarsky) → review-
Comment on attachment 823153 [details] [diff] [review]
Part 11: Test.

This seems fine, but please add tests for descendant combinators (which would presumably fail until my issues with part 5 are fixed).

I'm sorry this took so long.  Now that I've paged all this back in the next round should be much faster!
Attachment #823153 - Flags: review?(bzbarsky) → review+
Attachment #823142 - Flags: review?(bzbarsky) → review-
Comment on attachment 823147 [details] [diff] [review]
Part 5: Store pseudo-element nsCSSSelectors directly in hashtables.

Ah, I was wrong about CollectAncestorHashes.  It has a continue, not a break, no a non-ancestor combinator, so things are in fact ok.  r=me here.
Attachment #823147 - Flags: review- → review+
(In reply to Boris Zbarsky [:bz] from comment #22)
> Comment on attachment 823142 [details] [diff] [review]
> Part 1: Parse selectors with user action pseudo-classes after
> pseudo-elements.
> 
> What makes sure that "::foo::bar" doesn't get parsed with this patch?

We set the SEL_MASK_PELEM flag in aDataMask when we parse the first pseudo-element, so the next time around we won't parse the second one.

> Or for that matter "::foo:invalid"?  Or is that coming later?

That one does parse, but shouldn't.  Will fix.
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
(In reply to Boris Zbarsky [:bz] from comment #23)
> Comment on attachment 823144 [details] [diff] [review]
> Part 2: Use a different operator to represent the element -> pseudo-element
> relationship in selectors.
> 
> >+      if (pseudoElement &&
> >+          (pseudoElementType != nsCSSPseudoElements::ePseudo_AnonBox)) {
> 
> That check should be in part 1, right?

Right, and it is in my local commit.  Must have mistakenly attached an old patch.

> >+        aList->mWeight += selector->CalcWeight();
> 
> Doesn't this end up doing the CalcWeight() twice now?  Once here, and once
> at the end of the method...

But it does it on different nsCSSSelector objects.  The first time, in here, it will calculate the weight of the stuff before the pseudo-element.  The one at the end of the function will add on the weight of the user action pseudo-class, if any.
> Right, and it is in my local commit.  

OK, good.

> The one at the end of the function will add on the weight of the user action
> pseudo-class, if any.

Ah, I see.  So really, this should have been in part 1 as well, since we need to do the first CalcWeight before we do the AddSelector() call, right?  Or is it already there in your local patches?
(In reply to Boris Zbarsky [:bz] from comment #36)
> Ah, I see.  So really, this should have been in part 1 as well, since we
> need to do the first CalcWeight before we do the AddSelector() call, right? 
> Or is it already there in your local patches?

Yeah, you're right it should be in part 1.  I'll move it up there too.
(In reply to Boris Zbarsky [:bz] from comment #31)
> Comment on attachment 823151 [details] [diff] [review]
> Part 9: Restyle pseudo-elements when their content state changes.
> 
> Do you not need to stop at binding parent boundaries?  What if a native anon
> element itself has native anon stuff it causes to happen (e.g. someone stick
> an <input type="color"> inside the native anon content for some other
> element)?


I'm not 100% sure what I should be stopping at.  I run into the

  MOZ_ASSERT(element != pseudoElement);

assertion that I added in this patch, if I don't have this special handling in RestyleManager::ContentStateChange.  Which makes sense; we need to know the actual element that the pseudo-element is a part of, to correctly restyle.  So I think if we're calling PostRestyleEvent, we really need the element that we pass in not to be a native anonymous element that is pseudo-element.

For the native anonymous content within native anonymous case, yes, I think we need to keep looking further up the tree than the first native anonymous root we find.  We could loop while the element is native anonymous *and* its style context is for a pseudo-element (or doesn't have a frame, as we'd be unable to tell what pseudo-element it matches) -- does that sound right?

> Do you not need to stop at binding parent boundaries?

Can you explain this one a bit more?

> How does restyling the ancestor cause the pseudo to be restyled exactly?  In
> particular, how does it cause that to happen if the ancestor just gets an
> eRestyle_Self?  Do you need to force eRestyle_Subtree here?

Oh, yes, eRestyle_Subtree would definitely be needed (now that I have been digging in to RestyleManager a bit more and understand what the restyle hints really mean!).
We discussed on IRC, and came to the conclusion that ElementForStyleContext should be returning the <input> if given one of its pseudo-elements's frames, and a null aParentElement.  That will make FindNonAnonymousAncestor work unnecessary.
Depends on: 943746
And means that we can call PostRestyleEvent directly on the anonymous content.
Created attachment 8339120 [details] [diff] [review]
wrong patch
Attachment #823142 - Attachment is obsolete: true
Attachment #8339120 - Flags: review?(bzbarsky)
Created attachment 8339121 [details] [diff] [review]
Part 1: Parse selectors with user action pseudo-classes after pseudo-elements. (v1.1)
Attachment #8339120 - Attachment is obsolete: true
Attachment #8339120 - Flags: review?(bzbarsky)
Attachment #8339121 - Flags: review?(bzbarsky)
Attachment #8339120 - Attachment description: Part 1: Parse selectors with user action pseudo-classes after pseudo-elements. (v1.1) → wrong patch
Created attachment 8339123 [details] [diff] [review]
Part 2: Use a different operator to represent the element -> pseudo-element relationship in selectors. (v1.1)
Attachment #823144 - Attachment is obsolete: true
Attachment #8339123 - Flags: review?(bzbarsky)
Created attachment 8339124 [details] [diff] [review]
Part 9a: Move ElementForStyleContext earlier in the file.
Attachment #8339124 - Flags: review?(bzbarsky)
Created attachment 8339125 [details] [diff] [review]
Part 9b: Restyle pseudo-elements when their content state changes. (v1.1)

Is it OK that when the element we're restyling has no frame that we don't call in to the HasStateDependentStyle overload for pseudo-elements?  Since we have no frame we can't tell what pseudo it should be.  If it's important, we could have a function that hard codes the existing native pseudo-elements and which takes the anonymous content and somehow divines the pseudo-element type by looking at its parent and its position in the parent's child list.
Attachment #8339125 - Flags: review?(bzbarsky)
Created attachment 8339127 [details] [diff] [review]
Part 11: Tests. (v1.1)

Added a new test to make sure ::moz-progress-bar::moz-progress-bar doesn't parse, and a new subtest in test_pseudoelement_state.html that has uses a selector with a descendant combinator in it.
Attachment #823153 - Attachment is obsolete: true
Attachment #8339127 - Flags: review?(bzbarsky)
https://tbpl.mozilla.org/?tree=Try&rev=25cc87a43fbf
A few assertions there it turns out, so cancelling reviews while I sort them out.
Attachment #8339121 - Flags: review?(bzbarsky)
Attachment #8339123 - Flags: review?(bzbarsky)
Attachment #8339124 - Flags: review?(bzbarsky)
Attachment #8339125 - Flags: review?(bzbarsky)
Attachment #8339127 - Flags: review?(bzbarsky)
Created attachment 8339674 [details] [diff] [review]
Part 12: Pass in pseudo-element when resolving style for nsNumberControlFrame's anonymous content.

New frame class that creates native anonymous content popped up, which was the cause of the assertions.
Attachment #8339674 - Flags: review?(bzbarsky)
Created attachment 8339676 [details] [diff] [review]
Part 12: Pass in pseudo-element when resolving style for nsNumberControlFrame's anonymous content.

Right patch this time...
Attachment #8339676 - Flags: review?(bzbarsky)
Attachment #8339674 - Attachment is obsolete: true
Attachment #8339674 - Flags: review?(bzbarsky)
Comment on attachment 8339676 [details] [diff] [review]
Part 12: Pass in pseudo-element when resolving style for nsNumberControlFrame's anonymous content.

r=me, but this should probably move earlier in the patch stack.

And we should consider renaming the version of ResolvePseudoElementStyle that doesn't take the actual pseudo-element so people can't accidentally call it like this...
Attachment #8339676 - Flags: review?(bzbarsky) → review+
https://tbpl.mozilla.org/?tree=Try&rev=996c15d70a22 (M3 assertions are bug 892638)
(In reply to Boris Zbarsky [:bz] from comment #51)
> Comment on attachment 8339676 [details] [diff] [review]
> Part 12: Pass in pseudo-element when resolving style for
> nsNumberControlFrame's anonymous content.
> 
> r=me, but this should probably move earlier in the patch stack.

How about I fold it into part 4 when I land.

> And we should consider renaming the version of ResolvePseudoElementStyle
> that doesn't take the actual pseudo-element so people can't accidentally
> call it like this...

Wonder what name would be appropriate.  "ResolvePseudoElementThatTakesUserActionPseudoStyle" is a bit long. :)
Attachment #8339121 - Flags: review?(bzbarsky)
Attachment #8339123 - Flags: review?(bzbarsky)
Attachment #8339124 - Flags: review?(bzbarsky)
Attachment #8339125 - Flags: review?(bzbarsky)
Attachment #8339127 - Flags: review?(bzbarsky)
Oh and the assertions from part 4 should hopefully stop people calling it incorrectly.
> ResolvePseudoElementThatTakesUserActionPseudoStyle

Hmm.  How about we just require passing in the Element* for ResolvePseudoElementStyle and just ignore it inside that call if !PseudoElementSupportsUserActionState(pseudoType)?
Comment on attachment 8339121 [details] [diff] [review]
Part 1: Parse selectors with user action pseudo-classes after pseudo-elements. (v1.1)

r=me
Attachment #8339121 - Flags: review?(bzbarsky) → review+
Comment on attachment 8339123 [details] [diff] [review]
Part 2: Use a different operator to represent the element -> pseudo-element relationship in selectors. (v1.1)

r=me
Attachment #8339123 - Flags: review?(bzbarsky) → review+
Comment on attachment 8339124 [details] [diff] [review]
Part 9a: Move ElementForStyleContext earlier in the file.

I guess this is fine, though you could also just forward-declare it to avoid having blame.  ;)
Attachment #8339124 - Flags: review?(bzbarsky) → review+
Comment on attachment 8339125 [details] [diff] [review]
Part 9b: Restyle pseudo-elements when their content state changes. (v1.1)

>+    Element* ancestor = ElementForStyleContext(nullptr, primaryFrame,

This is getting called even if !PseudoElementSupportsUserActionState, right?

Seems like it might be nicer to structure this like:

 if (pseudoType >= nsCSSPseudoElements::ePseudo_PseudoElementCount) {
   rshint = styleSet->HasStateDependentStyle(mPresContext, aElement,
                                             aStateMask);
 } else if (nsCSSPseudoElements::PseudoElementSupportsUserActionState(pseudoType)) {
   Element* ancestor = // etc
 } else {
   rshint = nsRestyleHint(0);
 }

r=me with that.
Attachment #8339125 - Flags: review?(bzbarsky) → review+
Comment on attachment 8339127 [details] [diff] [review]
Part 11: Tests. (v1.1)

r=me
Attachment #8339127 - Flags: review?(bzbarsky) → review+
Filed bug 944246 to make getComputedStyle return the right state-affected styles for pseudo-elements.
Created attachment 8339747 [details] [diff] [review]
Part 4: Pass in anonymous content nodes when restyling any pseudo-elements that can match user action pseudo-classes. (v1.1)

Part 4 with comments addressed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3315c6b05d23
https://hg.mozilla.org/integration/mozilla-inbound/rev/da0224b9ef9f
https://hg.mozilla.org/integration/mozilla-inbound/rev/d150f83a5415
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae82eacc5bff
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e10f4677311
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e580ff4d7ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/771432bdb545
https://hg.mozilla.org/integration/mozilla-inbound/rev/78c9cc186967
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c0cdf0d2c84
https://hg.mozilla.org/integration/mozilla-inbound/rev/00c87bd8f8aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f7925e3933b
https://hg.mozilla.org/mozilla-central/rev/3315c6b05d23
https://hg.mozilla.org/mozilla-central/rev/da0224b9ef9f
https://hg.mozilla.org/mozilla-central/rev/d150f83a5415
https://hg.mozilla.org/mozilla-central/rev/ae82eacc5bff
https://hg.mozilla.org/mozilla-central/rev/5e10f4677311
https://hg.mozilla.org/mozilla-central/rev/8e580ff4d7ca
https://hg.mozilla.org/mozilla-central/rev/771432bdb545
https://hg.mozilla.org/mozilla-central/rev/78c9cc186967
https://hg.mozilla.org/mozilla-central/rev/2c0cdf0d2c84
https://hg.mozilla.org/mozilla-central/rev/00c87bd8f8aa
https://hg.mozilla.org/mozilla-central/rev/3f7925e3933b
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Depends on: 944493
Depends on: 945048
Depends on: 946312

Comment 65

4 years ago
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #2)
> The result should then hopefully extend in a reasonable way to being able to
> do http://dev.w3.org/csswg/css-overflow/#style-in-fragments

I also hope to achieve "max-lines" property. Because truncated multiple lines of text is very common needs. Firefox does not know whether it is difficult to achieve?

http://dev.w3.org/csswg/css-overflow/#max-lines
Whiteboard: [DocArea=CSS]

Comment 66

3 years ago
https://hg.mozilla.org/mozilla-central/rev/3f7925e3933b
Flags: in-testsuite+
Whiteboard: [DocArea=CSS] → [DocArea=CSS][qa-]
Depends on: 989965

Updated

3 years ago
Depends on: 986509

Updated

3 years ago
Depends on: 1016063
Comment on attachment 8339121 [details] [diff] [review]
Part 1: Parse selectors with user action pseudo-classes after pseudo-elements. (v1.1)

This patch caused bug 1016063.
Doc updated:
https://developer.mozilla.org/en-US/docs/Web/CSS/:hover
https://developer.mozilla.org/en-US/Firefox/Releases/28
Keywords: dev-doc-needed → dev-doc-complete
See Also: → bug 1122965
Depends on: 1290825
You need to log in before you can comment on or make changes to this bug.