Last Comment Bug 783213 - :active and :hover quirk should only apply to links
: :active and :hover quirk should only apply to links
Status: RESOLVED FIXED
[lang=c++]
: dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
P4 normal with 1 vote (vote)
: mozilla36
Assigned To: Brian Marshall
:
: Jet Villegas (:jet)
Mentors: David Baron :dbaron: ⌚️UTC-8
Depends on: 1176695
Blocks: quirks-mode-spec
  Show dependency treegraph
 
Reported: 2012-08-16 03:35 PDT by Simon Pieters
Modified: 2015-06-22 11:32 PDT (History)
7 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase for some selector conditions (640 bytes, text/html)
2014-11-05 23:03 PST, Brian Marshall
no flags Details
Part 1: Only apply the :active and :hover quirk to links, and not when the selector uses other pseudo-classes (3.99 KB, patch)
2014-11-09 12:50 PST, Brian Marshall
dbaron: review+
Details | Diff | Splinter Review
Part 2: Don't apply the quirk to selectors that have a pseudo-element or are part of a pseudo-class argument (12.40 KB, patch)
2014-11-09 13:01 PST, Brian Marshall
dbaron: review-
Details | Diff | Splinter Review
Part 3: Tests (3.67 KB, patch)
2014-11-09 13:05 PST, Brian Marshall
dbaron: review+
Details | Diff | Splinter Review
Part 3: Tests (4.06 KB, patch)
2014-11-10 21:54 PST, Brian Marshall
bmarsd: review+
dbaron: review+
Details | Diff | Splinter Review
Part 2: Don't apply the quirk to selectors that use a pseudo-element or are part of a pseudo-class argument (15.28 KB, patch)
2014-11-12 13:21 PST, Brian Marshall
dbaron: review+
Details | Diff | Splinter Review
Part 2: Don't apply the quirk to selectors that use a pseudo-element or are part of a pseudo-class argument (16.14 KB, patch)
2014-11-13 21:57 PST, Brian Marshall
bmarsd: review+
dbaron: review+
Details | Diff | Splinter Review

Description User image Simon Pieters 2012-08-16 03:35:14 PDT
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 User image Boris Zbarsky [:bz] (still a bit busy) 2012-08-21 22:17:58 PDT
David, thoughts?
Comment 2 User image Boris Zbarsky [:bz] (still a bit busy) 2012-08-21 22:18:29 PDT
Note that the definition of "link" is different in different browsers, by the way....
Comment 3 User image David Baron :dbaron: ⌚️UTC-8 2012-08-22 12:27:15 PDT
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 5 User image Brian Marshall 2014-11-03 17:15:16 PST
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 User image David Baron :dbaron: ⌚️UTC-8 2014-11-03 17:54:14 PST
(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 User image Brian Marshall 2014-11-04 16:32:03 PST
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 User image Brian Marshall 2014-11-05 23:03:41 PST
Created attachment 8517947 [details]
Testcase for some selector conditions

Simple testcase for the three conditions in comment 5.
Comment 9 User image David Baron :dbaron: ⌚️UTC-8 2014-11-06 07:35:57 PST
(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 User image Brian Marshall 2014-11-06 22:17:53 PST
David, thanks for your help so far! I'll try to get some patches ready soon.
Comment 11 User image Brian Marshall 2014-11-09 12:50:32 PST
Created attachment 8519587 [details] [diff] [review]
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 User image Brian Marshall 2014-11-09 13:01:59 PST
Created 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

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 User image Brian Marshall 2014-11-09 13:05:13 PST
Created attachment 8519590 [details] [diff] [review]
Part 3: Tests

This is the last part. I used a mochitest in order to synthesize the :hover state.
Comment 14 User image David Baron :dbaron: ⌚️UTC-8 2014-11-10 14:06:23 PST
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 User image David Baron :dbaron: ⌚️UTC-8 2014-11-10 14:13:42 PST
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 User image David Baron :dbaron: ⌚️UTC-8 2014-11-10 14:16:33 PST
(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 User image David Baron :dbaron: ⌚️UTC-8 2014-11-10 14:29:59 PST
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 User image Brian Marshall 2014-11-10 17:59:38 PST
(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 User image Brian Marshall 2014-11-10 21:54:50 PST
Created attachment 8520411 [details] [diff] [review]
Part 3: Tests

Addressed review comments, carrying forward r=dbaron.
Comment 20 User image David Baron :dbaron: ⌚️UTC-8 2014-11-11 13:57:21 PST
(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 User image Brian Marshall 2014-11-12 13:21:22 PST
Created 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

In addition to the other changes, ActiveHoverQuirkMatches now checks aSelector->IsPseudoElement() to handle selectors like "::-moz-progress-bar:hover".
Comment 22 User image David Baron :dbaron: ⌚️UTC-8 2014-11-13 14:28:56 PST
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 User image Brian Marshall 2014-11-13 16:07:13 PST
(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 User image David Baron :dbaron: ⌚️UTC-8 2014-11-13 16:12:38 PST
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 User image Brian Marshall 2014-11-13 21:57:29 PST
Created attachment 8522714 [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

Addressed review comments and added explicit #includes for TypedEnum.h/TypedEnumBits.h. Carrying forward r=dbaron.
Comment 26 User image David Baron :dbaron: ⌚️UTC-8 2014-11-13 23:01:39 PST
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 User image David Baron :dbaron: ⌚️UTC-8 2014-11-14 08:50:17 PST
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 User image Brian Marshall 2014-11-14 10:19:39 PST
Thanks for the help!
Comment 30 User image Jean-Yves Perrier [:teoli] 2014-12-08 08:21:04 PST
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 User image Brian Marshall 2014-12-09 23:27:08 PST
(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 User image Simon Pieters 2014-12-18 08:00:36 PST
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 User image Simon Pieters 2014-12-18 22:18:19 PST
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 User image Simon Pieters 2014-12-19 00:15:45 PST
(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.

Note You need to log in before you can comment on or make changes to this bug.