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 662d48622052856ccd32e60a860c8120ce5359f1..d66b8f5d2cf5540d5018ecd0b7a23ddfb484980f 100644
--- a/Source/web/WebViewImpl.cpp
+++ b/Source/web/WebViewImpl.cpp
@@ -1168,14 +1168,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)
@@ -1185,29 +1185,54 @@ 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 tapIsOverLink = 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()))
-        bestTouchNode = bestTouchNode->parentNode();
+    // Walk up the tree until we have an element node with an attached renderer
+    while (bestTouchNode && (!bestTouchNode->isElementNode() || !bestTouchNode->renderer()))
+        bestTouchNode = NodeRenderingTraversal::parent(bestTouchNode);
 
-    if (!bestTouchNode)
+    // We show a highlight on tap only when the current node shows a hand cursor
+    if (!showsHandCursor(bestTouchNode, m_page->deprecatedLocalMainFrame(), tapIsOverLink))
         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();
+    // Find the largest enclosing node with a hand cursor set
+    Node* largestHandCursorNode = bestTouchNode;
+    Node* currentNode;
+    bool currentIsOverLink = tapIsOverLink;
+    while ((currentNode = NodeRenderingTraversal::parent(largestHandCursorNode))) {

[Rick Byers, 2014/07/31 20:15:10]

d'oh - looks like my suggestion to remove the '!= 0' caused a compiler error on
Windows, sorry!  I see both of the following styles elsewhere in the code:

while (Node* currentNode = ...) {
  ...
}

and:

Node* currentNode = ...;
while(currentNode) {
  currentNode = ...;
}

[mustaq, 2014/07/31 21:04:48]

Yeah, the msvc failures plus the style requirement forced me to add a bit of
redundancy in the code (which is IMO worse than comparing with null).

+        if (largestHandCursorNode->isLink() && !currentNode->isLink())
+            currentIsOverLink = false;
+
+        if (!showsHandCursor(currentNode, toLocalFrame(m_page->mainFrame()), currentIsOverLink))
+            break;

[Rick Byers, 2014/07/31 20:15:10]

nit: rather than break and test below, you can simplify this by returning
immediately (and then no need for the return statement below).

[mustaq, 2014/07/31 21:04:48]

Done, thanks.

+
+        largestHandCursorNode = currentNode;
+    }
+
+    if (currentNode)
+        return largestHandCursorNode;
+
+    // Otherwise, bestTouchNode is an image map w/o an enclosing anchor tag.

[Rick Byers, 2014/07/31 20:15:10]

Ah, interesting.  Doing another pass like this seems unfortunate but fine - it's
rare we'll need it and I can see we won't necessarily know up front which case
we're in (although HitTestResult::urlElement() might be able to tell you). 
Anyway good catch!

[mustaq, 2014/07/31 21:04:48]

HitTestResult::isOverLink() returns true for both anchors and the map elements
used in images, and this method already relies on (to be precise, performs an
AND with) HitTestResult::m_innerURLElement. So I thought the two cases are
indistinguishable for that class.

Moreover, we can have an <a> inside a <map>, or vice versa. I think the url
returned from hit test is ambiguous in either case, right?

+    largestHandCursorNode = bestTouchNode;
+    currentIsOverLink = tapIsOverLink;
+    while ((currentNode = NodeRenderingTraversal::parent(largestHandCursorNode))) {
+        if (isHTMLMapElement(largestHandCursorNode) && !isHTMLMapElement(currentNode))
+            currentIsOverLink = false;
+
+        if (!showsHandCursor(currentNode, toLocalFrame(m_page->mainFrame()), currentIsOverLink))
+            break;

[Rick Byers, 2014/07/31 20:15:10]

same as above

[mustaq, 2014/07/31 21:04:48]

Done.

+
+        largestHandCursorNode = currentNode;
+    }
+
+    if (currentNode)
+        return largestHandCursorNode;
 
+    // Couldn't find a parent with a hand cursor

[Rick Byers, 2014/07/31 20:15:10]

nit: you mean 'without' not 'with', right?

[mustaq, 2014/07/31 21:04:48]

Good catch!

     return bestTouchNode;
 }