Bug 57206 – Don't apply :hover to elements that are :active, but don't contain the mouse

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Shane Stephens]

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

[Kulanthaivel Palanichamy]

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;

[Shane Stephens]

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.

[Shane Stephens]

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 :)

[Jeremy Apthorp]

Created attachment 100438 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Jeremy Apthorp]

Created attachment 100444 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Jeremy Apthorp]

Created attachment 101124 [details]
Patch

[Jeremy Apthorp]

Short patch-- Simon/Eric, I'm told you guys know this code :)

[Ryosuke Niwa]

+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.

[Darin Adler]

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.

[Jeremy Apthorp]

(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)

[Darin Adler]

You may have to test with Safari instead, since Chrome did not exist back when I fixed this bug. Chrome may do something differently.

[Jeremy Apthorp]

I can't reproduce it in Safari either.

[Jeremy Apthorp]

(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.

[Darin Adler]

Sounds like bug that was the original reason for doing this is gone.

[Jeremy Apthorp]

Created attachment 102605 [details]
Patch

[Eric Seidel (no email)]

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).

[Simon Fraser (smfr)]

The title of this bug should be more explicit about the actual issue.

[Jeremy Apthorp]

Created attachment 107071 [details]
Patch

[Jeremy Apthorp]

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.

[Darin Adler]

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.

[Jeremy Apthorp]

Created attachment 115303 [details]
Patch

[Jeremy Apthorp]

(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.

[Ryosuke Niwa]

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.

[Jeremy Apthorp]

Created attachment 116888 [details]
Patch

[Jeremy Apthorp]

(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.

[WebKit Review Bot]

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

[Jeremy Apthorp]

Created attachment 117321 [details]
Patch

[WebKit Review Bot]

Comment on attachment 117321 [details]
Patch

Clearing flags on attachment: 117321

Committed r101619: <http://trac.webkit.org/changeset/101619>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

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

[Jeremy Apthorp]

Reopening to attach new patch.

[Jeremy Apthorp]

Created attachment 118727 [details]
Patch

[Ryosuke Niwa]

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.

[Jeremy Apthorp]

Created attachment 118732 [details]
Patch

[WebKit Review Bot]

Comment on attachment 118732 [details]
Patch

Clearing flags on attachment: 118732

Committed r102632: <http://trac.webkit.org/changeset/102632>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

This caused pages in background Safari windows to continue to respond to hover, which is bad.

[Beth Dakin]

This patch caused a regression: https://bugs.webkit.org/show_bug.cgi?id=80536

[Adele Peterson]

Should we consider rolling this out?