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.
This is the same problem I tried to explain here: https://bugs.webkit.org/show_bug.cgi?id=18161
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
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.
*** Bug 18161 has been marked as a duplicate of this bug. ***
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.
*** Bug 15173 has been marked as a duplicate of this bug. ***
Hum... This bug is a big pain to fix.
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>
Do we want the Gecko behavior or the Trident behavior? The Gecko behavior seems to have lower entropy.
(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.
Working on a new approach.
Ojan, can you test what happens for DOM-created events (as opposed to UI-created events)?
Created attachment 55445 [details] Sample fix, no tests
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>
I'm going to try to finish this now.
Created attachment 55914 [details] Patch
I'm not 100% confident in this patch. Any additional ideas about how to test this would be appreciated.
how do we install the patch?
(In reply to comment #18) > how do we install the patch? and will it be included in chromium release soon?
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 on attachment 55914 [details] Patch Clearing review flags for abarth's patch since there is a newer patch here.
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 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.
(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 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?
> 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.
(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.)
(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?
(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.
> 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.
hi, any new updates on this fix?
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.
I’ll take a crack at it.
(In reply to comment #33) > I’ll take a crack at it. Thanks Darin.
I’ve made some progress on this and will be uploading some patches soon.
(In reply to comment #35) > I’ve made some progress on this and will be uploading some patches soon. You're my hero.
Created attachment 66729 [details] Work in progress -- not ready to go yet and probably should be broken up and landed in pieces.
Created attachment 66793 [details] Patch
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. :)
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.
I'll take a shot today at splitting Darin's patch into a cleanup patch and a functional patch.
(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.
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.
(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.
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 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.
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?
(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.
OK, thanks; that's very helpful since I wasn't sure what the desired behavior was.
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.
(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.
(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.
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 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
(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.
(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.
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.
Created attachment 67225 [details] patch, cleanup portion, v1.1 This should address all Darin's comments. I elected to rename isLiveLinkImpl() as treatLinkAsLiveForEventType().
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 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.
Landed cleanup patch in r67246. Functional patch coming soon.
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.
Real-world tests show that the patch works as advertised, hooray!
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.
(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 :)
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 on attachment 67266 [details] patch, functional portion, v1 (Clearing flags)
Thanks, all! :)
http://trac.webkit.org/changeset/67246 broke WML builds. Sadly we don't have bots for WML :-( See bug 45628.
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.
(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.
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.
(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.
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.
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.
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.
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.
(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.
i am reading this from the fact that the click event has a button attribute.
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.
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.
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).
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.
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
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
Sorry... that URL should be https://chrome.google.com/webstore/detail/ndinbeikllkfjbmlfghokbkmcciflpek
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
*** Bug 70070 has been marked as a duplicate of this bug. ***
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.
(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.
<rdar://problem/25988904>
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.
*** Bug 166849 has been marked as a duplicate of this bug. ***
Would be nice if this was re-looked at. Support for auxclick is important for gaming with a mouse that has multiple buttons on macOS, iOS, iPadOS, and visionOS.