Closed Bug 783213 Opened 12 years ago Closed 10 years ago

:active and :hover quirk should only apply to links

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla36

People

(Reporter: zcorpan, Assigned: bmarsd, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [lang=c++])

Attachments

(4 files, 3 obsolete files)

Testcase for some selector conditions
640 bytes, text/html
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
dbaron
: review+
Details | Diff | Splinter Review
Part 3: Tests
4.06 KB, patch
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
16.14 KB, patch
bmarsd
: review+
dbaron
: review+
Details | Diff | Splinter Review 
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
Blocks: quirks-mode-spec
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.
OS: Mac OS X → All
Priority: -- → P4
Hardware: x86 → All
Whiteboard: [mentor=dbaron] → [mentor=dbaron][lang=c++]
Mentor: dbaron
Whiteboard: [mentor=dbaron][lang=c++] → [lang=c++]
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?
Flags: needinfo?(dbaron)
(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.
Flags: needinfo?(dbaron)
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?
Flags: needinfo?(dbaron)
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.
Flags: needinfo?(dbaron)
David, thanks for your help so far! I'll try to get some patches ready soon.
Assignee: nobody → bmarsd
Status: NEW → ASSIGNED
This patch updates the quirk using data already available, without adding extra mechanisms.
Attachment #8519587 - Flags: review?(dbaron)
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.
Attachment #8519589 - Flags: review?(dbaron)
Attached patch Part 3: Tests (obsolete) — — Splinter Review 
This is the last part. I used a mochitest in order to synthesize the :hover state.
Attachment #8519590 - Flags: review?(dbaron)
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.
Flags: needinfo?(bmarsd)
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
Attachment #8519590 - Flags: review?(dbaron) → review+
(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.
Flags: needinfo?(bmarsd)
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.
Attachment #8519589 - Flags: review?(dbaron) → review-
(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?
Flags: needinfo?(dbaron)
Attached patch Part 3: Tests — — Splinter Review 
Addressed review comments, carrying forward r=dbaron.
Attachment #8519590 - Attachment is obsolete: true
Attachment #8520411 - Flags: review+
(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.)
Flags: needinfo?(dbaron)
In addition to the other changes, ActiveHoverQuirkMatches now checks aSelector->IsPseudoElement() to handle selectors like "::-moz-progress-bar:hover".
Attachment #8519589 - Attachment is obsolete: true
Attachment #8521669 - Flags: review?(dbaron)
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
Attachment #8521669 - Flags: review?(dbaron) → review+
(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?
Flags: needinfo?(dbaron)
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.
Flags: needinfo?(dbaron)
Addressed review comments and added explicit #includes for TypedEnum.h/TypedEnumBits.h. Carrying forward r=dbaron.
Attachment #8521669 - Attachment is obsolete: true
Attachment #8522714 - Flags: review+
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
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
Keywords: dev-doc-needed → dev-doc-complete
(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.
Depends on: 1176695
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: