783213 - :active and :hover quirk should only apply to links

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P4)

Description

[Simon Pieters [:zcorpan]]

For :active and :hover selector in quirks mode, Opera/IE/WebKit only match links, while Gecko also matches images and form controls. I think Gecko should align with the rest.

Spec http://dvcs.w3.org/hg/quirks-mode/raw-file/tip/Overview.html#the-:active-and-:hover-quirk

Comment 1

[Boris Zbarsky [:bzbarsky]]

David, thoughts?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Note that the definition of "link" is different in different browsers, by the way....

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Sounds ok to me.

It basically means removing the function IsQuirkEventSensitive in nsCSSRuleProcessor.cpp, assuming the other details match, though there are a bunch of other conditions around the call to that function that may or may not match the spec.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/file/88e47f6905e9/layout/style/nsCSSRuleProcessor.cpp#l2051

Comment 5

[Brian Marshall]

Is making the quirk match all the conditions in the spec part of this bug, or is it only for the link matching?

It looks like these conditions from <https://quirks.spec.whatwg.org/#the-:active-and-:hover-quirk> aren't checked by us:

1. selector does not use a pseudo-class selector other than ':active' and ':hover'.
2. selector does not use a pseudo-element selector. 
3. selector is not part of an argument to a functional pseudo-class or pseudo-element.

I think #3 is already partly checked by !isNegated (for :not()). Is that OK, or is there a way to check directly if the selector is being used as an argument?

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Brian Marshall from comment #5)
> Is making the quirk match all the conditions in the spec part of this bug,
> or is it only for the link matching?

Could be either.  If this is fixed without fixing all the conditions, another bug should be filed.

> I think #3 is already partly checked by !isNegated (for :not()). Is that OK,
> or is there a way to check directly if the selector is being used as an
> argument?

We'd also need to handle :-moz-any() (which in the future should be changed to :matches()).

So something would need to be added to handle this.

It seems a little bit wasteful to add a whole extra parameter to SelectorMatches, although maybe that's the right thing.  It might also be possible to use a member variable on the NodeMatchContext that's saved and restored around the call.

Comment 7

[Brian Marshall]

OK, I'm trying to do the "selector does not use a pseudo-element selector" condition. However, the pseudo-element is separated into another selector. So with ':hover::after', StateSelectorMatches only sees the ':hover' selector: aSelector->IsPseudoElement() is false, and aSelector->mNext is null.

Can you give me some tips on how to tell StateSelectorMatches that a selector at some point had a pseudo-element attached to it?

Comment 8

[Brian Marshall]

Attachment: Testcase for some selector conditions

Simple testcase for the three conditions in comment 5.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Brian Marshall from comment #7)
> OK, I'm trying to do the "selector does not use a pseudo-element selector"
> condition. However, the pseudo-element is separated into another selector.
> So with ':hover::after', StateSelectorMatches only sees the ':hover'
> selector: aSelector->IsPseudoElement() is false, and aSelector->mNext is
> null.
> 
> Can you give me some tips on how to tell StateSelectorMatches that a
> selector at some point had a pseudo-element attached to it?

pseudo-elements are stored like they're a child, since, conceptually, they are.  So you'd probably need to add a mechanism to detect this (it's possible you could store it on the TreeMatchContext, perhaps?), but I think it's worth keeping the patch to do that separated from the patch to fix the other issues.

Comment 10

[Brian Marshall]

David, thanks for your help so far! I'll try to get some patches ready soon.

Comment 11

[Brian Marshall]

Attachment: Part 1: Only apply the :active and :hover quirk to links, and not when the selector uses other pseudo-classes

This patch updates the quirk using data already available, without adding extra mechanisms.

Comment 12

[Brian Marshall]

Attachment: Part 2: Don't apply the quirk to selectors that have a pseudo-element or are part of a pseudo-class argument

This patch adds a new mechanism for passing selector context to SelectorMatches. At first, I tried using NodeMatchContext or TreeMatchContext, but the data could change with each call to SelectorMatches, where a single {Node,Tree}MatchContext would be re-used. So I felt that adding a new optional parameter was best, to avoid having to reset data on longer-lived objects. Let me know if you think the new parameter isn't justified, though.

Comment 13

[Brian Marshall]

Attachment: Part 3: Tests

This is the last part. I used a mochitest in order to synthesize the :hover state.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8519589 [details] [diff] [review]
Part 2: Don't apply the quirk to selectors that have a pseudo-element or are part of a pseudo-class argument

What was wrong with just putting this on the TreeMatchContext?  I'd rather avoid adding an extra class just to fix this.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8519590 [details] [diff] [review]
Part 3: Tests

>diff --git a/layout/style/test/test_bug783213.html b/layout/style/test/test_bug783213.html

I'd prefer naming the test file as test_hover_quirk.html or similar.  Descriptive filenames are better than bug numbers.

>+    #content :hover #link-child::after {
>+      content: "?";
>+    }

