Expand tap highlight to allow multiple highlights for touch disambiguation.

Expand tap highlight to allow multiple highlights for touch disambiguation.

Tap gestures may hit multiple targets, causing a disambiguation window to popup on Android. In addition to this, we would like all of the candidate targets highlighted rather than just the target which was scored highest.

This change allows multiple nodes to be highlighted simultaneously. The touch disambiguation is also tapped into to select nodes for highlighting.

BUG=286244
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157947

[wjmaclean, 2013-06-12 15:09:05]

Overall I'm basically ok with the code changes, though (since there's no bug
linked to this cl) it would be nice to have a motivating use case - can you
include an example that demonstrates why this is a useful thing to do?

It needs a layout test added (this can be part of the demonstration I mentioned
above), plus a unit tests that actually creates multiple highlight targets.

rbyers@ - is there any use-case conflict you can think of?

https://codereview.chromium.org/15663005/diff/1/Source/WebKit/chromium/tests/...
File Source/WebKit/chromium/tests/LinkHighlightTest.cpp (right):

https://codereview.chromium.org/15663005/diff/1/Source/WebKit/chromium/tests/...
Source/WebKit/chromium/tests/LinkHighlightTest.cpp:182:
webViewImpl->enableTapHighlightAtPoint(platformEvent);
We should add a test that verifies that multiple highlights work, i.e. calls
enableTapHighlights.

https://codereview.chromium.org/15663005/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/15663005/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/GraphicsLayer.cpp:567:
m_linkHighlights[i]->layer()->setDebugName(debugName);
It may be slightly confusing having multiple link highlight layers all with the
same name ... can they at least be numbered or something to indicate that it's
ok for there to be more than one?

[Rick Byers, 2013-06-12 16:32:44]

On 2013/06/12 15:09:05, wjmaclean wrote:
> Overall I'm basically ok with the code changes, though (since there's no bug
> linked to this cl) it would be nice to have a motivating use case - can you
> include an example that demonstrates why this is a useful thing to do?
> 
> It needs a layout test added (this can be part of the demonstration I
mentioned
> above), plus a unit tests that actually creates multiple highlight targets.
> 
> rbyers@ - is there any use-case conflict you can think of?

I can't think of any offhand.  I've had in the back of my head that we'll want
to support scenarios including this some day - eg. for when there are multiple
distinct pointing devices, potentially being used by different users (something
pointer events v2 will probably tackle more explicitly).  So I think removing
assumptions about a single GestureTap being in flight at once is reasonable.

But making such multi-pointer scenarios work correctly is probably a lot of
work.  For example, if there can be multiple outstanding GestureTapDown events
at once, then we'll try to apply CSS :active state to more than one place -
violating assumptions across a bunch of WebCore (they'll end up fighting I think
- last one wins).  Mathias, can you point us to any details on the overall plan
here?  Is this just an isolated easy high-profile fix for multi-pointer
scenarios, or part of a larger effort to really make multi-pointer work across
blink?

>
https://codereview.chromium.org/15663005/diff/1/Source/WebKit/chromium/tests/...
> File Source/WebKit/chromium/tests/LinkHighlightTest.cpp (right):
> 
>
https://codereview.chromium.org/15663005/diff/1/Source/WebKit/chromium/tests/...
> Source/WebKit/chromium/tests/LinkHighlightTest.cpp:182:
> webViewImpl->enableTapHighlightAtPoint(platformEvent);
> We should add a test that verifies that multiple highlights work, i.e. calls
> enableTapHighlights.
> 
>
https://codereview.chromium.org/15663005/diff/1/Source/core/platform/graphics...
> File Source/core/platform/graphics/GraphicsLayer.cpp (right):
> 
>
https://codereview.chromium.org/15663005/diff/1/Source/core/platform/graphics...
> Source/core/platform/graphics/GraphicsLayer.cpp:567:
> m_linkHighlights[i]->layer()->setDebugName(debugName);
> It may be slightly confusing having multiple link highlight layers all with
the
> same name ... can they at least be numbered or something to indicate that it's
> ok for there to be more than one?

[Mathias Hällman, 2013-06-14 11:51:03]

