[webkit-reviews] review granted: [Bug 32613] REGRESSION (r52008): Middle-clicking on a linked image starts a pan scroll, but should follow the link instead : [Attachment 45163] [PATCH] Fix with some refactoring + image map tests

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted 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 45163: [PATCH] Fix with some refactoring + image map tests
https://bugs.webkit.org/attachment.cgi?id=45163&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 52323)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,28 @@
> +2009-12-18  Brian Weinstein	<bweinstein at apple.com>
> +
> +	   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  <aroben at apple.com>
>  
>	   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".