Last Comment Bug 758885 - CSS :hover regression in Firefox 13 when an element's class name is set by Javascript
: CSS :hover regression in Firefox 13 when an element's class name is set by Ja...
Status: RESOLVED FIXED
[qa+]
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: 13 Branch
: All All
: -- normal (vote)
: mozilla15
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks: 732667
  Show dependency treegraph
 
Reported: 2012-05-26 08:20 PDT by mjh563
Modified: 2012-08-02 08:33 PDT (History)
8 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+
verified
+
verified


Attachments
testcase (473 bytes, text/html)
2012-05-26 08:20 PDT, mjh563
no flags Details
New testcase (503 bytes, text/html)
2012-05-26 08:39 PDT, mjh563
no flags Details
Don't apply the dynamic :hover reresolution skipping optimization to selectors which can match on mutable state other than :hover. (3.51 KB, patch)
2012-05-28 20:30 PDT, Boris Zbarsky [:bz]
dbaron: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
Details | Diff | Splinter Review

Description mjh563 2012-05-26 08:20:02 PDT
Created attachment 627486 [details]
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 2012-05-26 08:39:17 PDT
Created attachment 627487 [details]
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-7 (review requests must explain patch) 2012-05-26 09:42:17 PDT
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-7 (review requests must explain patch) 2012-05-26 09:55:56 PDT
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 [:bz] 2012-05-28 19:23:54 PDT
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 [:bz] 2012-05-28 19:29:11 PDT
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 [:bz] 2012-05-28 19:37:36 PDT
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 [:bz] 2012-05-28 19:45:43 PDT
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 [:bz] 2012-05-28 20:30:25 PDT
Created 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.

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-7 (review requests must explain patch) 2012-05-28 21:09:16 PDT
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 [:bz] 2012-05-28 21:15:14 PDT
Yes, exactly.
Comment 12 Boris Zbarsky [:bz] 2012-05-28 21:20:30 PDT
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] 2012-05-29 09:51:04 PDT
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] 2012-05-29 10:16:11 PDT
https://hg.mozilla.org/mozilla-central/rev/6aa5239fe656
Comment 16 Mustafa 2012-06-13 04:07:38 PDT
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 2012-06-13 04:30:04 PDT
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 [:bz] 2012-06-13 08:16:34 PDT
Indeed.  See the Firefox 13 release notes...
Comment 19 Mustafa 2012-06-14 01:57:05 PDT
Thanks for getting back to me so quickly guys :)
Comment 20 Simona B [:simonab ] -PTO- back Sept 5th 2012-06-22 05:52:52 PDT
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 B [:simonab ] -PTO- back Sept 5th 2012-08-02 08:33:57 PDT
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

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