Bug 15173 - Middle click should not fire onclick event
Summary: Middle click should not fire onclick event
Status: RESOLVED DUPLICATE of bug 22382
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 523.x (Safari 3)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL: http://bugs.blaut.biz/webkit/onclick-...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2007-09-10 13:45 PDT by Robert Blaut
Modified: 2010-05-06 15:05 PDT (History)
4 users (show)

See Also:


Attachments
A minimal test case (474 bytes, text/html)
2007-09-10 13:46 PDT, Robert Blaut
no flags Details
Only dispatch clicks for left-button clicks (2.16 KB, patch)
2008-06-26 11:40 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
First attempt at adding eventSender.mouseDown(1) support (10.11 KB, patch)
2008-06-27 13:19 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Add multi-button mouseevent support to DRT and test case (12.64 KB, patch)
2008-06-27 19:37 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Add multi-button mouseevent support to DRT and test case (15.60 KB, patch)
2008-06-27 21:04 PDT, Eric Seidel (no email)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Blaut 2007-09-10 13:45:45 PDT
I've noticed that Safari is the only browser I've tested which fire onclick event after using middle click (wheel button) to load a link on a new tab. This is very frustrating behaviour, not visible in other popular browsers on Windows (Internet Explorer 7, Firefox 2 and Opera 9).

Steps to reproduce:

1. Go to a page: http://bugs.blaut.biz/webkit/onclick-issue.htm
2. Middle click on the only link on the page.

Expected result:

Safari should open a new tab and load a page http://webkit.org. The tab with the clicked link should not load webkit.org site.

Current result:

