1915174 - Restore closed tabs to their original tab group

Status: RESOLVED FIXED

(Firefox :: Session Restore, task, P2)

Comment 1

[Amy Lee [:amylee] UX]

Hi, this is the section in the spec where it addresses the question of what happens to the tab when restoring the tab from "Recently closed tabs" but the group no longer exists.

https://www.figma.com/design/f6J2lH6XDxLxyzOLr2E8oE/Tab-Grouping-Spec?node-id=10601-222215&t=juV4GgTIcXNP9AZV-1

If a user clicks on a link in "Recently closed tabs" and it used to belong to a group but the group has been closed/deleted, it opens as a single tab (not in a group). If the deleted/closed group is reopened, the single tab remains to be single (doesn’t rejoin the group)

Comment 2

[Stephen Thompson [:sthompson]]

Current behavior:

• 4 tabs (grouped 1, grouped 2, grouped 3, standalone 1), grouped 2 active, close tab grouped 2, Cmd + Shift + T reopens grouped 2 in the same position and within the same tab group
• 4 tabs (grouped 1, grouped 2, grouped 3, standalone 1), grouped 3 active, close tab grouped 3, Cmd + Shift + T reopens grouped 3 in the same position but it is no longer in the tab group
• 4 tabs (standalone 1, grouped 1, grouped 2, standalone 2), standalone 1 active, close tab standalone 1, Cmd + Shift + T reopens standalone 1 in the same position but it is part of the group with grouped 1 and grouped 2

Desired behavior:

• If the closed tab was in a group and the group still exists, the tab should reopen inside of the group and we should attempt to restore the tab to the position that the tab had inside of the group
• If the closed tab was in a group and the group no longer exists, reopen the tab as a standalone tab. Try to reopen the tab to its previous position, but if that is not available, it's reasonable to open the tab at the end of the tab strip
• If the closed tab was not in a group, the reopened tab should always be opened as a standalone tab

Comment 4

[Jeremy Swinarton]

While working on bug1908422 I did some investigation, and came up with a list of session restore methods that will need to be modified in order to support undoing of closed tab groups: undoClosedTab, undoClosedTabFromClosedWindow, forgetClosedTab, forgetClosedTabById, undoCloseById, restoreLastSession, restoreWindows. We may also need an undoCloseGroup method in /base/content/browser.js.

Comment 5

[Jeremy Swinarton]

There was some conversation on https://phabricator.services.mozilla.com/D226397 about how to respect the max_tabs_undo and max_windows_undo preferences for session restore. I did some investigation to get a better understanding of how this all works:

• max_tabs_undo limits the number of tabs that can be restored in a window. This limits the number of tabs shown in History > Recently Closed Tabs to 25 (by default) and limits the number of tabs that can be restored by keyboard (Ctrl+Shift+T).
• However, if an entire window is closed, this pref is not respected. If I close a window with 50 tabs, under History > Recently Closed Windows , an entry appears called something like “New Tab (and 49 other tabs)”, and clicking this restores the whole window, i.e. max_tabs_undo is ignored.
• Quitting the app with 50 open tabs while having the “Open previous windows and tabs” setting checked in about:preferences restores all tabs on startup.
• max_windows_undo limits the number of windows that can be restored in a session. Similar to max_tabs_undo , only 5 (by default) windows appear in History > Recently Closed Windows and can be restored by Ctrl+Shift+T. Quitting the app with 10+ windows and re-opening with the “Open previous windows and tabs” setting set restores all windows.

In my opinion, the simplest and most intuitive option is to enforce the same rules for tabs within tab groups without adding additional logic to count tabs differently if they are in a group. Therefore, if a user has 3 groups with 10 tabs each and closes them all and tries to reopen them, the third tab group would only have 5 tabs in it.

I think this is reasonable considering that with the “saved groups” feature we are effectively implementing a separate layer of history management just for tabs within groups. These rules only apply if a user “deletes” a group, meaning that there should be no expectation that these groups can be restored in a perfect state.

Comment 6

[DJ (:dwalker)]

(In reply to Jeremy Swinarton from comment #5)

In my opinion, the simplest and most intuitive option is to enforce the same rules for tabs within tab groups without adding additional logic to count tabs differently if they are in a group. Therefore, if a user has 3 groups with 10 tabs each and closes them all and tries to reopen them, the third tab group would only have 5 tabs in it.

I think this is reasonable considering that with the “saved groups” feature we are effectively implementing a separate layer of history management just for tabs within groups. These rules only apply if a user “deletes” a group, meaning that there should be no expectation that these groups can be restored in a perfect state.

I agree, it would be fine to obey the existing limits given that it applies to deleted groups and not closed groups, and given the semantics around deleting.

Comment 7

[Stephen Thompson [:sthompson]]

Sweet, that makes sense to me! Sounds like the limits are more nuanced than I expected.

Comment 8

[DJ (:dwalker)]

The above was discussed in a meeting between myself, :sthompson, :jswinarton, and :dao. Our decision was to introduce a new pref, e.g. max_groups_undo, that will limit the number of groups that get stored. Similar to max_window_undo, there is no limit on the number of tabs that can be restored to a group this way.

Comment 9

[DJ (:dwalker)]

Attachment: WIP: Bug 1915174 - restore tabs to their group if it still exists. r?dao,sthompson

Comment 10

[DJ (:dwalker)]

(In reply to Stephen Thompson [:sthompson] from comment #2)

4 tabs (standalone 1, grouped 1, grouped 2, standalone 2), standalone 1 active, close tab standalone 1, Cmd + Shift + T reopens standalone 1 in the same position but it is part of the group with grouped 1 and grouped 2

Noting here that this behavior is unrelated to session restore; moving a tab to index 0 when there is a group at index 0 will always place the tab in the group. This is somewhat related to bug 1921830 but we could also file a separate bug to sort out this tab placement with respect to groups.

Comment 11

[Stephen Thompson [:sthompson]]

(In reply to DJ (:dwalker) from comment #10)

(In reply to Stephen Thompson [:sthompson] from comment #2)

4 tabs (standalone 1, grouped 1, grouped 2, standalone 2), standalone 1 active, close tab standalone 1, Cmd + Shift + T reopens standalone 1 in the same position but it is part of the group with grouped 1 and grouped 2

Noting here that this behavior is unrelated to session restore; moving a tab to index 0 when there is a group at index 0 will always place the tab in the group. This is somewhat related to bug 1921830 but we could also file a separate bug to sort out this tab placement with respect to groups.

OK, cool, if that specific behavior is just coming from the pre-existing issues with placing tabs at ambiguous tab indexes (right before tab group/first tab in tab group, last tab in tab group/right after tab group), then I would just note which gBrowser method session restore is using to replace the closed tabs. Looks like it might be addTrustedTab https://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.sys.mjs#3790 which uses addTab which uses _insertTabAtIndex, so I'd hope that this behavior was fixed on central a day or two ago https://bugzilla.mozilla.org/show_bug.cgi?id=1922268

Comment 12

[Jeremy Swinarton]

Attachment: Bug 1915174: Restore closed tabs to their original tab group r?dao,dwalker

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/72b41ba4471f
Restore closed tabs to their original tab group r=dao,sessionstore-reviewers

Comment 14

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/72b41ba4471f