Bug 32303 - Middle-mouse button not firing mousedown DOM event when autoscroll happens
Summary: Middle-mouse button not firing mousedown DOM event when autoscroll happens
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows Vista
: P2 Normal
Assignee: Brian Weinstein
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-08 21:39 PST by Brenton
Modified: 2010-09-30 14:35 PDT (History)
7 users (show)

See Also:


Attachments
[PATCH] WIP Patch (3.29 KB, patch)
2009-12-10 15:30 PST, Brian Weinstein
no flags Details | Formatted Diff | Diff
[PATCH] Fix (11.45 KB, patch)
2009-12-10 16:40 PST, Brian Weinstein
aroben: review+
bweinstein: commit-queue-
Details | Formatted Diff | Diff
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

Note You need to log in before you can comment on or make changes to this bug.
Description Brenton 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.
Comment 1 Brenton 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.
Comment 2 Brenton 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
Comment 3 Brian Weinstein 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.
Comment 4 Brian Weinstein 2009-12-10 15:31:54 PST
Note, this needs a ChangeLog and layout tests, this was just for discussion about the Change.
Comment 5 Adam Roben (:aroben) 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.
Comment 6 Brian Weinstein 2009-12-10 16:40:36 PST
Created attachment 44649 [details]
[PATCH] Fix

With ChangeLog and test goodness.
Comment 7 WebKit Review Bot 2009-12-10 16:44:15 PST
style-queue ran check-webkit-style on attachment 44649 [details] without any errors.
Comment 8 Brenton 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.
Comment 9 Brenton 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.
Comment 10 Maxime Britto 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).
Comment 11 Adam Roben (:aroben) 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>
Comment 12 Adam Roben (:aroben) 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.
Comment 13 Brian Weinstein 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).
Comment 14 Brian Weinstein 2009-12-11 11:03:05 PST
Landed in r52008.
Comment 15 keremonal 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?
Comment 16 Adam Roben (:aroben) 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.
Comment 17 Brenton 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.