|Summary:||Middle-mouse button not firing mousedown DOM event when autoscroll happens|
|Severity:||Normal||CC:||aroben, bweinstein, eric, keremonal, maxime.britto, webkit, webkit.review.bot|
|Version:||528+ (Nightly build)|
Description Brenton 2009-12-08 21:39:23 PST
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
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 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.