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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jonathan Watt [:jwatt]]

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.

Comment 1

[Jonathan Watt [:jwatt]]

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

Comment 2

[David Baron :dbaron:]

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

Comment 3

[Cameron McCormack (:heycam)]

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?

Comment 4

[Cameron McCormack (:heycam)]

So that might correspond to supporting pseudo-elements for anonymous boxes only.

Comment 5

[Cameron McCormack (:heycam)]

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.

Comment 6

[Cameron McCormack (:heycam)]

Going to assume comment 1 means bz is happy taking the reviews here. :)

Comment 7

[Cameron McCormack (:heycam)]

Attachment: Part 1: Parse selectors with user action pseudo-classes after pseudo-elements.

Comment 8

[Cameron McCormack (:heycam)]

Attachment: Part 2: Use a different operator to represent the element -> pseudo-element relationship in selectors.

Comment 9

[Cameron McCormack (:heycam)]

Attachment: Part 3: Add a flag to represent whether a pseudo-element supports user action pseudo-classes (currently :hover and :active).

Comment 10

[Cameron McCormack (:heycam)]

Attachment: Part 4: Pass in anonymous content nodes when restyling any treelike pseudo-elements.

Comment 11

[Cameron McCormack (:heycam)]

Attachment: Part 5: Store pseudo-element nsCSSSelectors directly in hashtables.

Comment 12

[Cameron McCormack (:heycam)]

Attachment: Part 6: Split out user action pseudo-class matching from SelectorMatches.

Comment 13

[Cameron McCormack (:heycam)]

Attachment: Part 7: Add nsStyleSet::HasStateDependentStyle and nsCSSRuleProcessor:HasStateDependentStyle overrides that work on pseudo-elements.

Comment 14

[Cameron McCormack (:heycam)]

Attachment: Part 8: Look at user action pseudo-classes on pseudo-elements during selector matching.

Comment 15

[Cameron McCormack (:heycam)]

Attachment: Part 9: Restyle pseudo-elements when their content state changes.

Comment 16

[Cameron McCormack (:heycam)]

Attachment: Part 10: Don't capture the <input> element when dragging a range control's thumb.

Comment 17

[Cameron McCormack (:heycam)]

Attachment: Part 11: Test.

Comment 18

[Cameron McCormack (:heycam)]

Green try run: https://tbpl.mozilla.org/?tree=Try&rev=7dc20c601c27

Comment 19

[Jonathan Watt [:jwatt]]

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

Comment 20

[Boris Zbarsky [:bzbarsky]]

Cameron, I'm sorry for the terrible lag here.  I'm hoping to get to this in the next few days.

Comment 21

[Cameron McCormack (:heycam)]

No problem; I won't be in a position to revise and land the patches until next week.

Comment 22

[Boris Zbarsky [:bzbarsky]]

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 23

[Boris Zbarsky [:bzbarsky]]

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

Comment 24

[Boris Zbarsky [:bzbarsky]]

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

Comment 25

[Boris Zbarsky [:bzbarsky]]

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.

Comment 26

[Boris Zbarsky [:bzbarsky]]

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?

Comment 27

[Boris Zbarsky [:bzbarsky]]

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

Comment 28

[Boris Zbarsky [:bzbarsky]]

> but then HasStateDependentStyle() will call SelectorMatches on the pseudo-element selector

Ah, part 7 fixes that, ok.

Comment 29

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 823150 [details] [diff] [review]
Part 8: Look at user action pseudo-classes on pseudo-elements during selector matching.

r=me

Comment 31

[Boris Zbarsky [:bzbarsky]]

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?

Comment 32

[Boris Zbarsky [:bzbarsky]]

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!

Comment 33

[Boris Zbarsky [:bzbarsky]]

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.

Comment 34

[Cameron McCormack (:heycam)]

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

Comment 35

[Cameron McCormack (:heycam)]

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

Comment 36

[Boris Zbarsky [:bzbarsky]]

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

Comment 37

[Cameron McCormack (:heycam)]

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

Comment 38

[Cameron McCormack (:heycam)]

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

Comment 39

[Cameron McCormack (:heycam)]

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.

Comment 40

[Cameron McCormack (:heycam)]

And means that we can call PostRestyleEvent directly on the anonymous content.

Comment 41

[Cameron McCormack (:heycam)]

Attachment: wrong patch

Comment 42

[Cameron McCormack (:heycam)]

Attachment: Part 1: Parse selectors with user action pseudo-classes after pseudo-elements. (v1.1)

Comment 43

[Cameron McCormack (:heycam)]

Attachment: Part 2: Use a different operator to represent the element -> pseudo-element relationship in selectors. (v1.1)

Comment 44

[Cameron McCormack (:heycam)]

Attachment: Part 9a: Move ElementForStyleContext earlier in the file.

Comment 45

[Cameron McCormack (:heycam)]

Attachment: 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.

Comment 46

[Cameron McCormack (:heycam)]

Attachment: 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.

Comment 47

[Cameron McCormack (:heycam)]

https://tbpl.mozilla.org/?tree=Try&rev=25cc87a43fbf

Comment 48

[Cameron McCormack (:heycam)]

A few assertions there it turns out, so cancelling reviews while I sort them out.

Comment 49

[Cameron McCormack (:heycam)]

Attachment: 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.

Comment 50

[Cameron McCormack (:heycam)]

Attachment: Part 12: Pass in pseudo-element when resolving style for nsNumberControlFrame's anonymous content.

Right patch this time...

Comment 51

[Boris Zbarsky [: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...

Comment 52

[Cameron McCormack (:heycam)]

https://tbpl.mozilla.org/?tree=Try&rev=996c15d70a22 (M3 assertions are bug 892638)

Comment 53

[Cameron McCormack (:heycam)]

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

Comment 54

[Cameron McCormack (:heycam)]

Oh and the assertions from part 4 should hopefully stop people calling it incorrectly.

Comment 55

[Boris Zbarsky [:bzbarsky]]

> ResolvePseudoElementThatTakesUserActionPseudoStyle

Hmm.  How about we just require passing in the Element* for ResolvePseudoElementStyle and just ignore it inside that call if !PseudoElementSupportsUserActionState(pseudoType)?

Comment 56

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 57

[Boris Zbarsky [:bzbarsky]]

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

Comment 58

[Boris Zbarsky [:bzbarsky]]

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.  ;)

Comment 59

[Boris Zbarsky [:bzbarsky]]

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.

Comment 60

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8339127 [details] [diff] [review]
Part 11: Tests. (v1.1)

r=me

Comment 61

[Cameron McCormack (:heycam)]

Filed bug 944246 to make getComputedStyle return the right state-affected styles for pseudo-elements.

Comment 62

[Cameron McCormack (:heycam)]

Attachment: 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.

Comment 63

[Cameron McCormack (:heycam)]

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

Comment 64

[Carsten Book [:Tomcat]]

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

Comment 65

[yiorsi]

(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

Comment 66

[Ioana (away)]

https://hg.mozilla.org/mozilla-central/rev/3f7925e3933b

Comment 67

[[e-mail]]

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.

Comment 68

[Jean-Yves Perrier [:teoli]]

Doc updated:
https://developer.mozilla.org/en-US/docs/Web/CSS/:hover
https://developer.mozilla.org/en-US/Firefox/Releases/28