Bug 25811

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.

Created attachment 189050 [details] Patch

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

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

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

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

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

Created attachment 189269 [details] Patch

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

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

Created attachment 189350 [details] Patch

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

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

Comment on attachment 189350 [details] Patch Thanks to you both. :)

Comment on attachment 189350 [details] Patch Clearing flags on attachment: 189350 Committed r143560: <http://trac.webkit.org/changeset/143560>

All reviewed patches have been landed. Closing bug.

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?

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.