Bug 25811 - Mouseup event does not fire on Scroll Bar
Summary: Mouseup event does not fire on Scroll Bar
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.5
: P2 Normal
Assignee: Mike West
URL:
Keywords:
Depends on:
Blocks: 110007
  Show dependency treegraph
 
Reported: 2009-05-14 16:39 PDT by Bryan Haggerty
Modified: 2013-07-26 01:47 PDT (History)
11 users (show)

See Also:


Attachments
mouseup is not fired on a scrollbar (but mousedown is) (729 bytes, text/html)
2010-07-11 20:34 PDT, jaroslav.benc
no flags Details
Patch (5.04 KB, patch)
2013-02-19 04:16 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch (8.77 KB, patch)
2013-02-20 02:31 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch (10.52 KB, patch)
2013-02-20 11:46 PST, Mike West
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bryan Haggerty 2009-05-14 16:39:00 PDT
When pressing down on a scroll bar and then releasing, the mouseup event is not triggered. Oddly though, if a user double clicks on a scroll bar the mouseup event is triggered, but only once.
Comment 1 jaroslav.benc 2010-07-11 20:34:38 PDT
Created attachment 61189 [details]
mouseup is not fired on a scrollbar (but mousedown is)
Comment 2 jaroslav.benc 2010-07-11 20:35:20 PDT
This bug is specially critical when using drag and drop - if user starts dragging clicking on the scrollbar a mouseup event is not fired and dragging can't be stopped, document mouseup is not fired as well. There is a test case attached... Confirmed on Windows/Linux, Chrome, Safari.
Comment 3 Mike West 2013-02-19 04:16:18 PST
Created attachment 189050 [details]
Patch
Comment 4 Mike West 2013-02-19 04:33:36 PST
Reported downstream at https://code.google.com/p/chromium/issues/detail?id=14204, and way downstream at http://bugs.jqueryui.com/ticket/6925
Comment 5 WebKit Review Bot 2013-02-19 05:58:43 PST
Comment on attachment 189050 [details]
Patch

Attachment 189050 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16629436

New failing tests:
platform/chromium/virtual/gpu/compositedscrolling/scrollbars/scrollbar-middleclick-nopaste.html
scrollbars/scrollbar-middleclick-nopaste.html
Comment 6 Mike West 2013-02-19 06:02:29 PST
(In reply to comment #5)
> (From update of attachment 189050 [details])
> Attachment 189050 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/16629436
> 
> New failing tests:
> platform/chromium/virtual/gpu/compositedscrolling/scrollbars/scrollbar-middleclick-nopaste.html
> scrollbars/scrollbar-middleclick-nopaste.html

Hrm. These pass for me locally on the chromium/mac port. I'll poke at it on Linux tomorrow, I suppose.
Comment 7 Mike West 2013-02-19 09:01:57 PST
It's somewhat unclear what should happen here. If we send a mousedown, we should certainly send a mouseup as well. I'm not sure we should send a mousedown in the first place, honestly.

Ojan, Alexey, do you have opinions? :)
Comment 8 Ojan Vafai 2013-02-19 15:18:57 PST
Elliot spent some time looking into this and might have thoughts.

This case seems reasonable to me, especially since it matches Firefox. I don't know of any native platform conventions in either direction here, so we may as well match Firefox and simplify things for web developers.

FWIW, this isn't the only context in which we fire a mousedown without a corresponding mouseup. If you right-click, we fire mouseup on some OS's and not on others. It's a big mess, but it's consistent with platform native behavior. For example, on Linux, it's the native behavior that the context menu is popped up on mousedown and that other windows don't receive any events while the context menu is up. I'm not convinced we want to maintain platform conventions in this case over simplicity of the web platform, but that's a whole bigger discussion that should happen elsewhere. :)
Comment 9 Elliott Sprehn 2013-02-19 15:25:31 PST
Comment on attachment 189050 [details]
Patch