On 2013/06/12 16:32:44, Rick Byers wrote:
> On 2013/06/12 15:09:05, wjmaclean wrote:
> > Overall I'm basically ok with the code changes, though (since there's no bug
> > linked to this cl) it would be nice to have a motivating use case - can you
> > include an example that demonstrates why this is a useful thing to do?
> > 
> > It needs a layout test added (this can be part of the demonstration I
> mentioned
> > above), plus a unit tests that actually creates multiple highlight targets.
> > 
> > rbyers@ - is there any use-case conflict you can think of?
> 
> I can't think of any offhand.  I've had in the back of my head that we'll want
> to support scenarios including this some day - eg. for when there are multiple
> distinct pointing devices, potentially being used by different users
(something
> pointer events v2 will probably tackle more explicitly).  So I think removing
> assumptions about a single GestureTap being in flight at once is reasonable.
> 
> But making such multi-pointer scenarios work correctly is probably a lot of
> work.  For example, if there can be multiple outstanding GestureTapDown events
> at once, then we'll try to apply CSS :active state to more than one place -
> violating assumptions across a bunch of WebCore (they'll end up fighting I
think
> - last one wins).  Mathias, can you point us to any details on the overall
plan
> here?  Is this just an isolated easy high-profile fix for multi-pointer
> scenarios, or part of a larger effort to really make multi-pointer work across
> blink?
> 
> >
>
https://codereview.chromium.org/15663005/diff/1/Source/WebKit/chromium/tests/...
> > File Source/WebKit/chromium/tests/LinkHighlightTest.cpp (right):
> > 
> >
>
https://codereview.chromium.org/15663005/diff/1/Source/WebKit/chromium/tests/...
> > Source/WebKit/chromium/tests/LinkHighlightTest.cpp:182:
> > webViewImpl->enableTapHighlightAtPoint(platformEvent);
> > We should add a test that verifies that multiple highlights work, i.e. calls
> > enableTapHighlights.
> > 
> >
>
https://codereview.chromium.org/15663005/diff/1/Source/core/platform/graphics...
> > File Source/core/platform/graphics/GraphicsLayer.cpp (right):
> > 
> >
>
https://codereview.chromium.org/15663005/diff/1/Source/core/platform/graphics...
> > Source/core/platform/graphics/GraphicsLayer.cpp:567:
> > m_linkHighlights[i]->layer()->setDebugName(debugName);
> > It may be slightly confusing having multiple link highlight layers all with
> the
> > same name ... can they at least be numbered or something to indicate that
it's
> > ok for there to be more than one?

Please excuse the delay, I've had troubles getting the layout tests running on
my ArchLinux setup.*

Thanks for the feedback, however, I'm afraid our plans for the moment are less
grand than to support multi-pointer scenarios. This is useful for us simply
because in Opera we wish to help distinguish multiple tapped links by zooming in
and highlighting them. I do believe this is a good thing in Chrome as well, even
if you use the popup window to help the user instead.

I hope to get the layout tests running asap so that I can continue with this
review.

*
https://groups.google.com/a/chromium.org/forum/?fromgroups#!topic/blink-dev/s...

[Rick Byers, 2013-06-14 13:10:40]

On 2013/06/14 11:51:03, Mathias Hällman wrote:
> On 2013/06/12 16:32:44, Rick Byers wrote:
> > On 2013/06/12 15:09:05, wjmaclean wrote:
> > > Overall I'm basically ok with the code changes, though (since there's no
bug
> > > linked to this cl) it would be nice to have a motivating use case - can
you
> > > include an example that demonstrates why this is a useful thing to do?
> > > 
> > > It needs a layout test added (this can be part of the demonstration I
> > mentioned
> > > above), plus a unit tests that actually creates multiple highlight
targets.
> > > 
> > > rbyers@ - is there any use-case conflict you can think of?
> > 
> > I can't think of any offhand.  I've had in the back of my head that we'll
want
> > to support scenarios including this some day - eg. for when there are
multiple
> > distinct pointing devices, potentially being used by different users
> (something
> > pointer events v2 will probably tackle more explicitly).  So I think
removing
> > assumptions about a single GestureTap being in flight at once is reasonable.
> > 
> > But making such multi-pointer scenarios work correctly is probably a lot of
> > work.  For example, if there can be multiple outstanding GestureTapDown
events
> > at once, then we'll try to apply CSS :active state to more than one place -
> > violating assumptions across a bunch of WebCore (they'll end up fighting I
> think
> > - last one wins).  Mathias, can you point us to any details on the overall
> plan
> > here?  Is this just an isolated easy high-profile fix for multi-pointer
> > scenarios, or part of a larger effort to really make multi-pointer work
across
> > blink?
> > 
> > >
> >
>
https://codereview.chromium.org/15663005/diff/1/Source/WebKit/chromium/tests/...
> > > File Source/WebKit/chromium/tests/LinkHighlightTest.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/15663005/diff/1/Source/WebKit/chromium/tests/...
> > > Source/WebKit/chromium/tests/LinkHighlightTest.cpp:182:
> > > webViewImpl->enableTapHighlightAtPoint(platformEvent);
> > > We should add a test that verifies that multiple highlights work, i.e.
calls
> > > enableTapHighlights.
> > > 
> > >
> >
>
https://codereview.chromium.org/15663005/diff/1/Source/core/platform/graphics...
> > > File Source/core/platform/graphics/GraphicsLayer.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/15663005/diff/1/Source/core/platform/graphics...
> > > Source/core/platform/graphics/GraphicsLayer.cpp:567:
> > > m_linkHighlights[i]->layer()->setDebugName(debugName);
> > > It may be slightly confusing having multiple link highlight layers all
with
> > the
> > > same name ... can they at least be numbered or something to indicate that
> it's
> > > ok for there to be more than one?
> 
> Please excuse the delay, I've had troubles getting the layout tests running on
> my ArchLinux setup.*
> 
> Thanks for the feedback, however, I'm afraid our plans for the moment are less
> grand than to support multi-pointer scenarios. This is useful for us simply
> because in Opera we wish to help distinguish multiple tapped links by zooming
in
> and highlighting them. I do believe this is a good thing in Chrome as well,
even
> if you use the popup window to help the user instead.

