Bug 32613 – REGRESSION (r52008): Middle-clicking on a linked image starts a pan scroll, but should follow the link instead

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

To reproduce:

1. Go to http://webkit.org/
2. Middle-click on the "Download Nightly builds" image

A pan scroll starts. But the link should open in a new tab instead!

This seems to be a regression introduced in r52008, which was fixing bug 32303.

[Adam Roben (:aroben)]

<rdar://problem/7476743>

[Brian Weinstein]

Created attachment 45023 [details]
[PATCH] Fix

[WebKit Review Bot]

Attachment 45023 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/dom/Node.cpp:2835:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/dom/Node.cpp:2836:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/dom/Node.cpp:2837:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/dom/Node.cpp:2838:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/dom/Node.cpp:2839:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/dom/Node.cpp:2840:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/dom/Node.cpp:2841:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/dom/Node.cpp:2843:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/dom/Node.cpp:2844:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/dom/Node.cpp:2846:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/dom/Node.cpp:2847:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/dom/Node.cpp:2849:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/dom/Node.cpp:2850:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/dom/Node.cpp:2851:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/dom/Node.cpp:2852:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/dom/Node.cpp:2853:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 16

[Adam Roben (:aroben)]

Comment on attachment 45023 [details]
[PATCH] Fix

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!)

[Brian Weinstein]

(In reply to comment #4)
> (From update of attachment 45023 [details])
> This patch contains tabs.

Will fix, sorry about that, using a different machine.

> 
> > +        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;
> }
> 

That makes and seems cleaner. I was just wary about adding early returns in case someone decides to add more to that part of the function, but you said on IRC earlier that an early return would work, so I'll switch to using that.

> RenderObject* renderer = ...
> 
> I think it would be good to share this code with the hit-testing code.
> 

What do you mean?

> 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!)

I don't see any special casing in RenderLayer::hitTest for area and map elements, won't area and map elements have isLink = true? I can write a test for that though. Plus, that code is trying to figure out which element has the link, I just need to know if there is one, which makes me thing I don't need to special case area + map. I will write a test though.

Thanks for the review.

[Adam Roben (:aroben)]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 45023 [details] [details])
> > RenderObject* renderer = ...
> > 
> > I think it would be good to share this code with the hit-testing code.
> > 
> 
> What do you mean?

I meant that we should find a way to unify the code in RenderLayer::hitTest that finds the enclosing isLink element with your new code here in defaultEventHandler. The RenderLayer code I'm talking about is <http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderLayer.cpp?rev=52034#L2349>.

> > 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!)
> 
> I don't see any special casing in RenderLayer::hitTest for area and map
> elements, won't area and map elements have isLink = true? I can write a test
> for that though. Plus, that code is trying to figure out which element has the
> link, I just need to know if there is one, which makes me thing I don't need to
> special case area + map. I will write a test though.

A test sounds like the right way to go.

[Brian Weinstein]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (From update of attachment 45023 [details] [details] [details])
> > > RenderObject* renderer = ...
> > > 
> > > I think it would be good to share this code with the hit-testing code.
> > > 
> > 
> > What do you mean?
> 
> I meant that we should find a way to unify the code in RenderLayer::hitTest
> that finds the enclosing isLink element with your new code here in
> defaultEventHandler. The RenderLayer code I'm talking about is
> <http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderLayer.cpp?rev=52034#L2349>.
> 

I could add a function for Node.h - something like enclosingAnchorNode or isInsideAnchorNode, and call it in both places, but one call expects a boolean, where the other expects a Node, but if you think that's a good solution, I could do that.

> > > 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!)
> > 
> > I don't see any special casing in RenderLayer::hitTest for area and map
> > elements, won't area and map elements have isLink = true? I can write a test
> > for that though. Plus, that code is trying to figure out which element has the
> > link, I just need to know if there is one, which makes me thing I don't need to
> > special case area + map. I will write a test though.
> 
> A test sounds like the right way to go.

Will do.

