Commit 9b10783b authored by Donn Denman's avatar Donn Denman Committed by Commit Bot

[TTS] Fix tap-far-from-previous suppression.

A tap that's far from the previous tap should generally be ignored,
and just dismiss our UI instead of starting a new tap-handling sequence.
This CL updates our logic to look at whether there was any selection
before the tap rather than whether the previous tap was suppressed.

Our old logic ignored whether there was any selection before the tap,
and used whether the previous tap was suppressed as a proxy for
no-selection.  Recent code changes make it fairly easy to determine
if there was a selection just before a tap gesture so we use that
instead.

The difference becomes clear when "invalid taps" are considered.
These are taps on non-text characters, e.g. a period or comma.  These
taps don't select, but they also are not technically suppressed, so
a subsequent tap would be ignored.  Now any tap that's far from a previous
tap will be ignored only if there was a selection before the tap.

BUG=713471

Change-Id: Ie94e345a87cf580a3d1993370938982e6e273e89
Reviewed-on: https://chromium-review.googlesource.com/794020Reviewed-by: default avatarTheresa <twellington@chromium.org>
Commit-Queue: Donn Denman <donnd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520161}
parent a736e5c9
...@@ -76,6 +76,9 @@ public class ContextualSearchSelectionController { ...@@ -76,6 +76,9 @@ public class ContextualSearchSelectionController {
// When the last tap gesture happened. // When the last tap gesture happened.
private long mTapTimeNanoseconds; private long mTapTimeNanoseconds;
// Whether the selection was empty before the most recent tap gesture.
private boolean mWasSelectionEmptyBeforeTap;
// The duration of the last tap gesture in milliseconds, or 0 if not set. // The duration of the last tap gesture in milliseconds, or 0 if not set.
private int mTapDurationMs = INVALID_DURATION; private int mTapDurationMs = INVALID_DURATION;
...@@ -100,6 +103,7 @@ public class ContextualSearchSelectionController { ...@@ -100,6 +103,7 @@ public class ContextualSearchSelectionController {
@Override @Override
public void onTouchDown() { public void onTouchDown() {
mTapTimeNanoseconds = System.nanoTime(); mTapTimeNanoseconds = System.nanoTime();
mWasSelectionEmptyBeforeTap = TextUtils.isEmpty(mSelectedText);
} }
} }
...@@ -361,8 +365,9 @@ public class ContextualSearchSelectionController { ...@@ -361,8 +365,9 @@ public class ContextualSearchSelectionController {
int adjustedTapsSinceOpen = prefs.getContextualSearchTapCount() int adjustedTapsSinceOpen = prefs.getContextualSearchTapCount()
- prefs.getContextualSearchTapQuickAnswerCount(); - prefs.getContextualSearchTapQuickAnswerCount();
assert mTapDurationMs != INVALID_DURATION : "mTapDurationMs not set!"; assert mTapDurationMs != INVALID_DURATION : "mTapDurationMs not set!";
TapSuppressionHeuristics tapHeuristics = new TapSuppressionHeuristics(this, mLastTapState, TapSuppressionHeuristics tapHeuristics =
x, y, adjustedTapsSinceOpen, contextualSearchContext, mTapDurationMs); new TapSuppressionHeuristics(this, mLastTapState, x, y, adjustedTapsSinceOpen,
contextualSearchContext, mTapDurationMs, mWasSelectionEmptyBeforeTap);
// TODO(donnd): Move to be called when the panel closes to work with states that change. // TODO(donnd): Move to be called when the panel closes to work with states that change.
tapHeuristics.logConditionState(); tapHeuristics.logConditionState();
......
...@@ -4,6 +4,8 @@ ...@@ -4,6 +4,8 @@
package org.chromium.chrome.browser.contextualsearch; package org.chromium.chrome.browser.contextualsearch;
import android.support.annotation.Nullable;
/** /**
* Implements the policy that a Tap relatively far away from an existing Contextual Search * Implements the policy that a Tap relatively far away from an existing Contextual Search
* selection should just dismiss our UX. When a Tap is close by, we assume the user must have * selection should just dismiss our UX. When a Tap is close by, we assume the user must have
...@@ -20,14 +22,17 @@ class TapFarFromPreviousSuppression extends ContextualSearchHeuristic { ...@@ -20,14 +22,17 @@ class TapFarFromPreviousSuppression extends ContextualSearchHeuristic {
* Constructs a heuristic to determine if the current Tap should be suppressed because it is * Constructs a heuristic to determine if the current Tap should be suppressed because it is
* far from the previous tap. * far from the previous tap.
* @param controller The {@link ContextualSearchSelectionController}. * @param controller The {@link ContextualSearchSelectionController}.
* @param previousTapState The state of the previous tap gesture, or {@code null}.
* @param x The x coordinate of the tap gesture. * @param x The x coordinate of the tap gesture.
* @param y The y coordinate of the tap gesture. * @param y The y coordinate of the tap gesture.
* @param wasSelectionEmptyBeforeTap Whether the selection was empty just before this tap.
*/ */
TapFarFromPreviousSuppression(ContextualSearchSelectionController controller, TapFarFromPreviousSuppression(ContextualSearchSelectionController controller,
ContextualSearchTapState previousTapState, int x, int y) { @Nullable ContextualSearchTapState previousTapState, int x, int y,
boolean wasSelectionEmptyBeforeTap) {
mPxToDp = controller.getPxToDp(); mPxToDp = controller.getPxToDp();
mPreviousTapState = previousTapState; mPreviousTapState = previousTapState;
mShouldHandleTap = shouldHandleTap(x, y); mShouldHandleTap = shouldHandleTap(x, y, wasSelectionEmptyBeforeTap);
} }
@Override @Override
...@@ -36,17 +41,21 @@ class TapFarFromPreviousSuppression extends ContextualSearchHeuristic { ...@@ -36,17 +41,21 @@ class TapFarFromPreviousSuppression extends ContextualSearchHeuristic {
} }
/** /**
* Determines whether the tap should be handled based on whether it's near a previous tap and
* whether the selection was visible just before that tap. Uses the previous tap state.
* @param x The x coordinate of the current tap.
* @param y The y coordinate of the current tap.
* @param wasSelectionEmptyBeforeTap Whether the selection was empty before the current tap.
* @return whether a tap at the given coordinates should be handled or not. * @return whether a tap at the given coordinates should be handled or not.
*/ */
private boolean shouldHandleTap(int x, int y) { private boolean shouldHandleTap(int x, int y, boolean wasSelectionEmptyBeforeTap) {
if (mPreviousTapState == null) return true; if (mPreviousTapState == null || wasSelectionEmptyBeforeTap) return true;
return mPreviousTapState.wasSuppressed() || wasTapCloseToPreviousTap(x, y); return wasTapCloseToPreviousTap(x, y);
} }
/** /**
* Determines whether a tap at the given coordinates is considered "close" to the previous * @return Whether a tap at the given coordinates is considered "close" to the previous tap.
* tap.
*/ */
private boolean wasTapCloseToPreviousTap(int x, int y) { private boolean wasTapCloseToPreviousTap(int x, int y) {
float deltaXDp = (mPreviousTapState.getX() - x) * mPxToDp; float deltaXDp = (mPreviousTapState.getX() - x) * mPxToDp;
......
...@@ -4,6 +4,8 @@ ...@@ -4,6 +4,8 @@
package org.chromium.chrome.browser.contextualsearch; package org.chromium.chrome.browser.contextualsearch;
import android.support.annotation.Nullable;
/** /**
* A set of {@link ContextualSearchHeuristic}s that support experimentation and logging. * A set of {@link ContextualSearchHeuristic}s that support experimentation and logging.
*/ */
...@@ -17,16 +19,20 @@ public class TapSuppressionHeuristics extends ContextualSearchHeuristics { ...@@ -17,16 +19,20 @@ public class TapSuppressionHeuristics extends ContextualSearchHeuristics {
* @param x The x position of the Tap. * @param x The x position of the Tap.
* @param y The y position of the Tap. * @param y The y position of the Tap.
* @param tapsSinceOpen the number of Tap gestures since the last open of the panel. * @param tapsSinceOpen the number of Tap gestures since the last open of the panel.
* @param contextualSearchContext The {@link ContextualSearchContext} of this tap.
* @param tapDurationMs The duration of this tap in milliseconds.
* @param wasSelectionEmptyBeforeTap Whether the selection was empty before this tap.
*/ */
TapSuppressionHeuristics(ContextualSearchSelectionController selectionController, TapSuppressionHeuristics(ContextualSearchSelectionController selectionController,
ContextualSearchTapState previousTapState, int x, int y, int tapsSinceOpen, @Nullable ContextualSearchTapState previousTapState, int x, int y, int tapsSinceOpen,
ContextualSearchContext contextualSearchContext, int tapDurationMs) { ContextualSearchContext contextualSearchContext, int tapDurationMs,
boolean wasSelectionEmptyBeforeTap) {
super(); super();
mCtrSuppression = new CtrSuppression(); mCtrSuppression = new CtrSuppression();
mHeuristics.add(mCtrSuppression); mHeuristics.add(mCtrSuppression);
mHeuristics.add(new RecentScrollTapSuppression(selectionController)); mHeuristics.add(new RecentScrollTapSuppression(selectionController));
mHeuristics.add( mHeuristics.add(new TapFarFromPreviousSuppression(
new TapFarFromPreviousSuppression(selectionController, previousTapState, x, y)); selectionController, previousTapState, x, y, wasSelectionEmptyBeforeTap));
mHeuristics.add(new TapDurationSuppression(tapDurationMs)); mHeuristics.add(new TapDurationSuppression(tapDurationMs));
mHeuristics.add(new TapWordLengthSuppression(contextualSearchContext)); mHeuristics.add(new TapWordLengthSuppression(contextualSearchContext));
mHeuristics.add(new TapWordEdgeSuppression(contextualSearchContext)); mHeuristics.add(new TapWordEdgeSuppression(contextualSearchContext));
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment