Skip to content

Commit

Permalink
[M-103] Do not autoscroll when tab is closed from tab strip
Browse files Browse the repository at this point in the history 
When an unselected tab is closed from the tab strip, the tab strip should not auto scroll to focus on the currently selected tab. After a unselected closed tab is restored after an undo action, the tab strip should not auto scroll.

Bug: 1326719
Change-Id: Id0fa0cff63fb4d4500a74fb560ba3f31eab16be7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3655126
Reviewed-by: Sirisha Kavuluru <skavuluru@google.com>
Reviewed-by: Theresa Sullivan <twellington@chromium.org>
Commit-Queue: Gaurav Jayasawal <gauravjj@google.com>
Cr-Commit-Position: refs/branch-heads/5060@{#182}
Cr-Branched-From: b83393d-refs/heads/main@{#1002911}
  • Loading branch information
Gaurav Jayasawal authored and Chromium LUCI CQ committed May 23, 2022
1 parent 2f1a719 commit 8bf4b88
Show file tree
Hide file tree
Showing 4 changed files with 121 additions and 28 deletions.
39 changes: 24 additions & 15 deletions ...oid/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -526,18 +526,21 @@ public void tabModelSelected(boolean selected) {
* @param time The current time of the app in ms.
* @param id The id of the selected tab.
* @param prevId The id of the previously selected tab.
* @param skipAutoScroll Whether autoscroll to bring selected tab to view can be skipped.
*/
public void tabSelected(long time, int id, int prevId) {
public void tabSelected(long time, int id, int prevId, boolean skipAutoScroll) {
StripLayoutTab stripTab = findTabById(id);
if (stripTab == null) {
tabCreated(time, id, prevId, true);
tabCreated(time, id, prevId, true, false);
} else {
updateVisualTabOrdering();
updateCloseButtons();

// If the tab was selected through a method other than the user tapping on the strip, it
// may not be currently visible. Scroll if necessary.
bringSelectedTabToVisibleArea(time, true);
if (!skipAutoScroll) {
// If the tab was selected through a method other than the user tapping on the
// strip, it may not be currently visible. Scroll if necessary.
bringSelectedTabToVisibleArea(time, true);
}

mUpdateHost.requestUpdate();

Expand Down Expand Up @@ -596,7 +599,7 @@ public void willCloseAllTabs() {
*/
public void tabClosureCancelled(long time, int id) {
final boolean selected = TabModelUtils.getCurrentTabId(mModel) == id;
tabCreated(time, id, Tab.INVALID_TAB_ID, selected);
tabCreated(time, id, Tab.INVALID_TAB_ID, selected, true);
}

/**
Expand All @@ -605,8 +608,9 @@ public void tabClosureCancelled(long time, int id) {
* @param id The id of the newly created tab.
* @param prevId The id of the source tab.
* @param selected Whether the tab will be selected.
* @param restoredTab Whether the tab was restored by a tab closure cancellation.
*/
public void tabCreated(long time, int id, int prevId, boolean selected) {
public void tabCreated(long time, int id, int prevId, boolean selected, boolean restoredTab) {
if (findTabById(id) != null) return;

// 1. Build any tabs that are missing.
Expand All @@ -630,10 +634,12 @@ public void tabCreated(long time, int id, int prevId, boolean selected) {
allowLeftExpand = true;
}

boolean tabStripImprovementsEnabled =
CachedFeatureFlags.isEnabled(ChromeFeatureList.TAB_STRIP_IMPROVEMENTS);
if (!mShouldCascadeTabs) {
int selIndex = mModel.index();
if (CachedFeatureFlags.isEnabled(ChromeFeatureList.TAB_STRIP_IMPROVEMENTS) && !selected
&& selIndex >= 0 && selIndex < mStripTabs.length) {
if (tabStripImprovementsEnabled && !selected && selIndex >= 0
&& selIndex < mStripTabs.length) {
// Prioritize focusing on selected tab over newly created unselected tabs.
fastExpandTab = mStripTabs[selIndex];
} else {
Expand All @@ -643,18 +649,17 @@ public void tabCreated(long time, int id, int prevId, boolean selected) {
canExpandSelectedTab = true;
}

// 4. Scroll the stack so that the fast expand tab is visible.
if (fastExpandTab != null) {
// 4. Scroll the stack so that the fast expand tab is visible. Skip if tab was restored.
boolean skipAutoScroll = tabStripImprovementsEnabled && restoredTab;
if (fastExpandTab != null && !skipAutoScroll) {
float delta = calculateOffsetToMakeTabVisible(
fastExpandTab, canExpandSelectedTab, allowLeftExpand, true, selected);

if (!mShouldCascadeTabs) {
// If the ScrollingStripStacker is being used and the new tab button is visible, go
// directly to the new scroll offset rather than animating. Animating the scroll
// causes the new tab button to disappear for a frame.
boolean shouldAnimate = (!mNewTabButton.isVisible()
|| CachedFeatureFlags.isEnabled(
ChromeFeatureList.TAB_STRIP_IMPROVEMENTS))
boolean shouldAnimate = (!mNewTabButton.isVisible() || tabStripImprovementsEnabled)
&& !mAnimationsDisabledForTesting;
setScrollForScrollingTabStacker(delta, shouldAnimate, time);
} else if (delta != 0.f) {
Expand Down Expand Up @@ -999,8 +1004,12 @@ public void onAnimationEnd(Animator animation) {
tab.setIsDying(true);

// 3. Fake a selection on the next tab now.
// If a tab other than the selected tab was closed, do not autoscroll to bring next tab to
// visible area.
boolean skipAutoScroll = mStripTabs[mModel.index()].getId() != tab.getId()
&& CachedFeatureFlags.isEnabled(ChromeFeatureList.TAB_STRIP_IMPROVEMENTS);
Tab nextTab = mModel.getNextTabIfClosed(tab.getId(), /*uponExit=*/false);
if (nextTab != null) tabSelected(time, nextTab.getId(), tab.getId());
if (nextTab != null) tabSelected(time, nextTab.getId(), tab.getId(), skipAutoScroll);
}

@Override
Expand Down
7 changes: 4 additions & 3 deletions ...a/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelperManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -592,16 +592,17 @@ public void allTabsClosureCommitted(boolean incognito) {
@Override
public void didSelectTab(Tab tab, @TabSelectionType int type, int lastId) {
if (tab.getId() == lastId) return;
getStripLayoutHelper(tab.isIncognito()).tabSelected(time(), tab.getId(), lastId);
getStripLayoutHelper(tab.isIncognito())
.tabSelected(time(), tab.getId(), lastId, false);
}

@Override
public void didAddTab(Tab tab, int type, int creationState) {
boolean selected = type != TabLaunchType.FROM_LONGPRESS_BACKGROUND
|| (mTabModelSelector.isIncognitoSelected() && tab.isIncognito());
getStripLayoutHelper(tab.isIncognito())
.tabCreated(
time(), tab.getId(), mTabModelSelector.getCurrentTabId(), selected);
.tabCreated(time(), tab.getId(), mTabModelSelector.getCurrentTabId(),
selected, false);
}
};

Expand Down
87 changes: 77 additions & 10 deletions ...unit/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelperTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
Expand Down Expand Up @@ -190,7 +191,7 @@ public void testStripStacker_UpdateCloseButtons() {
StripLayoutTab[] tabs = getMockedStripLayoutTabs(TAB_WIDTH_1);
mStripLayoutHelper.setStripLayoutTabsForTest(tabs);

mStripLayoutHelper.tabSelected(1, 3, 0);
mStripLayoutHelper.tabSelected(1, 3, 0, false);

// Close btn should be visible on the selected tab.
Mockito.verify(tabs[3]).setCanShowCloseButton(true);
Expand All @@ -212,7 +213,7 @@ public void testTabSelected_SelectedTab_EdgeTab_HideCloseBtn() {
mStripLayoutHelper.setStripLayoutTabsForTest(tabs);
when(tabs[3].getDrawX()).thenReturn(600.f);

mStripLayoutHelper.tabSelected(1, 3, 0);
mStripLayoutHelper.tabSelected(1, 3, 0, false);

// Close btn should be hidden on the selected tab as its an edge tab.
Mockito.verify(tabs[3]).setCanShowCloseButton(false);
Expand All @@ -238,7 +239,7 @@ public void testTabSelected_EdgeTab_Start_Ltr_HideCloseBtn() {
mStripLayoutHelper.setStripLayoutTabsForTest(tabs);

// Act
mStripLayoutHelper.tabSelected(1, 3, 0);
mStripLayoutHelper.tabSelected(1, 3, 0, false);

// Assert
// Close btn should be hidden for the partially visible edge tab.
Expand Down Expand Up @@ -266,7 +267,7 @@ public void testTabSelected_EdgeTab_End_Ltr_HideCloseBtn() {
mStripLayoutHelper.setStripLayoutTabsForTest(tabs);

// Act
mStripLayoutHelper.tabSelected(1, 3, 0);
mStripLayoutHelper.tabSelected(1, 3, 0, false);

// Assert
// Close button is visible for the rest of the tabs.
Expand All @@ -292,7 +293,7 @@ public void testTabSelected_LastTab_ShowCloseBtn() {
mStripLayoutHelper.setStripLayoutTabsForTest(tabs);

// Act
mStripLayoutHelper.tabSelected(1, 3, 0);
mStripLayoutHelper.tabSelected(1, 3, 0, false);

// Assert
// Close button is visible for all tabs.
Expand All @@ -318,7 +319,7 @@ public void testTabSelected_LastTab_EdgeTab_HideCloseBtn() {
mStripLayoutHelper.setStripLayoutTabsForTest(tabs);

// Act
mStripLayoutHelper.tabSelected(1, 3, 0);
mStripLayoutHelper.tabSelected(1, 3, 0, false);

// Assert
// Close btn should be visible for rest of the tabs.
Expand Down Expand Up @@ -348,7 +349,7 @@ public void testTabSelected_EdgeTab_End_Ltr_NoModelSelBtn_HideCloseBtn() {
mStripLayoutHelper.setStripLayoutTabsForTest(tabs);

// Act
mStripLayoutHelper.tabSelected(1, 3, 0);
mStripLayoutHelper.tabSelected(1, 3, 0, false);

// Assert
// Close button is visible for the rest of the tabs.
Expand All @@ -374,7 +375,7 @@ public void testTabSelected_EdgeTab_Start_Rtl_HideCloseBtn() {
mStripLayoutHelper.setStripLayoutTabsForTest(tabs);

// Act
mStripLayoutHelper.tabSelected(1, 3, 0);
mStripLayoutHelper.tabSelected(1, 3, 0, false);

// Assert
// Close btn should be hidden for the partially visible edge tab.
Expand All @@ -401,7 +402,7 @@ public void testTabSelected_EdgeTab_End_Rtl_HideCloseBtn() {
mStripLayoutHelper.setStripLayoutTabsForTest(tabs);

// Act
mStripLayoutHelper.tabSelected(1, 3, 0);
mStripLayoutHelper.tabSelected(1, 3, 0, false);

// Assert
// Close button is visible for the rest of the tabs.
Expand Down Expand Up @@ -549,6 +550,72 @@ public void testScrollOffset_OnOrientationChange_SelectedTabNotVisible() {
assertEquals(initialFinalX, mStripLayoutHelper.getScroller().getFinalX());
}

@Test
@Feature("Tab Strip Improvements")
public void testTabSelected_AfterTabClose_SkipsAutoScroll() {
initializeTest(false, true, 3);
StripLayoutTab[] tabs = getMockedStripLayoutTabs(TAB_WIDTH_MEDIUM);
mStripLayoutHelper.setStripLayoutTabsForTest(tabs);
// Set initial scroller position to 1000.
mStripLayoutHelper.getScroller().setFinalX(1000);

// Act: close a non selected tab.
mStripLayoutHelper.handleCloseButtonClick(tabs[1], TIMESTAMP);

// Assert: scroller position is not modified.
assertEquals(1000, mStripLayoutHelper.getScroller().getFinalX());
}

@Test
@Feature("Tab Strip Improvements")
public void testTabSelected_AfterSelectedTabClose_DoesNotSkipAutoScroll() {
initializeTest(false, true, 3);
StripLayoutTab[] tabs = getMockedStripLayoutTabs(TAB_WIDTH_MEDIUM);
mStripLayoutHelper.setStripLayoutTabsForTest(tabs);
// Set initial scroller position to 1000.
mStripLayoutHelper.getScroller().setFinalX(1000);

// Act: close the selected tab.
mStripLayoutHelper.handleCloseButtonClick(tabs[3], TIMESTAMP);

// Assert: scroller position is modified.
assertNotEquals(1000, mStripLayoutHelper.getScroller().getFinalX());
}

@Test
@Feature("Tab Strip Improvements")
public void testTabCreated_RestoredTab_SkipsAutoscroll() {
initializeTest(false, true, 3);
StripLayoutTab[] tabs = getMockedStripLayoutTabs(TAB_WIDTH_MEDIUM);
mStripLayoutHelper.setStripLayoutTabsForTest(tabs);
// Set initial scroller position to 1000.
mStripLayoutHelper.getScroller().setFinalX(1000);

// Act: Tab was restored after undoing a tab closure.
boolean restoredTab = true;
mStripLayoutHelper.tabCreated(TIMESTAMP, 6, 3, false, restoredTab);

// Assert: scroller position is not modified.
assertEquals(1000, mStripLayoutHelper.getScroller().getFinalX());
}

@Test
@Feature("Tab Strip Improvements")
public void testTabCreated_NonRestoredTab_SkipsAutoscroll() {
initializeTest(false, true, 3);
StripLayoutTab[] tabs = getMockedStripLayoutTabs(TAB_WIDTH_MEDIUM);
mStripLayoutHelper.setStripLayoutTabsForTest(tabs);
// Set initial scroller position to 1000.
mStripLayoutHelper.getScroller().setFinalX(1000);

// Act: Tab was restored after undoing a tab closure.
boolean restoredTab = false;
mStripLayoutHelper.tabCreated(TIMESTAMP, 6, 3, false, restoredTab);

// Assert: scroller position is modified.
assertNotEquals(1000, mStripLayoutHelper.getScroller().getFinalX());
}

@Test
@Feature("Tab Strip Improvements")
public void testScrollDuration() {
Expand Down Expand Up @@ -601,7 +668,7 @@ private void initializeTest(boolean rtl, boolean incognito, int tabIndex, int nu
}
mModel.setIndex(tabIndex);
mStripLayoutHelper.setTabModel(mModel, null);
mStripLayoutHelper.tabSelected(0, tabIndex, 0);
mStripLayoutHelper.tabSelected(0, tabIndex, 0, false);
// Flush UI updated
}

Expand Down
16 changes: 16 additions & 0 deletions ...android/junit/src/org/chromium/chrome/browser/compositor/overlays/strip/TestTabModel.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,4 +64,20 @@ public void setIndex(int i, @TabSelectionType int type, boolean skipLoadingTab)
public List<Tab> getAllTabs() {
return mMockTabs;
}

/**
* Returns the next tab to be selected when a tab is closed.
* @param id Id of the tab being closed.
* @param uponExit ignored.
* @return The next tab if available or null.
*/
@Override
public Tab getNextTabIfClosed(int id, boolean uponExit) {
if (id > 0 && id < mMockTabs.size()) {
return mMockTabs.get(id - 1);
} else if (id == 0 && mMockTabs.size() > 1) {
return mMockTabs.get(1);
}
return null;
}
}

0 comments on commit 8bf4b88

Please sign in to comment.