Skip to content

Commit

Permalink
[TabGroup] Refocus previously selected tab on undo close
Browse files Browse the repository at this point in the history 
Demo: https://drive.google.com/file/d/1GJLFU9FRgRPvhnruaS1ZnBGyBsAIYWUw/view?usp=sharing
Bug: 1323356
Change-Id: Id08cd519e41319860c9082a091e7fb10d37317bd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3803965
Commit-Queue: Sirisha Kavuluru <skavuluru@google.com>
Reviewed-by: Theresa Sullivan <twellington@chromium.org>
Reviewed-by: Neil Coronado <nemco@google.com>
Reviewed-by: Mei Liang <meiliang@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1033101}
  • Loading branch information
Sirisha Kavuluru authored and Chromium LUCI CQ committed Aug 9, 2022
1 parent ee98b56 commit fa7fa6e
Show file tree
Hide file tree
Showing 7 changed files with 235 additions and 88 deletions.
23 changes: 23 additions & 0 deletions ...res/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabListMediator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import static org.chromium.chrome.browser.tasks.tab_management.TabListModel.CardProperties.CARD_TYPE;
import static org.chromium.chrome.browser.tasks.tab_management.TabListModel.CardProperties.ModelType.TAB;
import static org.chromium.chrome.browser.tasks.tab_management.TabProperties.CLOSE_BUTTON_DESCRIPTION_STRING;
import static org.chromium.chrome.browser.tasks.tab_management.TabProperties.TAB_ID;

import android.app.Activity;
import android.content.ComponentCallbacks;
Expand Down Expand Up @@ -599,6 +600,22 @@ public void didSelectTab(Tab tab, int type, int lastId) {
}

