Bug 32303

Summary: Middle-mouse button not firing mousedown DOM event when autoscroll happens
Product: WebKit Reporter: Brenton <webkit>
Component: WebCore JavaScriptAssignee: Brian Weinstein <bweinstein>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, bweinstein, eric, keremonal, maxime.britto, webkit, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows Vista   
Attachments:
Description Flags
[PATCH] WIP Patch
none
[PATCH] Fix
aroben: review+, bweinstein: commit-queue-
Same as Brian's fix, but corrects references to bug 28023 in his test HTML files. none

Brenton
Reported 2009-12-08 21:39:23 PST
In Chrome and Safari on Windows, middle-clicking only dispatches a MouseEvent if auto-scroll doesn't spawn. Firefox, IE, and Opera dispatch the mousedown event, even if they support and spawn autoscroll. The Mac versions of Chrome and Safari don't spawn autoscroll, and they dispatch the event properly. Steps to reproduce: 1. Navigate to a page with a scrollbar. 2. Open the JavaScript console and enter the following: addEventListener('mousedown', function(event) { console.log(event.button) } ); 3. Middle-click on the page Expected result: '1' is traced in the JavaScript console. If event.preventDefault() is not called, the scroll arrows appear. Actual result: The auto-scroll arrows appear, but no event is dispatched. This has been tested in Chrome 4.0.249.30, Safari 4.0.3, and the Windows WebKit nightly. Each fails.
Attachments
[PATCH] WIP Patch (3.29 KB, patch)
2009-12-10 15:30 PST, Brian Weinstein 		no flags
DetailsFormatted DiffDiff
[PATCH] Fix (11.45 KB, patch)
2009-12-10 16:40 PST, Brian Weinstein 		aroben: review+
bweinstein: commit-queue-
DetailsFormatted DiffDiff
Same as Brian's fix, but corrects references to bug 28023 in his test HTML files. (11.36 KB, application/octet-stream)
2009-12-10 18:33 PST, Brenton 		no flags
Details
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Brenton
Comment 1 2009-12-09 16:29:39 PST
I've gone through the source and believe I've confirmed the bug. In trunk/WebCore/page/EventHandler.cpp, a routine starts on line 1141 that sniffs if mouseEvent.button() == MiddleButton. If it does and a renderer is present, the function returns on line 1179. dispatchMouseEvent doesn't occur til line 1183, which is AFTER the return statement for middle mouse events. I believe this bug started in rev 35083 when Maxime Britto added pan-scrolling. I attempted to confirm this via nightly builds, but the only Safari version I could find from that era was 3.2.3, which appears to be incompatible with those nightlies. (WebKitShouldUseFontSmoothing could not be found in WebKit.dll). However, in rev 35083, the PAN_SCROLLING routine returns on line 926 and the dispatch call doesn't occur until line 944.
Brenton
Comment 2 2009-12-10 13:57:07 PST
I'm lobbying hard to get this fixed because it is blocking Windows installs of any Chrome extension that uses the middle-mouse button. For instance: https://chrome.google.com/extensions/detail/dhbobbhcjchoconllpepbcanpfbmebgc Chromium bug reports: http://code.google.com/p/chromium/issues/detail?id=17234 http://code.google.com/p/chromium/issues/detail?id=29826
Brian Weinstein
Comment 3 2009-12-10 15:30:14 PST
Created attachment 44643 [details] [PATCH] WIP Patch This patch changes to behaviors: 1) Fires mousedown event on middle click, event when pan scrolling would start. 2) Allows event.preventDefault to prevent pan scrolling from starting.
Brian Weinstein
Comment 4 2009-12-10 15:31:54 PST
Note, this needs a ChangeLog and layout tests, this was just for discussion about the Change.
Adam Roben (:aroben)
Comment 5 2009-12-10 15:35:37 PST
Comment on attachment 44643 [details] [PATCH] WIP Patch This looks pretty good! I think there's an unintended change in behavior here: previously, I think pan-scrolling was always handled by the main frame's EventHandler. Now it looks like it will be handled by the target node's EventHandler. I assume this could cause trouble when pan-scrolling starts in a subframe.
Brian Weinstein
Comment 6 2009-12-10 16:40:36 PST
Created attachment 44649 [details] [PATCH] Fix With ChangeLog and test goodness.
WebKit Review Bot
Comment 7 2009-12-10 16:44:15 PST
style-queue ran check-webkit-style on attachment 44649 [details] without any errors.
Brenton
Comment 8 2009-12-10 18:28:48 PST
This is a trivial change, but in your HTML tests you reference bug 28023 while linking to bug 32303.
Brenton
Comment 9 2009-12-10 18:33:15 PST
Created attachment 44656 [details] Same as Brian's fix, but corrects references to bug 28023 in his test HTML files.
Maxime Britto
Comment 10 2009-12-11 00:33:14 PST
Thanks for adding me to the CC list. I just have one question for the event.preventDefault() : Does it also prevent the scrolling with the mouse wheel if called on a DOMMouseScroll event ? I'm not sure if the preventDefault() function is there to prevent only action with the page (clicks or any interaction with the page elements) or also navigation actions (scrolling et autoscroll).
Adam Roben (:aroben)
Comment 11 2009-12-11 10:11:40 PST
(In reply to comment #10) > I just have one question for the event.preventDefault() : > Does it also prevent the scrolling with the mouse wheel if called on a > DOMMouseScroll event ? I don't think WebKit supports that event. > I'm not sure if the preventDefault() function is there to prevent only action > with the page (clicks or any interaction with the page elements) or also > navigation actions (scrolling et autoscroll). preventDefault() is supposed to prevent the event's "default action". See <http://www.w3.org/TR/2001/WD-DOM-Level-3-Events-20010823/events.html#Events-Event-preventDefault>
Adam Roben (:aroben)
Comment 12 2009-12-11 10:16:40 PST
Comment on attachment 44649 [details] [PATCH] Fix > + if (renderer) > + if (Frame* frame = renderer->document()->frame()) > + frame->eventHandler()->startPanScrolling(renderer); This should be enclosed in braces. r=me if you fix this and correct the links as mentioned above.
Brian Weinstein
Comment 13 2009-12-11 10:37:21 PST
(In reply to comment #10) > Thanks for adding me to the CC list. > > I just have one question for the event.preventDefault() : > Does it also prevent the scrolling with the mouse wheel if called on a > DOMMouseScroll event ? > I'm not sure if the preventDefault() function is there to prevent only action > with the page (clicks or any interaction with the page elements) or also > navigation actions (scrolling et autoscroll). Yes, the mouse wheel's scrolling is prevented if you call event.preventDefault on mousewheel (I think FF uses DOMMouseScroll, we use mousewheel).
Brian Weinstein
Comment 14 2009-12-11 11:03:05 PST
Landed in r52008.
keremonal
Comment 15 2010-02-18 00:22:08 PST
The status is RESOLVED FIXED. But it is still not fixed for Google Chrome 5.0.322.2 for Windows. I asked about the reason on http://code.google.com/p/chromium/issues/detail?id=17234. It is said that "Perhaps the patch didn't fix the issue? We don't cherry-pick the changes. Unless they are rolled out in WebKit, they land in Chromium pretty much within 24 hours." Does the bug still exist because of webkit or chrome?
Adam Roben (:aroben)
Comment 16 2010-02-18 06:45:51 PST
(In reply to comment #15) > The status is RESOLVED FIXED. But it is still not fixed for Google Chrome > 5.0.322.2 for Windows. > > I asked about the reason on > http://code.google.com/p/chromium/issues/detail?id=17234. It is said that > "Perhaps the patch didn't fix the issue? We don't cherry-pick the changes. > Unless they > are rolled out in WebKit, they land in Chromium pretty much within 24 hours." > > Does the bug still exist because of webkit or chrome? One good way to answer this question is to download Safari for Windows and try to reproduce the bug using a WebKit nightly build (from <http://nightly.webkit.org/>). If the bug reproduces in the nightly, it's almost certainly due to a bug in WebKit, not in Chrome or Safari.
Brenton
Comment 17 2010-09-30 14:35:35 PDT
r67261 stopped calling a.@onclick when a link was middle clicked. That patch made a mess of the way middle-mouse events are handled, and now middle-mouse up is spawning new tabs, even if the links were not pressed to begin with. That bug is here: https://bugs.webkit.org/show_bug.cgi?id=46733 In the comments, Peter Kasting is talking about rolling back Brian's fix that made auto-scroll preventable. I thought you guys would want to know.
Note You need to log in before you can comment on or make changes to this bug.