But how can multiple links be tapped at once with a single pointer?  Is this
just how Opera handles disambiguation (sends gestureTapDown to all candidates,
where Chrome selects the best candidate and sends gestureTapDown only to it)?

> I hope to get the layout tests running asap so that I can continue with this
> review.
> 
> *
>
https://groups.google.com/a/chromium.org/forum/?fromgroups#%21topic/blink-dev...

[Mathias Hällman, 2013-06-14 13:30:10]

> But how can multiple links be tapped at once with a single pointer?  Is this
just how Opera handles disambiguation (sends gestureTapDown to all candidates,
where Chrome selects the best candidate and sends gestureTapDown only to it)?

gestureTapDown is not sent to the candidates, they're just highlighted to show
the user what the finger touched. IIRC, Chrome doesn't send any tap events in
this case either. It only selects the best candidate for highlighting (whether
you hit one target or more), then shows a zoomed in popup if there were more
than one targets.

[Rick Byers, 2013-06-14 13:46:21]

On 2013/06/14 13:30:10, Mathias Hällman wrote:
> > But how can multiple links be tapped at once with a single pointer?  Is this
> just how Opera handles disambiguation (sends gestureTapDown to all candidates,
> where Chrome selects the best candidate and sends gestureTapDown only to it)?
> 
> gestureTapDown is not sent to the candidates, they're just highlighted to show
> the user what the finger touched. IIRC, Chrome doesn't send any tap events in
> this case either. It only selects the best candidate for highlighting (whether
> you hit one target or more), then shows a zoomed in popup if there were more
> than one targets.

Ah I see, you're triggering the highlight without the gesture event - got it. 
I'm not familiar with the Chrome Android path, but on desktop we send
GestureTapDown and that's what triggers the highlight (and :active state).  But
we don't do disambiguation on desktop (at least not yet) since the screens are
generally big enough that it's not as useful.

[wjmaclean, 2013-06-19 18:12:58]

On 2013/06/14 13:46:21, Rick Byers wrote:
> On 2013/06/14 13:30:10, Mathias Hällman wrote:
> > > But how can multiple links be tapped at once with a single pointer?  Is
this
> > just how Opera handles disambiguation (sends gestureTapDown to all
candidates,
> > where Chrome selects the best candidate and sends gestureTapDown only to
it)?
> > 
> > gestureTapDown is not sent to the candidates, they're just highlighted to
show
> > the user what the finger touched. IIRC, Chrome doesn't send any tap events
in
> > this case either. It only selects the best candidate for highlighting
(whether
> > you hit one target or more), then shows a zoomed in popup if there were more
> > than one targets.
> 
> Ah I see, you're triggering the highlight without the gesture event - got it. 
> I'm not familiar with the Chrome Android path, but on desktop we send
> GestureTapDown and that's what triggers the highlight (and :active state). 
But
> we don't do disambiguation on desktop (at least not yet) since the screens are
> generally big enough that it's not as useful.

Sorry, I meant to get back to you earlier.

Depending on your intended use cases, it may be that we won't want Chromium's
default behaviour to change (i.e. we would like to highlight just the target
that will receive the GestureTapDown), though it's easy to parameterise this so
Opera can have a different behaviour.

Did you have an example layout test you could show me, along with expected
output (or even a verbal description of the expected output)?

[Mathias Hällman, 2013-06-28 14:16:27]

