Revert "Only consume tab navigation events in omnibox + autocomplete"
This reverts commit a09310c92020c29224b18ecdd3b4f70862da8de4.
Reason for revert:
LUCI Bisection has identified this change as the culprit of a build failure. See the analysis: https://ci.chromium.org/ui/p/chromium/bisection/compile-analysis/b/8701346283135995025
Sample failed build: https://ci.chromium.org/b/8701346283135995025
If this is a false positive, please report it at http://b.corp.google.com/createIssue?component=1199205&description=Analysis%3A+https%3A%2F%2Fchromium-review.googlesource.com%2Fc%2Fchromium%2Fsrc%2F%2B%2F7032703&format=PLAIN&priority=P3&title=Wrongly+blamed+https%3A%2F%2Fci.chromium.org%2Fui%2Fp%2Fchromium%2Fbisection%2Fcompile-analysis%2Fb%2F8701346283135995025&type=BUG
Original change's description:
> Only consume tab navigation events in omnibox + autocomplete
>
> Only process tab + shift-tab. If other modifiers are present, it might
> be a keyboard shortcut.
>
> Demo: https://screencast.googleplex.com/cast/NjQ1NDk0MTA4NTMzNTU1Mnw2MWM3NmExOS05Yg
>
> Fixed: 443766005
> Change-Id: I3ac23967d32da00574b9b4d2d2f851fbf4e46f45
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7032703
> Reviewed-by: Wenyu Fu <wenyufu@chromium.org>
> Commit-Queue: Jenna Himawan <jhimawan@google.com>
> Cr-Commit-Position: refs/heads/main@{#1528476}
>
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Change-Id: Ic1bb156160fd96a7bb9599da330ded5c669704df
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7033631
Commit-Queue: luci-bisection@appspot.gserviceaccount.com <luci-bisection@appspot.gserviceaccount.com>
Owners-Override: luci-bisection@appspot.gserviceaccount.com <luci-bisection@appspot.gserviceaccount.com>
Bot-Commit: luci-bisection@appspot.gserviceaccount.com <luci-bisection@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1528477}
diff --git a/chrome/browser/ui/android/omnibox/BUILD.gn b/chrome/browser/ui/android/omnibox/BUILD.gn
index 12c41be..088c8368 100644
--- a/chrome/browser/ui/android/omnibox/BUILD.gn
+++ b/chrome/browser/ui/android/omnibox/BUILD.gn
@@ -461,7 +461,6 @@
"java/src/org/chromium/chrome/browser/omnibox/styles/OmniboxImageSupplierUnitTest.java",
"java/src/org/chromium/chrome/browser/omnibox/styles/OmniboxResourceProviderTest.java",
"java/src/org/chromium/chrome/browser/omnibox/styles/SuggestionSpannableUnitTest.java",
- "java/src/org/chromium/chrome/browser/omnibox/suggestions/AutocompleteCoordinatorUnitTest.java",
"java/src/org/chromium/chrome/browser/omnibox/suggestions/AutocompleteMediatorUnitTest.java",
"java/src/org/chromium/chrome/browser/omnibox/suggestions/CachedZeroSuggestionsManagerUnitTest.java",
"java/src/org/chromium/chrome/browser/omnibox/suggestions/DropdownItemViewInfoListBuilderUnitTest.java",
diff --git a/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/AutocompleteCoordinator.java b/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/AutocompleteCoordinator.java
index d3945ef..f330b0e 100644
--- a/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/AutocompleteCoordinator.java
+++ b/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/AutocompleteCoordinator.java
@@ -5,7 +5,6 @@
package org.chromium.chrome.browser.omnibox.suggestions;
import static org.chromium.build.NullUtil.assumeNonNull;
-import static org.chromium.ui.base.KeyNavigationUtil.isTabNavigation;
import android.content.Context;
import android.os.Handler;
@@ -19,7 +18,6 @@
import org.chromium.base.Callback;
import org.chromium.base.ObserverList;
-import org.chromium.base.ResettersForTesting;
import org.chromium.base.supplier.ObservableSupplier;
import org.chromium.build.annotations.Initializer;
import org.chromium.build.annotations.NullMarked;
@@ -386,7 +384,7 @@
// Suggestion, simulating press/long press of the UI element.
if ((keyCode == KeyEvent.KEYCODE_DPAD_UP)
|| (keyCode == KeyEvent.KEYCODE_DPAD_DOWN)
- || isTabNavigation(event)) {
+ || (keyCode == KeyEvent.KEYCODE_TAB)) {
mMediator.allowPendingItemSelection();
assumeNonNull(mContainer).onKeyDown(keyCode, event);
return true;
@@ -454,12 +452,6 @@
return mContainer;
}
- public void setSuggestionsContainerForTest(OmniboxSuggestionsContainer container) {
- OmniboxSuggestionsContainer oldValue = mContainer;
- mContainer = container;
- ResettersForTesting.register(() -> mContainer = oldValue);
- }
-
/**
* @return The current receiving OnSuggestionsReceived events.
*/
diff --git a/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/AutocompleteCoordinatorUnitTest.java b/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/AutocompleteCoordinatorUnitTest.java
deleted file mode 100644
index a011d1b..0000000
--- a/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/AutocompleteCoordinatorUnitTest.java
+++ /dev/null
@@ -1,163 +0,0 @@
-// Copyright 2025 The Chromium Authors
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-package org.chromium.chrome.browser.omnibox.suggestions;
-
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
-import static org.mockito.Mockito.doReturn;
-import static org.mockito.Mockito.when;
-
-import android.content.Context;
-import android.view.ContextThemeWrapper;
-import android.view.KeyEvent;
-import android.view.ViewGroup;
-
-import androidx.test.core.app.ApplicationProvider;
-
-import org.junit.Before;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.mockito.Mock;
-import org.mockito.junit.MockitoJUnit;
-import org.mockito.junit.MockitoRule;
-
-import org.chromium.base.Callback;
-import org.chromium.base.supplier.ObservableSupplier;
-import org.chromium.base.supplier.ObservableSupplierImpl;
-import org.chromium.base.test.BaseRobolectricTestRunner;
-import org.chromium.chrome.browser.browser_controls.BrowserControlsStateProvider.ControlsPosition;
-import org.chromium.chrome.browser.lifecycle.ActivityLifecycleDispatcher;
-import org.chromium.chrome.browser.omnibox.DeferredIMEWindowInsetApplicationCallback;
-import org.chromium.chrome.browser.omnibox.LocationBarDataProvider;
-import org.chromium.chrome.browser.omnibox.UrlBarEditingTextStateProvider;
-import org.chromium.chrome.browser.omnibox.navattach.NavigationAttachmentsCoordinator;
-import org.chromium.chrome.browser.omnibox.suggestions.basic.BasicSuggestionProcessor;
-import org.chromium.chrome.browser.omnibox.test.R;
-import org.chromium.chrome.browser.profiles.Profile;
-import org.chromium.chrome.browser.share.ShareDelegate;
-import org.chromium.chrome.browser.tab.Tab;
-import org.chromium.chrome.browser.tabwindow.TabWindowManager;
-import org.chromium.components.omnibox.action.OmniboxActionDelegate;
-import org.chromium.ui.base.WindowAndroid;
-import org.chromium.ui.modaldialog.ModalDialogManager;
-
-import java.util.function.Supplier;
-
-/** Unit tests for {@link AutocompleteCoordinator}. */
-@RunWith(BaseRobolectricTestRunner.class)
-public class AutocompleteCoordinatorUnitTest {
- public @Rule MockitoRule mMockitoRule = MockitoJUnit.rule();
-
- private Context mContext;
- private AutocompleteCoordinator mAutocompleteCoordinator;
- private final ObservableSupplierImpl<@ControlsPosition Integer> mControlsPositionSupplier =
- new ObservableSupplierImpl<>();
-
- @Mock private AutocompleteDelegate mAutocompleteDelegate;
- @Mock private OmniboxSuggestionsDropdownEmbedder mDropdownEmbedder;
- @Mock private UrlBarEditingTextStateProvider mUrlBarEditingTextProvider;
- @Mock private Supplier<ModalDialogManager> mModalDialogManagerSupplier;
- @Mock private Supplier<Tab> mActivityTabSupplier;
- @Mock private Supplier<ShareDelegate> mShareDelegateSupplier;
- @Mock private LocationBarDataProvider mLocationBarDataProvider;
- @Mock private ObservableSupplier<Profile> mProfileObservableSupplier;
- @Mock private Callback<Tab> mBringToForegroundCallback;
- @Mock private Callback<String> mBringTabGroupToForegroundCallback;
- @Mock private Supplier<TabWindowManager> mTabWindowManagerSupplier;
- @Mock private BasicSuggestionProcessor.BookmarkState mBookmarkState;
- @Mock private OmniboxActionDelegate mOmniboxActionDelegate;
- @Mock private ActivityLifecycleDispatcher mLifecycleDispatcher;
- @Mock private WindowAndroid mWindowAndroid;
- @Mock private DeferredIMEWindowInsetApplicationCallback mDeferredImeInsetCb;
- @Mock private NavigationAttachmentsCoordinator mNavigationAttachmentsCoordinator;
- @Mock private OmniboxSuggestionsContainer mSuggestionsContainer;
- @Mock private ViewGroup mParentView;
-
- @Before
- public void setUp() {
- mContext =
- new ContextThemeWrapper(
- ApplicationProvider.getApplicationContext(),
- R.style.Theme_BrowserUI_DayNight);
- when(mParentView.getContext()).thenReturn(mContext);
- mControlsPositionSupplier.set(ControlsPosition.TOP);
- when(mLocationBarDataProvider.getToolbarPositionSupplier())
- .thenReturn(mControlsPositionSupplier);
-
- mAutocompleteCoordinator =
- new AutocompleteCoordinator(
- mParentView,
- mAutocompleteDelegate,
- mDropdownEmbedder,
- mUrlBarEditingTextProvider,
- mModalDialogManagerSupplier,
- mActivityTabSupplier,
- mShareDelegateSupplier,
- mLocationBarDataProvider,
- mProfileObservableSupplier,
- mBringToForegroundCallback,
- mBringTabGroupToForegroundCallback,
- mTabWindowManagerSupplier,
- mBookmarkState,
- mOmniboxActionDelegate,
- null,
- mLifecycleDispatcher,
- false,
- mWindowAndroid,
- mDeferredImeInsetCb,
- mNavigationAttachmentsCoordinator);
-
- mAutocompleteCoordinator.setSuggestionsContainerForTest(mSuggestionsContainer);
- }
-
- @Test
- public void testHandleKeyEvent() {
- // Suggestions are shown.
- doReturn(true).when(mSuggestionsContainer).isShown();
-
- // Tab navigation is handled.
- assertTrue(
- mAutocompleteCoordinator.handleKeyEvent(
- KeyEvent.KEYCODE_TAB,
- new KeyEvent(KeyEvent.ACTION_DOWN, KeyEvent.KEYCODE_TAB)));
-
- // Shift+Tab navigation is handled.
- assertTrue(
- mAutocompleteCoordinator.handleKeyEvent(
- KeyEvent.KEYCODE_TAB,
- new KeyEvent(
- 0,
- 0,
- KeyEvent.ACTION_DOWN,
- KeyEvent.KEYCODE_TAB,
- 0,
- KeyEvent.META_SHIFT_ON)));
-
- // Ctrl+Tab is not handled.
- assertFalse(
- mAutocompleteCoordinator.handleKeyEvent(
- KeyEvent.KEYCODE_TAB,
- new KeyEvent(
- 0,
- 0,
- KeyEvent.ACTION_DOWN,
- KeyEvent.KEYCODE_TAB,
- 0,
- KeyEvent.META_CTRL_ON)));
-
- // Alt+Tab is not handled.
- assertFalse(
- mAutocompleteCoordinator.handleKeyEvent(
- KeyEvent.KEYCODE_TAB,
- new KeyEvent(
- 0,
- 0,
- KeyEvent.ACTION_DOWN,
- KeyEvent.KEYCODE_TAB,
- 0,
- KeyEvent.META_ALT_ON)));
- }
-}
diff --git a/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/OmniboxSuggestionsDropdown.java b/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/OmniboxSuggestionsDropdown.java
index 47eeeca..4b70aef 100644
--- a/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/OmniboxSuggestionsDropdown.java
+++ b/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/OmniboxSuggestionsDropdown.java
@@ -5,7 +5,6 @@
package org.chromium.chrome.browser.omnibox.suggestions;
import static org.chromium.build.NullUtil.assumeNonNull;
-import static org.chromium.ui.base.KeyNavigationUtil.isTabNavigation;
import android.content.Context;
import android.content.res.Resources;
@@ -437,7 +436,7 @@
return true;
}
- if (isTabNavigation(event)) {
+ if (keyCode == KeyEvent.KEYCODE_TAB) {
boolean maybeProcessed = super.onKeyDown(keyCode, event);
if (maybeProcessed) return true;
if (event.isShiftPressed()) {
diff --git a/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/OmniboxSuggestionsDropdownUnitTest.java b/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/OmniboxSuggestionsDropdownUnitTest.java
index 330a426..03139f6 100644
--- a/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/OmniboxSuggestionsDropdownUnitTest.java
+++ b/chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/OmniboxSuggestionsDropdownUnitTest.java
@@ -10,17 +10,13 @@
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.ArgumentMatchers.anyInt;
-import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
-import static org.mockito.Mockito.when;
import android.content.Context;
import android.view.ContextThemeWrapper;
import android.view.Gravity;
-import android.view.KeyEvent;
import android.view.View;
import android.widget.FrameLayout;
@@ -50,7 +46,6 @@
private @Mock Runnable mDropdownScrollListener;
private @Mock Runnable mDropdownScrollToTopListener;
private @Mock OmniboxSuggestionsDropdownAdapter mAdapter;
- private @Mock View mView;
private Context mContext;
private OmniboxSuggestionsDropdown mDropdown;
@@ -63,11 +58,10 @@
new ContextThemeWrapper(
ApplicationProvider.getApplicationContext(),
R.style.Theme_BrowserUI_DayNight);
- mListener = spy(new OmniboxSuggestionsDropdown.SuggestionLayoutScrollListener(mContext));
- when(mListener.getItemCount()).thenReturn(3);
- when(mListener.findViewByPosition(anyInt())).thenReturn(mView);
- when(mView.isFocusable()).thenReturn(true);
- mDropdown = spy(new OmniboxSuggestionsDropdown(mContext, null, mListener));
+ mListener =
+ Mockito.spy(
+ new OmniboxSuggestionsDropdown.SuggestionLayoutScrollListener(mContext));
+ mDropdown = new OmniboxSuggestionsDropdown(mContext, null, mListener);
mDropdown.setId(R.id.omnibox_suggestions_dropdown);
mDropdown.setAdapter(mAdapter);
@@ -309,77 +303,4 @@
assertFalse(mDropdown.getToolbarOnTopForTesting());
assertEquals(Gravity.BOTTOM, mLayoutParams.gravity);
}
-
- @Test
- public void onKeyDown_beforeShownDoesNotHandleTabNavigation() {
- when(mDropdown.isShown()).thenReturn(false);
- assertFalse(
- mDropdown.onKeyDown(
- KeyEvent.KEYCODE_TAB,
- new KeyEvent(KeyEvent.ACTION_DOWN, KeyEvent.KEYCODE_TAB)));
- assertFalse(
- mDropdown.onKeyDown(
- KeyEvent.KEYCODE_TAB,
- new KeyEvent(
- 0,
- 0,
- KeyEvent.ACTION_DOWN,
- KeyEvent.KEYCODE_TAB,
- 0,
- KeyEvent.META_SHIFT_ON)));
- }
-
- @Test
- public void onKeyDown_handlesTabNavigationEvents() {
- when(mDropdown.isShown()).thenReturn(true);
-
- // Tab should be handled the first time to put focus on the first item.
- assertTrue(
- mDropdown.onKeyDown(
- KeyEvent.KEYCODE_TAB,
- new KeyEvent(KeyEvent.ACTION_DOWN, KeyEvent.KEYCODE_TAB)));
-
- // Tab should be handled to move to the next item.
- assertTrue(
- mDropdown.onKeyDown(
- KeyEvent.KEYCODE_TAB,
- new KeyEvent(KeyEvent.ACTION_DOWN, KeyEvent.KEYCODE_TAB)));
-
- // Shift+Tab should be handled to bring focus back to the first item.
- assertTrue(
- mDropdown.onKeyDown(
- KeyEvent.KEYCODE_TAB,
- new KeyEvent(
- 0,
- 0,
- KeyEvent.ACTION_DOWN,
- KeyEvent.KEYCODE_TAB,
- 0,
- KeyEvent.META_SHIFT_ON)));
-
- // Other modifiers should be ignored.
- // We expect super.onKeyDown to be called, which we can't directly verify.
- // But we can verify that our adapter methods are not called.
- // And we can check the return value. Let's assume super.onKeyDown returns false.
- assertFalse(
- mDropdown.onKeyDown(
- KeyEvent.KEYCODE_TAB,
- new KeyEvent(
- 0,
- 0,
- KeyEvent.ACTION_DOWN,
- KeyEvent.KEYCODE_TAB,
- 0,
- KeyEvent.META_CTRL_ON)));
- assertFalse(
- mDropdown.onKeyDown(
- KeyEvent.KEYCODE_TAB,
- new KeyEvent(
- 0,
- 0,
- KeyEvent.ACTION_DOWN,
- KeyEvent.KEYCODE_TAB,
- 0,
- KeyEvent.META_ALT_ON)));
- }
}
diff --git a/ui/android/java/src/org/chromium/ui/base/KeyNavigationUtil.java b/ui/android/java/src/org/chromium/ui/base/KeyNavigationUtil.java
index 5598d2a..2ccdab64 100644
--- a/ui/android/java/src/org/chromium/ui/base/KeyNavigationUtil.java
+++ b/ui/android/java/src/org/chromium/ui/base/KeyNavigationUtil.java
@@ -80,28 +80,6 @@
|| (isGoRight(event) && LocalizationUtils.isLayoutRtl());
}
- /** Returns whether {@param event} is tab navigation forward. */
- public static boolean isTabForward(KeyEvent event) {
- return isActionDown(event)
- && event.getKeyCode() == KeyEvent.KEYCODE_TAB
- && event.hasNoModifiers();
- }
-
- /** Returns whether {@param event} is tab navigation backward. */
- public static boolean isTabBackward(KeyEvent event) {
- return isActionDown(event)
- && event.getKeyCode() == KeyEvent.KEYCODE_TAB
- &&
- // The only modifier we allow is shift.
- // If it has other modifiers, it might be a keyboard shortcut, not tab navigation.
- event.getModifiers() == KeyEvent.META_SHIFT_ON;
- }
-
- /** Returns whether {@param event} is tab navigation (tab or shift+tab, no other modifiers). */
- public static boolean isTabNavigation(KeyEvent event) {
- return isTabForward(event) || isTabBackward(event);
- }
-
/**
* Whether this event should move keyboard focus in the forward direction.
*
@@ -109,7 +87,9 @@
* @return Whether this event should move keyboard focus in the forward direction.
*/
public static boolean isMoveFocusForward(KeyEvent event) {
- return isActionDown(event) && (isGoForward(event) || isTabForward(event));
+ return isActionDown(event)
+ && (isGoForward(event)
+ || (event.getKeyCode() == KeyEvent.KEYCODE_TAB && !event.isShiftPressed()));
}
/**
@@ -119,7 +99,9 @@
* @return Whether this event should move keyboard focus in the backward direction.
*/
public static boolean isMoveFocusBackward(KeyEvent event) {
- return isActionDown(event) && (isGoBackward(event) || isTabBackward(event));
+ return isActionDown(event)
+ && (isGoBackward(event)
+ || (event.getKeyCode() == KeyEvent.KEYCODE_TAB && event.isShiftPressed()));
}
/**
