Skip to content

Commit

Permalink
[Tab Switcher] Delay tab addition to TabListModel if marked for selec…
Browse files Browse the repository at this point in the history 
…tion

We see an empty blue tab card when GTS is dismissed via new tab button click. This occurs since GTS dismiss occurs after a new tab is added to TabListMode and marked as selected. Updated this logic to move the add+select steps to post GTS hiding.

Demo: https://drive.google.com/file/d/1ZAJPvYMldZKIV10DE-otwTfxDTU3bxnC/view?usp=sharing

Bug: 1346739
Change-Id: Icd984fe3c1206be35dc49010bb0f525e924cb15d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3931789
Reviewed-by: Neil Coronado <nemco@google.com>
Reviewed-by: Theresa Sullivan <twellington@chromium.org>
Reviewed-by: Sinan Sahin <sinansahin@google.com>
Commit-Queue: Sirisha Kavuluru <skavuluru@google.com>
Cr-Commit-Position: refs/heads/main@{#1096778}
  • Loading branch information
Sirisha Kavuluru authored and Chromium LUCI CQ committed Jan 25, 2023
1 parent bc81e0d commit de80974
Show file tree
Hide file tree
Showing 48 changed files with 197 additions and 126 deletions.
2 changes: 1 addition & 1 deletion ...junit/src/org/chromium/chrome/browser/keyboard_accessory/ManualFillingControllerTest.java
View file
Expand Up @@ -1182,7 +1182,7 @@ private Tab addBrowserTab(ManualFillingMediator mediator, int id, @Nullable Tab
when(mMockTabModelSelector.getCurrentTab()).thenReturn(tab);
mActivityTabProvider.set(tab);
mediator.getTabModelObserverForTesting().didAddTab(
tab, FROM_BROWSER_ACTIONS, TabCreationState.LIVE_IN_FOREGROUND);
tab, FROM_BROWSER_ACTIONS, TabCreationState.LIVE_IN_FOREGROUND, false);
mediator.getTabObserverForTesting().onShown(tab, FROM_NEW);
mediator.getTabModelObserverForTesting().didSelectTab(tab, FROM_NEW, lastId);
setContentAreaDimensions(2.f, 300, 128);
Expand Down
4 changes: 2 additions & 2 deletions ...b_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabGridDialogMediator.java
View file
Expand Up @@ -166,8 +166,8 @@ interface AnimationSourceViewProvider {
// Register for tab model.
mTabModelObserver = new TabModelObserver() {
@Override
public void didAddTab(
Tab tab, @TabLaunchType int type, @TabCreationState int creationState) {
public void didAddTab(Tab tab, @TabLaunchType int type,
@TabCreationState int creationState, boolean markedForSelection) {
if (!mTabModelSelector.isTabStateInitialized()) {
return;
}
Expand Down
3 changes: 2 additions & 1 deletion .../tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabGroupUiMediator.java
View file
Expand Up @@ -165,7 +165,8 @@ public void willCloseTab(Tab tab, boolean animate, boolean didCloseAlone) {
}

@Override
public void didAddTab(Tab tab, int type, @TabCreationState int creationState) {
public void didAddTab(Tab tab, int type, @TabCreationState int creationState,
boolean markedForSelection) {
if (type == TabLaunchType.FROM_CHROME_UI || type == TabLaunchType.FROM_RESTORE
|| type == TabLaunchType.FROM_STARTUP) {
return;
Expand Down
63 changes: 45 additions & 18 deletions ...res/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabListMediator.java
View file
Expand Up @@ -113,6 +113,7 @@ class TabListMediator {
// screen.
private boolean mVisible;
private boolean mShownIPH;
private Tab mTabToAddDelayed;

/**
* An interface to get the thumbnails to be shown inside the tab grid cards.
Expand Down Expand Up @@ -581,15 +582,6 @@ public void didSelectTab(Tab tab, int type, int lastId) {
if (tab.getId() == lastId) return;

int oldIndex = mModel.indexFromId(lastId);
mLastSelectedTabListModelIndex = oldIndex;
if (oldIndex != TabModel.INVALID_TAB_INDEX) {
mModel.get(oldIndex).model.set(TabProperties.IS_SELECTED, false);
if (mActionsOnAllRelatedTabs && mThumbnailProvider != null && mVisible) {
mModel.get(oldIndex).model.set(TabProperties.THUMBNAIL_FETCHER,
new ThumbnailFetcher(mThumbnailProvider, lastId, true, false));
}
}

int newIndex = mModel.indexFromId(tab.getId());
if (newIndex == TabModel.INVALID_TAB_INDEX && mActionsOnAllRelatedTabs
&& type == TabSelectionType.FROM_UNDO) {
Expand All @@ -598,12 +590,13 @@ public void didSelectTab(Tab tab, int type, int lastId) {
// the related ids are present in model.
newIndex = getIndexForTabWithRelatedTabs(tab);
}
if (newIndex == TabModel.INVALID_TAB_INDEX) return;
mModel.get(newIndex).model.set(TabProperties.IS_SELECTED, true);
if (mThumbnailProvider != null && mVisible) {
mModel.get(newIndex).model.set(TabProperties.THUMBNAIL_FETCHER,
new ThumbnailFetcher(mThumbnailProvider, tab.getId(), true, false));

mLastSelectedTabListModelIndex = oldIndex;
if (mTabToAddDelayed != null && mTabToAddDelayed == tab) {
// If tab is being added later, it will be selected later.
return;
}
selectTab(oldIndex, newIndex);
}

@Override
Expand Down Expand Up @@ -651,9 +644,16 @@ public void tabClosureUndone(Tab tab) {
}

@Override
public void didAddTab(
Tab tab, @TabLaunchType int type, @TabCreationState int creationState) {
public void didAddTab(Tab tab, @TabLaunchType int type,
@TabCreationState int creationState, boolean markedForSelection) {
if (!mTabModelSelector.isTabStateInitialized()) return;
// Check if we need to delay tab addition to model.
boolean delayAdd =
(type == TabLaunchType.FROM_TAB_SWITCHER_UI) && markedForSelection;
if (delayAdd) {
mTabToAddDelayed = tab;
return;
}
onTabAdded(tab, !mActionsOnAllRelatedTabs);
if (type == TabLaunchType.FROM_RESTORE && mActionsOnAllRelatedTabs) {
// When tab is restored after restoring stage (e.g. exiting multi-window mode,
Expand Down Expand Up @@ -786,6 +786,26 @@ public void onItemMoved(ListObservable source, int curIndex, int newIndex) {
}
}

private void selectTab(int oldIndex, int newIndex) {
if (oldIndex != TabModel.INVALID_TAB_INDEX) {
int lastId = mModel.get(oldIndex).model.get(TAB_ID);
mModel.get(oldIndex).model.set(TabProperties.IS_SELECTED, false);
if (mActionsOnAllRelatedTabs && mThumbnailProvider != null && mVisible) {
mModel.get(oldIndex).model.set(TabProperties.THUMBNAIL_FETCHER,
new ThumbnailFetcher(mThumbnailProvider, lastId, true, false));
}
}

if (newIndex != TabModel.INVALID_TAB_INDEX) {
int newId = mModel.get(newIndex).model.get(TAB_ID);
mModel.get(newIndex).model.set(TabProperties.IS_SELECTED, true);
if (mThumbnailProvider != null && mVisible) {
mModel.get(newIndex).model.set(TabProperties.THUMBNAIL_FETCHER,
new ThumbnailFetcher(mThumbnailProvider, newId, true, false));
}
}
}

public void initWithNative(Profile profile) {
mTabListFaviconProvider.initWithNative(profile);
mTabModelSelector.getTabModelFilterProvider().addTabModelFilterObserver(mTabModelObserver);
Expand Down Expand Up @@ -1121,11 +1141,12 @@ private int getIndexOfTab(Tab tab, boolean onlyShowRelatedTabs) {
return index;
}

private void onTabAdded(Tab tab, boolean onlyShowRelatedTabs) {
private int onTabAdded(Tab tab, boolean onlyShowRelatedTabs) {
int index = getIndexOfTab(tab, onlyShowRelatedTabs);
if (index == TabList.INVALID_TAB_INDEX) return;
if (index == TabList.INVALID_TAB_INDEX) return index;

addTabInfoToModel(PseudoTab.fromTab(tab), index, mTabModelSelector.getCurrentTab() == tab);
return index;
}

private void onTabMoved(int newIndex, int curIndex) {
Expand Down Expand Up @@ -1246,6 +1267,12 @@ private void addViewedTabId(int tabIndex) {
void postHiding() {
mVisible = false;
unregisterOnScrolledListener();
// if tab was marked for add later, add to model and mark as selected.
if (mTabToAddDelayed != null) {
int index = onTabAdded(mTabToAddDelayed, !mActionsOnAllRelatedTabs);
selectTab(mLastSelectedTabListModelIndex, index);
mTabToAddDelayed = null;
}
}

private boolean isSelectedTab(PseudoTab tab, int tabModelSelectedTabId) {
Expand Down
3 changes: 2 additions & 1 deletion ...java/src/org/chromium/chrome/browser/tasks/tab_management/TabSelectionEditorMediator.java
View file
Expand Up @@ -119,7 +119,8 @@ public void onClick(View v) {

mTabModelObserver = new TabModelSelectorTabModelObserver(mTabModelSelector) {
@Override
public void didAddTab(Tab tab, int type, @TabCreationState int creationState) {
public void didAddTab(Tab tab, int type, @TabCreationState int creationState,
boolean markedForSelection) {
if (!mTabModelSelector.isTabStateInitialized()) return;
// When tab is added due to multi-window close or moving between multiple windows,
// force hiding the selection editor.
Expand Down
3 changes: 2 additions & 1 deletion ...tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabSwitcherMediator.java
View file
Expand Up @@ -327,7 +327,8 @@ public void onTabModelSelected(TabModel newModel, TabModel oldModel) {

mTabModelObserver = new TabModelObserver() {
@Override
public void didAddTab(Tab tab, int type, @TabCreationState int creationState) {
public void didAddTab(Tab tab, int type, @TabCreationState int creationState,
boolean markedForSelection) {
// TODO(wychen): move didAddTab and didSelectTab to another observer and inject
// after restoreCompleted.
if (!mTabModelSelector.isTabStateInitialized()) {
Expand Down
4 changes: 2 additions & 2 deletions ...ava/src/org/chromium/chrome/browser/tasks/tab_management/UndoGroupSnackbarController.java
View file
Expand Up @@ -97,8 +97,8 @@ public void onTabModelSelected(TabModel newModel, TabModel oldModel) {
mTabModelSelectorTabModelObserver =
new TabModelSelectorTabModelObserver(mTabModelSelector) {
@Override
public void didAddTab(
Tab tab, @TabLaunchType int type, @TabCreationState int creationState) {
public void didAddTab(Tab tab, @TabLaunchType int type,
@TabCreationState int creationState, boolean markedForSelection) {
mSnackbarManager.dismissSnackbars(UndoGroupSnackbarController.this);
}

Expand Down
3 changes: 2 additions & 1 deletion .../src/org/chromium/chrome/browser/tasks/tab_management/suggestions/TabContextObserver.java
View file
Expand Up @@ -33,7 +33,8 @@ public void didFirstVisuallyNonEmptyPaint(Tab tab) {

mTabModelObserver = new TabModelObserver() {
@Override
public void didAddTab(Tab tab, int type, @TabCreationState int creationState) {
public void didAddTab(Tab tab, int type, @TabCreationState int creationState,
boolean markedForSelection) {
onTabContextChanged(TabContextChangeReason.TAB_ADDED);
}

Expand Down
2 changes: 1 addition & 1 deletion ...t/src/org/chromium/chrome/browser/tasks/tab_management/TabGridDialogMediatorUnitTest.java
View file
Expand Up @@ -471,7 +471,7 @@ public void tabAddition() {

doReturn(true).when(mTabModelSelector).isTabStateInitialized();
mTabModelObserverCaptor.getValue().didAddTab(
newTab, TabLaunchType.FROM_CHROME_UI, TabCreationState.LIVE_IN_FOREGROUND);
newTab, TabLaunchType.FROM_CHROME_UI, TabCreationState.LIVE_IN_FOREGROUND, false);

assertThat(mModel.get(TabGridPanelProperties.ANIMATION_SOURCE_VIEW), equalTo(null));
verify(mDialogController).resetWithListOfTabs(null);
Expand Down
20 changes: 11 additions & 9 deletions ...unit/src/org/chromium/chrome/browser/tasks/tab_management/TabGroupUiMediatorUnitTest.java
View file
Expand Up @@ -533,9 +533,9 @@ public void tabAddition_SingleTab() {
doReturn(tabs).when(mTabGroupModelFilter).getRelatedTabList(TAB4_ID);

mTabModelObserverArgumentCaptor.getValue().didAddTab(
newTab, TabLaunchType.FROM_CHROME_UI, TabCreationState.LIVE_IN_FOREGROUND);
newTab, TabLaunchType.FROM_CHROME_UI, TabCreationState.LIVE_IN_FOREGROUND, false);
mTabModelObserverArgumentCaptor.getValue().didAddTab(
newTab, TabLaunchType.FROM_RESTORE, TabCreationState.FROZEN_ON_RESTORE);
newTab, TabLaunchType.FROM_RESTORE, TabCreationState.FROZEN_ON_RESTORE, false);

// Strip should be not be reset when adding a single new tab.
verifyNeverReset();
Expand All @@ -552,7 +552,8 @@ public void tabAddition_SingleTab_Refresh_WithAutoGroupCreation() {
doReturn(tabs).when(mTabGroupModelFilter).getRelatedTabList(TAB4_ID);

mTabModelObserverArgumentCaptor.getValue().didAddTab(newTab,
TabLaunchType.FROM_LONGPRESS_BACKGROUND, TabCreationState.LIVE_IN_FOREGROUND);
TabLaunchType.FROM_LONGPRESS_BACKGROUND, TabCreationState.LIVE_IN_FOREGROUND,
false);

// Strip should be be reset when long pressing a link and add a tab into group.
verifyResetStrip(true, tabs);
Expand All @@ -570,7 +571,7 @@ public void tabAddition_SingleTab_Refresh_WithoutAutoGroupCreation() {

mTabModelObserverArgumentCaptor.getValue().didAddTab(newTab,
TabLaunchType.FROM_LONGPRESS_BACKGROUND_IN_GROUP,
TabCreationState.LIVE_IN_FOREGROUND);
TabCreationState.LIVE_IN_FOREGROUND, false);

// Strip should be be reset when long pressing a link and add a tab into group.
verifyResetStrip(true, tabs);
Expand All @@ -589,11 +590,12 @@ public void tabAddition_TabGroup_NoRefresh() {
doReturn(mTabGroup1).when(mTabGroupModelFilter).getRelatedTabList(TAB4_ID);

mTabModelObserverArgumentCaptor.getValue().didAddTab(
newTab, TabLaunchType.FROM_CHROME_UI, TabCreationState.LIVE_IN_FOREGROUND);
newTab, TabLaunchType.FROM_CHROME_UI, TabCreationState.LIVE_IN_FOREGROUND, false);
mTabModelObserverArgumentCaptor.getValue().didAddTab(
newTab, TabLaunchType.FROM_RESTORE, TabCreationState.FROZEN_ON_RESTORE);
newTab, TabLaunchType.FROM_RESTORE, TabCreationState.FROZEN_ON_RESTORE, false);
mTabModelObserverArgumentCaptor.getValue().didAddTab(newTab,
TabLaunchType.FROM_LONGPRESS_BACKGROUND, TabCreationState.LIVE_IN_FOREGROUND);
TabLaunchType.FROM_LONGPRESS_BACKGROUND, TabCreationState.LIVE_IN_FOREGROUND,
false);

// Strip should be not be reset through these two types of launching.
verifyNeverReset();
Expand All @@ -609,8 +611,8 @@ public void tabAddition_TabGroup_ScrollToTheLast() {
mTabGroup2.add(newTab);
doReturn(mTabGroup2).when(mTabGroupModelFilter).getRelatedTabList(TAB4_ID);

mTabModelObserverArgumentCaptor.getValue().didAddTab(
newTab, TabLaunchType.FROM_TAB_GROUP_UI, TabCreationState.LIVE_IN_FOREGROUND);
mTabModelObserverArgumentCaptor.getValue().didAddTab(newTab,
TabLaunchType.FROM_TAB_GROUP_UI, TabCreationState.LIVE_IN_FOREGROUND, false);

// Strip should be not be reset through adding tab from UI.
verifyNeverReset();
Expand Down
24 changes: 12 additions & 12 deletions ...i/junit/src/org/chromium/chrome/browser/tasks/tab_management/TabListMediatorUnitTest.java
View file
Expand Up @@ -620,7 +620,7 @@ public void updatesFavicon_Navigation_NoOpNtpUrl() {
assertThat(mModel.size(), equalTo(2));

mTabModelObserverCaptor.getValue().didAddTab(
newTab, TabLaunchType.FROM_CHROME_UI, TabCreationState.LIVE_IN_FOREGROUND);
newTab, TabLaunchType.FROM_CHROME_UI, TabCreationState.LIVE_IN_FOREGROUND, false);

assertThat(mModel.size(), equalTo(3));
assertThat(mModel.get(2).model.get(TabProperties.TAB_ID), equalTo(TAB3_ID));
Expand Down Expand Up @@ -897,7 +897,7 @@ public void tabAddition_RestoreNotComplete() {
assertThat(mModel.size(), equalTo(2));

mTabModelObserverCaptor.getValue().didAddTab(
newTab, TabLaunchType.FROM_RESTORE, TabCreationState.LIVE_IN_FOREGROUND);
newTab, TabLaunchType.FROM_RESTORE, TabCreationState.LIVE_IN_FOREGROUND, false);

// When tab restoring stage is not yet finished, this tab info should not be added to
// property model.
Expand Down Expand Up @@ -925,7 +925,7 @@ public void tabAddition_Restore() {
assertThat(mModel.size(), equalTo(2));

mTabModelObserverCaptor.getValue().didAddTab(
newTab, TabLaunchType.FROM_RESTORE, TabCreationState.LIVE_IN_FOREGROUND);
newTab, TabLaunchType.FROM_RESTORE, TabCreationState.LIVE_IN_FOREGROUND, false);

TabListMediator.TabActionListener actionListenerAfterUpdate =
mModel.get(1).model.get(TabProperties.TAB_SELECTED_LISTENER);
Expand All @@ -952,11 +952,11 @@ public void tabAddition_Restore_SyncingTabListModelWithTabModel() {
mModel.clear();

mMediatorTabModelObserver.didAddTab(
mTab2, TabLaunchType.FROM_RESTORE, TabCreationState.LIVE_IN_FOREGROUND);
mTab2, TabLaunchType.FROM_RESTORE, TabCreationState.LIVE_IN_FOREGROUND, false);
assertThat(mModel.size(), equalTo(0));

mMediatorTabModelObserver.didAddTab(
mTab1, TabLaunchType.FROM_RESTORE, TabCreationState.LIVE_IN_FOREGROUND);
mTab1, TabLaunchType.FROM_RESTORE, TabCreationState.LIVE_IN_FOREGROUND, false);
assertThat(mModel.size(), equalTo(1));
}

Expand All @@ -975,7 +975,7 @@ public void tabAddition_GTS() {
assertThat(mModel.size(), equalTo(2));

mTabModelObserverCaptor.getValue().didAddTab(
newTab, TabLaunchType.FROM_CHROME_UI, TabCreationState.LIVE_IN_FOREGROUND);
newTab, TabLaunchType.FROM_CHROME_UI, TabCreationState.LIVE_IN_FOREGROUND, false);

assertThat(mModel.size(), equalTo(3));
assertThat(mModel.get(2).model.get(TabProperties.TAB_ID), equalTo(TAB3_ID));
Expand All @@ -997,7 +997,7 @@ public void tabAddition_GTS_Skip() {
assertThat(mModel.size(), equalTo(2));

mTabModelObserverCaptor.getValue().didAddTab(
newTab, TabLaunchType.FROM_CHROME_UI, TabCreationState.LIVE_IN_FOREGROUND);
newTab, TabLaunchType.FROM_CHROME_UI, TabCreationState.LIVE_IN_FOREGROUND, false);

assertThat(mModel.size(), equalTo(2));
}
Expand All @@ -1017,7 +1017,7 @@ public void tabAddition_GTS_Middle() {
assertThat(mModel.size(), equalTo(2));

mTabModelObserverCaptor.getValue().didAddTab(
newTab, TabLaunchType.FROM_CHROME_UI, TabCreationState.LIVE_IN_FOREGROUND);
newTab, TabLaunchType.FROM_CHROME_UI, TabCreationState.LIVE_IN_FOREGROUND, false);

assertThat(mModel.size(), equalTo(3));
assertThat(mModel.get(1).model.get(TabProperties.TAB_ID), equalTo(TAB3_ID));
Expand All @@ -1037,7 +1037,7 @@ public void tabAddition_Dialog_End() {
assertThat(mModel.size(), equalTo(2));

mTabModelObserverCaptor.getValue().didAddTab(
newTab, TabLaunchType.FROM_CHROME_UI, TabCreationState.LIVE_IN_FOREGROUND);
newTab, TabLaunchType.FROM_CHROME_UI, TabCreationState.LIVE_IN_FOREGROUND, false);

assertThat(mModel.size(), equalTo(3));
assertThat(mModel.get(2).model.get(TabProperties.TAB_ID), equalTo(TAB3_ID));
Expand All @@ -1057,7 +1057,7 @@ public void tabAddition_Dialog_Middle() {
assertThat(mModel.size(), equalTo(2));

mTabModelObserverCaptor.getValue().didAddTab(
newTab, TabLaunchType.FROM_CHROME_UI, TabCreationState.LIVE_IN_FOREGROUND);
newTab, TabLaunchType.FROM_CHROME_UI, TabCreationState.LIVE_IN_FOREGROUND, false);

assertThat(mModel.size(), equalTo(3));
assertThat(mModel.get(1).model.get(TabProperties.TAB_ID), equalTo(TAB3_ID));
Expand All @@ -1075,7 +1075,7 @@ public void tabAddition_Dialog_Skip() {
assertThat(mModel.size(), equalTo(2));

mTabModelObserverCaptor.getValue().didAddTab(
newTab, TabLaunchType.FROM_CHROME_UI, TabCreationState.LIVE_IN_FOREGROUND);
newTab, TabLaunchType.FROM_CHROME_UI, TabCreationState.LIVE_IN_FOREGROUND, false);

assertThat(mModel.size(), equalTo(2));
}
Expand All @@ -1091,7 +1091,7 @@ public void tabAddition_Redundant() {
// Try to do a redundant addition by adding the PropertyModel of an existing tab to the
// TabListModel.
mTabModelObserverCaptor.getValue().didAddTab(
mTab1, TabLaunchType.FROM_CHROME_UI, TabCreationState.LIVE_IN_FOREGROUND);
mTab1, TabLaunchType.FROM_CHROME_UI, TabCreationState.LIVE_IN_FOREGROUND, false);

assertThat(mModel.size(), equalTo(2));
}
Expand Down
2 changes: 1 addition & 1 deletion .../org/chromium/chrome/browser/tasks/tab_management/suggestions/TabContextObserverTest.java
View file
Expand Up @@ -78,7 +78,7 @@ public void testAddTab() {
TabContextObserverTestHelper tabContextObserverTestHelper =
new TabContextObserverTestHelper(mTabModelSelector);
tabContextObserverTestHelper.mTabModelObserver.didAddTab(
null, 0, TabCreationState.LIVE_IN_FOREGROUND);
null, 0, TabCreationState.LIVE_IN_FOREGROUND, false);
Assert.assertEquals(TabContextObserver.TabContextChangeReason.TAB_ADDED,
tabContextObserverTestHelper.getChangeReason());
}
Expand Down

0 comments on commit de80974

Please sign in to comment.