Summary: | REGRESSION: style change where :hover changes only an :after style doesn't work | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | arnau siches <asiches> | ||||||||||||
Component: | CSS | Assignee: | Beth Dakin <bdakin> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Major | CC: | alice.barraclough, bdakin, darin, hyatt, jlukas, joost, mitz, smenor | ||||||||||||
Priority: | P1 | Keywords: | HasReduction, InRadar, Regression | ||||||||||||
Version: | 420+ | ||||||||||||||
Hardware: | Mac | ||||||||||||||
OS: | OS X 10.4 | ||||||||||||||
URL: | http://css.artnau.com/exemples/safari/bugs/safari-bug-absolute-pseudoelement.html | ||||||||||||||
Attachments: |
|
Description
arnau siches
2006-01-08 10:23:57 PST
This bug is resolved in Safari 2.0.3 (with Mac OS X 10.4.4) ToT behaves erratically on this test for me - the text doesn't change on each mouse in/out. *** Bug 6924 has been marked as a duplicate of this bug. *** 6924 has a testcase for this as well. Marking as Regression, as i just realized it is one, hence also P1 major. The problem seems to be simply that NodeImpl::diff() doesn't consider pseudo styles. If you make the base style for :hover different, recalcStyle() will notice the difference and update correctly. You can verify by adding div:hover { color: maroon; } to the testcase. Particularly telling is the comment in RenderStyle::operator== that says: // compare everything except the pseudoStyle pointer Clearly at the very least we need a version of == that *does* compare the pseudoStyle pointer. Created attachment 6413 [details] work in progress The first part of this unfinished patch is about making diff notice :before and :after. It's not quite right yet. The second part is needed because without it, in the detach-attach sequence, the detach resets the hovered state (as part of the fix for bug 5983 and bug 6821), making the attach compute the non-hovered style, causing infinite looping. That part probably needs work too, in case the hovered node does detach permanently. Created attachment 6780 [details]
Patch
Comment on attachment 6780 [details]
Patch
You shouldn't have to do a detach just because :before/:after content changed.
All you have to do is set the new RenderStyle, and the rest will be taken care of by updatePseudoChild.
A change of NoInherit returned from the diff function should be sufficient to cause the right thing to happen.
Created attachment 7256 [details]
Patch using NoInherit instead of Detach
Here is a new patch based on Mitz's patch that uses NoInherit instead of Detach.
Created attachment 7261 [details]
Prettier patch made after talking with Hyatt
Created attachment 7262 [details]
Even prettier
An even prettier patch after talking with Hyatt and Darin. Is that comment clear enough?
Comment on attachment 7262 [details]
Even prettier
Needs a test, but still: r=me
*** Bug 9104 has been marked as a duplicate of this bug. *** |