It seems like you should actually have a child on the other two tests as well, so that you actually test that this only applies to links.  (Although I suppose part of the point is testing that something in a separate compound selector doesn't affect the test.)

r=dbaron with that

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #14)
> Comment on attachment 8519589 [details] [diff] [review]
> Part 2: Don't apply the quirk to selectors that have a pseudo-element or are
> part of a pseudo-class argument
> 
> What was wrong with just putting this on the TreeMatchContext?  I'd rather
> avoid adding an extra class just to fix this.

Actually, maybe this is fine.  I'll look at this patch more closely.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8519589 [details] [diff] [review]
Part 2: Don't apply the quirk to selectors that have a pseudo-element or are part of a pseudo-class argument

>+  SelectorMatchContext(bool aIsPseudoArgument, bool aHasPseudoElement)
>+    : mIsPseudoArgument(aIsPseudoArgument)
>+    , mHasPseudoElement(aHasPseudoElement)
>+  {
>+  }

So we've generally found that constructors that take long sequences
of booleans lead to bugs.  It's often better to use flags, e.g.:

MOZ_BEGIN_ENUM_CLASS(SelectorMatchesFlags, uint8_t)
  HAS_PSEUDO_ELEMENT = (1<<0),
  IS_PSEUDOCLASS_ARGUMENT = (1<<1)
MOZ_END_ENUM_CLASS(SelectorMatchesFlags)
MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS(

SelectorMatchesFlags aSelectorFlags

SelectorMatchesFlags::HAS_PSEUDO_ELEMENT | ...

>+      // Without context, assume the quirk doesn't match.

This is extremely suspicious.  You should make all the callers pass
the right information.  (This means at least the 4-parameter
StateSelectorMatches.)

I want to review the patch again, because I want to look at all the
cases once you've made the parameter mandatory.  Therefore I'm marking
this as review-.  You're still on the right track, though.

Comment 18

[Brian Marshall]

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #17)
> >+      // Without context, assume the quirk doesn't match.
> 
> This is extremely suspicious.  You should make all the callers pass
> the right information.  (This means at least the 4-parameter
> StateSelectorMatches.)

Yeah, I did that for HasStateDependentStyle, which can start from the middle of a selector. So with ":hover::after", it only grabs the selector ":hover" from cascade->mStateSelectors (since that's where the state pseudo-class is). It incorrectly reports that there is no pseudo-element, which can trigger the quirk and make the selector fail to match non-link elements.

This meant that HasStateDependentStyle could return a zero hint, even if the selector would've matched later on in ContentEnumFunc (because the pseudo-element would be noticed there). So by assuming the quirk doesn't match when we don't know, no elements are accidentally excluded from restyling.

That's my understanding of the code, at least. Does an explicit SelectorMatchesFlags::UNKNOWN make sense for this case? Or is there a way to do this properly?

Comment 19

[Brian Marshall]

Attachment: Part 3: Tests

Addressed review comments, carrying forward r=dbaron.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Brian Marshall from comment #18)
> That's my understanding of the code, at least. Does an explicit
> SelectorMatchesFlags::UNKNOWN make sense for this case? Or is there a way to
> do this properly?

Yeah, I guess an UNKNOWN flag is ok, since it's ok if HasStateDependentStyle returns false positives.  (It's important not to return false negatives.)

Comment 21

[Brian Marshall]

Attachment: Part 2: Don't apply the quirk to selectors that use a pseudo-element or are part of a pseudo-class argument

In addition to the other changes, ActiveHoverQuirkMatches now checks aSelector->IsPseudoElement() to handle selectors like "::-moz-progress-bar:hover".

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8521669 [details] [diff] [review]
Part 2: Don't apply the quirk to selectors that use a pseudo-element or are part of a pseudo-class argument

>+  // The selector's flags are unknown.  This is the case when you don't
>+  // know if you're starting from the top of a selector.
>+  UNKNOWN = 1 << 0,

You should add that this is only used in cases where it's ok to be wrong
on the side of saying that selectors do match when they really
shouldn't, but it's still not ok to be wrong on the side of saying that
selectors don't match when they actually do.


Also, in ActiveHoverQuirkMatches you should comment that testing for
UNKNOWN makes sense since the quirk matching means that selectors do
not match, so we want to err on the side of not having the quirk when
we're not sure.


>       // We know that the selector can't match, since there is no element for
>       // the user action pseudo-class to match against.
>       return;
>     }
>     if (!StateSelectorMatches(pdata->mPseudoElement, aSelector, nodeContext,
>-                              data->mTreeMatchContext)) {
>+                              data->mTreeMatchContext,
>+                              SelectorMatchesFlags::NONE)) {

It looks like this one should be HAS_PSEUDO_ELEMENT.


In SelectorListMatches:
>     NS_ASSERTION(!sel->IsPseudoElement(), "Shouldn't have been called");
>     NodeMatchContext nodeContext(EventStates(), false);
>-    if (SelectorMatches(aElement, sel, nodeContext, aTreeMatchContext)) {
>+    if (SelectorMatches(aElement, sel, nodeContext, aTreeMatchContext,
>+                        SelectorMatchesFlags::UNKNOWN)) {

I think you should pass NONE rather than UNKNOWN.  (See the assertion,
quoted, about !sel->IsPseudoElement().)

r=dbaron with those changes

Comment 23

[Brian Marshall]

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #22)
> >     if (!StateSelectorMatches(pdata->mPseudoElement, aSelector, nodeContext,
> >-                              data->mTreeMatchContext)) {
> >+                              data->mTreeMatchContext,
> >+                              SelectorMatchesFlags::NONE)) {
> 
> It looks like this one should be HAS_PSEUDO_ELEMENT.

Currently HAS_PSEUDO_ELEMENT isn't set when the selector itself is a pseudo-element, since ActiveHoverQuirkMatches is able to check IsPseudoElement() directly for that. I think if I changed that, StateSelectorMatches should MOZ_ASSERT_IF(aSelector->IsPseudoElement(), aSelectorFlags & HAS_PSEUDO_ELEMENT) to prevent them from contradicting each other. Does that sound good?

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No, that's fine as is; I missed that detail.

Maybe the comment describing HAS_PSEUDO_ELEMENT should be clearer that it's set only for the selector for which the other half of the compound selector (i.e., where the spec describes it as a single compound selector and we represent it as two) is a pseudo-element.

Comment 25

[Brian Marshall]

Attachment: Part 2: Don't apply the quirk to selectors that use a pseudo-element or are part of a pseudo-class argument

Addressed review comments and added explicit #includes for TypedEnum.h/TypedEnumBits.h. Carrying forward r=dbaron.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I did a try push to make sure tests all pass:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bc7393d75596
https://tbpl.mozilla.org/?tree=Try&rev=bc7393d75596

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I landed the patches on mozilla-inbound:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3e00e4612e09
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5bbc9537f533
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e036207e0be3

Once they get merged to mozilla-central (within a day or so, usually, although today might be slower), they'll appear in the next nightly.

Thanks for the patches.

Comment 28

[Brian Marshall]

Thanks for the help!

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3e00e4612e09
https://hg.mozilla.org/mozilla-central/rev/5bbc9537f533
https://hg.mozilla.org/mozilla-central/rev/e036207e0be3

Comment 30

[Jean-Yves Perrier [:teoli]]

Documentation updated (please review, the changes are quite subtle and I hope to have grasped them correctly):
https://developer.mozilla.org/en-US/docs/Mozilla_Quirks_Mode_Behavior
and
https://developer.mozilla.org/en-US/Firefox/Releases/36#CSS

Comment 31

[Brian Marshall]

(In reply to Jean-Yves Perrier [:teoli] from comment #30)
> Documentation updated (please review, the changes are quite subtle and I
> hope to have grasped them correctly):
> https://developer.mozilla.org/en-US/docs/Mozilla_Quirks_Mode_Behavior
> and
> https://developer.mozilla.org/en-US/Firefox/Releases/36#CSS

I think the changes can basically be described as:
Since Gecko 36, the :active and :hover quirk is no longer triggered when the selector uses a pseudo-class other than :active and :hover, a pseudo-element, or is part of a pseudo-class argument. If the quirk is triggered, the selector no longer matches images and form controls, only links.

The documentation looks correct, although this paragraph (in "Mozilla Quirks Mode Behavior") is somewhat confusing:
> The :hover and :active pseudo-classes will only be applied to links, and only
> if there is no other pseudo-class in the selector. To match other elements,
> the selector must include a tag name, id, class or attribute.

It should probably be something like:
The :hover and :active pseudo-classes will only match links if the selector does not use a tag, attribute, ID, class, or pseudo-element selector, does not use other pseudo-class selectors, and is not part of a pseudo-class argument.

Also, in "Firefox 36 for developers", "[...] if it isn't part of a pseudo-class element" should say "argument" instead of "element".

Thanks!

Comment 32

[Simon Pieters [:zcorpan]]

I wrote some tests in this area for web-platform-tests (review welcome):

https://github.com/w3c/web-platform-tests/pull/1475

Some things that fail in Gecko:
* :active is not restricted to matching the elements given in https://html.spec.whatwg.org/multipage/scripting.html#selector-active
* :matches() is not supported
* It seems :active doesn't work on <area>

Comment 33

[Simon Pieters [:zcorpan]]

The tests can be run at http://w3c-test.org/submissions/1475/quirks-mode/active-and-hover-manual.html (until they are merged, at which point remove "submissions/1475/").

Comment 34

[Simon Pieters [:zcorpan]]

(In reply to Simon Pieters from comment #32)
> * :active is not restricted to matching the elements given in
> https://html.spec.whatwg.org/multipage/scripting.html#selector-active

Nevermind this part. I missed the second bullet point.