[Webkit-unassigned] [Bug 22382] Middle click fires onclick event

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=22382


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #57911|review?                     |review-
               Flag|                            |




--- Comment #25 from Darin Adler <darin at apple.com>  2010-06-12 18:21:19 PST ---
(From update of attachment 57911)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.