[Adam Roben (:aroben)]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > (From update of attachment 45023 [details] [details] [details] [details])
> > > > RenderObject* renderer = ...
> > > > 
> > > > I think it would be good to share this code with the hit-testing code.
> > > > 
> > > 
> > > What do you mean?
> > 
> > I meant that we should find a way to unify the code in RenderLayer::hitTest
> > that finds the enclosing isLink element with your new code here in
> > defaultEventHandler. The RenderLayer code I'm talking about is
> > <http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderLayer.cpp?rev=52034#L2349>.
> > 
> 
> I could add a function for Node.h - something like enclosingAnchorNode or
> isInsideAnchorNode, and call it in both places, but one call expects a boolean,
> where the other expects a Node, but if you think that's a good solution, I
> could do that.

A function that returns a node will serve both purposes.

[Brian Weinstein]

Created attachment 45163 [details]
[PATCH] Fix with some refactoring + image map tests

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45163 [details] without any errors.

[Adam Roben (:aroben)]

Comment on attachment 45163 [details]
[PATCH] Fix with some refactoring + image map tests

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 52323)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,28 @@
> [phone]  Brian Weinstein  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Fixes <http://webkit.org/b/32613>.
> +        REGRESSION (r52008): Middle-clicking on a linked image starts a pan scroll,
> +        but should follow the link instead.
> +
> +        The node itself isn't the only possible node that can be a link (that was the
> +        original check), any of its ancestors could be links as well, we need to climb
> +        up the tree to see. Created a new function (enclosingAnchorNode), that finds a
> +        node's enclosing anchor element (if it exists), that we can share between RenderLayer
> +        and Node, and also added tests that test pan scrolling behavior in image maps.
> +
> +        Tests: platform/win/fast/events/panScroll-image-no-scroll.html
> +               platform/win/fast/events/panScroll-imageMap-href-no-scroll.html
> +               platform/win/fast/events/panScroll-imageMap-noHref-scroll.html
> +
> +        * dom/Node.cpp:
> +        (WebCore::Node::enclosingAnchorNode):
> +        (WebCore::Node::defaultEventHandler):
> +        * dom/Node.h:
> +        * rendering/RenderLayer.cpp:
> +        (WebCore::RenderLayer::hitTest):
> +
>  2009-12-18  Adam Roben  <[e-mail]>
>  
>          GTK build fix
> Index: WebCore/dom/Node.cpp
> ===================================================================
> --- WebCore/dom/Node.cpp	(revision 52231)
> +++ WebCore/dom/Node.cpp	(working copy)
> @@ -2288,6 +2288,19 @@ ContainerNode* Node::eventParentNode()
>      return static_cast<ContainerNode*>(parent);
>  }
>  
> +Node* Node::enclosingAnchorNode()

This name is a bit misleading in two ways:
1) The function can return "this", but its name implies it can only return an ancestor of "this"
2) It can return something other than an HTMLAnchorElement, which is what it names implies it will return.

I'd call it enclosingLinkNodeOrSelf or enclosingURLNodeOrSelf or something like that (enclosingLinkNode parallels isLink(), and enclosingURLNode (kind of) parallels HitTestResult::URLElement()). In either case, I think it needs a comment in the header explaining what it does. Maybe there's even a better name that would be so clear it wouldn't need a comment.