Just a note, the review is stalling because it requires a test which will be
Android-specific, and this doesn't seem to be supported yet with Blink.
(https://code.google.com/p/chromium/issues/detail?id=232044)

[Mathias Hällman, 2013-08-02 13:33:38]

On 2013/06/28 14:16:27, Mathias Hällman wrote:
> Just a note, the review is stalling because it requires a test which will be
> Android-specific, and this doesn't seem to be supported yet with Blink.
> (https://code.google.com/p/chromium/issues/detail?id=232044)

I'd really like to continue with this review, but I'm still having problems with
layout tests. Since the added layout test will be android specific, I've managed
to "sort of run" the tests on a device, but I'm not getting the image results
back. But perhaps that's to be expected with the current state of layout tests
on Android? This is somewhat unclear to me from the above issue I posted.. If
I've understood this framework correctly, this makes it hard to generate a
reference image for the test. Any help on this would be very appreciated.

Anyhow, I've rebased the patch and added the test without the reference image
for now. Please let me know if I need to do anything else to get other platforms
to ignore this test. Alternatively, would it be possible to take in this patch
without the test, and open an issue to deal with it when the layout tests
framework on Android is in a better condition?

[Mathias Hällman, 2013-08-28 08:49:07]

New rebase and a gentle push for the forgotten review.

[wjmaclean, 2013-08-28 21:08:52]

On 2013/08/28 08:49:07, Mathias Hällman wrote:
> New rebase and a gentle push for the forgotten review.

Have we settled the issue of whether we actually want this behaviour in Chrome?
I'm thinking 'no', but I could be wrong. One way to handle this would be to make
it a WebSettings value that indicates whether multiple highlights or just a
single highlight is desired.

I'd like to wait for rbyers@ to weigh in on this.

[Mathias Hällman, 2013-08-29 12:49:23]

On 2013/08/28 21:08:52, wjmaclean wrote:
> On 2013/08/28 08:49:07, Mathias Hällman wrote:
> > New rebase and a gentle push for the forgotten review.
> 
> Have we settled the issue of whether we actually want this behaviour in
Chrome?
> I'm thinking 'no', but I could be wrong. One way to handle this would be to
make
> it a WebSettings value that indicates whether multiple highlights or just a
> single highlight is desired.
> 
> I'd like to wait for rbyers@ to weigh in on this.

Yes, please let me know if I should put anything behind a setting/flag, that can
be quickly done. Up til now I have been more worried about the layout tests
which still don't seem to work fully on Android.

[Rick Byers, 2013-09-05 17:24:30]

On 2013/08/29 12:49:23, Mathias Hällman wrote:
> On 2013/08/28 21:08:52, wjmaclean wrote:
> > On 2013/08/28 08:49:07, Mathias Hällman wrote:
> > > New rebase and a gentle push for the forgotten review.
> > 
> > Have we settled the issue of whether we actually want this behaviour in
> Chrome?
> > I'm thinking 'no', but I could be wrong. One way to handle this would be to
> make
> > it a WebSettings value that indicates whether multiple highlights or just a
> > single highlight is desired.
> > 
> > I'd like to wait for rbyers@ to weigh in on this.
> 
> Yes, please let me know if I should put anything behind a setting/flag, that
can
> be quickly done. Up til now I have been more worried about the layout tests
> which still don't seem to work fully on Android.

We definitely don't want to change behavior here on Chrome desktop, so yes
please ensure our existing behavior is preserved and that we have suitable test
coverage for both options.  We use highlighting as a signal of what you hit (or
are about to hit) when tapping - highlighting more than one thing would be very
confusing in that case.

But in the case of a tap disambiguation window I can see how there could be some
value in showing what the candidate elements were.  I don't know anything about
how the disambiguation window works on Chrome Android - aelias may want to weigh
in.

By the way, I think this usage of higlights is pretty different conceptually
from what link highlighting was originally designed for.  I haven't looked at
the code changes in detail, but I'd suggest at least making this scope a little
clearer in your CL description.  Eg. "expand link highlighting to optionally
highlight all candidates for use in a disambiguation window".  It would also
help to have a bug tracking the feature request here that this CL can be linked
to.  Without the context in the discussion here, the CL doesn't seem to make any
sense :-)

Rick

[Mathias Hällman, 2013-09-06 08:37:12]

On 2013/09/05 17:24:30, Rick Byers wrote:
> On 2013/08/29 12:49:23, Mathias Hällman wrote:
> > On 2013/08/28 21:08:52, wjmaclean wrote:
> > > On 2013/08/28 08:49:07, Mathias Hällman wrote:
> > > > New rebase and a gentle push for the forgotten review.
> > > 
> > > Have we settled the issue of whether we actually want this behaviour in
> > Chrome?
> > > I'm thinking 'no', but I could be wrong. One way to handle this would be
to
> > make
> > > it a WebSettings value that indicates whether multiple highlights or just
a
> > > single highlight is desired.
> > > 
> > > I'd like to wait for rbyers@ to weigh in on this.
> > 
> > Yes, please let me know if I should put anything behind a setting/flag, that
> can
> > be quickly done. Up til now I have been more worried about the layout tests
> > which still don't seem to work fully on Android.
> 
> We definitely don't want to change behavior here on Chrome desktop, so yes
> please ensure our existing behavior is preserved and that we have suitable
test
> coverage for both options.  We use highlighting as a signal of what you hit
(or
> are about to hit) when tapping - highlighting more than one thing would be
very
> confusing in that case.
> 
> But in the case of a tap disambiguation window I can see how there could be
some
> value in showing what the candidate elements were.  I don't know anything
about
> how the disambiguation window works on Chrome Android - aelias may want to
weigh
> in.
> 
> By the way, I think this usage of higlights is pretty different conceptually
> from what link highlighting was originally designed for.  I haven't looked at
> the code changes in detail, but I'd suggest at least making this scope a
little
> clearer in your CL description.  Eg. "expand link highlighting to optionally
> highlight all candidates for use in a disambiguation window".  It would also
> help to have a bug tracking the feature request here that this CL can be
linked
> to.  Without the context in the discussion here, the CL doesn't seem to make
any
> sense :-)
> 
> Rick

