[webkit-reviews] review denied: [Bug 22382] Middle click fires onclick event : [Attachment 57911] Patch with middle dblclick blocking + shared code with right click event blocking

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Adam Barth <abarth at webkit.org>'s
request for review:
Bug 22382: Middle click fires onclick event
https://bugs.webkit.org/show_bug.cgi?id=22382

Attachment 57911: Patch with middle dblclick blocking + shared code with right
click event blocking
https://bugs.webkit.org/attachment.cgi?id=57911&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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?