Closed Bug 758885 Opened 8 years ago Closed 8 years ago
503 bytes, text/html
Don't apply the dynamic :hover reresolution skipping optimization to selectors which can match on mutable state other than :hover.
3.51 KB, patch
|Details | Diff | Splinter Review|
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
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: x86 → All
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)
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+
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
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.
Status: NEW → RESOLVED
Closed: 8 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
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
You need to log in before you can comment on or make changes to this bug.