| Summary: | Don't apply :hover to elements that are :active, but don't contain the mouse | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Shane Stephens <shanestephens> | ||||||||||||||||||||||||||||
| Component: | UI Events | Assignee: | Jeremy Apthorp <jeremya> | ||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | adele, bdakin, darin, dglazkov, eric, hyatt, kevin.cs.oh, kulanthaivel, mikelawther, noel.gordon, rniwa, simon.fraser, webkit.review.bot | ||||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||||||||
| Bug Depends on: | 74158 | ||||||||||||||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||
When the mouse button is pressed and dragged out, the mouse event HitTestRequest is explicitly made HitTestRequest::ReadOnly expecting some selection to happen as a result of this drag. So there won't any style change due to hover/active state change at this event.
EventHandler.cpp, EventHandler::handleMouseMoveEvent()
// Treat mouse move events while the mouse is pressed as "read-only" in prepareMouseEvent
// if we are allowed to select.
// This means that :hover and :active freeze in the state they were in when the mouse
// was pressed, rather than updating for nodes the mouse moves over as you hold the mouse down.
HitTestRequest::HitTestRequestType hitType = HitTestRequest::MouseMove;
if (m_mousePressed && m_mouseDownMayStartSelect)
hitType |= HitTestRequest::ReadOnly;
if (m_mousePressed)
hitType |= HitTestRequest::Active;
It's not clear from the specification whether Firefox's behavior is correct, or Chrome/Safari's is. I'll request clarification from the CSSWG. From Tab Atkins on www-style@: "We defer to the host language to define what those actually mean, which HTML does in <http://www.whatwg.org/specs/web-apps/current-work/complete/links.html#pseudo-classes>. Webkit's doing it wrong." So we should fix this :) Created attachment 100438 [details]
Patch
Comment on attachment 100438 [details] Patch Attachment 100438 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9018055 New failing tests: http/tests/misc/slow-loading-mask.html editing/pasteboard/drop-text-without-selection.html svg/transforms/text-with-mask-with-svg-transform.svg fast/blockflow/japanese-rl-selection.html fast/backgrounds/background-leakage.html fast/box-shadow/scaled-box-shadow.html fast/backgrounds/repeat/negative-offset-repeat.html svg/W3C-SVG-1.1/struct-use-01-t.svg transforms/2d/hindi-rotated.html svg/repaint/filter-repaint.svg fast/blockflow/japanese-lr-selection.html Created attachment 100441 [details]
Archive of layout-test-results from ec2-cr-linux-03
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 100444 [details]
Patch
Comment on attachment 100444 [details] Patch Attachment 100444 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9036001 New failing tests: http/tests/misc/slow-loading-mask.html editing/pasteboard/drop-text-without-selection.html svg/transforms/text-with-mask-with-svg-transform.svg fast/blockflow/japanese-rl-selection.html fast/backgrounds/background-leakage.html fast/box-shadow/scaled-box-shadow.html fast/backgrounds/repeat/negative-offset-repeat.html svg/W3C-SVG-1.1/struct-use-01-t.svg transforms/2d/hindi-rotated.html svg/repaint/filter-repaint.svg fast/blockflow/japanese-lr-selection.html Created attachment 100446 [details]
Archive of layout-test-results from ec2-cr-linux-03
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 101124 [details]
Patch
Short patch-- Simon/Eric, I'm told you guys know this code :) +darin because the line being deleted was added by http://trac.webkit.org/changeset/5343. +hyatt since he appears to be the reviewer for that patch. Comment on attachment 101124 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=101124&action=review > Source/WebCore/page/EventHandler.cpp:-1600 > - // This means that :hover and :active freeze in the state they were in when the mouse > - // was pressed, rather than updating for nodes the mouse moves over as you hold the mouse down. Here is the bug I was fixing 8 years ago with this change: 1. go to www.google.com (or probably any page) 2. Press on one of the underlined links (e.g. "Advertise with Us") Note that the pressed-link feedback appears but then vanishes an instant later, though the mouse is still down over the link. Releasing the mouse in this state looks like it will do nothing, since the link isn't drawn highlighted, but in fact it visits the link. (In reply to comment #13) > (From update of attachment 101124 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=101124&action=review > > > Source/WebCore/page/EventHandler.cpp:-1600 > > - // This means that :hover and :active freeze in the state they were in when the mouse > > - // was pressed, rather than updating for nodes the mouse moves over as you hold the mouse down. > > Here is the bug I was fixing 8 years ago with this change: > > 1. go to www.google.com (or probably any page) > 2. Press on one of the underlined links (e.g. "Advertise with Us") > > Note that the pressed-link feedback appears but then vanishes an instant later, though the mouse is still down over the link. Releasing the mouse in this state looks like it will do nothing, since the link isn't drawn highlighted, but in fact it visits the link. I can't see any difference in behaviour between a tip of tree Mac Chromium with my patch applied and Chrome 12 when I follow those steps. I presume the bug has been fixed elsewhere? I'm not sure I could write a test for this, since I don't know how to get it to fail (and DumpRenderTree's event handling is still a bit of a mystery to me...) (recent google.com front pages don't appear to have a different style for :active links, so I used http://google.com/privacy to test) You may have to test with Safari instead, since Chrome did not exist back when I fixed this bug. Chrome may do something differently. I can't reproduce it in Safari either. (In reply to comment #16) > I can't reproduce it in Safari either. To clarify: I built WebKit with my patch applied and ran it with ./Tools/Scripts/run-webkit, and couldn't see any difference between the patched Safari and Safari 5.0.5. Sounds like bug that was the original reason for doing this is gone. Created attachment 102605 [details]
Patch
Comment on attachment 102605 [details] Patch You'll need ChangeLogs if you want this reviewed. :( see http://www.webkit.org/coding/contributing.html Or use webkit-patch upload (it will prompt you to create the ChangeLogs). The ChangeLogs would explain what your doing and why (for posterity as well as for reviewers when reviewing your patch). The title of this bug should be more explicit about the actual issue. Created attachment 107071 [details]
Patch
I did use webkit-patch upload... it doesn't prompt me for a LayoutTests/ ChangeLog update. (There's already a Source/WebCore ChangeLog update in here.) Strange! Anyway, I've uploaded a new patch with updated changelogs. Comment on attachment 107071 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107071&action=review > LayoutTests/ChangeLog:10 > + * fast/css/hover-active-drag.html: Added. Does this single test case cover all the branches in all the changes? If you leave each part of your patch out, does a test fail? > LayoutTests/fast/css/hover-active-drag.html:32 > + var background = window.getComputedStyle(box, null).getPropertyValue("background-color") For future reference, you typically only need "window." when you are accessing something that might not be defined. You can just call getComputedStyle without the window. part. Created attachment 115303 [details]
Patch
(In reply to comment #24) > (From update of attachment 107071 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=107071&action=review > > > LayoutTests/ChangeLog:10 > > + * fast/css/hover-active-drag.html: Added. > > Does this single test case cover all the branches in all the changes? If you leave each part of your patch out, does a test fail? With the most recent patch, yes. > > LayoutTests/fast/css/hover-active-drag.html:32 > > + var background = window.getComputedStyle(box, null).getPropertyValue("background-color") > > For future reference, you typically only need "window." when you are accessing something that might not be defined. You can just call getComputedStyle without the window. part. Done. Comment on attachment 115303 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115303&action=review Looks sane. Please fix the nits before you land. > Source/WebCore/ChangeLog:4 > + When the mouse is dragged out of an :active element, it should lose > + :hover. Please fit these two lines in one line to avoid the awkward line break. > Source/WebCore/ChangeLog:13 > + (WebCore::EventHandler::handleMouseMoveEvent): > + Don't mark mouse-drag hit tests read-only, since they no longer are. We typically put the description after : followed by one space. > Source/WebCore/ChangeLog:15 > + (WebCore::EventHandler::dragSourceEndedAt): > + Send a hit test request when the mouse goes up after a drag, so Ditto. > Source/WebCore/ChangeLog:19 > + (WebCore::RenderLayer::updateHoverActiveState): > + Only allow the :active state to change on mouse down or mouse up. Ditto. > LayoutTests/ChangeLog:4 > + When the mouse is dragged out of an :active element, it should lose > + :hover. Ditto. Created attachment 116888 [details]
Patch
(In reply to comment #27) > (From update of attachment 115303 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=115303&action=review > > Looks sane. Please fix the nits before you land. > > > Source/WebCore/ChangeLog:4 > > + When the mouse is dragged out of an :active element, it should lose > > + :hover. > > Please fit these two lines in one line to avoid the awkward line break. Done. > > Source/WebCore/ChangeLog:13 > > + (WebCore::EventHandler::handleMouseMoveEvent): > > + Don't mark mouse-drag hit tests read-only, since they no longer are. > > We typically put the description after : followed by one space. Done, though it looks like that varies wildly. > > Source/WebCore/ChangeLog:15 > > + (WebCore::EventHandler::dragSourceEndedAt): > > + Send a hit test request when the mouse goes up after a drag, so > > Ditto. Done. > > Source/WebCore/ChangeLog:19 > > + (WebCore::RenderLayer::updateHoverActiveState): > > + Only allow the :active state to change on mouse down or mouse up. > > Ditto. Done. > > LayoutTests/ChangeLog:4 > > + When the mouse is dragged out of an :active element, it should lose > > + :hover. > > Ditto. Done. Comment on attachment 116888 [details] Patch Rejecting attachment 116888 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: rce/WebCore/rendering/RenderLayer.cpp Hunk #1 succeeded at 3951 (offset 15 lines). Hunk #2 succeeded at 3970 (offset 15 lines). Hunk #3 succeeded at 4015 (offset 15 lines). patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/fast/css/hover-active-drag-expected.txt patching file LayoutTests/fast/css/hover-active-drag.html Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Ryosuke Niwa', u'--for..." exit_code: 1 Full output: http://queues.webkit.org/results/10697385 Created attachment 117321 [details]
Patch
Comment on attachment 117321 [details] Patch Clearing flags on attachment: 117321 Committed r101619: <http://trac.webkit.org/changeset/101619> All reviewed patches have been landed. Closing bug. This is causing a failure on lion: http://build.webkit.org/results/Lion%20Intel%20Release%20(Tests)/r101621%20(3109)/fast/css/hover-active-drag-pretty-diff.html Reopening to attach new patch. Created attachment 118727 [details]
Patch
Comment on attachment 118727 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118727&action=review > LayoutTests/fast/css/hover-active-drag.html:38 > + // This mouse click seems to be required for WebKit's event handling to > + // pick up the :hover class. > + eventSender.mouseDown() > + eventSender.mouseUp() This doesn't look right. We should fix this bug first. Created attachment 118732 [details]
Patch
Comment on attachment 118732 [details] Patch Clearing flags on attachment: 118732 Committed r102632: <http://trac.webkit.org/changeset/102632> All reviewed patches have been landed. Closing bug. This caused pages in background Safari windows to continue to respond to hover, which is bad. This patch caused a regression: https://bugs.webkit.org/show_bug.cgi?id=80536 Should we consider rolling this out? |
Created attachment 87101 [details] test case for bug Content which has a :hover:active and an :active rule applied to it does not switch from the :hover:active state to the :active state when the mouse button is clicked while hovered over the content and then dragged out. the attached test case demonstrates this behaviour. A minimal reproduction is: <style> #button:hover:active {background: green;} #button:active {background: blue;} </style> <div id="button">Div button is div</div> After after clicking on this button, the background is (correctly) green, however on drag-out the background should be blue but remains green. Affects: Chrome 10, Safari 5 Does not affect: Firefox 4