Bug 22382 - Middle click fires onclick event
Summary: Middle click fires onclick event
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Nobody
URL: http://www.guildportal.com/Guild.aspx...
Keywords: InRadar, WebExposed
: 15173 18161 166849 (view as bug list)
Depends on: 46733
Blocks: 45628
  Show dependency treegraph
 
Reported: 2008-11-20 06:34 PST by Mads Sig Ager
Modified: 2017-07-18 08:30 PDT (History)
27 users (show)

See Also:


Attachments
Sample fix, no tests (6.56 KB, patch)
2010-05-07 18:02 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (11.28 KB, patch)
2010-05-12 17:25 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch with middle dblclick blocking + shared code with right click event blocking (10.42 KB, patch)
2010-06-04 13:48 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
Work in progress -- not ready to go yet and probably should be broken up and landed in pieces. (22.12 KB, patch)
2010-09-07 09:33 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (29.86 KB, patch)
2010-09-07 17:07 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
patch, cleanup portion, v1 (21.11 KB, patch)
2010-09-09 17:37 PDT, Peter Kasting
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff
patch, cleanup portion, v1.1 (23.74 KB, patch)
2010-09-10 13:10 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
patch, functional portion, v1 (8.91 KB, patch)
2010-09-10 16:56 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
onmouseup target testcase (534 bytes, text/html)
2011-01-24 17:54 PST, Peter Kasting
no flags Details
opening links on events testcase (1.62 KB, text/html)
2011-01-24 18:00 PST, Peter Kasting
no flags Details
middle-mouse autoscroll testcase (486 bytes, text/html)
2011-01-24 18:02 PST, Peter Kasting
no flags Details
Safari extension to cancel unwanted click events (4.59 KB, application/x-safari-safariextz)
2012-04-24 23:24 PDT, Will Hirsch
no flags Details
Example of click event that Gecko fires on all clicks (481 bytes, text/html)
2012-04-25 10:58 PDT, Will Hirsch
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mads Sig Ager 2008-11-20 06:34:16 PST
Middle clicking fires an onclick event in Safari 3.1.2 on Windows and in the latests nightly build on Windows (r38492).

Middle clicking does not fire an onclick event in Firefox and IE.

Here is a simple test example:

<html>
<body>
<script>
function f() {
  alert('onclick');
}
</script>
<div onclick="f()">
<a href="http://webkit.org">middle click here</a>
... or middle click here.
</div>
</body>
</html>

Some websites have links inside other elements that have onclick event handlers that navigate.  If you middle click on one of those links, you will open the link in a new tab, but you will also fire the onclick event handler which will navigate the original window as well.  This is happening for instance on the URL that I have entered for this bug when middle clicking one of the sub forum links.
Comment 1 Franck 2009-07-27 15:51:33 PDT
This is the same problem I tried to explain here:
https://bugs.webkit.org/show_bug.cgi?id=18161
Comment 2 mkterra 2009-11-05 02:25:23 PST
Google Chrome is being affected by this bug too.  Downstream bug here:
http://code.google.com/p/chromium/issues/detail?id=5447

And a simple test page:
http://pimpbot.gifpaste.net/gid/test/chrome1.html
Comment 3 Eric Swanson 2009-12-12 14:52:00 PST
I can verify that this bug is occurring in the latest nightly build of Google Chromium (4.0.269.0 (34425)) for Windows 7 x64. Behaviour is just as described on mkterra's demo page.
Comment 4 Peter Kasting 2010-05-06 13:44:24 PDT
*** Bug 18161 has been marked as a duplicate of this bug. ***
Comment 5 Peter Kasting 2010-05-06 13:50:19 PDT
This was originally filed as bug 15173, which was RESOLVED FIXED with, as far as I can tell, absolutely no behavior change in terms of whether middle mouse fires onclick.  I'm not sure why Eric closed this.

This bug still trips all kinds of different sites.  It is a major compat bug.
Comment 6 Adam Barth 2010-05-06 15:05:54 PDT
*** Bug 15173 has been marked as a duplicate of this bug. ***
Comment 7 Adam Barth 2010-05-06 16:21:33 PDT
Hum...  This bug is a big pain to fix.
Comment 8 Ojan Vafai 2010-05-07 17:03:17 PDT
I did a quick bit of testing.

Gecko: Click only fires for left-click.
WebKit: Click fires for left/middle-click, but not right-click.
Trident: Click fires for left/middle-click, but not right-click and not if the middle-click was on a link.

Not firing click events for middle-clicks is probably the only sane direction to take things.