Safari loads on a new tab a page http://webkit.org and also load a page webkit.org on the tab with the clicked link.
Comment 1 Robert Blaut 2007-09-10 13:46:59 PDT
Created attachment 16247 [details]
A minimal test case
Comment 2 Mark Rowe (bdash) 2007-09-10 20:09:53 PDT
<rdar://problem/5473092>
Comment 3 Robert Blaut 2008-02-18 02:06:40 PST
(In reply to comment #2)
> <rdar://problem/5473092>
> 

I assume the bug is confirmed.
Comment 4 Tony Chang 2008-04-17 12:46:32 PDT
It looks like in Firefox, click event handlers only fire for left click.

In IE7, click events fire for left click or middle click.  However, anchor tags seem to be a special case where if there's a href, the href is opened in a new tab.  Otherwise, the click event handler runs.

As far as I can tell, it seems like the click event should fire for middle clicks, but the spec seems ambiguous:
http://www.w3.org/TR/DOM-Level-3-Events/events.html#event-click

Here's a good test case:
http://www.w3.org/WAI/UA/TS/html401/cp0102/0102-ONCLICK-MULTIPLE-EVENTS.html

Also, this is what causes middle clicking a link in Gmail to open in a new window rather than as a background tab in Safari.
Comment 5 Eric Seidel (no email) 2008-06-24 12:43:18 PDT
The "fix" to this would be a one line change in:
EventHandler.cpp:
bool EventHandler::handleMouseReleaseEvent(const PlatformMouseEvent& mouseEvent)

There is already code in that function to avoid ever dispatching click events for right-clicks, we'd just have to change it to not dispatch clicks for middle clicks as well (possibly only to dispatch click events for left-clicks in general).
Comment 6 Peter Kasting 2008-06-24 12:48:43 PDT
"A pointing device button" on the spec seems unambiguous that any button should fire clicks.  It's not clear to me whether to claim the other browsers are wrong (certainly not firing clicks on middle/right clicks _seems_ bizarre, and makes life harder on JS authors who want to use those buttons to trigger actions), or to claim that the spec should change.
Comment 7 Ian 'Hixie' Hickson 2008-06-24 17:44:30 PDT
It seems like the better behaviour would be to fire the click event normally, but then catch any attempt at navigating the page, and instead make another browsing context navigate, or something like that. Then it would work with all kinds of weird scripts, too. That's more work, though.
Comment 8 Eric Seidel (no email) 2008-06-26 11:40:46 PDT
Created attachment 21957 [details]
Only dispatch clicks for left-button clicks

 WebCore/ChangeLog             |   13 +++++++++++++
 WebCore/page/EventHandler.cpp |    7 ++++---
 2 files changed, 17 insertions(+), 3 deletions(-)
Comment 9 Eric Seidel (no email) 2008-06-26 11:42:44 PDT
I see there are differing opinions above as to how this should be solved.  Regardless, I wrote a patch which solves this by making us not send these click events like other browsers.

I also noticed in my testing that we do not send a mouse-release event on right-clicks.  FF does (right as it's displaying the context menu).
Comment 10 Darin Adler 2008-06-27 09:10:58 PDT
Comment on attachment 21957 [details]
Only dispatch clicks for left-button clicks

Lets add something to eventSender so we can do an automated tests. Should be simple.
Comment 11 Ian 'Hixie' Hickson 2008-06-27 12:56:38 PDT
I think Eric's patch is fine for now, my earlier suggestion is what I think we should do on the long term though. It would work for far more cases, and would be really cool.
Comment 12 Eric Seidel (no email) 2008-06-27 13:19:35 PDT
Created attachment 21976 [details]
First attempt at adding eventSender.mouseDown(1) support

 .../DumpRenderTree.xcodeproj/project.pbxproj       |    2 +-
 .../DumpRenderTree/mac/EventSendingController.h    |    2 +-
 .../DumpRenderTree/mac/EventSendingController.mm   |   95 ++++++++++++++++----
 3 files changed, 79 insertions(+), 20 deletions(-)
Comment 13 Eric Seidel (no email) 2008-06-27 19:37:23 PDT
Created attachment 21977 [details]
Add multi-button mouseevent support to DRT and test case

 .../fast/events/mouse-click-events-expected.txt    |   32 +++++++
 LayoutTests/fast/events/mouse-click-events.html    |   13 +++
 .../fast/events/resources/mouse-click-events.js    |   45 ++++++++++
 .../DumpRenderTree.xcodeproj/project.pbxproj       |    2 +-
 .../DumpRenderTree/mac/EventSendingController.h    |    2 +-
 .../DumpRenderTree/mac/EventSendingController.mm   |   88 ++++++++++++++++----
 6 files changed, 165 insertions(+), 17 deletions(-)
Comment 14 Eric Seidel (no email) 2008-06-27 21:04:06 PDT
Created attachment 21980 [details]
Add multi-button mouseevent support to DRT and test case

 LayoutTests/ChangeLog                              |   12 ++
 .../fast/events/mouse-click-events-expected.txt    |   32 ++++++
 LayoutTests/fast/events/mouse-click-events.html    |   13 +++
 .../fast/events/resources/mouse-click-events.js    |   45 ++++++++
 WebKitTools/ChangeLog                              |   22 ++++
 .../DumpRenderTree.xcodeproj/project.pbxproj       |    2 +-
 .../DumpRenderTree/mac/EventSendingController.h    |    2 +-
 .../DumpRenderTree/mac/EventSendingController.mm   |  111 ++++++++++++++++----
 8 files changed, 214 insertions(+), 25 deletions(-)
Comment 15 Darin Adler 2008-06-27 21:15:30 PDT
Comment on attachment 21957 [details]
Only dispatch clicks for left-button clicks

r=me, assuming you land the test too
Comment 16 Darin Adler 2008-06-27 21:15:32 PDT
Comment on attachment 21980 [details]
Add multi-button mouseevent support to DRT and test case

Looks good, but we need to add this test to the skipped list for the other platforms, since the new support in DumpRenderTree is Mac-only.

r=me
Comment 17 Eric Seidel (no email) 2008-06-27 21:50:02 PDT
Actually.  My patch just breaks middle-click-to-open-links entirely.  I guess the "middle click to open links" behavior depends on there being a click event.   Sigh.
Comment 18 Eric Seidel (no email) 2008-06-27 21:55:20 PDT
(In reply to comment #17)
> Actually.  My patch just breaks middle-click-to-open-links entirely.  I guess
> the "middle click to open links" behavior depends on there being a click event.
>   Sigh.

Yeah, of course it does.  We do navigation based on HTMLAnchorElement::defaultEventHandler.  I'm not sure why I ever thought this would work.  I guess the test case is still useful, but there is no easy way for us to replicate FF/IE behavior here.  I think we should scratch this patch and implement a better "window.open opens in new tab if called from a middle-click" behavior.  However I bet that's a Safari change more than a WebKit change.
Comment 19 Eric Seidel (no email) 2008-06-27 21:55:54 PDT
Comment on attachment 21957 [details]
Only dispatch clicks for left-button clicks

Removing Review flag.  This patch breaks more than it helps.  I'll still land the test case patch though.
Comment 20 Eric Seidel (no email) 2008-06-29 12:27:36 PDT
Ok.  I landed the test case and DRT enhancements as part of:
http://trac.webkit.org/changeset/34871

We've decided to continue to send these events but instead fix window.open to open in a new tab when the result of a command-click or middle-click.  The rest needs to be done in Safari (And is thus tracked internally by Apple).

Closing.
Comment 21 Eric Seidel (no email) 2008-07-14 14:06:54 PDT
So unfortunately Gmail still isn't fixed (middle clicking on links, still opens in a new window).

Part of that is going to be a Safari fix, but part of it is going to need to be a WebKit fix.  I don't believe this could be fixed in Safari TOT, since WebKit TOT isn't passing any event context information to the WebViewLoadDelegate as part of the open new url callback.  the NavigationAction does not have an event() set on it when you middle-click a gmail link, so there is no way this information could be being passed up to Safari.

I don't think this is the right bug to track the brokeness of gmail-middle-clicking.  But I was asked by some avid Safari users internally to finally see if I couldn't get to the bottom of this Safari + Gmail bug.  As far as I can tell, there are still fixes which need to happen in WebKit (and possibly Safari as well).

Perhaps someone on the Apple side of things can give me some guidance as to any work you all have done to fix the "Safari + middle-click in Gmail opens a new window instead of a new-tab behind the current" bug.
Comment 22 Adam Barth 2010-05-06 15:05:54 PDT

*** This bug has been marked as a duplicate of bug 22382 ***