int newIndex = mModel.indexFromId(tab.getId());
if (newIndex == TabModel.INVALID_TAB_INDEX && mActionsOnAllRelatedTabs
&& type == TabSelectionType.FROM_UNDO) {
// If a tab in tab group does not exist in model and needs to be selected from
// undo, identify the related TabIds and determine newIndex based on if any of
// the related ids are present in model.
List<Integer> relatedTabIds = getRelatedTabsIds(tab.getId());
if (!relatedTabIds.isEmpty()) {
for (int i = 0; i < mModel.size(); i++) {
int modelTabId = mModel.get(i).model.get(TAB_ID);
if (relatedTabIds.contains(modelTabId)) {
newIndex = i;
break;
}
}
}
}
if (newIndex == TabModel.INVALID_TAB_INDEX) return;
mModel.get(newIndex).model.set(TabProperties.IS_SELECTED, true);
if (mThumbnailProvider != null && mVisible) {
Expand Down Expand Up @@ -1079,6 +1096,12 @@ private List<Tab> getRelatedTabsForId(int id) {
return filter == null ? new ArrayList<>() : filter.getRelatedTabList(id);
}

private List<Integer> getRelatedTabsIds(int id) {
TabModelFilter filter =
mTabModelSelector.getTabModelFilterProvider().getCurrentTabModelFilter();
return filter == null ? new ArrayList<>() : filter.getRelatedTabIds(id);
}

private int getIndexOfTab(Tab tab, boolean onlyShowRelatedTabs) {
int index = TabList.INVALID_TAB_INDEX;
if (tab == null) return index;
Expand Down
82 changes: 53 additions & 29 deletions chrome/android/java/src/org/chromium/chrome/browser/UndoRefocusHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.chromium.ui.base.DeviceFormFactor;

import java.util.HashSet;
import java.util.List;
import java.util.Set;

/**
Expand All @@ -39,7 +40,7 @@ public class UndoRefocusHelper implements DestroyObserver {
private LayoutStateProvider.LayoutStateObserver mLayoutStateObserver;
private TabModelSelectorTabModelObserver mTabModelSelectorTabModelObserver;
private Integer mSelectedTabIdWhenTabClosed;
private Boolean mClosedAllTabs;
private boolean mWillCloseMultipleTabs;
private boolean mTabSwitcherActive;
private Callback<LayoutManagerImpl> mLayoutManagerSupplierCallback;
private boolean mIsTablet;
Expand Down Expand Up @@ -89,24 +90,14 @@ private void observeTabModel() {
mTabModelSelectorTabModelObserver = new TabModelSelectorTabModelObserver(mModelSelector) {
@Override
public void willCloseTab(Tab tab, boolean animate) {
// TODO(crbug.com/1324405) Extract common logic between this method and
// willCloseAllTabs into a helper method.
if (mClosedAllTabs != null || tab.isIncognito()) return;
if (mWillCloseMultipleTabs || tab.isIncognito()) return;

int tabId = tab.getId();
TabModel model = mModelSelector.getModel(false);

if (!mTabSwitcherActive && mIsTablet) {
mTabsClosedFromTabStrip.add(tabId);
}

int selTabIndex = model.index();
if (selTabIndex > -1 && selTabIndex < model.getCount()) {
Tab selectedTab = model.getTabAt(selTabIndex);
if (selectedTab != null && tabId == selectedTab.getId()) {
mSelectedTabIdWhenTabClosed = tabId;
}
}
maybeSetSelectedTabId(tab);
}

@Override
Expand All @@ -119,28 +110,48 @@ public void didSelectTab(Tab tab, int type, int lastId) {
}
}

// TODO (crbug.com/1351406) Fix case of undo closing multiple tabs followed by single
// tab.
@Override
public void willCloseAllTabs(boolean incognito) {
if (!incognito) {
TabModel model = mModelSelector.getCurrentModel();
int selTabIndex = model.index();
mClosedAllTabs = true;
if (selTabIndex > -1 && selTabIndex < model.getCount()) {
Tab tab = model.getTabAt(selTabIndex);
if (tab != null) {
mSelectedTabIdWhenTabClosed = tab.getId();
if (!mTabSwitcherActive && mIsTablet) {
mTabsClosedFromTabStrip.add(tab.getId());
}
}
public void willCloseMultipleTabs(boolean allowUndo, List<Tab> tabs) {
if (!allowUndo || tabs.size() < 1) return;
mWillCloseMultipleTabs = true;

// Record metric only once for the set.
// Use the first id to track the set.
if (!mTabSwitcherActive && mIsTablet) {
mTabsClosedFromTabStrip.add(tabs.get(0).getId());
}
for (Tab tab : tabs) {
if (maybeSetSelectedTabId(tab)) {
break;
}
}
}

@Override
public void willCloseAllTabs(boolean incognito) {
if (incognito) return;
int selectedTabIdx = mModelSelector.getModel(false).index();
Tab selectedTab = mModelSelector.getModel(false).getTabAt(selectedTabIdx);
maybeSetSelectedTabId(selectedTab);
mWillCloseMultipleTabs = true;
// Record metric only once for the set.
// Use the selected id to track the set.
if (!mTabSwitcherActive && mIsTablet) {
mTabsClosedFromTabStrip.add(selectedTab.getId());
}
}

@Override
public void tabClosureUndone(Tab tab) {
if (mClosedAllTabs != null) return;
int id = tab.getId();
if (mWillCloseMultipleTabs) {
// allTabsClosureUndone does not receive set of Ids to pass to record method.
// Hence we record here first.
recordClosureCancellation(id);
return;
}
recordClosureCancellation(id);
if (mSelectedTabIdWhenTabClosed != null && mSelectedTabIdWhenTabClosed == id) {
selectPreviouslySelectedTab();
Expand All @@ -150,7 +161,6 @@ public void tabClosureUndone(Tab tab) {
@Override
public void allTabsClosureUndone() {
if (mSelectedTabIdWhenTabClosed != null) {
recordClosureCancellation(mSelectedTabIdWhenTabClosed);
selectPreviouslySelectedTab();
}

Expand All @@ -174,6 +184,20 @@ public void allTabsClosureCommitted(boolean isIncognito) {
}
}

private boolean maybeSetSelectedTabId(Tab tab) {
TabModel model = mModelSelector.getModel(false);
int tabId = tab.getId();
int selTabIndex = model.index();
if (selTabIndex > -1 && selTabIndex < model.getCount()) {
Tab selectedTab = model.getTabAt(selTabIndex);
if (selectedTab != null && tabId == selectedTab.getId()) {
mSelectedTabIdWhenTabClosed = tabId;
return true;
}
}
return false;
}

private void recordClosureCancellation(int id) {
// Only record the action if the tab was previously closed from the tab strip.
if (mTabsClosedFromTabStrip.contains(id)) {
Expand Down Expand Up @@ -226,8 +250,8 @@ private void selectPreviouslySelectedTab() {
* are reset so the next undo closure action does not reselect the reopened tab.
*/
private void resetSelectionsForUndo() {
mWillCloseMultipleTabs = false;
mSelectedTabIdWhenTabClosed = null;
mClosedAllTabs = null;
}

@VisibleForTesting
Expand Down
1 change: 1 addition & 0 deletions chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,7 @@ public void closeMultipleTabs(List<Tab> tabs, boolean canUndo) {
if (!allowUndo) {
notifyOnFinishingMultipleTabClosure(tabs);
}
for (TabModelObserver obs : mObservers) obs.willCloseMultipleTabs(allowUndo, tabs);
for (Tab tab : tabs) {
closeTab(tab, null, false, false, canUndo, false, false);
}
Expand Down
Loading

0 comments on commit fa7fa6e

Please sign in to comment.