1819675 - Expand recently closed tabs to include all Windows

Status: RESOLVED FIXED

(Firefox :: Session Restore, enhancement, P2)

Description

[Sarah Clements [:sclements]]

Per the UX and product tickets, we want to modify recently closed tabs to include closed tabs for all windows, and this should apply to all desktop surfaces where recently closed tabs are displayed.

Some technical details that have been agreed upon by the UX team in the working doc:

• If multiple windows are open, any tabs closed from those different windows will be displayed in Firefox View.
• If a user selects a recently closed tab, it will open in a new tab in that window regardless of the window it was originally opened in.
• Tabs should persist in the list even after the windows that contain the tabs are closed
• After a user ends a session/closes the last window, the closed tabs from all previous windows will persist for 7 days (moved to bug 1820660).
• If the Firefox View tab is selected on one window (window A), and a tab is closed in another window (window B), the recently closed tab list in window A will immediately update with the closed tab from window B.
• Don't include private windows in the list (keep private tabs local to private windows)
• String/content change - TBD

Comment 1

[Sam Foster [:sfoster] (he/him)]

(In reply to Sarah Clements [:sclements] from comment #0)

• After a user ends a session/closes the last window, the closed tabs from all previous windows will persist for 24 hours.

I'm proposing to move this bit into its own task, in bug 1820660.

Comment 2

[Sarah Clements [:sclements]]

(In reply to Sam Foster [:sfoster] (he/him) from comment #1)

(In reply to Sarah Clements [:sclements] from comment #0)

• After a user ends a session/closes the last window, the closed tabs from all previous windows will persist for 24 hours.

I'm proposing to move this bit into its own task, in bug 1820660.

Ok, but we want to ship those two bugs together since that's part of the feature change. Can I assign you to the new bug you created or would you rather someone else picked it up? We should also combine that bug with any string changes that Katie decides - I believe that is still TBD but I'll get clarity on when we can expect a decision.

Comment 3

[klower]

Strings to change because of this interaction change:

• Remove this line of body copy: Reopen pages you’ve closed in this window. No replacement copy is needed. We will just have the header here.
• Change header
  Current: Recently closed
  Update to: Recently closed tabs
  Why? We will not have body copy, so the header needs to be more specific about the scope of this section.
• In the empty state, change this line of copy:
  Current: When you close a tab in this window, you can fetch it from here.
  Update to: When you close a tab, you can fetch if from here.

Comment 4

[Sam Foster [:sfoster] (he/him)]

(In reply to Sarah Clements [:sclements] from comment #2)

I'm proposing to move this bit into its own task, in bug 1820660.
Ok, but we want to ship those two bugs together since that's part of the feature change. Can I assign you to the new bug you created or would you rather someone else picked it up?

I don't have strong opinions on who implements it, it just seemed orthogonal to the rest of the changes and something we could work on independently, and would increase the scope and risk of this bug significantly if they remained bundled together. My plan is to implement this part first - listing recently closed tabs from all windows. It seems a clean split? This part will work with or without the other. And 1820660 can be worked on with or without this - as we already expose recently closed tabs and windows elsewhere. It also looks like no strings would need to be changed if we land a patch here then later land bug 1820660. We can put both behind our feature pref, or hold to nightly or whatever.

Comment 5

[Sarah Clements [:sclements]]

(In reply to Sam Foster [:sfoster] (he/him) from comment #4)

I don't have strong opinions on who implements it, it just seemed orthogonal to the rest of the changes and something we could work on independently, and would increase the scope and risk of this bug significantly if they remained bundled together. My plan is to implement this part first - listing recently closed tabs from all windows. It seems a clean split? This part will work with or without the other. And 1820660 can be worked on with or without this - as we already expose recently closed tabs and windows elsewhere.

Sure, but we are changing the recently closed tabs API behavior at the session store level for this bug, not just in Firefox View (but the string changes are just for Firefox View). You probably want to adjust the story points for this bug though, unless you think it should still be an 8 - it was sized with the persistence change in mind. But yeah, to minimize risk and avoid having a large patch it makes sense to separate them. I'll pick that bug up and we can coordinate landing them.

Comment 6

[Sam Foster [:sfoster] (he/him)]

The requirement that we be able to re-open (undo-close) tabs in the current window - rather than the window they were closed in - will be new functionality for SessionStore. I envisage passing an optional aTargetWindow into undoCloseTab and undoCloseById. That should let us separate out the retrieving and removing of the closed tab entry from which tabbrowser we open it into.

I'm getting a bit lost in the tests in browser/base/content/test/tabs/ and browser/components/sessionstore/tests though - can you advise on what kind of test coverage you'd expect for that kind of change to SessionStore :mak? I'll obviously have coverage for the FirefoxView interactions but a unit test or other less-integration-y test for SessionStore itself seems appropriate. Any other major gotchas I'm not spotting here?

Comment 7

[Marco Bonardo [:mak]]

(In reply to Sam Foster [:sfoster] (he/him) from comment #6)

The requirement that we be able to re-open (undo-close) tabs in the current window - rather than the window they were closed in - will be new functionality for SessionStore. I envisage passing an optional aTargetWindow into undoCloseTab and undoCloseById. That should let us separate out the retrieving and removing of the closed tab entry from which tabbrowser we open it into.

I think that should work, though it will only work as far as the windows are open, if the user closes the last tab of a window (and as a consequence that window is closed), afaict those 2 methods won't be able to reopen the tab, because they only go through alive windows.
Though that may fall into the persistence problem.

I'm getting a bit lost in the tests in browser/base/content/test/tabs/ and browser/components/sessionstore/tests though - can you advise on what kind of test coverage you'd expect for that kind of change to SessionStore :mak?

I must first put a disclaimer that I'm not an expert of Session Restore (I have not touched its code in ages), Dao is the current owner and may have a clearer picture in mind of its direction. That said, as a browser peer, I'd expect a test of the Session restore APIs if you're adding new arguments to them. Probably a browser_undoCloseInDifferentWindow.js under browser/components/sessionstore/test (There is a browser_undoCloseById.js test you could add to, but it seems cleaner to put specific target window testing in their own test file).

Comment 8

[Sam Foster [:sfoster] (he/him)]

:dao, I was looking at the closeById tests and wondered at the setTimeout(callback, 20); in the closeWindow helper in there. I tried a patch using waitForTick and topicObserved. It seemed ok with the few re-triggers I did there. I guess we'd want lots and lots to be confident it wouldn't immediately go intermittent again?

Comment 9

[Dão Gottwald [:dao]]

(In reply to Sam Foster [:sfoster] (he/him) from comment #8)

:dao, I was looking at the closeById tests and wondered at the setTimeout(callback, 20); in the closeWindow helper in there. I tried a patch using waitForTick and topicObserved. It seemed ok with the few re-triggers I did there. I guess we'd want lots and lots to be confident it wouldn't immediately go intermittent again?

Yeah, that's a good change, definitely better than setTimeout(callback, 20) which is inherently fragile. It looks like sessionstore-closed-objects-changed was implemented after that test got added so that's why it didn't use this from the start.

Comment 10

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1819675 - Include tabs from all windows in all views of recently-closed tabs and re-open them into the current window. r?sclements!,dao

• Ensure we filter out tabs without any useful state from recently-closed windows
• Add a target window argument to undoCloseTab and undoCloseById
• undoCloseTab will remove the tab data from the source window collection and re-open the tab into the target window
• WIP on session store tests for the new undoClose* argument

TODO:

• This only covers the case where we close a tab in a window that remains open.
• Restoring tabs from recently-closed windows will be a seperate patch on this bug.

Comment 11

[Sam Foster [:sfoster] (he/him)]

"modify recently closed tabs to include closed tabs for all windows, and this should apply to all desktop surfaces where recently closed tabs are displayed"

Moving component to better reflect the scope of this bug, which touches both Firefox View and the Library/History menu.

Comment 12

[Sarah Clements [:sclements]]

(In reply to Sam Foster [:sfoster] (he/him) from comment #11)

"modify recently closed tabs to include closed tabs for all windows, and this should apply to all desktop surfaces where recently closed tabs are displayed"

Moving component to better reflect the scope of this bug, which touches both Firefox View and the Library/History menu.

I'd think it would make sense to move it to session store, since that's where the changes will be made?

Comment 13

[:Gijs (he/him)]

(In reply to Sarah Clements [:sclements] from comment #0)

• Don't include private windows (I believe this is already implemented)

Can you elaborate a little bit on this?

What we implement right now is that the lists are per-window, so naturally private tabs don't show up in non-private windows because no tabs ever show up cross-window. We also don't support reopening closed private windows. But we do support reopening tabs in private windows. This is different from other browsers and occasionally surprises people, though as an "undo" mechanism, I think it's really quite useful so I'd personally be sad if we turned that off entirely (and I imagine I won't be the only one, but who knows).

The other twist there is permanent private browsing / never remember history. Those windows will return true for PrivateBrowsingUtils.isWindowPrivate() but should probably retain undo close tab functionality.

Other than that, I see a few different but reasonable interpretations of your statement:

1. stop supporting "undo close tab" in private windows entirely.
2. separate out 2 collections, one for all private windows and one for all non-private windows. When the last private window closes (last-pb-context-exiting), clear the private window collection.
3. like (2) but remove each window's closed tabs from the collection whenever a window closes (more closely resembles current behaviour in terms of not being able to resurrect tabs from closed private windows)
4. keep private tabs local to each private window

Which one did you and UX agree upon?

Comment 14

[Sarah Clements [:sclements]]

Thanks for the questions Gijs. What was meant by "don't include private windows" was not having closed tabs from private windows appear in the recently closed tabs for all windows list. So it should exclude private windows and keep private tabs local to each private window. I've updated the description.

Comment 15

[Sam Foster [:sfoster] (he/him)]

(In reply to Sarah Clements [:sclements] from comment #12)

I'd think it would make sense to move it to session store, since that's where the changes will be made?

There's a Session Restore component, but not one for SessionStore. Those 2 are obviously related, but the changes we're making here should have no impact on Session Restore behavior. Looking at hg blame, Bug 887515 was in Firefox Menus, Bug 254021 was in Tabbed Browser, Bug 1308061 in Session Restore. So I dont know what the correct component is for this..

Comment 16

[:Gijs (he/him)]

(In reply to Sam Foster [:sfoster] (he/him) from comment #15)

(In reply to Sarah Clements [:sclements] from comment #12)

I'd think it would make sense to move it to session store, since that's where the changes will be made?

There's a Session Restore component, but not one for SessionStore.

I don't think the difference between these two concepts is meaningful, I think it's just a naming quirk. The SessionStore code has its bugs filed in Firefox :: Session Restore, and the metadata in m-c indicates as much. Putting this in Session Restore seems reasonable to me.

Comment 17

[Sam Foster [:sfoster] (he/him)]

(In reply to :Gijs (he/him) from comment #16)

I don't think the difference between these two concepts is meaningful, I think it's just a naming quirk. The SessionStore code has its bugs filed in Firefox :: Session Restore, and the metadata in m-c indicates as much. Putting this in Session Restore seems reasonable to me.

Done.

Comment 18

[Sam Foster [:sfoster] (he/him)]

There's a comment at https://searchfox.org/mozilla-central/source/browser/components/extensions/parent/ext-sessions.js#36-41 which suggests the extension.canAccessWindow(window) check is really about determining if the window is private one. Is this guarding against any other cases where the extension shouldn't be accessing recent tabs data from some browser window? Or would a PrivateBrowsingUtils.isBrowserPrivate(window) be interchangeable there?

Another question: SessionStore gives a __Ssid property to browser windows at browser-window-before-show. It uses this id as a key into the various session data collections. The sessions extension API uses ids from windowTracker.getId(window). Do you know why the difference there? This ext-sessions.js implementation is actually a nice shape for what we need in firefox view, but I think this identifier disagreement is going to spell trouble for trying to share any code here. Any background or context you have would be great - here or you can find me on matrix.

Comment 19

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Sam Foster [:sfoster] (he/him) from comment #18)

There's a comment at https://searchfox.org/mozilla-central/source/browser/components/extensions/parent/ext-sessions.js#36-41 which suggests the extension.canAccessWindow(window) check is really about determining if the window is private one. Is this guarding against any other cases where the extension shouldn't be accessing recent tabs data from some browser window? Or would a PrivateBrowsingUtils.isBrowserPrivate(window) be interchangeable there?

extension.canAccessWindow(window) is meant to check if the window can be accessed based on the window being private or not and the extension being allowed to access private window or not:

That call to extension.canAccessWindow(window) will trigger a call to WebExtensionPolicy::CanAccessWindow method defined here in the WebExtensionPolicy C++ implementation and the method is going to return true earlier if the extension has the "privateBrowsingAllowed" extension permission (user-controlled from a checkbox in the post install doorhanger and in the addon detail view in about:addons) otherwise is going to return true only for non-private windows.

parent/ext-sessions.js runs in the parent process and so it is meant to be checking that on browser windows, and extension.canAccessWindow(window) should be similar to checking extension.privateBrowsingAllowed ? true : !PrivateBrowsingUtils.isBrowserPrivate(window).

Do you have some more details about why we would consider changing extension.canAccessWindow(window) with something else?
(I want to be sure I'm not missing something about the context around your question).

Another question: SessionStore gives a __Ssid property to browser windows at browser-window-before-show. It uses this id as a key into the various session data collections. The sessions extension API uses ids from windowTracker.getId(window). Do you know why the difference there? This ext-sessions.js implementation is actually a nice shape for what we need in firefox view, but I think this identifier disagreement is going to spell trouble for trying to share any code here. Any background or context you have would be great - here or you can find me on matrix.

The session API is part of the WebExtensions APIs we share with the other browser vendors and the sessions.Session type is documented to be expected to include an tabs.Tab or windows.Window that have been, and so the id of the windows.Window objects are expected to have the same id that it would have had when returned by the windows API, see the related API docs on MDN:

• https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/sessions/Session#window
• https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/windows/Window#id

and so in the session API we will be using the windowTracker.getId(window) to get the exact same id that was assigned to that window also for the windows API.

The actual value is assigned here by this DefaultMap and it is currently the derived from window.docShell.outerWindowID.

Let me know if these additional details were the ones you were looking for (or also if I misunderstood your question, or if the answer actual raises more questions ;-)).

Comment 20

[Kris Maglione [:kmag]]

I think Luca answered this sufficiently. Though I would add that CanAccessWindow could potentially add more checks in the future based on other attributes. For instance, the user could potentially allow a specific extension to access a specific window, similar to the way the activeTab extension allows access to the contents of a specific tab after user interaction. So I wouldn't want to replace any of its uses with more specific checks unless it can't be avoided.

Comment 21

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1819675 - rename SessionStore.getClosedTabCount and getClosedTabData to getClosedTabCountForWindow and getClosedTabDataForWindow. r?dao!

• As closed tabs will change to mean closed tabs from all windows, rename this function to make
  changes in later patches clearer when we mean closed tabs from this window specifically, or closed
  tabs for all private/non-private windows

Comment 22

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1819675 - Add some basic tests for SessionStore's closed tab functions with multiple open windows r?dao!

windows r?dao!

• Verify the main recently-closed tabs functions of SessionStore as applied to different windows

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5b25c664a976
Add some basic tests for SessionStore's closed tab functions with multiple open windows r=dao
https://hg.mozilla.org/integration/autoland/rev/dff93b6ca833
rename SessionStore.getClosedTabCount and getClosedTabData to getClosedTabCountForWindow and getClosedTabDataForWindow. r=dao,fxview-reviewers,kcochrane

Comment 24

[Sam Foster [:sfoster] (he/him)]

Adding leave-open as I'm landing the first couple patches which add tests and some refactoring while the rest is in review & iteration.

Comment 25

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/5b25c664a976
https://hg.mozilla.org/mozilla-central/rev/dff93b6ca833

Comment 26

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1819675 - Introduce a feature pref to toggle the recently-closed tabs from all windows behavior.r?dao!,sclements

• Add a default-true pref to provide an escape hatch allowing us to revert to previous behavior
• in which recently-closed tabs are per-window,
• and undoing closed tabs restores them to the window they were closed from.

Depends on D174501

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e5be8edfd9cf
Include tabs from all windows in all views of recently-closed tabs and re-open them into the current window. r=dao,sclements,fxview-reviewers,fluent-reviewers,flod,robwu,sessionstore-reviewers,tabbrowser-reviewers
https://hg.mozilla.org/integration/autoland/rev/aa4368bfb81a
Introduce a feature pref to toggle the recently-closed tabs from all windows behavior.r=sclements,dao,extension-reviewers,fxview-reviewers,robwu,sessionstore-reviewers

Comment 28

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/e5be8edfd9cf
https://hg.mozilla.org/mozilla-central/rev/aa4368bfb81a

Comment 29

[Sam Foster [:sfoster] (he/him)]

This is resolved with the landing of the patches in comment #28.
Further work will continue in bug 1832064

Comment 30

[Cosmin Sabou [:CosminS]]

Sam, there's currently quite a large (and growing) number of new filed bugs that all seem to have at least this line in the log:
JavaScript error: resource:///modules/AsyncTabSwitcher.sys.mjs, line 1187: TypeError: can't access property "spec", tab.linkedBrowser.currentURI is null
eg of failure log: https://treeherder.mozilla.org/logviewer?job_id=421960149&repo=autoland&lineNumber=5342
Could the changes in this bug have affected these tests that are failing? They are several and across different components.

There are also these new bugs that all have this failure line
uncaught exception - TypeError: this.mBrowser.urlbarChangeTracker.startedLoad is not a function at onStateChange@chrome://browser/content/tabbrowser.js:6670:47
eg of failure log: https://treeherder.mozilla.org/logviewer?job_id=421964403&repo=autoland&lineNumber=7355
Could you have a look over them and investigate what is going on? Thank you.

Comment 31

[Cosmin Sabou [:CosminS]]

For now comment 28 has been backed out for reasons exposed in comment 30. Let's see in the coming days if those kind of bugs are still present even after this backout.

Backout link: https://hg.mozilla.org/integration/autoland/rev/617973a17fb8fa9e833bd1e5a5c2d029fb94f31f

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cae698261ec2
Include tabs from all windows in all views of recently-closed tabs and re-open them into the current window. r=dao,sclements,fxview-reviewers,fluent-reviewers,flod,robwu,sessionstore-reviewers,tabbrowser-reviewers
https://hg.mozilla.org/integration/autoland/rev/b5dd5d7e4b86
Introduce a feature pref to toggle the recently-closed tabs from all windows behavior.r=sclements,dao,extension-reviewers,fxview-reviewers,robwu,sessionstore-reviewers

Comment 33

[Cosmin Sabou [:CosminS]]

I've relanded this because those failures still happen even after the backout, will file general bugs for those failures.

Comment 34

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/cae698261ec2
https://hg.mozilla.org/mozilla-central/rev/b5dd5d7e4b86

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Is this something we wanted to call out in the Fx117 relnotes?

Comment 36

[Sam Foster [:sfoster] (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #35)

Is this something we wanted to call out in the Fx117 relnotes?

It is a behavior change for nightly users though, but let me ask around. This is part of a series of changes we are making to recently-closed tabs, and we're holding this to nightly for now until the rest lands and settles.

Comment 37

[Sam Foster [:sfoster] (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #35)

Is this something we wanted to call out in the Fx117 relnotes?

I think until we land a patch to let this ride the trains only nightly users are going to see this, so I think the answer is no?
We can and should let nightly users know though - I'll add it to the minutes of the desktop biweekly meeting.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

We can add a nightly-only relnote too to give broader exposure.

Comment 39

[tabmix.onemen]

Both Reopen Closed Tab menu items "context_undoCloseTab" and "toolbar-context-undoCloseTab" are enabled When SessionStore.getClosedTabCount() > 0

tabbrowser.js - updateContextMenu

    document.getElementById("context_undoCloseTab").disabled =
      SessionStore.getClosedTabCount() == 0;

browser.js - onViewToolbarsPopupShowing

    document.getElementById("toolbar-context-undoCloseTab").disabled =
      SessionStore.getClosedTabCount() == 0;

however in undoCloseTab function we only restore "lastClosedTabCount for the source window. When aIndex is undefined".

  // We are specifically interested in the lastClosedTabCount for the source window.
  // When aIndex is undefined, we restore all the lastClosedTabCount tabs.
  let lastClosedTabCount = SessionStore.getLastClosedTabCount(sourceWindow);

The result is that when browser.sessionstore.closedTabsFromAllWindows is true user can see enabled Reopen Closed Tab but it will not reopen any tabs if all closed tabs are from other opened windows.

Related issue is when also browser.sessionstore.closedTabsFromClosedWindows is true
If last closed tab is from closed window, and there are no closed tabs from opened windows, should restoreLastClosedTabOrWindowOrSession (Ctrl-Shift-T), open the closed tab/window ???.

Comment 40

[:Gijs (he/him)]

(In reply to tabmix.onemen from comment #39)

(Sorry, I missed this comment when originally made and now I'm finding it when digging through unread bugmail. Long time no chat, onemen!)

The result is that when browser.sessionstore.closedTabsFromAllWindows is true user can see enabled Reopen Closed Tab but it will not reopen any tabs if all closed tabs are from other opened windows.

Can you provide some steps that reproduce this issue? I tried e.g.:

1. clear all recent history (so previous session's tabs aren't in closed tabs)
2. open new window
3. open example.com in new window
4. open new tab in new window
5. close example.com first, then close new (blank) tab

and at this point restoring a tab is greyed out. (Which actually seems like a bug to me, but not quite what you're describing... I think?)

Related issue is when also browser.sessionstore.closedTabsFromClosedWindows is true
If last closed tab is from closed window, and there are no closed tabs from opened windows, should restoreLastClosedTabOrWindowOrSession (Ctrl-Shift-T), open the closed tab/window ???.

That is what it does for me - if I repeat the steps above but without opening the blank tab, then cmd-shift-t (macOS) will reopen the window for me.

Can you file a new bug with steps to reproduce the problem you're outlining? :-)

Comment 41

[tabmix.onemen]

(In reply to :Gijs (he/him) from comment #40)

see bug 191884