Closed Bug 1819675 Opened 1 year ago Closed 8 months ago

Expand recently closed tabs to include all Windows

Categories

(Firefox :: Session Restore, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
117 Branch
Tracking Status
firefox117 --- fixed

People

(Reporter: sclements, Assigned: sfoster)

References

(Blocks 3 open bugs, Regressed 2 open bugs)

Details

(Whiteboard: [fidefe-firefox-view])

Attachments

(4 files)

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
Assignee: nobody → sfoster

(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.

(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.

See Also: → 1820660

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.

(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.

Status: NEW → ASSIGNED

(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.

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?

Flags: needinfo?(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).

Flags: needinfo?(mak)

: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?

Flags: needinfo?(dao+bmo)

(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.

Flags: needinfo?(dao+bmo)
  • 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.

"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.

Component: Firefox View → General

(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?

(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?

Flags: needinfo?(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.

Flags: needinfo?(sclements)
Blocks: 1827393

(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..

(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.

Attachment #9326688 - Attachment description: WIP: Bug 1819675 - Included recently closed tabs from all windows in the fx-view list → WIP: Bug 1819675 - WIP Included recently closed tabs from all windows in the fx-view list

(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.

Component: General → Session Restore
Severity: -- → N/A
Priority: -- → P2
Attachment #9326688 - Attachment description: WIP: Bug 1819675 - WIP Included recently closed tabs from all windows in the fx-view list → Bug 1819675 - Include tabs from all windows in all views of recently-closed tabs. r?sclements!,dao
Attachment #9326688 - Attachment description: Bug 1819675 - Include tabs from all windows in all views of recently-closed tabs. r?sclements!,dao → WIP: Bug 1819675 - Include tabs from all windows in all views of recently-closed tabs. r?sclements!,dao
Attachment #9326688 - Attachment description: WIP: Bug 1819675 - Include tabs from all windows in all views of recently-closed tabs. r?sclements!,dao → Bug 1819675 - Include tabs from all windows in all views of recently-closed tabs. r?sclements!,dao
Blocks: 1831229

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.

Flags: needinfo?(kmaglione+bmo)

(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:

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 ;-)).

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.

Flags: needinfo?(kmaglione+bmo)
  • 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
Attachment #9326688 - Attachment description: Bug 1819675 - Include tabs from all windows in all views of recently-closed tabs. r?sclements!,dao → 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
Blocks: 1833522

windows r?dao!

  • Verify the main recently-closed tabs functions of SessionStore as applied to different windows
Attachment #9334664 - Attachment description: WIP: Bug 1819675 - Add some basic tests for SessionStore's closed tab functions with multiple open → Bug 1819675 - Add some basic tests for SessionStore's closed tab functions with multiple open windows r?dao!
Attachment #9333238 - Attachment description: Bug 1819675 - rename SessionStore.getClosedTabCount to SessionStore.getClosedTabCountForWindow. r?dao! → Bug 1819675 - rename SessionStore.getClosedTabCount and getClosedTabData to getClosedTabCountForWindow and getClosedTabDataForWindow. r?dao!
Pushed by sfoster@mozilla.com:
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
Regressions: 1835928

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

Keywords: leave-open
  • 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

Blocks: 1836877
Pushed by sfoster@mozilla.com:
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
Regressions: 1842119

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

Keywords: leave-open

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.

Flags: needinfo?(sfoster)

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

Pushed by csabou@mozilla.com:
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

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

Flags: needinfo?(sfoster)
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → 117 Branch

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

Flags: needinfo?(sfoster)
Regressions: 1842129

(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.

Flags: needinfo?(sfoster)

(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.

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

Regressions: 1843587
Regressions: 1843755
Blocks: 1850630
Regressions: 1850856
No longer depends on: 1832064
See Also: → 1832064
See Also: → 1839405
You need to log in before you can comment on or make changes to this bug.