Thanks for the feedback. It's important to note though that this highlight is an
entirely different thing from the regular highlight you have on desktop. (which
still exist in Android builds) This is a touch specific highlight happening on a
higher level, currently only used on Android as far as I know, as an additional
aid for the user to see what was touched. In short, this changes nothing on
Desktop. Furthermore, bug or not, I believe this highlight is not shown in the
disambiguation window. The question is then, does Android want highlighting of
all touch candidates? Or should it be put behind a flag.

I've updated the CL and opened a bug, hopefully things will be clearer now.

[aelias_OOO_until_Jul13, 2013-09-06 18:21:15]

lgtm

[commit-bot: I haz the power, 2013-09-06 18:21:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mathiash@opera.com/15663005/38001

[commit-bot: I haz the power, 2013-09-06 18:21:29]

Failed to apply patch for Source/web/WebViewImpl.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/web/WebViewImpl.cpp
  Hunk #1 succeeded at 695 (offset -2 lines).
  Hunk #2 succeeded at 733 (offset -2 lines).
  Hunk #3 FAILED at 1241.
  Hunk #4 succeeded at 1774 (offset -4 lines).
  Hunk #5 succeeded at 3628 (offset 4 lines).
  1 out of 5 hunks FAILED -- saving rejects to file
Source/web/WebViewImpl.cpp.rej

Patch:       Source/web/WebViewImpl.cpp
Index: Source/web/WebViewImpl.cpp
diff --git a/Source/web/WebViewImpl.cpp b/Source/web/WebViewImpl.cpp
index
9c7f9baa726108aa84e977ab7fd54c5e872ebff1..191bd6e98fae7dce8919e59874e4ef03e530d8d0
100644
--- a/Source/web/WebViewImpl.cpp
+++ b/Source/web/WebViewImpl.cpp
@@ -697,14 +697,14 @@ bool WebViewImpl::handleGestureEvent(const
WebGestureEvent& event)
         // Queue a highlight animation, then hand off to regular handler.
 #if OS(LINUX)
         if (settingsImpl()->gestureTapHighlightEnabled())
-            enableTapHighlight(platformEvent);
+            enableTapHighlightAtPoint(platformEvent);
 #endif
         break;
     case WebInputEvent::GestureTapCancel:
     case WebInputEvent::GestureTap:
     case WebInputEvent::GestureLongPress:
-        if (m_linkHighlight)
-            m_linkHighlight->startHighlightAnimationIfNeeded();
+        for (size_t i = 0; i < m_linkHighlights.size(); i++)
+            m_linkHighlights[i]->startHighlightAnimationIfNeeded();
         break;
     default:
         break;
@@ -735,10 +735,13 @@ bool WebViewImpl::handleGestureEvent(const
WebGestureEvent& event)
             scaledEvent.data.tap.height = event.data.tap.height /
pageScaleFactor();
             IntRect boundingBox(scaledEvent.x - scaledEvent.data.tap.width / 2,
scaledEvent.y - scaledEvent.data.tap.height / 2, scaledEvent.data.tap.width,
scaledEvent.data.tap.height);
             Vector<IntRect> goodTargets;
-            findGoodTouchTargets(boundingBox, mainFrameImpl()->frame(),
goodTargets);
+            Vector<Node*> highlightNodes;
+            findGoodTouchTargets(boundingBox, mainFrameImpl()->frame(),
goodTargets, highlightNodes);
             // FIXME: replace touch adjustment code when numberOfGoodTargets ==
1?
             // Single candidate case is currently handled by:
https://bugs.webkit.org/show_bug.cgi?id=85101
             if (goodTargets.size() >= 2 && m_client &&
m_client->didTapMultipleTargets(scaledEvent, goodTargets)) {
+                if (settingsImpl()->gestureTapHighlightEnabled())
+                    enableTapHighlights(highlightNodes);
                 eventSwallowed = true;
                 eventCancelled = true;
                 break;
@@ -1238,24 +1241,37 @@ Node* WebViewImpl::bestTapNode(const
PlatformGestureEvent& tapEvent)
     return bestTouchNode;
 }
 
-void WebViewImpl::enableTapHighlight(const PlatformGestureEvent& tapEvent)
+void WebViewImpl::enableTapHighlightAtPoint(const PlatformGestureEvent&
tapEvent)
 {
-    // Always clear any existing highlight when this is invoked, even if we
don't get a new target to highlight.
-    m_linkHighlight.clear();
-
     Node* touchNode = bestTapNode(tapEvent);
 
-    if (!touchNode || !touchNode->renderer() ||
!touchNode->renderer()->enclosingLayer())
-        return;
+    Vector<Node*> highlightNodes;
+    highlightNodes.append(touchNode);
 
-    Color highlightColor =
touchNode->renderer()->resolveColor(CSSPropertyWebkitTapHighlightColor);
-    // Safari documentation for -webkit-tap-highlight-color says if the
specified color has 0 alpha,
-    // then tap highlighting is disabled.
-    //
http://developer.apple.com/library/safari/#documentation/appleapplications/re...
-    if (!highlightColor.alpha())
-        return;
+    enableTapHighlights(highlightNodes);
+}
+
+void WebViewImpl::enableTapHighlights(Vector<Node*>& highlightNodes)
+{
+    // Always clear any existing highlight when this is invoked, even if we
+    // don't get a new target to highlight.
+    m_linkHighlights.clear();
+
+    for (size_t i = 0; i < highlightNodes.size(); i++) {
+        Node* node = highlightNodes[i];
+
+        if (!node || !node->renderer() || !node->renderer()->enclosingLayer())
+            continue;
 
-    m_linkHighlight = LinkHighlight::create(touchNode, this);
+        Color highlightColor =
node->renderer()->resolveColor(CSSPropertyWebkitTapHighlightColor);
+        // Safari documentation for -webkit-tap-highlight-color says if the
+        // specified color has 0 alpha, then tap highlighting is disabled.
+        //
http://developer.apple.com/library/safari/#documentation/appleapplications/re...
+        if (!highlightColor.alpha())
+            continue;
+
+        m_linkHighlights.append(LinkHighlight::create(node, this));
+    }
 }
 
 void WebViewImpl::animateDoubleTapZoom(const IntPoint& point)
@@ -1775,8 +1791,8 @@ void WebViewImpl::layout()
     if (m_layerTreeView)
         m_layerTreeView->setBackgroundColor(backgroundColor());
 
-    if (m_linkHighlight)
-        m_linkHighlight->updateGeometry();
+    for (size_t i = 0; i < m_linkHighlights.size(); i++)
+        m_linkHighlights[i]->updateGeometry();
 }
 
 void WebViewImpl::enterForceCompositingMode(bool enter)