I tested with the following html:
<div id=foo>
<a href="http://webkit.org">middle click here</a>
... or middle click here.
</div>
<script>
document.getElementById('foo').onclick = function() {
  alert('onclick');
};
</script>
Comment 9 Adam Barth 2010-05-07 17:18:44 PDT
Do we want the Gecko behavior or the Trident behavior? The Gecko behavior seems to have lower entropy.
Comment 10 Ojan Vafai 2010-05-07 17:19:39 PDT
(In reply to comment #9)
> Do we want the Gecko behavior or the Trident behavior? The Gecko behavior seems
> to have lower entropy.

I prefer the gecko behavior. 

1. WebKit more often gets sent down gecko codepaths than trident ones. 
2. The Trident behavior is confusingly inconsistent. Avoiding it gives one less gotcha for web developers to learn.
Comment 11 Adam Barth 2010-05-07 17:27:39 PDT
Working on a new approach.
Comment 12 Adam Barth 2010-05-07 17:34:44 PDT
Ojan, can you test what happens for DOM-created events (as opposed to UI-created events)?
Comment 13 Adam Barth 2010-05-07 18:02:47 PDT
Created attachment 55445 [details]
Sample fix, no tests
Comment 14 Adam Barth 2010-05-07 18:06:28 PDT
Here's one approach to a fix.  The patch probably needs some cleanup and testing (of course).  The idea is that we move the previous check for "right click" down a layer and add the middle click case.  The benefit of being at a lower layer is that we can call the default handler directly, bypassing the dispatch to the DOM-registered events.

One side-effect of this change, however, is that the ban might go into effect for DOM-created events as well.  We need testing for that case as well.

I'm not sure I'll be able to drive this patch to completion in the near term, so someone else who's interested might want to pick it up.  Here's a nice test case to play with:

data:text/html,<button onclick="this.innerHTML='clicked'">click here</button>
Comment 15 Adam Barth 2010-05-12 15:48:14 PDT
I'm going to try to finish this now.
Comment 16 Adam Barth 2010-05-12 17:25:04 PDT
Created attachment 55914 [details]
Patch
Comment 17 Adam Barth 2010-05-12 17:33:45 PDT
I'm not 100% confident in this patch.  Any additional ideas about how to test this would be appreciated.
Comment 18 Blinkjt 2010-05-23 14:15:18 PDT
how do we install the patch?
Comment 19 Blinkjt 2010-05-23 14:15:42 PDT
(In reply to comment #18)
> how do we install the patch?

and will it be included in chromium release soon?
Comment 20 Nate Chapin 2010-06-04 13:48:47 PDT
Created attachment 57911 [details]
Patch with middle dblclick blocking + shared code with right click event blocking

Building on abarth's patch...

This change also blocks dblclick events (which Gecko doesn't send for right or middle clicks either) and removes the FIXMEs in abarth's previous patch.

Thanks for doing the hard parts, Adam! :)
Comment 21 Peter Kasting 2010-06-09 14:09:02 PDT
Comment on attachment 55914 [details]
Patch

Clearing review flags for abarth's patch since there is a newer patch here.
Comment 22 Adam Barth 2010-06-09 14:15:29 PDT
Comment on attachment 57911 [details]
Patch with middle dblclick blocking + shared code with right click event blocking

I had hoped someone else would review this change since I wrote an earlier version, but this LGTM.
Comment 23 Adam Barth 2010-06-09 14:30:14 PDT
Comment on attachment 57911 [details]
Patch with middle dblclick blocking + shared code with right click event blocking

Evan thinks we should get a more "outside" review on this patch.
Comment 24 Nate Chapin 2010-06-09 15:18:09 PDT
(In reply to comment #23)
> (From update of attachment 57911 [details])
> Evan thinks we should get a more "outside" review on this patch.

FWIW, I quasi-reviewed your patch before I started working on it, but I agree that an "outside" review is a good idea.
Comment 25 Darin Adler 2010-06-12 18:21:19 PDT
Comment on attachment 57911 [details]
Patch with middle dblclick blocking + shared code with right click event blocking

For our internal convenience we have decided to retain an internal-only click event for middle clicks. And we are now creating such an event for right clicks.

To me, it seems unfortunate to do so and we may want to consider another way of handling that in the future. It seems confusing to have a click event that is used internally that is otherwise the same as the click events that go to the DOM. Is there really a need to create these click events and block them at the event dispatch level?

> +    bool dispatchEventToDefaultEventHandler(PassRefPtr<Event> event);
> +    void callDefaultEventHandler(Event* event, const Vector<RefPtr<ContainerNode> >& ancestors);

I think both of these functions should take PassRefPtr or neither should. It’s not obvious to me whether these functions should take ownership of the event or not, but I suppose the safer design is to use PassRefPtr, since a default event handler could do a lot of work and there’s a chance this could result in the last reference to the event being released.

The argument name "event" is not needed in either case.

> +    RefPtr<EventTarget> protect = this;

The use of "protect" here is OK, I suppose, but it's not explained. I guess it's just copied and pasted from dispatchEvent. This is one of the most troubling idioms in our code, because there's no good way to prove such protection is needed nor to notice a case where it's needed and not present!

> +    // For clicks by the non-left mouse button, Gecko doesn't dispatch the
> +    // event to the DOM.  However, we still want to call the default event
> +    // handler so that the we can open links in new tabs, etc.

This comment is oblique. It says what Gecko does, and then segues into what we will do. The implication is that we plan to do the same thing Gecko does, but no reason is given. In my mind this raises others questions, like what W3C specifications say we should do and what other browsers do. Is this just a Gecko/WebKit alignment, or something we have noticed and are would like to have match among all web browsers?

> +    if (eventType == eventNames().clickEvent && button != LeftButton)
> +        dispatchEventToDefaultEventHandler(mouseEvent);
> +    else
> +        dispatchEvent(mouseEvent);

I think that having this down here at the Node level is unfortunate layering. I'd like to see the code that creates the click event do this instead and it seems that the existing code for right click was at that more appropriate higher level; I suppose it was inelegant to have that in two places in EventHandler, but that code could have been refactored instead of moving the code down here.

The code here makes it clear to me that dispatchEventToDefaultEventHandler should be named dispatchEventToDefaultEventHandlerOnly. One of the reasons for that is that the function is not used in the normal dispatch code path. The sole reason for its existence is to have a code path that simulates event dispatch but only sends the event to the default event handler. It's not the "dispatch to default event handler" part of the normal dispatch machinery.

I’m going to say review-. Why do we need click events at all for middle clicks? Can’t we fix this at the EventHandler level?
Comment 26 Adam Barth 2010-06-13 14:08:21 PDT
> For our internal convenience we have decided to retain an internal-only click event for middle clicks. And we are now creating such an event for right clicks.

Yes.  I didn't see another technical solution.  The events remain, they just aren't dispatched to the DOM because that confuses sites into doing dumb things and is different from Gecko (note that these two facts are likely related).

> To me, it seems unfortunate to do so and we may want to consider another way of handling that in the future. It seems confusing to have a click event that is used internally that is otherwise the same as the click events that go to the DOM. Is there really a need to create these click events and block them at the event dispatch level?

I don't see another way to fix this bug, but I'm open to suggestions.  The problem is that current WebKit clients have behavior we want to retain as the default event handler for these events.  If we don't create an event, I'm not sure how to call the proper default event handler.

Note that Eric tried to fix this bug a couple years ago at the higher level but couldn't quite come up with a working solution.

> I think that having this down here at the Node level is unfortunate layering. I'd like to see the code that creates the click event do this instead and it seems that the existing code for right click was at that more appropriate higher level; I suppose it was inelegant to have that in two places in EventHandler, but that code could have been refactored instead of moving the code down here.

I tried to extend the code for handling right-click to handle middle-click, but I couldn't get the right behavior.  I posted the patch for review a month ago, so I've forgotten the details by now.

> The code here makes it clear to me that dispatchEventToDefaultEventHandler should be named dispatchEventToDefaultEventHandlerOnly. One of the reasons for that is that the function is not used in the normal dispatch code path. The sole reason for its existence is to have a code path that simulates event dispatch but only sends the event to the default event handler. It's not the "dispatch to default event handler" part of the normal dispatch machinery.

Well, that and it removes a nasty goto that's simulating a function call return address.  :)

> I’m going to say review-. Why do we need click events at all for middle clicks? Can’t we fix this at the EventHandler level?

I'm open to suggestions for how to do that.  I'm certainly not an expert at the event code and could be missing how to get that done.  I bet we could do it by breaking all existing clients that handle middle-click via default events, but that sounds like a bad idea.
Comment 27 Peter Kasting 2010-06-13 16:04:59 PDT
(In reply to comment #25)
> > +    // For clicks by the non-left mouse button, Gecko doesn't dispatch the
> > +    // event to the DOM.  However, we still want to call the default event
> > +    // handler so that the we can open links in new tabs, etc.
> 
> This comment is oblique. It says what Gecko does, and then segues into what we will do. The implication is that we plan to do the same thing Gecko does, but no reason is given. In my mind this raises others questions, like what W3C specifications say we should do and what other browsers do. Is this just a Gecko/WebKit alignment, or something we have noticed and are would like to have match among all web browsers?

The W3C spec (that I found) was extremely vague.  Some of the Chromium folks looked at this a month or so ago.  Here's some background discussion from that thread if you're interested.

***


<Ojan Vafai> Firing onclick on middle-click is clearly the more consistent behavior given that it has a property of which mouse button was clicked.. Hixie's suggestion to capture any navigations caused by middle-click and open them in a new tab instead seems like the ideal behavior.  ...  It's just a lot tricker.

<Peter Kasting> Capturing navigations and opening in a new tab seems hard in principle -- how do you know that a navigation was "caused by" this click in the presence of things like setTimeout()?  Note that no one fires onclick for right-mouse-button. The argument I'd make is that "click" means "left-click", which lines up well with UI conventions that speak of "click to ____" and mean left-click. Middle- and right-click (and clicking the thumb buttons, etc.) are special kinds of clicks.  If I were redoing things from the start I'd make all mouse buttons fire click and put in the button, or else I'd make each have a different event. Given what we have today, however, I'd say that click should be left-click, and if we really want, we should add a different event (or two) for middle click and/or right-click.

<Ojan Vafai> Ugh. It's more gross than I realized. I agree, just not firing click on middle-click is probably the only sane result. 
Gecko: Click only fires for left-click. 
WebKit: Click fires for left/middle-click, but not right-click. 
Trident: Click fires for left/middle-click, but not right-click and not if the middle-click was on a link.

<Adam Barth> Joy. Do we want the Gecko behavior or the Trident behavior? The Gecko behavior seems to have lower entropy.

<Ojan Vafai> I prefer the gecko behavior. 
1. WebKit more often gets sent down gecko codepaths than trident ones. 
2. The Trident behavior is confusingly inconsistent. Avoiding it gives one less gotcha for web developers to learn.

<Evan Martin> I like the idea that "click" is something you do with your primary mouse button, whereas the other behaviors can be caught with onmousedown etc. (Really I prefer click to just be any mouse button, but that ship obviously sailed.)
Comment 28 Darin Adler 2010-06-13 19:46:28 PDT
(In reply to comment #26)
> > For our internal convenience we have decided to retain an internal-only click event for middle clicks. And we are now creating such an event for right clicks.
> 
> Yes.  I didn't see another technical solution.

The alternative solution would be to move the code that responds to to middle and right clicks into event handlers for events that do get sent instead of the click event. Once we've changed all the event handlers so they don’t depend on receiving a click event for a middle click, then we can change the code to not emit that event.

> The problem is that current WebKit clients have behavior we want to retain as the default event handler for these events.

This is the part I am not clear on. What are these clients? Are they you talking about something outside the defaultEventHandler functions in WebCore itself?

> I bet we could do it by breaking all existing clients that handle middle-click via default events, but that sounds like a bad idea.

It all depends on what you mean by clients. Maybe you could give me a specific example of a client outside WebCore?
Comment 29 Darin Adler 2010-06-13 19:47:54 PDT
(In reply to comment #27)
> (In reply to comment #25)
> > > +    // For clicks by the non-left mouse button, Gecko doesn't dispatch the
> > > +    // event to the DOM.  However, we still want to call the default event
> > > +    // handler so that the we can open links in new tabs, etc.
> > 
> > This comment is oblique. It says what Gecko does, and then segues into what we will do. The implication is that we plan to do the same thing Gecko does, but no reason is given. In my mind this raises others questions, like what W3C specifications say we should do and what other browsers do. Is this just a Gecko/WebKit alignment, or something we have noticed and are would like to have match among all web browsers?
> 
> The W3C spec (that I found) was extremely vague.  Some of the Chromium folks looked at this a month or so ago.  Here's some background discussion from that thread if you're interested.

I think we should just write a comment saying what we do.

"Only clicks on the left mouse button trigger click events."

Then if we want to comment on other browser engines, we could, but I would mention both Gecko and Trident behavior.
Comment 30 Adam Barth 2010-06-13 20:50:02 PDT
> It all depends on what you mean by clients. Maybe you could give me a specific example of a client outside WebCore?

I'll study the code in more detail to see where this code is located.
Comment 31 Blinkjt 2010-07-27 18:29:37 PDT
hi, any new updates on this fix?
Comment 32 Adam Barth 2010-08-30 12:13:48 PDT
I've become frustrated with this bug.  As far as I can tell, this bug is causing pain for a bunch of users and the above patch fixes the bug in all observable ways.  Multiple people have tried to fix it at a higher level and were unable to.  I think it's likely that it can be done.  We just don't know how.

If someone would be willing to take this over, I'm sure that would make many folks very happy.
Comment 33 Darin Adler 2010-08-30 15:04:45 PDT
I’ll take a crack at it.
Comment 34 Adam Barth 2010-08-30 15:09:12 PDT
(In reply to comment #33)
> I’ll take a crack at it.

Thanks Darin.
Comment 35 Darin Adler 2010-09-03 17:00:33 PDT
I’ve made some progress on this and will be uploading some patches soon.
Comment 36 Adam Barth 2010-09-03 17:03:58 PDT
(In reply to comment #35)
> I’ve made some progress on this and will be uploading some patches soon.

You're my hero.
Comment 37 Darin Adler 2010-09-07 09:33:07 PDT
Created attachment 66729 [details]
Work in progress -- not ready to go yet and probably should be broken up and landed in pieces.
Comment 38 Darin Adler 2010-09-07 17:07:52 PDT
Created attachment 66793 [details]
Patch
Comment 39 Adam Barth 2010-09-08 02:16:34 PDT
Comment on attachment 66793 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=66793&action=prettypatch

It's a bit difficult for me to see what's going on here.  There seems to be a bunch of cleanup mixed in with the functional change.  Chasing down a bunch of the diffs, they turn out to just be changing an if to use an early return, etc.  If someone else is interested in reviewing this change, then great.  Otherwise, I'd appreciate separating out the functional change, mostly because I'm not as familiar with this code as I am with other parts of WebCore.

Thanks for working on this bug.  :)
Comment 40 Darin Adler 2010-09-08 08:45:31 PDT
I’m in no rush to get this landed. I might have a chance to refactor it some day, but I have other things to work on next.

If someone else wants to pick up the patch and refactor it that’s OK with me too.

The total patch size is relatively small, under 30kB.
Comment 41 Peter Kasting 2010-09-08 10:44:17 PDT
I'll take a shot today at splitting Darin's patch into a cleanup patch and a functional patch.
Comment 42 Darin Adler 2010-09-08 13:55:02 PDT
(In reply to comment #41)
> I'll take a shot today at splitting Darin's patch into a cleanup patch and a functional patch.

Thanks, Peter. I appreciate it.
Comment 43 Peter Kasting 2010-09-08 15:30:18 PDT
Darin, I'm not sure this patch is precisely correct.

In HTMLAnchorElement::defaultEventHandler(), if this is a focused contenteditable link that is EditableLinkNeverLive, and the event is a keydown of the Enter key,  I believe the old code calls the defaultEventHandler(), while your patch will setDefaultHandled() + dispatchSimulatedClick().

Unless this change is harmless, or the old code is actively wrong, it seems like you need to check for !shouldFollowLink() (if we're using your new helper functions) and bail if so.
Comment 44 Darin Adler 2010-09-08 15:45:25 PDT
(In reply to comment #43)
> Darin, I'm not sure this patch is precisely correct.
> 
> In HTMLAnchorElement::defaultEventHandler(), if this is a focused contenteditable link that is EditableLinkNeverLive, and the event is a keydown of the Enter key,  I believe the old code calls the defaultEventHandler(), while your patch will setDefaultHandled() + dispatchSimulatedClick().
> 
> Unless this change is harmless, or the old code is actively wrong, it seems like you need to check for !shouldFollowLink() (if we're using your new helper functions) and bail if so.

Sounds good.
Comment 45 Darin Adler 2010-09-08 15:55:21 PDT
I’m pretty sure that Enter key change is harmless. If you try to reproduce it I think you’ll discover that case does not exist. But still, good catch.
Comment 46 Darin Adler 2010-09-08 15:56:09 PDT
Comment on attachment 66793 [details]
Patch

Clearing review flag since Peter and Adam are looking to land this in an incremental fashion, and Peter spotted an unwanted minor change in the logic too.
Comment 47 Peter Kasting 2010-09-08 18:33:26 PDT
Here's another weird case.

According to ap on IRC, it's possible to have e.g. "keydown" events that are not keyboard events, say if JS does: document.createEvent("CustomEvent").initEvent("keydown", false, false)

The various modified defaultEventHandler() functions in the patch no longer behave the same when handling such events.  For example, take the simplest of the three, WMLAElement::defaultEventHandler().  When given such an event with a focused link before, it would run what Darin has helpfully factored into a function called handleLinkClicked().  Now it simply calls WMLElement::defaultEventHandler(event);.

I have no idea whether this matters; I'm trying to break this patch apart without actually well-understanding the bits of the web platform it underlies :/.  Any tips on what I should do here?
Comment 48 Darin Adler 2010-09-09 11:16:39 PDT
(In reply to comment #47)
> According to ap on IRC, it's possible to have e.g. "keydown" events that are not keyboard events, say if JS does: document.createEvent("CustomEvent").initEvent("keydown", false, false)

Yes, that’s true.

> The various modified defaultEventHandler() functions in the patch no longer behave the same when handling such events.  For example, take the simplest of the three, WMLAElement::defaultEventHandler().  When given such an event with a focused link before, it would run what Darin has helpfully factored into a function called handleLinkClicked().  Now it simply calls WMLElement::defaultEventHandler(event);.

What you describe is a programming mistake in WMLAElement. It was unintentional on the part of those who originally wrote the code.

That’s pretty obvious. For keydown events, "Enter" simulates a click and others are ignored. But incorrectly-constructed keydown events all cause the link to be followed. That’s clearly accidental mistaken code. My rewrite fixes those cases, but they are so obscure I didn’t add test cases for them.

> I have no idea whether this matters; I'm trying to break this patch apart without actually well-understanding the bits of the web platform it underlies :/.  Any tips on what I should do here?

I think you’ll have to decide for yourself. The behavior of these functions with some forms of badly-constructor DOM events was sloppy and a bit random. I changed the behavior to be more logical for these obscure edge cases.

My change was intentional, if that’s what you’re asking.
Comment 49 Peter Kasting 2010-09-09 11:20:01 PDT
OK, thanks; that's very helpful since I wasn't sure what the desired behavior was.
Comment 50 Peter Kasting 2010-09-09 15:28:49 PDT
I'm sorry to keep bringing up minutiae.  I think I've got this stuff right but I want to double-check because I'm still learning about event handling.

In SVGAElement::defaultEventHandler(), there are two notable changes:

(1) The block that begins "if (url[0] == '#')" and does some default handling is no longer protected by "if (!event->defaultPrevented())".  I think this is a bug in the patch and that condition needs to be restored.

(2) Inside that block, before calling the superclass defaultEventHandler(), "event->setDefaultHandled()" is removed.  I assume this is safe because the superclass event handler is supposed to be guaranteed to set this before it returns.
Comment 51 Darin Adler 2010-09-09 15:43:30 PDT
(In reply to comment #50)
> I'm sorry to keep bringing up minutiae.  I think I've got this stuff right but I want to double-check because I'm still learning about event handling.
> 
> In SVGAElement::defaultEventHandler(), there are two notable changes:
> 
> (1) The block that begins "if (url[0] == '#')" and does some default handling is no longer protected by "if (!event->defaultPrevented())".  I think this is a bug in the patch and that condition needs to be restored.

Node::dispatchGenericEvent is responsible for checking defaultPrevented and does it before calling defaultEventHandler. There is no need to check defaultPrevented inside defaultEventHandler functions. Unless there is something highly unusual going on here, that check is just dead code. The check for defaultPrevented inside handleLinkClick is also unneeded for the same reason, but I did not remove it because I didn’t think it through. Adding the defaultPrevented check back in the '#' case would be OK, but it would be better to remove it since it’s not needed.

> (2) Inside that block, before calling the superclass defaultEventHandler(), "event->setDefaultHandled()" is removed.  I assume this is safe because the superclass event handler is supposed to be guaranteed to set this before it returns.

handleLinkClick calls setDefaultHandled, so that was refactored inside that function, rather than removed.

The code no longer calls base class's defaultEventHandler function. It’s typically not correct to call the base class’s defaultEventHandler function if you’ve already handled the event, and I checked the base class defaultEventHandler in this case and saw there was no benefit to doing so.
Comment 52 Peter Kasting 2010-09-09 15:51:10 PDT
(In reply to comment #51)
> > (2) Inside that block, before calling the superclass defaultEventHandler(), "event->setDefaultHandled()" is removed.

Oops, I misread the diff here.  setDefaultHandled() was preserved while calling the base class handler was removed.  That's what you discussed in your last paragraph, so this is fine.  Thanks again.
Comment 53 Peter Kasting 2010-09-09 17:37:49 PDT
Created attachment 67124 [details]
patch, cleanup portion, v1

This is my shot at a "cleanup" patch which doesn't make any changes to how the middle mouse button generates click events.  It does contain the other functional changes from Darin's patch except one or two places that seems questionable (mostly discussed above), which are generally only noticeable in weird edge cases.
Comment 54 Darin Adler 2010-09-09 17:46:06 PDT
Comment on attachment 67124 [details]
patch, cleanup portion, v1

Thanks for doing this. I really appreciate you picking up the ball on this.

>  void DeleteButton::defaultEventHandler(Event* event)
>  {
> -    // FIXME: Is it really import to check the type of the event?
> -    // Seems OK to respond to any event named click even if it does not have the correct type.
> -    if (event->isMouseEvent()) {
> -        if (event->type() == eventNames().clickEvent) {
> -            document()->frame()->editor()->deleteButtonController()->deleteTarget();
> -            event->setDefaultHandled();
> -            // FIXME: Shouldn't we return here instead of falling through?
> -        }
> +    if (event->type() == eventNames().clickEvent) {
> +        document()->frame()->editor()->deleteButtonController()->deleteTarget();
> +        event->setDefaultHandled();
> +    } else {
> +        HTMLImageElement::defaultEventHandler(event);
>      }
> -
> -    HTMLImageElement::defaultEventHandler(event);
>  }

In the WebKit project we use early return, not else, for cases like this.

A single line else does not get braces around it in the WebKit coding style.

> +HTMLAnchorElement::EventType HTMLAnchorElement::eventTypeFromEvent(Event* event)
> +{
> +    if (!event->isMouseEvent())
> +        return NonMouseEvent;
> +    return static_cast<MouseEvent*>(event)->shiftKey() ? MouseEventWithShiftKey : MouseEventWithoutShiftKey;
> +}

This seems unnecessarily complicated. Why do we need to introduce this enum. Can't we just pass in an event?

> +bool HTMLAnchorElement::isLiveLinkImpl(EventType eventType) const

I am really confused by the function name "isLiveLinkImpl". When I translate it to English "is this a live link impl" it remains unclear.

What is this for? Can we give it a sensible name?

> +    case EditableLinkLiveWhenNotFocused:
> +        return (eventType == MouseEventWithShiftKey) || ((eventType == MouseEventWithoutShiftKey) && (m_rootEditableElementForSelectionOnMouseDown != rootEditableElement()));

This has too many parentheses in it. In the WebKit project we normally don't include parentheses when mixing "==" and "!=" with logical operators.

> +    return (event->type() == eventNames().keydownEvent) && event->isKeyboardEvent() && (static_cast<KeyboardEvent*>(event)->keyIdentifier() == "Enter");

Same here.

> +    return event->isMouseEvent() && (static_cast<MouseEvent*>(event)->button() == MiddleButton);

Here too.

> +    return (event->type() == eventNames().clickEvent) && (!event->isMouseEvent() || (static_cast<MouseEvent*>(event)->button() != RightButton));

And here.

> +void handleLinkClick(Event* event, Document* document, const String& url, const String& target, bool hideReferrer)
> +{
> +    event->setDefaultHandled();
> +    Frame* frame = document->frame();
> +    if (frame)
> +        frame->loader()->urlSelected(document->completeURL(url), target, event, false, false, true, hideReferrer ? NoReferrer : SendReferrer);
>  }

In WebKit, we normally use early return rather than nesting things inside an if.

> +    static EventType eventTypeFromEvent(Event* event);
> +    bool isLiveLinkImpl(EventType eventType) const;

We omit argument names when they are clear from the argument type as in these cases.

review- mainly because of isLiveLinkImpl, but also because you made many style changes that do not match the usual WebKit style
Comment 55 Peter Kasting 2010-09-09 18:07:54 PDT
(In reply to comment #54)
> > +    if (event->type() == eventNames().clickEvent) {
> > +        document()->frame()->editor()->deleteButtonController()->deleteTarget();
> > +        event->setDefaultHandled();
> > +    } else {
> > +        HTMLImageElement::defaultEventHandler(event);
> >      }
> > -
> > -    HTMLImageElement::defaultEventHandler(event);
> >  }
> 
> In the WebKit project we use early return, not else, for cases like this.

I'm not sure what is distinguishing "cases like this" as early-return cases?  I tried writing it with early return (each direction), but this was shorter and seemed more readable.

> A single line else does not get braces around it in the WebKit coding style.

Oops.  Will fix.

> > +HTMLAnchorElement::EventType HTMLAnchorElement::eventTypeFromEvent(Event* event)
> > +{
> > +    if (!event->isMouseEvent())
> > +        return NonMouseEvent;
> > +    return static_cast<MouseEvent*>(event)->shiftKey() ? MouseEventWithShiftKey : MouseEventWithoutShiftKey;
> > +}
> 
> This seems unnecessarily complicated. Why do we need to introduce this enum. Can't we just pass in an event?

Sadly can't, because one of the callers of isLiveLinkImpl() doesn't have an event to work with.  We also can't pass in a bool "is the shift key down" (which was my original, simpler choice) because the shift-key-related policies should only allow navigation for mouse events, and the case that comes from a keyboard event couldn't duplicate the correct return values by passing either true or false.

> > +bool HTMLAnchorElement::isLiveLinkImpl(EventType eventType) const
> 
> I am really confused by the function name "isLiveLinkImpl". When I translate it to English "is this a live link impl" it remains unclear.
> 
> What is this for? Can we give it a sensible name?

This is the low-level implementation of most of the high-level function isLiveLink().  I thought it was a standard pattern that "FooImpl" in any identifier means "implementation of foo".  I could call the function isLiveLink(), but I didn't because the existing isLiveLink() wasn't a pure passthrough to this function.  I'm not at all wedded to the name, but I'm not sure what would be more clear.  I can do the name overloading if you prefer though.

> > +    case EditableLinkLiveWhenNotFocused:
> > +        return (eventType == MouseEventWithShiftKey) || ((eventType == MouseEventWithoutShiftKey) && (m_rootEditableElementForSelectionOnMouseDown != rootEditableElement()));
> 
> This has too many parentheses in it. In the WebKit project we normally don't include parentheses when mixing "==" and "!=" with logical operators.

I'm going to push back a little here.  There's nothing at all in the style guidelines about this, and I've seen code that does both.  I personally find parentheses more readable.  If we're going to ban them, that's fine, but it should be written down somewhere so that my personal style opinions are not in question (or anyone else's).

More importantly in this particular case, I find:
    a || b && c
...to be extremely confusing as a reader as I don't happen to remember && vs. || precedence.  It's also highly error-prone as a writer.

If you're allergic to parentheses, how about a compromise on:
    return eventType == MouseEventWithShiftKey || (eventType == MouseEventWithoutShiftKey && m_rootEditableElementForSelectionOnMouseDown != rootEditableElement());
...dropping all but one set of parentheses, the critical one that makes precedence clear.  Is that acceptable?

I can remove the other parentheses, but I really would prefer to do it in the context of an update to the formal style guide.

> > +void handleLinkClick(Event* event, Document* document, const String& url, const String& target, bool hideReferrer)
> > +{
> > +    event->setDefaultHandled();
> > +    Frame* frame = document->frame();
> > +    if (frame)
> > +        frame->loader()->urlSelected(document->completeURL(url), target, event, false, false, true, hideReferrer ? NoReferrer : SendReferrer);
> >  }
> 
> In WebKit, we normally use early return rather than nesting things inside an if.

I'm even more confused here than when you mentioned early returns before.  I'm not really sure what you're saying when you say "nesting".  It sounds like you're saying:

"We never write:
    if (foo)
        bar();"

...which can't be true.  And

if (!frame)
    return;
foo();

...is more code and doesn't save big blocks of indenting or anything.  I really am mystified how early-return does anything other than add an unnecessary statement here.  (And I'm normally a huge fan of early return!  I evangelize it on Chromium code reviews constantly.)

> > +    static EventType eventTypeFromEvent(Event* event);
> > +    bool isLiveLinkImpl(EventType eventType) const;
> 
> We omit argument names when they are clear from the argument type as in these cases.

Yep, will fix, thanks.
Comment 56 Darin Adler 2010-09-09 18:30:43 PDT
(In reply to comment #55)
> (In reply to comment #54)
> > > +    if (event->type() == eventNames().clickEvent) {
> > > +        document()->frame()->editor()->deleteButtonController()->deleteTarget();
> > > +        event->setDefaultHandled();
> > > +    } else {
> > > +        HTMLImageElement::defaultEventHandler(event);
> > >      }
> > > -
> > > -    HTMLImageElement::defaultEventHandler(event);
> > >  }
> > 
> > In the WebKit project we use early return, not else, for cases like this.
> 
> I'm not sure what is distinguishing "cases like this" as early-return cases?  I tried writing it with early return (each direction), but this was shorter and seemed more readable.

In a function like this, detecting one type of event and handling it is an early return case. Then the next event handled, then the next. Finally, a call through to the base class. That’s the pattern. Generally speaking the function flows through to the base class, except for a few exceptional cases, events that are detected and handled.

The general concept of early return is that there's a "normal case", and then "unusual cases" that are handled early. In the case of an event handler, each kind of event we choose to handle is one of these "unusual cases", which is a little backwards from some other situations where the unusual cases are more often error cases, but still fits the early return pattern well.

Please change this. I am pretty passionate about this pattern in these event handler functions.

> I thought it was a standard pattern that "FooImpl" in any identifier means "implementation of foo".

I don’t think “implementation of is live link” is clear either. The Impl suffix is barely tolerable on class names. We try to avoid it, but we do have some notable exceptions. When the WebKit project began from KHTML, every single class in the DOM had an Impl suffix! But in function names it’s no good, and I’d prefer we not start to proliferate uses of it.

> I could call the function isLiveLink(), but I didn't because the existing isLiveLink() wasn't a pure passthrough to this function.  I'm not at all wedded to the name, but I'm not sure what would be more clear.  I can do the name overloading if you prefer though.

We want these helper functions to make the code more readable. Consider a call site like this:

    if (isLiveLinkImpl(eventType))

How can an event type be a link? What is "Impl"?

The original function was named based on being a member function and answered the question "is this a currently live link". Its name was good, but not great.

Given what you’re doing in this new function, one name I’d suggest is isLinkLiveForEvent or treatLinkAsLiveForEvent, since it is for use when you already know something is a link answers the question of whether the link should be treated as live for a particular event. I’m still finding the enum inelegant, but I suppose I can live with it.

> I find:
>     a || b && c
> ...to be extremely confusing as a reader as I don't happen to remember && vs. || precedence.

Yes, those parentheses you should definitely keep, even though they were not in my original patch. I suppose I can’t blame you for noticing those were missing and then going on a parenthesis addition binge!

    if (a || (b && c))

There's even a GCC warning for this, and we are recommending these parentheses everywhere, and fixing them when the GCC warning turns them up.

> how about a compromise on:
>     return eventType == MouseEventWithShiftKey || (eventType == MouseEventWithoutShiftKey && m_rootEditableElementForSelectionOnMouseDown != rootEditableElement());
> ...dropping all but one set of parentheses, the critical one that makes precedence clear.  Is that acceptable?

Yes, that's what I was asking for, in fact! Sorry I wasn’t more clear.

> > > +void handleLinkClick(Event* event, Document* document, const String& url, const String& target, bool hideReferrer)
> > > +{
> > > +    event->setDefaultHandled();
> > > +    Frame* frame = document->frame();
> > > +    if (frame)
> > > +        frame->loader()->urlSelected(document->completeURL(url), target, event, false, false, true, hideReferrer ? NoReferrer : SendReferrer);
> > >  }
> > 
> > In WebKit, we normally use early return rather than nesting things inside an if.
> 
> I'm even more confused here than when you mentioned early returns before.  I'm not really sure what you're saying when you say "nesting".  It sounds like you're saying:
> 
> "We never write:
>     if (foo)
>         bar();"
> 
> ...which can't be true.  And
> 
> if (!frame)
>     return;
> foo();
> 
> ...is more code and doesn't save big blocks of indenting or anything.  I really am mystified how early-return does anything other than add an unnecessary statement here.  (And I'm normally a huge fan of early return!  I evangelize it on Chromium code reviews constantly.)

When the main flow of the function needs to use something, and that something happens to be null, that's a case where we want to do an early return rather than nesting the real work of the function inside an if.

The logic here is not, "if there's a frame we'd like to tell it the URL was selected".

It's "we need to tell the frame a URL was selected -- oops, there's no frame!"

This kind of exceptional case is almost always done with a return. In this case, our job is to handle a link click, and there's an unusual case where the frame is 0. We deal with that with an early return and then continue with the rest of the work. The fact that the rest of the work is (currently) one line is not particularly important. The point is that the main flow of the function doesn't flow into an if statement body just because there's a need for a null check.
Comment 57 Peter Kasting 2010-09-09 21:43:14 PDT
Thanks for the detailed explanations, Darin!  They really help.  I only had time to skim your message now, so if there's still anything unclear I'll ask tomorrow, but I bet this addresses my questions.
Comment 58 Peter Kasting 2010-09-10 13:10:36 PDT
Created attachment 67225 [details]
patch, cleanup portion, v1.1

This should address all Darin's comments.  I elected to rename isLiveLinkImpl() as treatLinkAsLiveForEventType().
Comment 59 Darin Adler 2010-09-10 14:53:23 PDT
Comment on attachment 67225 [details]
patch, cleanup portion, v1.1

> +    return isLink() ? treatLinkAsLiveForEventType(m_wasShiftKeyDownOnMouseDown ? MouseEventWithShiftKey : MouseEventWithoutShiftKey) : false;

I think that && would be clearer here than ? : false.

> +    static EventType eventTypeFromEvent(Event*);

I don’t think this function’s name needs the “FromEvent” suffix.

> +            String target(isMiddleMouseButtonEvent(event) ? "_blank" : this->target());

I think this would read more clearly with "=" than with construction-style syntax.

> +            // FIXME: It's not clear why setting target to "_self" is ever
> +            // helpful.

This comment would be better if it was one line instead of two.

r=me
Comment 60 Peter Kasting 2010-09-10 15:25:16 PDT
Comment on attachment 67225 [details]
patch, cleanup portion, v1.1

Clearing flags, I'll land this myself with most of Darin's followup comments also addressed.
Comment 61 Peter Kasting 2010-09-10 15:43:20 PDT
Landed cleanup patch in r67246.  Functional patch coming soon.
Comment 62 Peter Kasting 2010-09-10 16:56:18 PDT
Created attachment 67266 [details]
patch, functional portion, v1

This should do the heavy lifting.  I'm still building this patch to test that it does what it claims.
Comment 63 Peter Kasting 2010-09-10 17:26:41 PDT
Real-world tests show that the patch works as advertised, hooray!
Comment 64 Adam Barth 2010-09-10 17:30:18 PDT
Comment on attachment 67266 [details]
patch, functional portion, v1

View in context: https://bugs.webkit.org/attachment.cgi?id=67266&action=prettypatch

Looks reasonable.  I'd need to look at more of the surrounding code to give it a full review.  Thanks for working on this bug.

> WebCore/ChangeLog:1
> -2010-09-109  Peter Kasting  <pkasting@google.com>
> +2010-09-10  Peter Kasting  <pkasting@google.com>
This changelog diff is kind of wonky.
Comment 65 Peter Kasting 2010-09-10 17:31:47 PDT
(In reply to comment #64)
> > WebCore/ChangeLog:1
> > -2010-09-109  Peter Kasting  <pkasting@google.com>
> > +2010-09-10  Peter Kasting  <pkasting@google.com>
> This changelog diff is kind of wonky.

That's because I accidentally typo'd the cleanup patch, and committed it.  So this fixes the typo :)
Comment 66 Peter Kasting 2010-09-10 18:18:03 PDT
Landed myself in r67261, in case the commit queue was confused by the ChangeLog mucking, because I'm gone all next week and wouldn't be able to fix.
Comment 67 Peter Kasting 2010-09-10 18:18:24 PDT
Comment on attachment 67266 [details]
patch, functional portion, v1

(Clearing flags)
Comment 68 mkterra 2010-09-10 22:12:56 PDT
Thanks, all! :)
Comment 69 Dirk Schulze 2010-09-13 01:05:12 PDT
http://trac.webkit.org/changeset/67246 broke WML builds. Sadly we don't have bots for WML :-( See bug 45628.
Comment 70 Peter Grabs 2010-10-13 00:28:04 PDT
i get the impression that you fixed this bug by firing click events only on left clicks. is that correct? before writing any more i just wanted to make sure that i got it right, because i cant believe it.
Comment 71 Darin Adler 2010-10-13 09:52:05 PDT
(In reply to comment #70)
> i get the impression that you fixed this bug by firing click events only on left clicks.

Yes. To be consistent with the behavior of all other web browsers.
Comment 72 Peter Grabs 2010-10-15 00:48:18 PDT
how can developing new browsers possibly be about "being like the other browsers"? wouldn't this defeat the whole point? shouldn't it be about being better than the others, e.g. by more standard compliance?

in webpage development it's commonplace to test for all major browsers anyway, so deviating from the others is no particular problem, especially if the developer after finding one such problem determines that the others are at fault resulting in him liking the new product even more (see IE bashing).

for me, being standard compliant vs. being like the others is a no-brainer. you can always add additional functionality and convenience functions to help the developer, but never at the price of losing standard compliance.
Comment 73 Adam Barth 2010-10-15 00:55:45 PDT
(In reply to comment #72)
> how can developing new browsers possibly be about "being like the other browsers"? wouldn't this defeat the whole point? shouldn't it be about being better than the others, e.g. by more standard compliance?

Ideally, the goal is to align the behavior of every browser and the standards so that developers can write code once and have it work properly in every browser.
Comment 74 Peter Grabs 2010-10-15 01:16:14 PDT
exactly. and the way to achieve this is typically that everyone tries to approach the standard as good as they can, not giving in to the most stubborn/unresponsive vendor. 

if standard compliance really doesn't sound as attractive to you as it does to me, at least make sure that you don't lose functionality, i.e. provide a new event like "standardClick" which behaves like the standard click event.
Comment 75 Adam Barth 2010-10-15 01:29:50 PDT
Thanks for sharing your perspective.  Indeed, the question of when to match other browsers and when to create new "more standard" ways of doing things is a tricky one.  We try to balance these competing factors in the WebKit project, sometimes with more success than others.

In the case of this bug, we're trying to achieve better behavior on sites that apparently didn't test this case on WebKit because they're surprised when a click even fires for a middle click.  Some of that might be because WebKit has historically been used more on Mac, where middle click is a less common operation than on other platforms, such as Linux.
Comment 76 Peter Grabs 2010-10-15 02:27:04 PDT
so we have a non standard compliant script and a browser. the script doesnt work. 

...

so we of course adapt the browser to it!




if it is one versus the other, i firmly remain on the side of standard compliance (especially if as in this case the standard is more logical than the suggested deviation; were it the other way round, one could hope that the standard will become reality compliant). but i see your point too, you want things to be easy for old code. this makes me think of visual basic's "option explicit". using that the programmer can decide that he wants stricter rules applied to him and thus make better code, without having to touch the old one. might something like that be possible in javascript? tell the browser at the beginning of the script that you want maximum standard compliance, this way newly created stuff will have higher quality while the old stuff keeps working. one might continue phantasizing about more that a "default mode" and a "standard mode", e.g. IE or FF compatibility modes ...

but all this is complicated and in my opinion destructive since it encourages sticking with bad old code while enforcing a standard makes code versions converge.
Comment 77 Adam Barth 2010-10-15 02:35:11 PDT
Keep in mind that another option is to change the standards to say what browsers actually do.  In any case, this sort of high-level discussion isn't really appropriate for the bug tracker.

If you'd like to influence the direction of the project, one path is to become a contributor and write patches that improve the project in your view.  You can find more information about contributing code on this web page:

http://webkit.org/coding/contributing.html

Kind regards.
Comment 78 Aryeh Gregor 2010-10-15 08:32:37 PDT
(In reply to comment #72)
> how can developing new browsers possibly be about "being like the other browsers"? wouldn't this defeat the whole point? shouldn't it be about being better than the others, e.g. by more standard compliance?

There is no standard that currently specifies this behavior, so this is irrelevant.  Generally speaking, web standards don't specify UI at all if they can avoid it.  Should such a standard be created (it probably will be sometime), it would undoubtedly specify that click events fire only on left clicks, since that's now what all major browsers do.  Standards are normally written to match what browsers do unless there are strong reasons not to.

However, as Adam says, Bugzilla isn't really the right place for this kind of discussion.
Comment 79 Peter Grabs 2010-10-15 09:20:58 PDT
i am reading this from the fact that the click event has a button attribute.
Comment 80 Peter Kasting 2011-01-24 17:50:48 PST
r67261 was rolled back due to regressions covered by bug 46733.

On that bug there was some discussion of the ideal behavior here and some testcases posted.  I will attempt to summarize and paraphrase:

Me, after much discussion: "We need to change our behavior to:
* Links are not opened in response to DOM click events.  Instead, they're opened directly in response to UI actions such as clicks and keypresses.
* For left-clicks, we need to process the DOM click event first, so that if it is default-prevented, we do not open the link."

Darin Adler: "We need to either:
1) Put some hidden state on the "real" click event so that the defaultEventHandler function can tell what to do.
2) Put the code to open links at the call site that sends the event rather than in the defaultEventHandler function. But to prevent EventHandler from becoming a giant god class we still may want to involve some virtual functions on the element involved."

Me: "We can't use the existing fromUserGesture() function because that just tells us whether we're in the chain of functionality triggered by a user event, meaning a click on one element could call a handler function that generates a click on a different element, and we'd go ahead and navigate."

Darin: "We should start by adding LayoutTests that cover everything we care about here.  The tests should be written so that PASS is what we want to eventually happen, and we should land expected results that show the current failures (no use of skipped lists).  This does not require expertise at code changes -- it's entirely about embodying our research and plans in test form.  To test the difference between DOM-created events and user-created ones we will need to use eventSender."

Next I'll attach some testcases to this bug that I attached to the other bug, and summarize how we ought to behave on them.
Comment 81 Peter Kasting 2011-01-24 17:54:30 PST
Created attachment 80001 [details]
onmouseup target testcase

This testcase can be used to verify where onmouseup is targeted, for both left and middle clicks.  Regardless of where mousedown occurs, mouseup should trigger an alert with the element that mouseup occurred over.
Comment 82 Peter Kasting 2011-01-24 18:00:03 PST
Created attachment 80002 [details]
opening links on events testcase

This testcase can be used to see what should happen in response to artificially-created events.  Neither mousing over link 2, nor mousing over link 1 and then link 2, should open any pages.

Modify the original testcase to change the mouse button from "1" to "0", then test again, and you should get the same results.

Modify the original testcase to uncomment the preventDefault() call in displayEventName(), then left-clicking link 3 should open nothing, while middle-clicking should open one background tab displaying bing.com.

Modify the original testcase to uncomment the preventDefault() call in the link 3 click listener, then you should get the same results as just above (left-clicking link 3 should open nothing, while middle-clicking should open one background tab displaying bing.com).
Comment 83 Peter Kasting 2011-01-24 18:02:45 PST
Created attachment 80003 [details]
middle-mouse autoscroll testcase

This testcase can be used to check default prevention on autoscrolling.  Resize your window small enough to get scrollbars, then middle-click.  An autoscroll ("pan scroll") cursor should not appear.
Comment 84 Franck 2011-10-27 00:36:01 PDT
10 months later...
Any news on this correction ?
I have read all the technical and philosophical discussion on this bug and bug #46733, but this one is still annoying
Comment 85 Will Hirsch 2012-04-24 23:24:17 PDT
Created attachment 138742 [details]
Safari extension to cancel unwanted click events

Here is a simple browser extension which effectively implements the Gecko behaviour, to fix those annoying sites that assume it while this is still being resolved.

A Chrome version is available at https://chrome.google.com/webstore/developer/edit/ndinbeikllkfjbmlfghokbkmcciflpek
Comment 86 Will Hirsch 2012-04-24 23:26:02 PDT
Sorry... that URL should be https://chrome.google.com/webstore/detail/ndinbeikllkfjbmlfghokbkmcciflpek
Comment 87 Peter Grabs 2012-04-24 23:59:10 PDT
there has been an extension for that for a long time:

https://chrome.google.com/webstore/detail/ikbkhpkapkmhaoiabhlkmicpeakhhpip

there is even a version as minimalist as yours:

https://chrome.google.com/webstore/detail/jnpgomjchhllpeehnmjfcfoceboliing
Comment 88 Will Hirsch 2012-04-25 02:29:17 PDT
*** Bug 70070 has been marked as a duplicate of this bug. ***
Comment 89 Will Hirsch 2012-04-25 10:58:52 PDT
Created attachment 138843 [details]
Example of click event that Gecko fires on all clicks

Just to complicate things, it seems that Gecko actually fires click for all click types on listeners attached to document or window. See the attached case.
Comment 90 Antonio Gomes 2016-04-28 13:31:30 PDT
(In reply to comment #89)
> Created attachment 138843 [details]
> Example of click event that Gecko fires on all clicks
> 
> Just to complicate things, it seems that Gecko actually fires click for all
> click types on listeners attached to document or window. See the attached
> case.

Please note that Blink has moved on here and now matches the spec: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/pYAh8bBl5Yc

On this same thread, one can see that Edge has filed an issue to follow up. Firefox has already been matching the spec for some time.
Comment 91 Radar WebKit Bug Importer 2016-04-28 14:00:46 PDT
<rdar://problem/25988904>
Comment 92 Rick Byers 2017-01-19 08:07:11 PST
Specifically we successfully changed blink by adding a new 'auxclick' event that's fired in special cases like middle click (and right click when the contextmenu is suppressed).  https://wicg.github.io/auxclick/

I'd encourage WebKit to do the same thing.  We've seen very little compat problems as a result.
Comment 93 Alexey Proskuryakov 2017-01-19 17:36:01 PST
*** Bug 166849 has been marked as a duplicate of this bug. ***