Bug 25811 – Mouseup event does not fire on Scroll Bar

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

[Bryan Haggerty]

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.

Created attachment 61189 [details]
mouseup is not fired on a scrollbar (but mousedown is)

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.

[Mike West]

Created attachment 189050 [details]
Patch

[Mike West]

Reported downstream at https://code.google.com/p/chromium/issues/detail?id=14204, and way downstream at http://bugs.jqueryui.com/ticket/6925

[WebKit Review Bot]

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

[Mike West]

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

[Mike West]

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

[Ojan Vafai]

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

[Elliott Sprehn]

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.

[Mike West]

Created attachment 189269 [details]
Patch

[Mike West]

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

[Mike West]

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

[Tony Chang]

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

[Mike West]

Created attachment 189350 [details]
Patch

[Mike West]

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

[Elliott Sprehn]

LGTM. Shame we have those silly boolean return values for Scrollbar::mouseUp and mouseDown. They always return true!

[Mike West]

Comment on attachment 189350 [details]
Patch

Thanks to you both. :)

[WebKit Review Bot]

Comment on attachment 189350 [details]
Patch

Clearing flags on attachment: 189350

Committed r143560: <http://trac.webkit.org/changeset/143560>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chen Zhixiang]

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?

[Chen Zhixiang]

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.