Disable touch highlight when the touched node has no hand-cursor.

Source/web/WebViewImpl.cpp

Index: Source/web/WebViewImpl.cpp
diff --git a/Source/web/WebViewImpl.cpp b/Source/web/WebViewImpl.cpp
index 92175fd6a8a5a120555b88b7b169f7b5fecd4e28..c4e7f1f92447f52eec6ca327ea01dca23a27dbd1 100644
--- a/Source/web/WebViewImpl.cpp
+++ b/Source/web/WebViewImpl.cpp
@@ -1169,14 +1169,14 @@ void WebViewImpl::computeScaleAndScrollForBlockRect(const WebPoint& hitPoint, co
     scroll = clampOffsetAtScale(scroll, scale);
 }
 
-static bool invokesHandCursor(Node* node, LocalFrame* frame)
+static bool showsHandCursor(Node* node, LocalFrame* frame, bool isOverLink)
 {
     if (!node || !node->renderer())
         return false;
 
     ECursor cursor = node->renderer()->style()->cursor();
     return cursor == CURSOR_POINTER
-        || (cursor == CURSOR_AUTO && frame->eventHandler().useHandCursor(node, node->isLink()));
+        || (cursor == CURSOR_AUTO && frame->eventHandler().useHandCursor(node, isOverLink));
 }
 
 Node* WebViewImpl::bestTapNode(const PlatformGestureEvent& tapEvent)
@@ -1186,28 +1186,30 @@ Node* WebViewImpl::bestTapNode(const PlatformGestureEvent& tapEvent)
     if (!m_page || !m_page->mainFrame())
         return 0;
 
-    Node* bestTouchNode = 0;
-
     // FIXME: Rely on earlier hit test instead of hit testing again.
-    GestureEventWithHitTestResults targetedEvent = m_page->deprecatedLocalMainFrame()->eventHandler().targetGestureEvent(tapEvent, true);
-    bestTouchNode = targetedEvent.hitTestResult().targetNode();
-
-    // We might hit something like an image map that has no renderer on it
-    // Walk up the tree until we have a node with an attached renderer
-    while (bestTouchNode && !bestTouchNode->renderer())
-        bestTouchNode = bestTouchNode->parentNode();
+    GestureEventWithHitTestResults targetedEvent =
+        m_page->deprecatedLocalMainFrame()->eventHandler().targetGestureEvent(tapEvent, true);
+    Node* bestTouchNode = targetedEvent.hitTestResult().targetNode();
+    bool isOverLink = targetedEvent.hitTestResult().isOverLink();
 
-    // Check if we're in the subtree of a node with a hand cursor
-    // this is the heuristic we use to determine if we show a highlight on tap
-    while (bestTouchNode && !invokesHandCursor(bestTouchNode, m_page->deprecatedLocalMainFrame()))
+    // Walk up the tree until we have an element node with an attached renderer
+    while (bestTouchNode && (!bestTouchNode->isElementNode() || !bestTouchNode->renderer()))
         bestTouchNode = bestTouchNode->parentNode();
 
-    if (!bestTouchNode)
+    // We show a highlight on tap only when the current node shows a hand cursor
+    if (!showsHandCursor(bestTouchNode, m_page->deprecatedLocalMainFrame(), isOverLink)) {

[Rick Byers, 2014/07/30 15:35:51]

nit: omit braces for single-line if
(http://www.chromium.org/blink/coding-style#TOC-Braces)

[mustaq, 2014/07/30 16:40:44]

Done.

         return 0;
+    }
 
-    // We should pick the largest enclosing node with hand cursor set.
-    while (bestTouchNode->parentNode() && invokesHandCursor(bestTouchNode->parentNode(), toLocalFrame(m_page->mainFrame())))
-        bestTouchNode = bestTouchNode->parentNode();
+    // We should pick the largest enclosing node with a hand cursor set.
+    Node* parentNode;
+    while ((parentNode = bestTouchNode->parentNode()) != 0) {

[Rick Byers, 2014/07/30 15:35:51]

style nit: don't explicitly compare pointers to 0, just rely on implicit
conversion to bool:
http://www.chromium.org/blink/coding-style#TOC-Null-false-and-0

[Rick Byers, 2014/07/30 15:35:52]

minor issue I should have mentioned earlier: please use
NodeRenderingTraversal::parentElement() instead of Node::parentNode() here and
above.  There's technically a pre-existing bug here with handling subtle cases
involving ShadowDOM.  Might as well just fix that at the same time (you
shouldn't notice any change in any of your tests).

[mustaq, 2014/07/30 16:40:44]

Missed this, sorry. I wanted to avoid double-parentheses to void the compiler
warning :-/

On 2014/07/30 15:35:51, Rick Byers wrote:

[mustaq, 2014/07/30 16:40:44]

Done fixing this method.

Not sure what other thing could be fixed in this CL---could you please clarify?
Did you mean replacing all "node->parentNode()" in this file?

[Rick Byers, 2014/07/30 17:50:28]

No, I just meant in this method (on this line, and in the "walk up the tree"
block above).

+        if (bestTouchNode->isLink() && !parentNode->isLink())

[Rick Byers, 2014/07/30 15:35:51]

Note the impact here on this case (which we should test explicitly):

<div id=mybutton style="cursor: pointer">
  Click me to go <a href=/>Home</a>
</div>

I originally argued that I expected tapping on "Home" to highlight all of
mybutton here, but I realize now that's debatable (it'll ultimately be up to
JavaScript code to decide if clicking here navigates to the link or doe
something else).  So I'm also OK with it highlighting just "Home" in this case.

[mustaq, 2014/07/30 16:40:44]

Acknowledged.

[mustaq, 2014/07/31 19:29:55]

Added a test for this.

On 2014/07/30 15:35:51, Rick Byers wrote:

+            break;
+        if (!showsHandCursor(parentNode, toLocalFrame(m_page->mainFrame()), parentNode->isLink()))

[Rick Byers, 2014/07/30 15:35:51]

What about a case like this:

<a>
  <span>My <b>link</b></span>
</a>

In that case tapping on the word "link" should cause a highlight around the
whole anchor element, but I believe your code will bail out (highlighting just
the "link") since the span node will have isLink=false.

To fix this I thought you were going to use your 'isOverLink' variable here,
instead of checking parentNode->isLink()?  You 'break' above ensures that
isOverLink always indicates whether bestTouchNode itself is inside of a link.

[mustaq, 2014/07/30 16:40:45]

Using isOverLink here instead of parentNode->isLink() causes one test failure
(gesture-tapHighlight-imagemap.html), not sure why. I think we have to handle
image maps separately, right?

On 2014/07/30 15:35:51, Rick Byers wrote:

[Rick Byers, 2014/07/30 17:50:28]

Oh, interesting.  Note that there's a TODO in that test that says "make link
highlighting work with image maps".  I believe bokan@ added this test when he
fixed a crash here, but it still wasn't doing the right thing.  Perhaps your
change makes it do something more reasonable now and so we should just update
the expected output?  You should probably play with a couple imagemap cases and
make sure the change seems reasonable to you/bokan.

+            break;
+        bestTouchNode = parentNode;
+    }
 
     return bestTouchNode;
 }