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

David, thoughts?

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

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.

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?

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

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?

Simple testcase for the three conditions in comment 5.

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

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

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

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.

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

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

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

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

Addressed review comments, carrying forward r=dbaron.

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

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

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

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

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.

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

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

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.

Thanks for the help!

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

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

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

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>

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/").

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