Jonathan Watt [:jwatt] Reporter
	Description • 11 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.

	Jonathan Watt [:jwatt] Reporter
	Comment 1 • 11 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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 2 • 11 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 3 • 11 years ago

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?

	Cameron McCormack (:heycam) Assignee
	Comment 4 • 11 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 5 • 11 years ago

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.

	Cameron McCormack (:heycam) Assignee
	Comment 6 • 11 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 18 • 11 years ago

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

	Jonathan Watt [:jwatt] Reporter
	Comment 19 • 11 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

	Boris Zbarsky [:bzbarsky]
	Comment 20 • 11 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 21 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 22 • 11 years ago

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?

	Boris Zbarsky [:bzbarsky]
	Comment 23 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 24 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 25 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 26 • 11 years ago

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?

	Boris Zbarsky [:bzbarsky]
	Comment 27 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 28 • 11 years ago

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

Ah, part 7 fixes that, ok.

	Boris Zbarsky [:bzbarsky]
	Comment 29 • 11 years ago

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

r=me

	Boris Zbarsky [:bzbarsky]
	Comment 30 • 11 years ago

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

r=me

	Boris Zbarsky [:bzbarsky]
	Comment 31 • 11 years ago

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?

	Boris Zbarsky [:bzbarsky]
	Comment 32 • 11 years ago

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!

	Boris Zbarsky [:bzbarsky]
	Comment 33 • 11 years ago

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.

	Cameron McCormack (:heycam) Assignee
	Comment 34 • 11 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 35 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 36 • 11 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 37 • 11 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 38 • 11 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 39 • 11 years ago

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.

	Cameron McCormack (:heycam) Assignee
	Comment 40 • 11 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 45 • 11 years ago

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.

	Cameron McCormack (:heycam) Assignee
	Comment 46 • 11 years ago

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.

	Cameron McCormack (:heycam) Assignee
	Comment 47 • 11 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 48 • 11 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 49 • 11 years ago

New frame class that creates native anonymous content popped up, which was the cause of the assertions.

	Cameron McCormack (:heycam) Assignee
	Comment 50 • 11 years ago

Right patch this time...

	Boris Zbarsky [:bzbarsky]
	Comment 51 • 11 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 52 • 11 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 53 • 11 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 54 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 55 • 11 years ago

> ResolvePseudoElementThatTakesUserActionPseudoStyle

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

	Boris Zbarsky [:bzbarsky]
	Comment 56 • 11 years ago

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

r=me

	Boris Zbarsky [:bzbarsky]
	Comment 57 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 58 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 59 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 60 • 11 years ago

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

r=me

	Cameron McCormack (:heycam) Assignee
	Comment 61 • 11 years ago

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

	Cameron McCormack (:heycam) Assignee
	Comment 62 • 11 years ago

Part 4 with comments addressed.

	Cameron McCormack (:heycam) Assignee
	Comment 63 • 11 years ago

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

	Carsten Book [:Tomcat]
	Comment 64 • 11 years ago

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

	yiorsi
	Comment 65 • 11 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

	Ioana (away)
	Comment 66 • 10 years ago

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

	neil@parkwaycc.co.uk
	Comment 67 • 10 years ago

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.

	Jean-Yves Perrier [:teoli]
	Comment 68 • 10 years ago

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