@@ -3621,7 +3637,7 @@ void WebViewImpl::didCommitLoad(bool* isNewNavigation,
bool isNavigationWithinPa
         m_pageScaleConstraintsSet.setNeedsReset(true);
 
     // Make sure link highlight from previous page is cleared.
-    m_linkHighlight.clear();
+    m_linkHighlights.clear();
     m_gestureAnimation.clear();
     if (m_layerTreeView)
         m_layerTreeView->didStopFlinging();

[aelias_OOO_until_Jul13, 2013-09-06 18:23:37]

Sorry, hit the l-g-t-m button accidentally.  Anyway, this is fine for Android,
this highlight is a relatively minor part of what the user sees in our
disambiguation approach and this change makes it slightly better.  Note that
this tap highlight is also used on touch laptops such as the Chromebook Pixel,
not exclusively Android.

[wjmaclean, 2013-09-06 18:30:58]

> Thanks for the feedback. It's important to note though that this highlight is
an
> entirely different thing from the regular highlight you have on desktop.
(which
> still exist in Android builds) This is a touch specific highlight happening on
a
> higher level, currently only used on Android as far as I know, as an
additional
> aid for the user to see what was touched.

The code paths you are touching *are* used on Pixel (ChromeOS) as best I can
tell, so this CL should not be landed without providing some way of making sure
this is only enable for Android, and not for ChromeOS.

Correct me if I'm wrong ... is there some mechanism I'm missing that makes sure
this isn't used *except* for Android?

> In short, this changes nothing on
> Desktop.

I'm not sure I agree ... we also use these pathways on Windows/Linux if the user
has a touch screen.

> Furthermore, bug or not, I believe this highlight is not shown in the
> disambiguation window. The question is then, does Android want highlighting of
> all touch candidates? Or should it be put behind a flag.
> 
> I've updated the CL and opened a bug, hopefully things will be clearer now.

[Rick Byers, 2013-09-06 19:25:35]

On 2013/09/06 18:30:58, wjmaclean wrote:
> > Thanks for the feedback. It's important to note though that this highlight
is
> an
> > entirely different thing from the regular highlight you have on desktop.
> (which
> > still exist in Android builds) This is a touch specific highlight happening
on
> a
> > higher level, currently only used on Android as far as I know, as an
> additional
> > aid for the user to see what was touched.
> 
> The code paths you are touching *are* used on Pixel (ChromeOS) as best I can
> tell, so this CL should not be landed without providing some way of making
sure
> this is only enable for Android, and not for ChromeOS.

We should probably be thinking about this as 'disambiguation window' vs.
'inline' rather than 'desktop' vs. 'android'.  When there is a window asking you
to select which item you meant, then highlighting multiple makes sense.  When
the affordance is mainly showing you what was just activated (the inline case),
then we definitely want only one.  If we add a disambiguation window on desktop
then we'd probably want this too for consistency with Android.  Similarly, if
there's some case where Android doesn't pop a disambiguation window and
immediately activates, then we shouldn't get this behavior.

> 
> Correct me if I'm wrong ... is there some mechanism I'm missing that makes
sure
> this isn't used *except* for Android?
> 
> > In short, this changes nothing on
> > Desktop.
> 
> I'm not sure I agree ... we also use these pathways on Windows/Linux if the
user
> has a touch screen.
> 
> > Furthermore, bug or not, I believe this highlight is not shown in the
> > disambiguation window. The question is then, does Android want highlighting
of
> > all touch candidates? Or should it be put behind a flag.
> > 
> > I've updated the CL and opened a bug, hopefully things will be clearer now.

[wjmaclean, 2013-09-06 19:44:38]

On 2013/09/06 19:25:35, Rick Byers wrote:
> 
> We should probably be thinking about this as 'disambiguation window' vs.
> 'inline' rather than 'desktop' vs. 'android'.  When there is a window asking
you
> to select which item you meant, then highlighting multiple makes sense.  When
> the affordance is mainly showing you what was just activated (the inline
case),
> then we definitely want only one.  If we add a disambiguation window on
desktop
> then we'd probably want this too for consistency with Android.  Similarly, if
> there's some case where Android doesn't pop a disambiguation window and
> immediately activates, then we shouldn't get this behavior.

Yes, I agree there are certainly different ways to define where the
multi-highlight behaviour is desired.

[Mathias Hällman, 2013-09-09 05:27:59]

On 2013/09/06 18:30:58, wjmaclean wrote:
> 
> The code paths you are touching *are* used on Pixel (ChromeOS) as best I can
> tell, so this CL should not be landed without providing some way of making
sure
> this is only enable for Android, and not for ChromeOS.
> 
> Correct me if I'm wrong ... is there some mechanism I'm missing that makes
sure
> this isn't used *except* for Android?
>

No you're right, I spoke to fast.


On 2013/09/06 19:25:35, Rick Byers wrote:
> 
> We should probably be thinking about this as 'disambiguation window' vs.
> 'inline' rather than 'desktop' vs. 'android'.  When there is a window asking
you
> to select which item you meant, then highlighting multiple makes sense.  When
> the affordance is mainly showing you what was just activated (the inline
case),
> then we definitely want only one.  If we add a disambiguation window on
desktop
> then we'd probably want this too for consistency with Android.  Similarly, if
> there's some case where Android doesn't pop a disambiguation window and
> immediately activates, then we shouldn't get this behavior.
> 

As it is now with this CL, multiple targets will only be highlighted if the
touch disambiguation has determined that it can not reliably choose one target
and activate it. That is, if a single target is hit and activated, or several
targets were hit but one target was scored high enough by the touch
disambiguation to be activated, then that one target alone will receive
highlight. (see changes around WebViewImpl.cpp:740)

[Mathias Hällman, 2013-09-11 09:13:33]

Let's not allow this one to slip into oblivion again. :) I think all concerns
have been addressed, what do we do from here? To sum up, this type of highlight
is only used for touch, and multiple targets will only be highlighted in the
case where nothing was activated because the disambiguation couldn't determine
what to activate. Other platforms than android may get multiple targets
highlighted by touch, and as it is now they will do so if they have this type of
highlight enabled at all. (through a flag in WebPreferences)

[Rick Byers, 2013-09-11 15:14:49]

I agree this approach seems very reasonable - thanks for the explanation!

https://codereview.chromium.org/15663005/diff/52001/Source/web/WebViewImpl.h
File Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/15663005/diff/52001/Source/web/WebViewImpl.h#...
Source/web/WebViewImpl.h:552: LinkHighlight* linkHighlight(int i) { return
m_linkHighlights[i].get(); }
Is there code somewhere I'm missing that calls this with a non-zero index?  I'm
wondering how anyone could really use this without also having an API to get the
length of the vector.  Are there tests missing?

[wjmaclean, 2013-09-11 15:34:56]

On 2013/09/11 09:13:33, Mathias Hällman wrote:
> Let's not allow this one to slip into oblivion again. :) I think all concerns
> have been addressed, what do we do from here? To sum up, this type of
highlight
> is only used for touch, and multiple targets will only be highlighted in the
> case where nothing was activated because the disambiguation couldn't determine
> what to activate. Other platforms than android may get multiple targets
> highlighted by touch, and as it is now they will do so if they have this type
of
> highlight enabled at all. (through a flag in WebPreferences)

One last thought from me: it would be nice to allow the user an easy way to
distinguish between the single highlight (indicating a successfully
disambiguated touch event), vs. the case where multiple targets are highlighted
due to non-conclusive disambiguation ... perhaps in the latter case the
highlight color should change? I know this sort of flies in the face of the css
property for setting the link highlight color, but then I don't imagine it
expected the highlight to be used in this way (since the property applies on a
per-element basis, and here we would be highlighting multiple elements). Given
our current grey highlight I would (personally) opt for red.

[Mathias Hällman, 2013-09-11 17:57:07]

On 2013/09/11 15:14:49, Rick Byers wrote:
> Is there code somewhere I'm missing that calls this with a non-zero index? 
I'm
> wondering how anyone could really use this without also having an API to get
the
> length of the vector.  Are there tests missing?

That's true, you should be able to get the length of the vector as well. I'll
get to that.


On 2013/09/11 15:34:56, wjmaclean wrote:
> One last thought from me: it would be nice to allow the user an easy way to
> distinguish between the single highlight (indicating a successfully
> disambiguated touch event), vs. the case where multiple targets are
highlighted
> due to non-conclusive disambiguation ... perhaps in the latter case the
> highlight color should change? I know this sort of flies in the face of the
css
> property for setting the link highlight color, but then I don't imagine it
> expected the highlight to be used in this way (since the property applies on a
> per-element basis, and here we would be highlighting multiple elements). Given
> our current grey highlight I would (personally) opt for red.

Absolutely, and actually we already do this in Opera. I wanted to keep this CL
as small as possible, however, so I kept it out for now. I'll gladly add it once
I get to the office tomorrow. One thing though, if you want it red, and we want
it yellow, how do we best let each "fork" determine for themselves? It seems
like a silly thing to bog down the WebPreferences with.

[wjmaclean, 2013-09-11 22:02:46]

On 2013/09/11 17:57:07, Mathias Hällman wrote:
> Absolutely, and actually we already do this in Opera. I wanted to keep this CL
> as small as possible, however, so I kept it out for now. I'll gladly add it
once
> I get to the office tomorrow. One thing though, if you want it red, and we
want
> it yellow, how do we best let each "fork" determine for themselves? It seems
> like a silly thing to bog down the WebPreferences with.

Well, I only suggested red as an off-the-top-of-my-head thing, though I imagine
we should refer this question to our UX people ... Rick, do you know who to
contact about this?

[Rick Byers, 2013-09-12 02:56:19]

On 2013/09/11 22:02:46, wjmaclean wrote:
> On 2013/09/11 17:57:07, Mathias Hällman wrote:
> > Absolutely, and actually we already do this in Opera. I wanted to keep this
CL
> > as small as possible, however, so I kept it out for now. I'll gladly add it
> once
> > I get to the office tomorrow. One thing though, if you want it red, and we
> want
> > it yellow, how do we best let each "fork" determine for themselves? It seems
> > like a silly thing to bog down the WebPreferences with.
> 
> Well, I only suggested red as an off-the-top-of-my-head thing, though I
imagine
> we should refer this question to our UX people ... Rick, do you know who to
> contact about this?

Probably roma@ (clank UX), but I doubt she's going to care much.  I'd suggest we
push that off to the next CL.  Maybe we can get agreement to just use whatever
Opera is already doing :-)

