758885 - CSS :hover regression in Firefox 13 when an element's class name is set by Javascript

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[mjh563]

Attachment: testcase

The attached testcase sets the class name of an element in window.onload, but in Firefox >= 13 the CSS rule that references that class name is not applied.

The testcase works as expected in Opera, Google Chrome and Firefox <= 12.

Comment 1

[mjh563]

Attachment: New testcase

Sorry, the previous attachment was wrong. 

In this testcase, the block should turn red when you hover the mouse over it. This works in Firefox 12 and other browsers, but not in Firefox 13.

Comment 2

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

Regression range from Linux m-c nightlies is (not surprisingly; I only downloaded 2 nightlies to test this):
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1ca7a94573f2&tochange=c71845b3b2a6

Comment 3

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

Confirmed as regression from https://hg.mozilla.org/mozilla-central/rev/4bd80687cb32 by local backout of the nsCSSFrameConstructor.cpp changes in it.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Taking.  Wish this had gotten reported earlier; at this point the chances of me being able to convince the beta drivers to take a fix for 13 might be slim.  :(

Comment 5

[Boris Zbarsky [:bzbarsky]]

So the key issue here is that during initial CSS matching the node does not have class="test", so we short-circuit matching the ".test:hover" selector before matching the :hover part, and hence don't set the "affected by :hover" flag on the element.

Then when the class is changed, we do HasAttributeDependentStyle, but the ".test:hover" selector doesn't match because ":hover" doesn't match, so we don't do a restyle on the element.  During HasAttributeDependentStyle, mForStyling is false, so we don't set the "affected by :hover" flag again.

Finally, when we enter hover the "affected by :hover" flag is not set, so we don't think we need to restyle.

Nice testcase!

Comment 6

[Boris Zbarsky [:bzbarsky]]

And even a naive change that fixed HasAttributeDependentStyle when matching :hover to flag the node would fail in general, I think, because we shortcut the matching in AttrEnumFunc altogether if the restyle hint for the current selector is subsumed by our pending restyle hint.  So we might not match the :hover during the restyle at all.

Comment 7

[Boris Zbarsky [:bzbarsky]]

So the obvious options are:

1)  Back out bug 732667.
2)  Add a boolean flag to nsCSSSelector (maybe shrinking the number of bits mPseudoType
    to avoid changing the struct size) that indicates whether the selector includes
    :hover.  Set this as needed while building the rule cascade; check for this flag in
    SelectorMatches() after the namespace/tag checks, and set the bit on the DOM node
    accordingly.  Need to watch out for :not here.
3)  Restrict the optimization from bug 732667 to selectors that only include :hover with a
    tag name and nothing else.  It would still be good enough for github, and would not be
    subject to the issues here because tag names are immutable.

Everything else I've thought of so far is a slower version of #2.

I think #3 is the way to go, personally.  I'll write up a patch.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Don't apply the dynamic :hover reresolution skipping optimization to selectors which can match on mutable state other than :hover.

I tried to add a test for this to test_hover, but couldn't get it to fail even without this patch....

Comment 9

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

Comment on attachment 627838 [details] [diff] [review]
Don't apply the dynamic :hover reresolution skipping optimization to selectors which can match on mutable state other than :hover.

r=dbaron

I guess AncestorFilter interactions are ok because any change to a class or ID on an ancestor will always trigger the full restyle when there's a selector that's relevant (because we're only looking at part of the selector).

Comment 10

[Boris Zbarsky [:bzbarsky]]

Yes, exactly.

Comment 11

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/6aa5239fe656

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 627838 [details] [diff] [review]
Don't apply the dynamic :hover reresolution skipping optimization to selectors which can match on mutable state other than :hover.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 732667 
User impact if declined: Some elements that should respond to hover won't
Testing completed (on m-c, etc.): Passes attached test and some other basic
   smoketesting.
Risk to taking this patch (and alternatives if risky): Risk should be low: the
   patch is making an optimization apply in fewer cases so it doesn't lead to
   incorrect behavior.  The main alternative is disabling the optimization
   altogether (i.e. backing out bug 732667).
String or UUID changes made by this patch: None

Comment 13

[Alex Keybl [:akeybl]]

Comment on attachment 627838 [details] [diff] [review]
Don't apply the dynamic :hover reresolution skipping optimization to selectors which can match on mutable state other than :hover.

[Triage Comment]
There isn't enough evidence that this is a common action in websites to warrant any additional risk between our final beta and RC. We haven't received feedback around site breakage caused by this bug. Please re-nominate if you disagree.

We will, however, release note this bug for FF13. Approving for Aurora 14.

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6aa5239fe656

Comment 15

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/89176b6d643c

Comment 16

[Mustafa]

Hi

Im having this same issue, even in the test case this problem still exists. Using Mac firefox 13, tested in earlier versions of Firefox, this works fine but the problem persists in firefox 13.

Comment 17

[mjh563]

Yes, the fix didn't quite make it into version 13. It's fixed in 14 though, which is due to be released in mid-July (it's currently in beta).

Comment 18

[Boris Zbarsky [:bzbarsky]]

Indeed.  See the Firefox 13 release notes...

Comment 19

[Mustafa]

Thanks for getting back to me so quickly guys :)

Comment 20

[Simona Badau, Desktop QA]

Verified using the test case attached in the Description that the CSS rule is applied. 

Verified using Firefox 14 beta 8 on Windows 7, Ubuntu 12.04 and Mac OS X 10.6:

Mozilla/5.0 (Windows NT 6.1; rv:14.0) Gecko/20100101 Firefox/14.0
Mozilla/5.0 (X11; Linux i686; rv:14.0) Gecko/20100101 Firefox/14.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:14.0) Gecko/20100101 Firefox/14.0

Comment 21

[Simona Badau, Desktop QA]

Verified on Firefox 15 beta 3, using the test case attached in the Description that - the CSS rule is applied. 

Verified on Windows XP, Ubuntu 12.04 and Mac OS X 10.7:

Mozilla/5.0 (Windows NT 5.1; rv:15.0) Gecko/20100101 Firefox/15.0
Mozilla/5.0 (X11; Linux i686; rv:15.0) Gecko/20100101 Firefox/15.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:15.0) Gecko/20100101 Firefox/15.0