Commit 6630c45c authored by Yue Zhang's avatar Yue Zhang Committed by Commit Bot

Hide tab strip when omnibox has focus

Since TabGridDialog and omnibox both use the ScrimView from
RootUiCoordinator, there will be a conflict when user try to open
dialog from tab strip when omnibox is focused. This CL fixes this issue
by suppressing tab strip when omnibox is focused and restoring
visibility when omnibox loses focus.

Bug: 1096358
Change-Id: I3b12fe1f1753a10df691af61ac23a6ff9a0ea61e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2486622Reviewed-by: default avatarYusuf Ozuysal <yusufo@chromium.org>
Reviewed-by: default avatarWei-Yin Chen (陳威尹) <wychen@chromium.org>
Commit-Queue: Yue Zhang <yuezhanggg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#820091}
parent 2333fd21
......@@ -12,6 +12,7 @@ import android.view.View;
import android.view.ViewGroup;
import org.chromium.base.metrics.RecordHistogram;
import org.chromium.base.supplier.ObservableSupplier;
import org.chromium.base.supplier.Supplier;
import org.chromium.chrome.browser.ChromeTabbedActivity;
import org.chromium.chrome.browser.app.ChromeActivity;
......@@ -56,6 +57,7 @@ public class TabGroupUiCoordinator implements TabGroupUiMediator.ResetHandler, T
private final TabGroupUiToolbarView mToolbarView;
private final ViewGroup mTabListContainerView;
private final ScrimCoordinator mScrimCoordinator;
private final ObservableSupplier<Boolean> mOmniboxFocusStateSupplier;
private PropertyModelChangeProcessor mModelChangeProcessor;
private TabGridDialogCoordinator mTabGridDialogCoordinator;
private TabListCoordinator mTabStripCoordinator;
......@@ -67,10 +69,12 @@ public class TabGroupUiCoordinator implements TabGroupUiMediator.ResetHandler, T
* Creates a new {@link TabGroupUiCoordinator}
*/
public TabGroupUiCoordinator(ViewGroup parentView, ThemeColorProvider themeColorProvider,
ScrimCoordinator scrimCoordinator) {
ScrimCoordinator scrimCoordinator,
ObservableSupplier<Boolean> omniboxFocusStateSupplier) {
mContext = parentView.getContext();
mThemeColorProvider = themeColorProvider;
mScrimCoordinator = scrimCoordinator;
mOmniboxFocusStateSupplier = omniboxFocusStateSupplier;
mModel = new PropertyModel(TabGroupUiProperties.ALL_KEYS);
mToolbarView = (TabGroupUiToolbarView) LayoutInflater.from(mContext).inflate(
R.layout.bottom_tab_strip_toolbar, parentView, false);
......@@ -121,7 +125,8 @@ public class TabGroupUiCoordinator implements TabGroupUiMediator.ResetHandler, T
mMediator = new TabGroupUiMediator(activity, visibilityController, this, mModel,
tabModelSelector, activity,
((ChromeTabbedActivity) activity).getOverviewModeBehaviorSupplier(),
mThemeColorProvider, dialogController, activity.getLifecycleDispatcher(), activity);
mThemeColorProvider, dialogController, activity.getLifecycleDispatcher(), activity,
mOmniboxFocusStateSupplier);
TabGroupUtils.startObservingForCreationIPH();
......
......@@ -14,9 +14,11 @@ import android.view.View;
import androidx.annotation.Nullable;
import androidx.annotation.VisibleForTesting;
import org.chromium.base.Callback;
import org.chromium.base.CallbackController;
import org.chromium.base.metrics.RecordHistogram;
import org.chromium.base.metrics.RecordUserAction;
import org.chromium.base.supplier.ObservableSupplier;
import org.chromium.base.supplier.OneshotSupplier;
import org.chromium.chrome.browser.compositor.layouts.EmptyOverviewModeObserver;
import org.chromium.chrome.browser.compositor.layouts.OverviewModeBehavior;
......@@ -112,6 +114,7 @@ public class TabGroupUiMediator implements SnackbarManager.SnackbarController {
private final ActivityLifecycleDispatcher mActivityLifecycleDispatcher;
private final SnackbarManager.SnackbarManageable mSnackbarManageable;
private final Snackbar mUndoClosureSnackBar;
private final ObservableSupplier<Boolean> mOmniboxFocusStateSupplier;
private CallbackController mCallbackController = new CallbackController();
private final OverviewModeBehavior.OverviewModeObserver mOverviewModeObserver;
......@@ -120,6 +123,7 @@ public class TabGroupUiMediator implements SnackbarManager.SnackbarController {
private TabGroupModelFilter.Observer mTabGroupModelFilterObserver;
private PauseResumeWithNativeObserver mPauseResumeWithNativeObserver;
private TabModelSelectorTabObserver mTabModelSelectorTabObserver;
private Callback<Boolean> mOmniboxFocusObserver;
private boolean mIsTabGroupUiVisible;
private boolean mIsShowingOverViewMode;
private boolean mActivatedButNotShown;
......@@ -132,7 +136,8 @@ public class TabGroupUiMediator implements SnackbarManager.SnackbarController {
ThemeColorProvider themeColorProvider,
@Nullable TabGridDialogMediator.DialogController dialogController,
ActivityLifecycleDispatcher activityLifecycleDispatcher,
SnackbarManager.SnackbarManageable snackbarManageable) {
SnackbarManager.SnackbarManageable snackbarManageable,
ObservableSupplier<Boolean> omniboxFocusStateSupplier) {
mContext = context;
mResetHandler = resetHandler;
mModel = model;
......@@ -143,6 +148,7 @@ public class TabGroupUiMediator implements SnackbarManager.SnackbarController {
mTabGridDialogController = dialogController;
mActivityLifecycleDispatcher = activityLifecycleDispatcher;
mSnackbarManageable = snackbarManageable;
mOmniboxFocusStateSupplier = omniboxFocusStateSupplier;
mUndoClosureSnackBar =
Snackbar.make(context.getString(R.string.undo_tab_strip_closure_message), this,
Snackbar.TYPE_ACTION,
......@@ -333,6 +339,17 @@ public class TabGroupUiMediator implements SnackbarManager.SnackbarController {
mActivityLifecycleDispatcher.register(mPauseResumeWithNativeObserver);
}
if (TabUiFeatureUtilities.isLaunchBugFixEnabled()) {
mOmniboxFocusObserver = isFocus -> {
// Hide tab strip when omnibox gains focus and try to re-show it when omnibox loses
// focus.
int tabId = (isFocus == null || !isFocus) ? mTabModelSelector.getCurrentTabId()
: Tab.INVALID_TAB_ID;
resetTabStripWithRelatedTabsForId(tabId);
};
mOmniboxFocusStateSupplier.addObserver(mOmniboxFocusObserver);
}
mThemeColorObserver =
(color, shouldAnimate) -> mModel.set(TabGroupUiProperties.PRIMARY_COLOR, color);
mTintObserver = (tint, useLight) -> mModel.set(TabGroupUiProperties.TINT, tint);
......@@ -524,6 +541,9 @@ public class TabGroupUiMediator implements SnackbarManager.SnackbarController {
mCallbackController.destroy();
mCallbackController = null;
}
if (mOmniboxFocusObserver != null) {
mOmniboxFocusStateSupplier.removeObserver(mOmniboxFocusObserver);
}
mThemeColorProvider.removeThemeColorObserver(mThemeColorObserver);
mThemeColorProvider.removeTintObserver(mTintObserver);
}
......
......@@ -88,10 +88,12 @@ public interface TabManagementDelegate {
* @param parentView The parent view of this UI.
* @param themeColorProvider The {@link ThemeColorProvider} for this UI.
* @param scrimCoordinator The {@link ScrimCoordinator} to control scrim view.
* @param omniboxFocusStateSupplier Supplier to access the focus state of the omnibox.
* @return The {@link TabGroupUi}.
*/
TabGroupUi createTabGroupUi(ViewGroup parentView, ThemeColorProvider themeColorProvider,
ScrimCoordinator scrimCoordinator);
ScrimCoordinator scrimCoordinator,
ObservableSupplier<Boolean> omniboxFocusStateSupplier);
/**
* Create the {@link StartSurfaceLayout}.
......
......@@ -80,8 +80,10 @@ public class TabManagementDelegateImpl implements TabManagementDelegate {
@Override
public TabGroupUi createTabGroupUi(ViewGroup parentView, ThemeColorProvider themeColorProvider,
ScrimCoordinator scrimCoordinator) {
return new TabGroupUiCoordinator(parentView, themeColorProvider, scrimCoordinator);
ScrimCoordinator scrimCoordinator,
ObservableSupplier<Boolean> omniboxFocusStateSupplier) {
return new TabGroupUiCoordinator(
parentView, themeColorProvider, scrimCoordinator, omniboxFocusStateSupplier);
}
@Override
......
......@@ -6,7 +6,11 @@ package org.chromium.chrome.browser.tasks.tab_management;
import static androidx.test.espresso.Espresso.onView;
import static androidx.test.espresso.action.ViewActions.click;
import static androidx.test.espresso.assertion.ViewAssertions.matches;
import static androidx.test.espresso.matcher.ViewMatchers.Visibility.INVISIBLE;
import static androidx.test.espresso.matcher.ViewMatchers.Visibility.VISIBLE;
import static androidx.test.espresso.matcher.ViewMatchers.isCompletelyDisplayed;
import static androidx.test.espresso.matcher.ViewMatchers.isDescendantOfA;
import static androidx.test.espresso.matcher.ViewMatchers.withEffectiveVisibility;
import static androidx.test.espresso.matcher.ViewMatchers.withId;
import static androidx.test.espresso.matcher.ViewMatchers.withParent;
......@@ -20,8 +24,12 @@ import static org.chromium.chrome.browser.tasks.tab_management.TabUiTestHelper.c
import static org.chromium.chrome.browser.tasks.tab_management.TabUiTestHelper.clickNthTabInDialog;
import static org.chromium.chrome.browser.tasks.tab_management.TabUiTestHelper.createTabs;
import static org.chromium.chrome.browser.tasks.tab_management.TabUiTestHelper.enterTabSwitcher;
import static org.chromium.chrome.browser.tasks.tab_management.TabUiTestHelper.finishActivity;
import static org.chromium.chrome.browser.tasks.tab_management.TabUiTestHelper.mergeAllNormalTabsToAGroup;
import static org.chromium.chrome.browser.tasks.tab_management.TabUiTestHelper.verifyTabSwitcherCardCount;
import static org.chromium.chrome.features.start_surface.InstantStartTest.createTabStateFile;
import static org.chromium.chrome.features.start_surface.InstantStartTest.createThumbnailBitmapAndWriteToFile;
import static org.chromium.chrome.test.util.ViewUtils.waitForView;
import android.view.ViewGroup;
......@@ -38,7 +46,9 @@ import org.chromium.base.test.util.Feature;
import org.chromium.base.test.util.Restriction;
import org.chromium.chrome.browser.ChromeTabbedActivity;
import org.chromium.chrome.browser.compositor.layouts.Layout;
import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.flags.ChromeSwitches;
import org.chromium.chrome.browser.tasks.pseudotab.TabAttributeCache;
import org.chromium.chrome.features.start_surface.StartSurfaceLayout;
import org.chromium.chrome.tab_ui.R;
import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
......@@ -61,6 +71,9 @@ import java.util.concurrent.atomic.AtomicReference;
public class TabGroupUiTest {
// clang-format on
private static final String TAB_GROUP_LAUNCH_BUG_FIX_PARAMS =
"force-fieldtrial-params=Study.Group:enable_launch_bug_fix/true";
@Rule
public ChromeTabbedActivityTestRule mActivityTestRule = new ChromeTabbedActivityTestRule();
......@@ -152,4 +165,34 @@ public class TabGroupUiTest {
.perform(click());
mRenderTestRule.render(recyclerViewReference.get(), "11th_tab_selected");
}
@Test
@MediumTest
// clang-format off
@Features.EnableFeatures({ChromeFeatureList.TAB_GROUPS_CONTINUATION_ANDROID,
ChromeFeatureList.TAB_GRID_LAYOUT_ANDROID + "<Study"})
@CommandLineFlags.Add({"force-fieldtrials=Study/Group", TAB_GROUP_LAUNCH_BUG_FIX_PARAMS})
public void testVisibilityChangeWithOmnibox() throws Exception {
// clang-format on
// Create a tab group with 2 tabs.
finishActivity(mActivityTestRule.getActivity());
createThumbnailBitmapAndWriteToFile(0);
createThumbnailBitmapAndWriteToFile(1);
TabAttributeCache.setRootIdForTesting(0, 0);
TabAttributeCache.setRootIdForTesting(1, 0);
createTabStateFile(new int[] {0, 1});
// Restart Chrome and make sure tab strip is showing.
mActivityTestRule.startMainActivityFromLauncher();
ChromeTabbedActivity cta = mActivityTestRule.getActivity();
CriteriaHelper.pollUiThread(cta.getTabModelSelector()::isTabStateInitialized);
waitForView(allOf(withId(R.id.tab_list_view), isDescendantOfA(withId(R.id.bottom_controls)),
isCompletelyDisplayed()));
// The strip should be hidden when omnibox is focused.
onView(withId(R.id.url_bar)).perform(click());
onView(allOf(withId(R.id.tab_list_view), isDescendantOfA(withId(R.id.bottom_controls))))
.check(matches(withEffectiveVisibility((INVISIBLE))));
}
}
......@@ -6,6 +6,7 @@ package org.chromium.chrome.browser.tasks.tab_management;
import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.instanceOf;
import static org.hamcrest.CoreMatchers.nullValue;
import static org.junit.Assert.assertThat;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyBoolean;
......@@ -40,6 +41,8 @@ import org.mockito.Mock;
import org.mockito.MockitoAnnotations;
import org.robolectric.annotation.Config;
import org.chromium.base.Callback;
import org.chromium.base.supplier.ObservableSupplier;
import org.chromium.base.supplier.OneshotSupplierImpl;
import org.chromium.base.test.BaseRobolectricTestRunner;
import org.chromium.chrome.browser.compositor.layouts.OverviewModeBehavior;
......@@ -131,6 +134,8 @@ public class TabGroupUiMediatorUnitTest {
SnackbarManager.SnackbarManageable mSnackbarManageable;
@Mock
SnackbarManager mSnackbarManager;
@Mock
ObservableSupplier<Boolean> mOmniboxFocusStateSupplier;
@Captor
ArgumentCaptor<TabModelObserver> mTabModelObserverArgumentCaptor;
@Captor
......@@ -147,6 +152,8 @@ public class TabGroupUiMediatorUnitTest {
ArgumentCaptor<PauseResumeWithNativeObserver> mPauseResumeWithNativeObserverArgumentCaptor;
@Captor
ArgumentCaptor<TabObserver> mTabObserverCaptor;
@Captor
ArgumentCaptor<Callback<Boolean>> mOmniboxFocusObserverCaptor;
private TabImpl mTab1;
private TabImpl mTab2;
......@@ -205,7 +212,8 @@ public class TabGroupUiMediatorUnitTest {
TabUiFeatureUtilities.isTabGroupsAndroidEnabled() ? mTabGridDialogController : null;
mTabGroupUiMediator = new TabGroupUiMediator(mContext, mVisibilityController, mResetHandler,
mModel, mTabModelSelector, mTabCreatorManager, mOverviewModeBehaviorSupplier,
mThemeColorProvider, controller, mActivityLifecycleDispatcher, mSnackbarManageable);
mThemeColorProvider, controller, mActivityLifecycleDispatcher, mSnackbarManageable,
mOmniboxFocusStateSupplier);
if (currentTab == null) {
verifyNeverReset();
......@@ -346,6 +354,11 @@ public class TabGroupUiMediatorUnitTest {
// Set up SnackbarManageable.
doReturn(mSnackbarManager).when(mSnackbarManageable).getSnackbarManager();
// Set up omnibox focus state observer.
doReturn(nullValue())
.when(mOmniboxFocusStateSupplier)
.addObserver(mOmniboxFocusObserverCaptor.capture());
mResetHandlerInOrder = inOrder(mResetHandler);
mVisibilityControllerInOrder = inOrder(mVisibilityController);
mModel = new PropertyModel(TabGroupUiProperties.ALL_KEYS);
......@@ -1329,4 +1342,17 @@ public class TabGroupUiMediatorUnitTest {
tabObserverDestroyInOrder.verify(mTab1, never())
.removeObserver(mTabObserverCaptor.capture());
}
@Test
public void testOmniboxFocusChange() {
TabUiFeatureUtilities.ENABLE_LAUNCH_BUG_FIX.setForTesting(true);
initAndAssertProperties(mTab2);
mOmniboxFocusObserverCaptor.getValue().onResult(true);
verifyResetStrip(false, null);
doReturn(TAB2_ID).when(mTabModelSelector).getCurrentTabId();
mOmniboxFocusObserverCaptor.getValue().onResult(false);
verifyResetStrip(true, mTabGroup2);
}
}
......@@ -138,6 +138,7 @@ public class ToolbarManager implements UrlFocusChangeListener, ThemeColorObserve
new ObservableSupplierImpl<>();
private final ObservableSupplierImpl<Boolean> mIdentityDiscStateSupplier =
new ObservableSupplierImpl<>();
private final ObservableSupplier<Boolean> mOmniboxFocusStateSupplier;
private BottomControlsCoordinator mBottomControlsCoordinator;
private TabModelSelector mTabModelSelector;
......@@ -233,6 +234,7 @@ public class ToolbarManager implements UrlFocusChangeListener, ThemeColorObserve
* profile.
* @param tabModelSelectorSupplier Supplier of the {@link TabModelSelector}.
* @param startSurfaceSupplier Supplier of the StartSurface.
* @param omniboxFocusStateSupplier Supplier to access the focus state of the omnibox.
*/
public ToolbarManager(ChromeActivity activity, BrowserControlsSizer controlsSizer,
FullscreenManager fullscreenManager, ToolbarControlContainer controlContainer,
......@@ -249,7 +251,8 @@ public class ToolbarManager implements UrlFocusChangeListener, ThemeColorObserve
OneshotSupplier<AppMenuCoordinator> appMenuCoordinatorSupplier,
boolean shouldShowUpdateBadge,
ObservableSupplier<TabModelSelector> tabModelSelectorSupplier,
OneshotSupplier<StartSurface> startSurfaceSupplier) {
OneshotSupplier<StartSurface> startSurfaceSupplier,
ObservableSupplier<Boolean> omniboxFocusStateSupplier) {
TraceEvent.begin("ToolbarManager.ToolbarManager");
mActivity = activity;
mBrowserControlsSizer = controlsSizer;
......@@ -260,6 +263,7 @@ public class ToolbarManager implements UrlFocusChangeListener, ThemeColorObserve
mCanAnimateNativeBrowserControls = canAnimateNativeBrowserControls;
mScrimCoordinator = scrimCoordinator;
mTabModelSelectorSupplier = tabModelSelectorSupplier;
mOmniboxFocusStateSupplier = omniboxFocusStateSupplier;
ToolbarLayout toolbarLayout = mActivity.findViewById(R.id.toolbar);
NewTabPageDelegate ntpDelegate = createNewTabPageDelegate(toolbarLayout);
......@@ -834,15 +838,14 @@ public class ToolbarManager implements UrlFocusChangeListener, ThemeColorObserve
* Enable the bottom controls.
*/
public void enableBottomControls() {
mBottomControlsCoordinator =
new BottomControlsCoordinator(mBrowserControlsSizer, mFullscreenManager,
mActivity.findViewById(R.id.bottom_controls_stub), mActivityTabProvider,
mAppThemeColorProvider, mShareDelegateSupplier,
mMenuButtonCoordinator.getMenuButtonHelperSupplier(),
mShowStartSurfaceSupplier, mToolbarTabController::openHomepage,
(reason)
-> setUrlBarFocus(true, reason),
mOverviewModeBehaviorSupplier, mScrimCoordinator);
mBottomControlsCoordinator = new BottomControlsCoordinator(mBrowserControlsSizer,
mFullscreenManager, mActivity.findViewById(R.id.bottom_controls_stub),
mActivityTabProvider, mAppThemeColorProvider, mShareDelegateSupplier,
mMenuButtonCoordinator.getMenuButtonHelperSupplier(), mShowStartSurfaceSupplier,
mToolbarTabController::openHomepage,
(reason)
-> setUrlBarFocus(true, reason),
mOverviewModeBehaviorSupplier, mScrimCoordinator, mOmniboxFocusStateSupplier);
}
/**
......
......@@ -77,6 +77,7 @@ public class BottomControlsCoordinator {
* whether the bar should be focused, and the second is the OmniboxFocusReason.
* @param overviewModeBehaviorSupplier Supplier for the overview mode manager.
* @param scrimCoordinator The {@link ScrimCoordinator} to control scrim view.
* @param omniboxFocusStateSupplier Supplier to access the focus state of the omnibox.
*/
@SuppressLint("CutPasteId") // Not actually cut and paste since it's View vs ViewGroup.
public BottomControlsCoordinator(BrowserControlsSizer controlsSizer,
......@@ -87,7 +88,8 @@ public class BottomControlsCoordinator {
Supplier<Boolean> showStartSurfaceCallable, Runnable openHomepageAction,
Callback<Integer> setUrlBarFocusAction,
OneshotSupplier<OverviewModeBehavior> overviewModeBehaviorSupplier,
ScrimCoordinator scrimCoordinator) {
ScrimCoordinator scrimCoordinator,
ObservableSupplier<Boolean> omniboxFocusStateSupplier) {
final ScrollingBottomViewResourceFrameLayout root =
(ScrollingBottomViewResourceFrameLayout) stub.inflate();
......@@ -108,7 +110,7 @@ public class BottomControlsCoordinator {
|| TabUiFeatureUtilities.isConditionalTabStripEnabled()) {
mTabGroupUi = TabManagementModuleProvider.getDelegate().createTabGroupUi(
root.findViewById(R.id.bottom_container_slot), themeColorProvider,
scrimCoordinator);
scrimCoordinator, omniboxFocusStateSupplier);
}
Toast.setGlobalExtraYOffset(
root.getResources().getDimensionPixelSize(bottomControlsHeightId));
......
......@@ -584,7 +584,7 @@ public class RootUiCoordinator
mScrimCoordinator, mActionModeControllerCallback, mFindToolbarManager,
mProfileSupplier, mBookmarkBridgeSupplier, mCanAnimateBrowserControls,
mOverviewModeBehaviorSupplier, mAppMenuSupplier, shouldShowMenuUpdateBadge(),
mTabModelSelectorSupplier, mStartSurfaceSupplier);
mTabModelSelectorSupplier, mStartSurfaceSupplier, mOmniboxFocusStateSupplier);
if (!mActivity.supportsAppMenu()) {
mToolbarManager.getToolbar().disableMenuButton();
}
......
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