[wjmaclean, 2013-09-12 22:25:37]

> Probably roma@ (clank UX), but I doubt she's going to care much.  I'd suggest
we
> push that off to the next CL.  Maybe we can get agreement to just use whatever
> Opera is already doing :-)

Works for me, lgtm

[Mathias Hällman, 2013-09-13 13:49:33]

Tests cleaned up, and I also added calls to disable the highlight shortly after
enabling them in the multi-case so that they don't linger. (well, immediately,
but there's a timer/animation making it behave essentially the same as when you
only hit one target)

Adding another reviewer for Source/core. @jochen, could you have a look please?

[jochen (gone - plz use gerrit), 2013-09-16 14:13:04]

lgtm with nits

https://chromiumcodereview.appspot.com/15663005/diff/63001/Source/core/platfo...
File Source/core/platform/graphics/GraphicsLayer.cpp (right):

https://chromiumcodereview.appspot.com/15663005/diff/63001/Source/core/platfo...
Source/core/platform/graphics/GraphicsLayer.cpp:124: for (size_t i = 0; i <
m_linkHighlights.size(); i++)
nit: ++i (and everywhere else)

https://chromiumcodereview.appspot.com/15663005/diff/63001/Source/core/platfo...
Source/core/platform/graphics/GraphicsLayer.cpp:1168:
ASSERT(!m_linkHighlights.contains(linkHighlight));
why not assert that you don't add the same client twice instead of here when
removing clients?

I would expect that the stack trace when addnig the client twice is (slightly)
more interesting

[commit-bot: I haz the power, 2013-09-17 13:03:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mathiash@opera.com/15663005/76001

[commit-bot: I haz the power, 2013-09-17 13:27:12]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_blink_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-18 07:18:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mathiash@opera.com/15663005/79001

[commit-bot: I haz the power, 2013-09-18 07:35:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mathiash@opera.com/15663005/100001

[Mathias Hällman, 2013-09-18 07:36:01]

Am I missing some build flag? Or why don't I get these errors when building
locally?

[commit-bot: I haz the power, 2013-09-18 09:00:49]

Change committed as 157947