From 795632a1e53a0b1cf760238b2d5c7cb392fc0f22 Mon Sep 17 00:00:00 2001 From: Tomasz Wiszkowski Date: Wed, 23 Aug 2023 19:22:52 +0000 Subject: [PATCH] Strip GestureDetector. The change removes GestureDetector used to differentiate between Omnibox taps and long-presses. Long-press gesture has lost all its meaning back when we introduced Search Ready Omnibox and Clipboard support: the omnibox comes up empty (meaning the user cannot use the gesture to select all its content), and cannot use this gesture to directly paste into the omnibox (it's never been directly supported; we have dedicated actions for that). As such, the long-press on the Omnibox does nothing else than focus the Omnibox. The change simplifies metrics reporting and obsoletes the OmniboxFocusReason.OmniboxLongClick action. The change further simplifies the focus flow by removing "set and reset omnibox text": upon Focus event, LocationBarMediator no longer sets the Omnibox text content (which next had to be cleared by the EditUrlSuggestionProcessor). Instead, the LocationBarMediator supplies an empty input right from the start. This works the same way the current logic does, skipping one extra content setting, and simplifying some of the Omnibox interfaces. Change-Id: I067d538c2c0f424603ed520f6d9fbd06c5f5da6f Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4803444 Reviewed-by: Theresa Sullivan Code-Coverage: findit-for-me@appspot.gserviceaccount.com Commit-Queue: Tomasz Wiszkowski Cr-Commit-Position: refs/heads/main@{#1187423} --- .../features/toolbar/CustomTabToolbar.java | 2 +- .../omnibox/LocationBarCoordinator.java | 5 --- .../browser/omnibox/LocationBarMediator.java | 16 +++---- .../omnibox/LocationBarMediatorTest.java | 6 +-- .../chrome/browser/omnibox/UrlBar.java | 45 +++---------------- .../omnibox/suggestions/UrlBarDelegate.java | 5 --- .../editurl/EditUrlSuggestionProcessor.java | 4 -- .../EditUrlSuggestionProcessorUnitTest.java | 30 +------------ tools/metrics/histograms/enums.xml | 2 +- 9 files changed, 17 insertions(+), 98 deletions(-) diff --git a/chrome/android/java/src/org/chromium/chrome/browser/customtabs/features/toolbar/CustomTabToolbar.java b/chrome/android/java/src/org/chromium/chrome/browser/customtabs/features/toolbar/CustomTabToolbar.java index d5cbb013d8cf8b..e30f33c206df09 100644 --- a/chrome/android/java/src/org/chromium/chrome/browser/customtabs/features/toolbar/CustomTabToolbar.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/customtabs/features/toolbar/CustomTabToolbar.java @@ -1084,7 +1084,7 @@ public boolean allowKeyboardLearning() { } @Override - public void gestureDetected(boolean isLongPress) {} + public void onFocusByTouch() {} // LocationBarDataProvider.Observer implementation // Using the default empty onIncognitoStateChanged. diff --git a/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/LocationBarCoordinator.java b/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/LocationBarCoordinator.java index 93202520ccf64b..87c5dcac997d89 100644 --- a/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/LocationBarCoordinator.java +++ b/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/LocationBarCoordinator.java @@ -462,11 +462,6 @@ public void setOmniboxEditingText(String text) { updateButtonVisibility(); } - @Override - public boolean shouldClearOmniboxOnFocus() { - return mLocationBarMediator.shouldClearOmniboxOnFocus(); - } - /** @see UrlBarCoordinator#getVisibleTextPrefixHint() */ public CharSequence getOmniboxVisibleTextPrefixHint() { return mUrlCoordinator.getVisibleTextPrefixHint(); diff --git a/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/LocationBarMediator.java b/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/LocationBarMediator.java index ba2605246a81b3..073d712a8c7f8d 100644 --- a/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/LocationBarMediator.java +++ b/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/LocationBarMediator.java @@ -275,9 +275,10 @@ public void setValue(LocationBarMediator object, float value) { if (hasFocus) { if (mNativeInitialized) RecordUserAction.record("FocusLocation"); - UrlBarData urlBarData = mLocationBarDataProvider.getUrlBarData(); - if (urlBarData.editingText != null) { - setUrlBarText(urlBarData, UrlBar.ScrollType.NO_SCROLL, SelectionState.SELECT_ALL); + // Don't clear Omnibox if the user just pasted text to NTP Omnibox. + if (mShouldClearOmniboxOnFocus) { + setUrlBarText( + UrlBarData.EMPTY, UrlBar.ScrollType.NO_SCROLL, SelectionState.SELECT_END); } } else { mUrlFocusedFromFakebox = false; @@ -908,10 +909,6 @@ public void onAnimationEnd(Animator animation) { mAutocompleteCoordinator.onTextChanged(textWithoutAutocomplete); } - /* package */ boolean shouldClearOmniboxOnFocus() { - return mShouldClearOmniboxOnFocus; - } - // Private methods private void setProfile(Profile profile) { @@ -1387,9 +1384,8 @@ public void backKeyPressed() { } @Override - public void gestureDetected(boolean isLongPress) { - recordOmniboxFocusReason(isLongPress ? OmniboxFocusReason.OMNIBOX_LONG_PRESS - : OmniboxFocusReason.OMNIBOX_TAP); + public void onFocusByTouch() { + recordOmniboxFocusReason(OmniboxFocusReason.OMNIBOX_TAP); } // BackPressHandler implementation. diff --git a/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/LocationBarMediatorTest.java b/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/LocationBarMediatorTest.java index 7f704caec4b4e7..be6d2829664c76 100644 --- a/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/LocationBarMediatorTest.java +++ b/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/LocationBarMediatorTest.java @@ -736,15 +736,13 @@ public void testSetUrlBarFocus_pastedText() { @Test public void testOnUrlFocusChange() { mMediator.addUrlFocusChangeListener(mUrlCoordinator); - UrlBarData urlBarData = UrlBarData.create(null, "text", 0, 0, "text"); - doReturn(urlBarData).when(mLocationBarDataProvider).getUrlBarData(); - mMediator.onUrlFocusChange(true); assertTrue(mMediator.isUrlBarFocused()); verify(mStatusCoordinator).setShouldAnimateIconChanges(true); verify(mUrlCoordinator) - .setUrlBarData(urlBarData, UrlBar.ScrollType.NO_SCROLL, SelectionState.SELECT_ALL); + .setUrlBarData( + UrlBarData.EMPTY, UrlBar.ScrollType.NO_SCROLL, SelectionState.SELECT_END); verify(mStatusCoordinator).onUrlFocusChange(true); verify(mUrlCoordinator).onUrlFocusChange(true); } diff --git a/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java b/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java index 9d9549a6097714..7e77097bf576cd 100644 --- a/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java +++ b/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java @@ -18,7 +18,6 @@ import android.text.TextUtils; import android.text.style.ReplacementSpan; import android.util.AttributeSet; -import android.view.GestureDetector; import android.view.KeyEvent; import android.view.MotionEvent; import android.view.View; @@ -39,7 +38,6 @@ import org.chromium.base.Log; import org.chromium.base.MathUtils; import org.chromium.base.SysUtils; -import org.chromium.base.ThreadUtils; import org.chromium.base.compat.ApiHelperForO; import org.chromium.base.metrics.RecordHistogram; import org.chromium.base.metrics.RecordUserAction; @@ -93,8 +91,6 @@ public abstract class UrlBar extends AutocompleteEditText { * The gesture detector is used to detect long presses. Long presses require special treatment * because the URL bar has custom touch event handling. See: {@link #onTouchEvent}. */ - private final GestureDetector mGestureDetector; - private final KeyboardHideHelper mKeyboardHideHelper; private boolean mFocused; @@ -165,10 +161,9 @@ public interface UrlBarDelegate { void backKeyPressed(); /** - * Called to notify that a tap or long press gesture has been detected. - * @param isLongPress Whether or not is a long press gesture. + * Called to notify that UrlBar has been focused by touch. */ - void gestureDetected(boolean isLongPress); + void onFocusByTouch(); } /** Provides updates about the URL text changes. */ @@ -221,24 +216,6 @@ public UrlBar(Context context, AttributeSet attrs) { setInputType(InputType.TYPE_CLASS_TEXT | InputType.TYPE_TEXT_FLAG_NO_SUGGESTIONS); - mGestureDetector = - new GestureDetector(getContext(), new GestureDetector.SimpleOnGestureListener() { - @Override - public void onLongPress(MotionEvent e) { - if (mUrlBarDelegate == null) return; - mUrlBarDelegate.gestureDetected(true); - performLongClick(); - } - - @Override - public boolean onSingleTapUp(MotionEvent e) { - if (mUrlBarDelegate == null) return true; - requestFocus(); - mUrlBarDelegate.gestureDetected(false); - return true; - } - }, ThreadUtils.getUiThreadHandler()); - mGestureDetector.setOnDoubleTapListener(null); mKeyboardHideHelper = new KeyboardHideHelper(this, () -> { if (mUrlBarDelegate != null && !BackPressManager.isEnabled()) { mUrlBarDelegate.backKeyPressed(); @@ -399,21 +376,11 @@ protected void onTextChanged(CharSequence text, int start, int lengthBefore, int @Override public boolean onTouchEvent(MotionEvent event) { - if (!mFocused) { - mGestureDetector.onTouchEvent(event); - return true; - } - - // Working around a platform bug (b/25562038) that was fixed in N that can throw - // NullPointerException during text selection. We let it happen rather than catching it - // since there can be a different issue here that we might want to know about. - try { - return super.onTouchEvent(event); - } catch (IndexOutOfBoundsException e) { - // Work around crash of unknown origin (https://crbug.com/837419). - Log.w(TAG, "Ignoring IndexOutOfBoundsException in UrlBar#onTouchEvent.", e); - return true; + if (!mFocused && event.getActionMasked() == MotionEvent.ACTION_UP + && mUrlBarDelegate != null) { + mUrlBarDelegate.onFocusByTouch(); } + return super.onTouchEvent(event); } @Override diff --git a/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/UrlBarDelegate.java b/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/UrlBarDelegate.java index 9b86ad18f71b1c..43da3a11c6b9b3 100644 --- a/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/UrlBarDelegate.java +++ b/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/UrlBarDelegate.java @@ -14,9 +14,4 @@ public interface UrlBarDelegate { * @param text The text that should be displayed in the omnibox. */ void setOmniboxEditingText(String text); - - /** - * Returns {@code true} when the omnibox should be cleared on focus, {@code false} otherwise. - */ - boolean shouldClearOmniboxOnFocus(); } diff --git a/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/editurl/EditUrlSuggestionProcessor.java b/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/editurl/EditUrlSuggestionProcessor.java index eb6041ebdd4e07..e7cbd504e77b21 100644 --- a/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/editurl/EditUrlSuggestionProcessor.java +++ b/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/editurl/EditUrlSuggestionProcessor.java @@ -77,10 +77,6 @@ public boolean doesProcessSuggestion(AutocompleteMatch suggestion, int position) return false; } - if (!mHasClearedOmniboxForFocus && mUrlBarDelegate.shouldClearOmniboxOnFocus()) { - mHasClearedOmniboxForFocus = true; - mUrlBarDelegate.setOmniboxEditingText(""); - } return true; } diff --git a/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/editurl/EditUrlSuggestionProcessorUnitTest.java b/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/editurl/EditUrlSuggestionProcessorUnitTest.java index 0e12b3266c9c0f..7243d8ee995ba8 100644 --- a/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/editurl/EditUrlSuggestionProcessorUnitTest.java +++ b/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/editurl/EditUrlSuggestionProcessorUnitTest.java @@ -8,7 +8,6 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.any; -import static org.mockito.Mockito.clearInvocations; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.eq; import static org.mockito.Mockito.never; @@ -22,7 +21,6 @@ import android.content.Context; import org.junit.After; -import org.junit.Assert; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -144,9 +142,6 @@ public void setUp() { doReturn(TAB_TITLE).when(mTab).getTitle(); doReturn(true).when(mTab).isInitialized(); - // This is used when the user long-presses the Location Bar to paste text in the Omnibox. - doReturn(true).when(mUrlBarDelegate).shouldClearOmniboxOnFocus(); - mProcessor.onUrlFocusChange(true); } @@ -172,7 +167,7 @@ public void doesProcessSuggestion_rejectNonMatchingUrlWhatYouTyped() { @Test public void doesProcessSuggestion_acceptMatchingUrlWhatYouTyped() { assertTrue(mProcessor.doesProcessSuggestion(mMatch, 0)); - verify(mUrlBarDelegate).setOmniboxEditingText(""); + verifyNoMoreInteractions(mUrlBarDelegate); } @Test @@ -220,29 +215,6 @@ public void doesProcessSuggestion_rejectSearchWhatYouTyped() { verify(mUrlBarDelegate, never()).setOmniboxEditingText(any()); } - @Test - public void doesProcessSuggestion_urlPasteDoesNotClearOmnibox() { - // When user initiates Paste via long-press, UrlBarDelegate asks us not to clear the - // omnibox. - doReturn(false).when(mUrlBarDelegate).shouldClearOmniboxOnFocus(); - Assert.assertTrue(mProcessor.doesProcessSuggestion(mMatch, 0)); - verify(mUrlBarDelegate, never()).setOmniboxEditingText(any()); - } - - @Test - public void doesProcessSuggestion_urlBarNotClearedUponUrlRetype() { - // We should only clear the url bar the first time the user focuses the Omnibox. If the - // suggestion (URL_WHAT_YOU_TYPED) comes again, it is 100% coming from the user (whether via - // paste or explicit re-type). Be sure we don't clear the omnibox. - Assert.assertTrue(mProcessor.doesProcessSuggestion(mMatch, 0)); - verify(mUrlBarDelegate).setOmniboxEditingText(any()); - - clearInvocations(mUrlBarDelegate); - - Assert.assertTrue(mProcessor.doesProcessSuggestion(mMatch, 0)); - verifyNoMoreInteractions(mUrlBarDelegate); - } - @Test public void populateModel_showInformationFromLoadedTab() { mProcessor.populateModel(mMatch, mModel, 0); diff --git a/tools/metrics/histograms/enums.xml b/tools/metrics/histograms/enums.xml index 1cb3648d7721ae..b38ea0e47b55a7 100644 --- a/tools/metrics/histograms/enums.xml +++ b/tools/metrics/histograms/enums.xml @@ -78967,7 +78967,7 @@ Called by update_net_trust_anchors.py.--> - +