From 74b245bc643e059783921a006645d6699a5d49af Mon Sep 17 00:00:00 2001 From: Carson Ramsden Date: Wed, 8 Oct 2025 10:58:52 -0600 Subject: [PATCH 1/3] Only persist tab data after tabs have been removed --- .../Client/TabManagement/TabManagerImplementation.swift | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/firefox-ios/Client/TabManagement/TabManagerImplementation.swift b/firefox-ios/Client/TabManagement/TabManagerImplementation.swift index 851ac019080e6..43c346b3bf994 100644 --- a/firefox-ios/Client/TabManagement/TabManagerImplementation.swift +++ b/firefox-ios/Client/TabManagement/TabManagerImplementation.swift @@ -253,13 +253,16 @@ class TabManagerImplementation: NSObject, } backupCloseTabs = tabs - for tab in currentModeTabs { - self.removeTab(tab.tabUUID) + for (index, tab) in currentModeTabs.enumerated() { + self.removeTab(tab, flushToDisk: false) + if tab == currentModeTabs.last { + self.updateSelectedTabAfterRemovalOf(tab, deletedIndex: index) + } } - // Save the tab state that existed prior to removals (preserves original selected tab) backupCloseTab = currentSelectedTab + // Persist changes after tabs have been removed commitChanges() } From c9d6fa1efdd908220fdb42954f87ea0dea143550 Mon Sep 17 00:00:00 2001 From: Carson Ramsden Date: Mon, 13 Oct 2025 12:19:33 -0600 Subject: [PATCH 2/3] wip --- .../Tests/ClientTests/TabManagement/TabManagerTests.swift | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/firefox-ios/firefox-ios-tests/Tests/ClientTests/TabManagement/TabManagerTests.swift b/firefox-ios/firefox-ios-tests/Tests/ClientTests/TabManagement/TabManagerTests.swift index 9cbefef261820..3b64bc3fb2609 100644 --- a/firefox-ios/firefox-ios-tests/Tests/ClientTests/TabManagement/TabManagerTests.swift +++ b/firefox-ios/firefox-ios-tests/Tests/ClientTests/TabManagement/TabManagerTests.swift @@ -104,10 +104,11 @@ class TabManagerTests: XCTestCase { @MainActor func testRemoveAllTabsCallsSaveTabSession() { let subject = createSubject() - _ = subject.addTab(URLRequest(url: URL(string: "https://mozilla.com")!), afterTab: nil, isPrivate: false) + let tab = subject.addTab(URLRequest(url: URL(string: "https://mozilla.com")!), afterTab: nil, isPrivate: false) + subject.selectTab(tab) subject.removeAllTabs(isPrivateMode: false) - XCTAssertEqual(mockSessionStore.saveTabSessionCallCount, 1) + XCTAssertEqual(mockSessionStore.saveTabSessionCallCount, 3) } @MainActor From eda3116ecbd751976814b6c5b9c5f25af49a3c53 Mon Sep 17 00:00:00 2001 From: Carson Ramsden Date: Tue, 14 Oct 2025 10:46:41 -0600 Subject: [PATCH 3/3] clean up behavior and fix tests --- .../TabManagerImplementation.swift | 22 ++++++++++++++----- .../TabManagement/TabManagerTests.swift | 22 +++++++++++++++++-- 2 files changed, 37 insertions(+), 7 deletions(-) diff --git a/firefox-ios/Client/TabManagement/TabManagerImplementation.swift b/firefox-ios/Client/TabManagement/TabManagerImplementation.swift index 43c346b3bf994..9ec95a7f1cb62 100644 --- a/firefox-ios/Client/TabManagement/TabManagerImplementation.swift +++ b/firefox-ios/Client/TabManagement/TabManagerImplementation.swift @@ -251,19 +251,31 @@ class TabManagerImplementation: NSObject, restorePosition: tabs.firstIndex(of: tab), isSelected: selectedTab?.tabUUID == tab.tabUUID) } + + // Backup tabs for tab undo, this is not a feature on iPhone but is on iPad backupCloseTabs = tabs - for (index, tab) in currentModeTabs.enumerated() { + // Scroll position for most tabs has been stored via session data when we navigate away, + // but we need to save the selected tabs session data to persist scroll position + saveSessionData(forTab: selectedTab) + + for tab in currentModeTabs { + // Remove each tab without persisting changes self.removeTab(tab, flushToDisk: false) if tab == currentModeTabs.last { - self.updateSelectedTabAfterRemovalOf(tab, deletedIndex: index) + // Select tab calls preserve tabs so we don't need to call it again + if tab.isPrivate, + let mostRecentActiveTab = mostRecentTab(inTabs: normalActiveTabs) { + // We remove all private tabs so select most recent normal tab + selectTab(mostRecentActiveTab) + } else { + // For normal tabs create a new tab and select it + selectTab(addTab()) + } } } // Save the tab state that existed prior to removals (preserves original selected tab) backupCloseTab = currentSelectedTab - - // Persist changes after tabs have been removed - commitChanges() } /// Remove a tab, will notify delegate of the tab removal diff --git a/firefox-ios/firefox-ios-tests/Tests/ClientTests/TabManagement/TabManagerTests.swift b/firefox-ios/firefox-ios-tests/Tests/ClientTests/TabManagement/TabManagerTests.swift index 3b64bc3fb2609..ba5bf1fce046a 100644 --- a/firefox-ios/firefox-ios-tests/Tests/ClientTests/TabManagement/TabManagerTests.swift +++ b/firefox-ios/firefox-ios-tests/Tests/ClientTests/TabManagement/TabManagerTests.swift @@ -108,17 +108,35 @@ class TabManagerTests: XCTestCase { subject.selectTab(tab) subject.removeAllTabs(isPrivateMode: false) + // Save tab session is actually called 3 times for one remove all call + // 1. Save tab session for currently selected tab before delete to preserve scroll position + // 1. AddTab for the new created homepage tab calls commitChanges + // 2. selectTab persists changes for currently selected tab before moving to new Tab XCTAssertEqual(mockSessionStore.saveTabSessionCallCount, 3) } @MainActor - func testRemoveAllTabsForNotPrivateMode() { + func testRemoveAllTabsForNotPrivateModeWhenClosePrivateTabsSettingIsFalse() { + (mockProfile.prefs as? MockProfilePrefs)?.things[PrefsKeys.Settings.closePrivateTabs] = false var tabs = generateTabs(count: 5) tabs.append(contentsOf: generateTabs(ofType: .privateAny, count: 4)) let subject = createSubject(tabs: tabs) XCTAssertEqual(subject.tabs.count, 9) subject.removeAllTabs(isPrivateMode: false) - XCTAssertEqual(subject.tabs.count, 4) + // 5, private mode tabs (4) plus one new normal tab (1) + XCTAssertEqual(subject.tabs.count, 5) + } + + @MainActor + func testRemoveAllTabsForNotPrivateModeWhenClosePrivateTabsSettingIsTrue() { + (mockProfile.prefs as? MockProfilePrefs)?.things[PrefsKeys.Settings.closePrivateTabs] = true + var tabs = generateTabs(count: 5) + tabs.append(contentsOf: generateTabs(ofType: .privateAny, count: 4)) + let subject = createSubject(tabs: tabs) + XCTAssertEqual(subject.tabs.count, 9) + subject.removeAllTabs(isPrivateMode: false) + // One new normal tab (1) + XCTAssertEqual(subject.tabs.count, 1) } @MainActor