[webkit-reviews] review granted: [Bug 46733] REGRESSION (r67261): Middle-mouse-release opens links regardless of "press" location : [Attachment 79778] Roll back r67261 (needs real review), v2

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Peter Kasting <pkasting at google.com>'s
request for review:
Bug 46733: REGRESSION (r67261): Middle-mouse-release opens links regardless of
"press" location
https://bugs.webkit.org/show_bug.cgi?id=46733

Attachment 79778: Roll back r67261 (needs real review), v2
https://bugs.webkit.org/attachment.cgi?id=79778&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=79778&action=review

> Source/WebCore/html/HTMLInputElement.cpp:945
> -    if (event->type() != eventNames().clickEvent)
> +    if (!event->isMouseEvent() || event->type() != eventNames().clickEvent
|| static_cast<MouseEvent*>(event)->button() != LeftButton)
>	   return 0;

We could put the re-added code in a separate return statement. That way it can
more easily be removed when we redo this change later.

I think it’s unnecessarily confusing to have the isMouseEvent check separated
from the code that casts the event to a MouseEvent. Putting the new check into
a separate if statement would fix that as well.

If we thought the code was going to be like this longer term, we’d want to
either pass the event in to the input type and let it judge if it wants to
handle the click event or rename willDispatchClick to willDispatchLeftClick.
But I supposed for now it’s OK to leave the button check here.

> Source/WebCore/html/HTMLInputElement.cpp:954
> -    if (event->type() != eventNames().clickEvent)
> +    if (!event->isMouseEvent() || event->type() != eventNames().clickEvent
|| static_cast<MouseEvent*>(event)->button() != LeftButton)
>	   return;

This change isn’t needed. In fact, the if statement that checks the event type
is unnecessary as well. We’re guaranteed that state will be 0 unless the
preDispatchEventHandler returned something, so we don’t need to redo the checks
it did.

> Source/WebCore/html/HTMLInputElement.cpp:963
> -    if (evt->isMouseEvent() && evt->type() == eventNames().clickEvent) {
> +    if (evt->isMouseEvent() && evt->type() == eventNames().clickEvent &&
static_cast<MouseEvent*>(evt)->button() == LeftButton) {
>	   m_inputType->handleClickEvent(static_cast<MouseEvent*>(evt));

If we thought the code was going to be like this longer term, we’d want to
either let RadioButtonType::handleClickEvent do the checking for the left
button or rename the InputType function to handleLeftClickEvent. But I suppose
for now it’s OK to have this match HTMLInputElement::preDispatchEventHandler;
both have the same issue.

> Source/WebCore/page/EventHandler.cpp:1428
> -    bool swallowClickEvent = mouseEvent.button() == LeftButton &&
mev.targetNode() == m_clickNode && dispatchMouseEvent(eventNames().clickEvent,
mev.targetNode(), true, m_clickCount, mouseEvent, true);
> +    bool swallowClickEvent = false;
> +    // Don't ever dispatch click events for right clicks
> +    if (mouseEvent.button() != RightButton && mev.targetNode() ==
m_clickNode)
> +	   swallowClickEvent = dispatchMouseEvent(eventNames().clickEvent,
mev.targetNode(), true, m_clickCount, mouseEvent, true);

I don’t think we should add back the comment. The code says != right button; a
comment that says the same thing just creates confusion about what the rest of
what the code does and why the rest of the code is there.

I don’t think we need to roll back the coding style change we made in the
original patch. We can just change the condition back from == LeftButton to !=
RightButton. That was the substantive change. The rest was coding style only
and there is no good reason to keep changing it back and forth.

> Source/WebCore/page/EventHandler.cpp:1435
> -
> -    bool swallowMouseReleaseEvent = !swallowMouseUpEvent &&
handleMouseReleaseEvent(mev);
> +	       
> +    bool swallowMouseReleaseEvent = false;
> +    if (!swallowMouseUpEvent)
> +	   swallowMouseReleaseEvent = handleMouseReleaseEvent(mev);

There’s no need to roll this out. It was a coding style change that has no
effect on behavior.

> Source/WebCore/page/EventHandler.cpp:1624
> -    bool swallowClickEvent = m_clickCount > 0 && mouseEvent.button() ==
LeftButton && mev.targetNode() == m_clickNode &&
dispatchMouseEvent(eventNames().clickEvent, mev.targetNode(), true,
m_clickCount, mouseEvent, true);
> +    // Don't ever dispatch click events for right clicks
> +    bool swallowClickEvent = false;
> +    if (m_clickCount > 0 && mouseEvent.button() != RightButton &&
mev.targetNode() == m_clickNode)
> +	   swallowClickEvent = dispatchMouseEvent(eventNames().clickEvent,
mev.targetNode(), true, m_clickCount, mouseEvent, true);

As above, I suggest rolling out the substantive change, and not the style
change. And, as above, I suggest leaving the comment out.