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

Source/web/WebViewImpl.cpp

 /*
  * Copyright (C) 2011, 2012 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 1151 matching lines @@
     else
         rect.x = std::max<float>(rect.x, hitPoint.x + padding - screenWidth);
     scroll.x = rect.x;
     scroll.y = rect.y;
 
     scale = clampPageScaleFactorToLimits(scale);
     scroll = mainFrameImpl()->frameView()->windowToContents(scroll);
     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)
 {
     TRACE_EVENT0("input", "WebViewImpl::bestTapNode");
 
     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);
+    Node* bestTouchNode = targetedEvent.hitTestResult().targetNode();
+    bool isOverLink = targetedEvent.hitTestResult().isOverLink();
 
-    // 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())
+    // Walk up the tree until we have an element node with an attached renderer
+    while (bestTouchNode && (!bestTouchNode->isElementNode() || !bestTouchNode->renderer()))
         bestTouchNode = bestTouchNode->parentNode();
 
-    // 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();
+    // 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;
+    }
 
-    if (!bestTouchNode)
-        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;
 }
 
 void WebViewImpl::enableTapHighlightAtPoint(const PlatformGestureEvent& tapEvent)
 {
     Node* touchNode = bestTapNode(tapEvent);
 
     WillBeHeapVector<RawPtrWillBeMember<Node> > highlightNodes;
     highlightNodes.append(touchNode);
@@ skipping 3019 matching lines @@
     const PageScaleConstraints& constraints = m_pageScaleConstraintsSet.pageDefinedConstraints();
 
     if (!mainFrameImpl() || !mainFrameImpl()->frameView())
         return false;
 
     return mainFrameImpl()->frameView()->layoutSize().width() == m_size.width
         || (constraints.minimumScale == constraints.maximumScale && constraints.minimumScale != -1);
 }
 
 } // namespace blink