> @@ -2831,9 +2844,11 @@ void Node::defaultEventHandler(Event* ev
>  #if ENABLE(PAN_SCROLLING)
>      } else if (eventType == eventNames().mousedownEvent) {
>          MouseEvent* mouseEvent = static_cast<MouseEvent*>(event);
> -        if (mouseEvent->button() == MiddleButton && !this->isLink()) {
> -            RenderObject* renderer = this->renderer();
> +        if (mouseEvent->button() == MiddleButton) {
> +            if (this->enclosingAnchorNode())
> +                return;

No need for this-> here.

> +    // The node's enclosing anchor element (if it exists).
> +    Node* enclosingAnchorNode();

This comment is incorrect, since this function doesn't always return an HTMLAnchorElement (see, I told you the name was confusing!).

> @@ -2346,15 +2346,10 @@ bool RenderLayer::hitTest(const HitTestR
>          }
>      }
>  
> -    // Now determine if the result is inside an anchor; make sure an image map wins if
> -    // it already set URLElement and only use the innermost.
> +    // Now determine if the result is inside an anchor.
>      Node* node = result.innerNode();
> -    while (node) {
> -        // for imagemaps, URLElement is the associated area element not the image itself
> -        if (node->isLink() && !result.URLElement() && !node->hasTagName(imgTag))
> -            result.setURLElement(static_cast<Element*>(node));
> -        node = node->eventParentNode();
> -    }
> +    if (node)
> +        result.setURLElement(static_cast<Element*>(node->enclosingAnchorNode()));

This changes the behavior of this function if result.URLElement() was already set. Can that ever happen? I think it might be possible for it to happen via nodeAtPoint() being called from RenderBlock::hitTestContents and RenderLineBoxList::hitTest. (You should see if you can come up with a testcase that demonstrates what bug the change you made causes.)

> +        <div id="overflow" style="width:500px; height:150px; overflow:auto; border:2px solid red; padding:10px">
> +            <a href="#"><img src="" width="100px" height="100px"></img></a>
> +            <h1>Test for <a href="https://bugs.webkit.org/show_bug.cgi?id=32399">bug 32399</a> This tests that middle
> +            clicking on a link that is an image doesn't start to scroll.</h1>

I'd maybe say "middle clicking on an image within a link".

[Brian Weinstein]

(In reply to comment #11) 
> > +Node* Node::enclosingAnchorNode()
> 
> This name is a bit misleading in two ways:
> 1) The function can return "this", but its name implies it can only return an
> ancestor of "this"
> 2) It can return something other than an HTMLAnchorElement, which is what it
> names implies it will return.
> 
> I'd call it enclosingLinkNodeOrSelf or enclosingURLNodeOrSelf or something like
> that (enclosingLinkNode parallels isLink(), and enclosingURLNode (kind of)
> parallels HitTestResult::URLElement()). In either case, I think it needs a
> comment in the header explaining what it does. Maybe there's even a better name
> that would be so clear it wouldn't need a comment.

Changed function title and comment to:

// The node's enclosing node or self where isLink() is true - and we are not an image (needed for image map issues).
    Node* enclosingLinkNodeOrSelf();


> 
> > @@ -2831,9 +2844,11 @@ void Node::defaultEventHandler(Event* ev
> >  #if ENABLE(PAN_SCROLLING)
> >      } else if (eventType == eventNames().mousedownEvent) {
> >          MouseEvent* mouseEvent = static_cast<MouseEvent*>(event);
> > -        if (mouseEvent->button() == MiddleButton && !this->isLink()) {
> > -            RenderObject* renderer = this->renderer();
> > +        if (mouseEvent->button() == MiddleButton) {
> > +            if (this->enclosingAnchorNode())
> > +                return;
> 
> No need for this-> here.

Done.

> 
> > +    // The node's enclosing anchor element (if it exists).
> > +    Node* enclosingAnchorNode();
> 
> This comment is incorrect, since this function doesn't always return an
> HTMLAnchorElement (see, I told you the name was confusing!).
> 
> > @@ -2346,15 +2346,10 @@ bool RenderLayer::hitTest(const HitTestR
> >          }
> >      }
> >  
> > -    // Now determine if the result is inside an anchor; make sure an image map wins if
> > -    // it already set URLElement and only use the innermost.
> > +    // Now determine if the result is inside an anchor.
> >      Node* node = result.innerNode();
> > -    while (node) {
> > -        // for imagemaps, URLElement is the associated area element not the image itself
> > -        if (node->isLink() && !result.URLElement() && !node->hasTagName(imgTag))
> > -            result.setURLElement(static_cast<Element*>(node));
> > -        node = node->eventParentNode();
> > -    }
> > +    if (node)
> > +        result.setURLElement(static_cast<Element*>(node->enclosingAnchorNode()));
> 
> This changes the behavior of this function if result.URLElement() was already
> set. Can that ever happen? I think it might be possible for it to happen via
> nodeAtPoint() being called from RenderBlock::hitTestContents and
> RenderLineBoxList::hitTest. (You should see if you can come up with a testcase
> that demonstrates what bug the change you made causes.)

Added a check for !result.urlElement to make sure we don't overwrite something that's already there.

> 
> > +        <div id="overflow" style="width:500px; height:150px; overflow:auto; border:2px solid red; padding:10px">
> > +            <a href="#"><img src="" width="100px" height="100px"></img></a>
> > +            <h1>Test for <a href="https://bugs.webkit.org/show_bug.cgi?id=32399">bug 32399</a> This tests that middle
> > +            clicking on a link that is an image doesn't start to scroll.</h1>
> 
> I'd maybe say "middle clicking on an image within a link".

Done.

Thanks!

[Brian Weinstein]

Landed in r52340.