This patch is wrong. In gecko if you click on a scrollbar, move your mouse to the other side of the page and then release the mouse up event is still fired on the scrollbar element. Your patch is going to do a new hit test and fire a mouseup somewhere else.
Comment 10 Mike West 2013-02-20 02:31:03 PST
Created attachment 189269 [details]
Patch
Comment 11 Mike West 2013-02-20 02:32:48 PST
(In reply to comment #9)
> (From update of attachment 189050 [details])
> This patch is wrong. In gecko if you click on a scrollbar, move your mouse to the other side of the page and then release the mouse up event is still fired on the scrollbar element. Your patch is going to do a new hit test and fire a mouseup somewhere else.

Indeed. It looks like we're storing the previously mousedowned node in m_lastNodeUnderMouse. Dispatching an event targeted at that node passes the new test I've put together, and I don't think it will have unpleasant side effects (especially considering that we're currently exiting early at this point).

I'd appreciate you taking another look while I poke at the linux failure. :)
Comment 12 Mike West 2013-02-20 02:57:41 PST
(In reply to comment #11)
> I'd appreciate you taking another look while I poke at the linux failure. :)

The linux test failures are avoided by not calling EventHandler::handleMouseReleaseEvent(), which ends up calling handlePasteGlobalSelection().
Comment 13 Tony Chang 2013-02-20 11:16:19 PST
Comment on attachment 189269 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189269&action=review

Do we want test cases for middle mouse click too?

> Source/WebCore/page/EventHandler.cpp:1829
> +        return !dispatchMouseEvent(eventNames().mouseupEvent, m_lastNodeUnderMouse.get(), true, m_clickCount, mouseEvent, false);

Nit: Can you make local variables for the bools?
bool cancelable = true;
bool setUnder = false;
return !dispatchMouseEvent(eventNames().mouseupEvent, m_lastNodeUnderMouse.get(), cancelable, m_clickCount, mouseEvent, setUnder);
Comment 14 Mike West 2013-02-20 11:46:29 PST
Created attachment 189350 [details]
Patch
Comment 15 Mike West 2013-02-20 11:48:02 PST
(In reply to comment #13)
> (From update of attachment 189269 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189269&action=review
> 
> Do we want test cases for middle mouse click too?

I've added middle clicks to both layout tests.

> > Source/WebCore/page/EventHandler.cpp:1829
> > +        return !dispatchMouseEvent(eventNames().mouseupEvent, m_lastNodeUnderMouse.get(), true, m_clickCount, mouseEvent, false);
> 
> Nit: Can you make local variables for the bools?
> bool cancelable = true;
> bool setUnder = false;
> return !dispatchMouseEvent(eventNames().mouseupEvent, m_lastNodeUnderMouse.get(), cancelable, m_clickCount, mouseEvent, setUnder);

Done. This pattern isn't used anywhere else in the method, though. I'm happy to change the other dispatchMouseEvent callsites to match, but I'm not sure that's worthwhile.
Comment 16 Elliott Sprehn 2013-02-20 14:28:20 PST
LGTM. Shame we have those silly boolean return values for Scrollbar::mouseUp and mouseDown. They always return true!
Comment 17 Mike West 2013-02-20 20:10:38 PST
Comment on attachment 189350 [details]
Patch

Thanks to you both. :)
Comment 18 WebKit Review Bot 2013-02-20 20:23:15 PST
Comment on attachment 189350 [details]
Patch

Clearing flags on attachment: 189350

Committed r143560: <http://trac.webkit.org/changeset/143560>
Comment 19 WebKit Review Bot 2013-02-20 20:23:20 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 Chen Zhixiang 2013-05-29 01:14:47 PDT
I've back ported this patch into Qt-4.8.0 and verified for http://www.actiz.jp/ (The route search draggable popup has this problem)

It partly fixes the problem, but there is still something wrong:

If you press on the scroll bar, hold the `pressed` state, and then drag, nothing will happen, but if use Firefox, it can drag the whole element.

Seems this patch solved the "No-MouseUp" problem, but it did not care the mouse's button pressed state?
Comment 21 Chen Zhixiang 2013-07-26 01:47:06 PDT
New problem:

http://openlayers.org/dev/tests/run-tests.html

When I use qtwebkit-2.3.1 to navigate, click "run-all", the right <div> element's vertical scrollbar seems to have the similar problem: when click on the <DOWN> button of vertical scrollbar, the <DOWN> button keeps `pressed ` state and the <div> keeps down-scrolling...

After applying this patch, problem still exists.