Commit 5c9a5ad8 authored by Donn Denman's avatar Donn Denman Committed by Commit Bot

[TTS] Add highlight of the tapped word on IPH show

Updates the UX for showing the in product help balloon for promoting
a longpress when switching away from tap.  When the user taps and
we show the balloon we want to also highlight the word tapped.

BUG=1096323

Change-Id: Ic048c4b5b410fb4389c60c0a5437dab3f493c45c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2266018
Commit-Queue: Donn Denman <donnd@chromium.org>
Reviewed-by: default avatarTheresa  <twellington@chromium.org>
Reviewed-by: default avatarMehran Mahmoudi <mahmoudi@chromium.org>
Auto-Submit: Donn Denman <donnd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#782678}
parent 98896435
...@@ -8,6 +8,7 @@ import android.graphics.Point; ...@@ -8,6 +8,7 @@ import android.graphics.Point;
import android.graphics.Rect; import android.graphics.Rect;
import android.text.TextUtils; import android.text.TextUtils;
import android.view.View; import android.view.View;
import android.widget.PopupWindow.OnDismissListener;
import org.chromium.chrome.R; import org.chromium.chrome.R;
import org.chromium.chrome.browser.compositor.bottombar.contextualsearch.ContextualSearchPanel; import org.chromium.chrome.browser.compositor.bottombar.contextualsearch.ContextualSearchPanel;
...@@ -34,6 +35,7 @@ public class ContextualSearchIPH { ...@@ -34,6 +35,7 @@ public class ContextualSearchIPH {
private boolean mIsPositionedByPanel; private boolean mIsPositionedByPanel;
private boolean mHasUserEverEngaged; private boolean mHasUserEverEngaged;
private Point mFloatingBubbleAnchorPoint; private Point mFloatingBubbleAnchorPoint;
private OnDismissListener mDismissListener;
/** /**
* Constructs the helper class. * Constructs the helper class.
...@@ -83,10 +85,13 @@ public class ContextualSearchIPH { ...@@ -83,10 +85,13 @@ public class ContextualSearchIPH {
* @param bubbleAnchorPoint The point where the bubble arrow should be positioned. * @param bubbleAnchorPoint The point where the bubble arrow should be positioned.
* @param hasUserEverEngaged Whether the user has ever engaged Contextual Search by opening * @param hasUserEverEngaged Whether the user has ever engaged Contextual Search by opening
* the panel. * the panel.
* @param dismissListener An {@link OnDismissListener} to call when the bubble is dismissed.
*/ */
void onNonTriggeringTap(Profile profile, Point bubbleAnchorPoint, boolean hasUserEverEngaged) { void onNonTriggeringTap(Profile profile, Point bubbleAnchorPoint, boolean hasUserEverEngaged,
OnDismissListener dismissListener) {
mFloatingBubbleAnchorPoint = bubbleAnchorPoint; mFloatingBubbleAnchorPoint = bubbleAnchorPoint;
mHasUserEverEngaged = hasUserEverEngaged; mHasUserEverEngaged = hasUserEverEngaged;
mDismissListener = dismissListener;
maybeShow(FeatureConstants.CONTEXTUAL_SEARCH_TAPPED_BUT_SHOULD_LONGPRESS_FEATURE, profile, maybeShow(FeatureConstants.CONTEXTUAL_SEARCH_TAPPED_BUT_SHOULD_LONGPRESS_FEATURE, profile,
false); false);
} }
...@@ -164,6 +169,10 @@ public class ContextualSearchIPH { ...@@ -164,6 +169,10 @@ public class ContextualSearchIPH {
mIsShowing = false; mIsShowing = false;
mHelpBubble = null; mHelpBubble = null;
}); });
if (mDismissListener != null) {
mHelpBubble.addOnDismissListener(mDismissListener);
mDismissListener = null;
}
mHelpBubble.show(); mHelpBubble.show();
mIsShowing = true; mIsShowing = true;
...@@ -185,6 +194,9 @@ public class ContextualSearchIPH { ...@@ -185,6 +194,9 @@ public class ContextualSearchIPH {
int yInsetPx = mParentView.getResources().getDimensionPixelOffset( int yInsetPx = mParentView.getResources().getDimensionPixelOffset(
R.dimen.contextual_search_bubble_y_inset); R.dimen.contextual_search_bubble_y_inset);
if (!mIsPositionedByPanel) { if (!mIsPositionedByPanel) {
// Position the bubble to point to the tap location, since there's no panel, just a
// selected word. It would be better to point to the rectangle of the selected word,
// but that's not easy to get.
return new Rect(mFloatingBubbleAnchorPoint.x, mFloatingBubbleAnchorPoint.y, return new Rect(mFloatingBubbleAnchorPoint.x, mFloatingBubbleAnchorPoint.y,
mFloatingBubbleAnchorPoint.x, mFloatingBubbleAnchorPoint.y); mFloatingBubbleAnchorPoint.x, mFloatingBubbleAnchorPoint.y);
} }
...@@ -205,6 +217,15 @@ public class ContextualSearchIPH { ...@@ -205,6 +217,15 @@ public class ContextualSearchIPH {
mIsShowing = false; mIsShowing = false;
} }
/**
* @return whether the bubble is currently showing for the tap-where-longpress-needed promo.
*/
boolean isShowingForTappedButShouldLongpress() {
return mIsShowing
&& FeatureConstants.CONTEXTUAL_SEARCH_TAPPED_BUT_SHOULD_LONGPRESS_FEATURE.equals(
mFeatureName);
}
/** /**
* Notifies the Feature Engagement backend and logs UMA metrics. * Notifies the Feature Engagement backend and logs UMA metrics.
* @param profile The current {@link Profile}. * @param profile The current {@link Profile}.
......
...@@ -205,6 +205,8 @@ class ContextualSearchInternalStateController { ...@@ -205,6 +205,8 @@ class ContextualSearchInternalStateController {
/** /**
* Enters the given starting state immediately. * Enters the given starting state immediately.
* Note: This will synchronously complete the given state and process all subsequent
* non-asynchronous states before returning. See https://crbug.com/1099383.
* @param state The new starting {@link InternalState} we're now in. * @param state The new starting {@link InternalState} we're now in.
*/ */
void enter(@InternalState int state) { void enter(@InternalState int state) {
...@@ -249,7 +251,8 @@ class ContextualSearchInternalStateController { ...@@ -249,7 +251,8 @@ class ContextualSearchInternalStateController {
} }
/** /**
* Confirms that work has been finished on the given state. * Confirms that work has been finished on the given state, and will process all subsequent
* non-asynchronous states before returning. See https://crbug.com/1099383.
* This should be called by every operation that waits for some kind of completion when it * This should be called by every operation that waits for some kind of completion when it
* completes. The operation's start must be flagged using {@link #notifyStartingWorkOn}. * completes. The operation's start must be flagged using {@link #notifyStartingWorkOn}.
* @param state The {@link InternalState} that we've finished working on. * @param state The {@link InternalState} that we've finished working on.
......
...@@ -1323,7 +1323,9 @@ public class ContextualSearchManager ...@@ -1323,7 +1323,9 @@ public class ContextualSearchManager
return; return;
} }
if (didSelect) { // Process normally unless something went wrong with the selection or an IPH triggered
// on tap when promoting longpress, otherwise just finish up.
if (didSelect && !mInProductHelp.isShowingForTappedButShouldLongpress()) {
assert mContext != null; assert mContext != null;
mContext.onSelectionAdjusted(startAdjust, endAdjust); mContext.onSelectionAdjusted(startAdjust, endAdjust);
// There's a race condition when we select the word between this Ack response and // There's a race condition when we select the word between this Ack response and
...@@ -1448,7 +1450,6 @@ public class ContextualSearchManager ...@@ -1448,7 +1450,6 @@ public class ContextualSearchManager
public void handleValidTap(int x, int y) { public void handleValidTap(int x, int y) {
if (isSuppressed()) return; if (isSuppressed()) return;
mInternalStateController.enter(InternalState.TAP_RECOGNIZED);
if (!mPolicy.isTapSupported() && mPolicy.canResolveLongpress()) { if (!mPolicy.isTapSupported() && mPolicy.canResolveLongpress()) {
// User tapped when Longpress is needed. Convert location to screen coordinates, and // User tapped when Longpress is needed. Convert location to screen coordinates, and
// put up some in-product help. // put up some in-product help.
...@@ -1457,8 +1458,13 @@ public class ContextualSearchManager ...@@ -1457,8 +1458,13 @@ public class ContextualSearchManager
mParentView.getLocationInWindow(parentScreenXy); mParentView.getLocationInWindow(parentScreenXy);
mInProductHelp.onNonTriggeringTap(Profile.getLastUsedRegularProfile(), mInProductHelp.onNonTriggeringTap(Profile.getLastUsedRegularProfile(),
new Point(x + parentScreenXy[0], y + yOffset + parentScreenXy[1]), new Point(x + parentScreenXy[0], y + yOffset + parentScreenXy[1]),
new CtrSuppression().getPrevious28DayCtr() > 0); new CtrSuppression().getPrevious28DayCtr() > 0,
() -> mSelectionController.clearSelection());
} }
// This will synchronously advance to the next state (and possibly others) before
// returning.
mInternalStateController.enter(InternalState.TAP_RECOGNIZED);
} }
@Override @Override
...@@ -1573,7 +1579,11 @@ public class ContextualSearchManager ...@@ -1573,7 +1579,11 @@ public class ContextualSearchManager
if (isSearchPanelShowing()) { if (isSearchPanelShowing()) {
mSearchPanel.closePanel(reason, false); mSearchPanel.closePanel(reason, false);
} else { } else {
if (mSelectionController.getSelectionType() == SelectionType.TAP) { // Also clear any tap-based selection unless the Tap IPH is showing. In the
// latter case we preserve the selection so the help bubble has something to
// point to.
if (mSelectionController.getSelectionType() == SelectionType.TAP
&& !mInProductHelp.isShowingForTappedButShouldLongpress()) {
mSelectionController.clearSelection(); mSelectionController.clearSelection();
} }
} }
...@@ -1625,6 +1635,7 @@ public class ContextualSearchManager ...@@ -1625,6 +1635,7 @@ public class ContextualSearchManager
public void tapGestureCommit() { public void tapGestureCommit() {
mInternalStateController.notifyStartingWorkOn(InternalState.TAP_GESTURE_COMMIT); mInternalStateController.notifyStartingWorkOn(InternalState.TAP_GESTURE_COMMIT);
if (!mPolicy.isTapSupported() if (!mPolicy.isTapSupported()
&& !mInProductHelp.isShowingForTappedButShouldLongpress()
|| mSelectionController.getSelectionType() || mSelectionController.getSelectionType()
== SelectionType.RESOLVING_LONG_PRESS) { == SelectionType.RESOLVING_LONG_PRESS) {
hideContextualSearch(StateChangeReason.UNKNOWN); hideContextualSearch(StateChangeReason.UNKNOWN);
...@@ -1647,6 +1658,16 @@ public class ContextualSearchManager ...@@ -1647,6 +1658,16 @@ public class ContextualSearchManager
@Override @Override
public void decideSuppression() { public void decideSuppression() {
mInternalStateController.notifyStartingWorkOn(InternalState.DECIDING_SUPPRESSION); mInternalStateController.notifyStartingWorkOn(InternalState.DECIDING_SUPPRESSION);
// We may have gotten here even without Tap being supported if an IPH for Tap
// is active. In that case we want to be sure to show, so skip the suppression
// decision.
if (mInProductHelp.isShowingForTappedButShouldLongpress()) {
mInternalStateController.notifyFinishedWorkOn(
InternalState.DECIDING_SUPPRESSION);
return;
}
// TODO(donnd): Move handleShouldSuppressTap out of the Selection Controller. // TODO(donnd): Move handleShouldSuppressTap out of the Selection Controller.
mSelectionController.handleShouldSuppressTap(mContext, mInteractionRecorder); mSelectionController.handleShouldSuppressTap(mContext, mInteractionRecorder);
} }
......
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