Closed Bug 758885 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

13 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15
Tracking Status
firefox13 + ---
firefox14 + verified
firefox15 + verified

People

(Reporter: mjh563, Assigned: bzbarsky)

References

Details

(Whiteboard: [qa+])

Attachments

(2 files, 1 obsolete file)

Attached file testcase (obsolete) —
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.
Attachment #627486 - Attachment mime type: text/plain → text/html
Attached file 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.
Attachment #627486 - Attachment is obsolete: true
Attachment #627487 - Attachment mime type: text/plain → text/html
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
Confirmed as regression from https://hg.mozilla.org/mozilla-central/rev/4bd80687cb32 by local backout of the nsCSSFrameConstructor.cpp changes in it.
Blocks: 732667
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: x86 → All
Summary: CSS 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 Javascript
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.  :(
Assignee: nobody → 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!
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.
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.
I tried to add a test for this to test_hover, but couldn't get it to fail even without this patch....
Attachment #627838 - Flags: review?(dbaron)
Whiteboard: [need review]
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).
Attachment #627838 - Flags: review?(dbaron) → review+
Yes, exactly.
http://hg.mozilla.org/integration/mozilla-inbound/rev/6aa5239fe656
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla15
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
Attachment #627838 - Flags: approval-mozilla-beta?
Attachment #627838 - Flags: approval-mozilla-aurora?
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.
Attachment #627838 - Flags: approval-mozilla-beta?
Attachment #627838 - Flags: approval-mozilla-beta-
Attachment #627838 - Flags: approval-mozilla-aurora?
Attachment #627838 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/6aa5239fe656
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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.
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).
Indeed.  See the Firefox 13 release notes...
Thanks for getting back to me so quickly guys :)
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
Whiteboard: [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
Duplicate of this bug: 774451
You need to log in before you can comment on or make changes to this bug.