Skip to content

Commit

Permalink
Strip GestureDetector.
Browse files Browse the repository at this point in the history 
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 <twellington@chromium.org>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Commit-Queue: Tomasz Wiszkowski <ender@google.com>
Cr-Commit-Position: refs/heads/main@{#1187423}
  • Loading branch information
@tomasz-wiszkowski
tomasz-wiszkowski authored and Chromium LUCI CQ committed Aug 23, 2023
1 parent 2e99090 commit 795632a
Show file tree
Hide file tree
Showing 9 changed files with 17 additions and 98 deletions.
2 changes: 1 addition & 1 deletion ...id/java/src/org/chromium/chrome/browser/customtabs/features/toolbar/CustomTabToolbar.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1084,7 +1084,7 @@ public boolean allowKeyboardLearning() {
}

@Override
public void gestureDetected(boolean isLongPress) {}
public void onFocusByTouch() {}

// LocationBarDataProvider.Observer implementation
// Using the default empty onIncognitoStateChanged.
Expand Down
5 changes: 0 additions & 5 deletions .../android/omnibox/java/src/org/chromium/chrome/browser/omnibox/LocationBarCoordinator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
16 changes: 6 additions & 10 deletions .../ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/LocationBarMediator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -908,10 +909,6 @@ public void onAnimationEnd(Animator animation) {
mAutocompleteCoordinator.onTextChanged(textWithoutAutocomplete);
}

/* package */ boolean shouldClearOmniboxOnFocus() {
return mShouldClearOmniboxOnFocus;
}

// Private methods

private void setProfile(Profile profile) {
Expand Down Expand Up @@ -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.
Expand Down
6 changes: 2 additions & 4 deletions ...android/omnibox/java/src/org/chromium/chrome/browser/omnibox/LocationBarMediatorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
45 changes: 6 additions & 39 deletions chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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. */
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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
Expand Down
5 changes: 0 additions & 5 deletions ...roid/omnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/UrlBarDelegate.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
4 changes: 0 additions & 4 deletions ...c/org/chromium/chrome/browser/omnibox/suggestions/editurl/EditUrlSuggestionProcessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,6 @@ public boolean doesProcessSuggestion(AutocompleteMatch suggestion, int position)
return false;
}

if (!mHasClearedOmniboxForFocus && mUrlBarDelegate.shouldClearOmniboxOnFocus()) {
mHasClearedOmniboxForFocus = true;
mUrlBarDelegate.setOmniboxEditingText("");
}
return true;
}

Expand Down
30 changes: 1 addition & 29 deletions ...romium/chrome/browser/omnibox/suggestions/editurl/EditUrlSuggestionProcessorUnitTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
}

Expand All @@ -172,7 +167,7 @@ public void doesProcessSuggestion_rejectNonMatchingUrlWhatYouTyped() {
@Test
public void doesProcessSuggestion_acceptMatchingUrlWhatYouTyped() {
assertTrue(mProcessor.doesProcessSuggestion(mMatch, 0));
verify(mUrlBarDelegate).setOmniboxEditingText("");
verifyNoMoreInteractions(mUrlBarDelegate);
}

@Test
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion tools/metrics/histograms/enums.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78967,7 +78967,7 @@ Called by update_net_trust_anchors.py.-->

<enum name="OmniboxFocusReason">
<int value="0" label="Omnibox tapped."/>
<int value="1" label="Omnibox long-pressed."/>
<int value="1" label="Omnibox long-pressed (obsolete)."/>
<int value="2" label="NTP omnibox tapped."/>
<int value="3" label="NTP omnibox long-pressed (from paste action)."/>
<int value="4" label="Accelerator tapped."/>
Expand Down

0 comments on commit 795632a

Please sign in to comment.