[webkit-reviews] review denied: [Bug 32613] REGRESSION (r52008): Middle-clicking on a linked image starts a pan scroll, but should follow the link instead : [Attachment 45023] [PATCH] Fix

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Brian Weinstein
<bweinstein at apple.com>'s request for review:
Bug 32613: REGRESSION (r52008): Middle-clicking on a linked image starts a pan
scroll, but should follow the link instead
https://bugs.webkit.org/show_bug.cgi?id=32613

Attachment 45023: [PATCH] Fix
https://bugs.webkit.org/attachment.cgi?id=45023&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
This patch contains tabs.

> +	   if (mouseEvent->button() == MiddleButton) {
> +			bool ancestorIsLink = false;
> +			Node* node = this;
> +			while (node && !ancestorIsLink) {
> +				if (node->isLink())
> +					ancestorIsLink = true;
> +				node = node->eventParentNode();
> +			}
> +
> +			if (!ancestorIsLink) {
> +				RenderObject* renderer = this->renderer();
> +
> +				while (renderer && (!renderer->isBox() ||
!toRenderBox(renderer)->canBeScrolledAndHasScrollableArea()))
> +					renderer = renderer->parent();
> +
> +				if (renderer) {
> +					if (Frame* frame = document()->frame())

> +					       
frame->eventHandler()->startPanScrolling(renderer);
> +				}
> +			}

I think this would all be clearer with an early return (and a for loop):

for (Node* node = this; node; node = node->eventParentNode()) {
    if (node->isLink())
	return;
}

RenderObject* renderer = ...

I think it would be good to share this code with the hit-testing code.

Your code doesn't consider <area> and <map> elements, but the hit-testing code
does. I think it's a bug that your code doesn't consider that. (You should
write a test for it!)