1852622 - Open tabs in Recent browsing is not updating correctly

Status: VERIFIED FIXED

(Firefox :: Firefox View, defect, P1)

Description

[Eric Pang [:epang] UX]

Attachment: recent-browsing.mp4

When you are browsing through tabs on multiple windows the Open tabs list in Recent browsing does not update as expected.

Each window appears to take a specific row number to update instead of moving to the top of the Open tabs section.

Window 1 seems to always update the first row
Window 2 updates the second row
Current window updates the 3rd row

See attached video.

Expected:
For the last active tab to move to the first position in the open tabs list

Also, I noticed a tab isn't moved to the top of the Open tabs card when the user selects the tab and scrolls the page. Only after a new page navigation or reload does it become active and move to the top of the list. Is it possible to consider selecting a tab/scrolling the contents as an interaction with a tab?

Let me know if you have any questions. Thanks

Comment 1

[Sarah Clements [:sclements]]

I'm also seeing an issue with the Open Tabs section now with the "Move to" submenu items that have been added (moving a tab to start or end is updated fine in the tabstrip, but the updated order is not correct in the list in the Open Tabs section).

Comment 2

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

I'm giving this an initial priority of P2. I.e. nice-to-have for 119. But we can discuss.

Comment 3

[Sarah Clements [:sclements]]

After discussion, we're going to try to get this fixed for 119. There might be two separate issues (per my comment 1) or the same root issue but we'll leave it to whomever picks this up to decide if it should be split out into two bugs or not.

Comment 4

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

So I think this is the smoking gun for this bug: updateLastAccessed

updateLastAccessed(aDate) {
  this._lastAccessed = this.selected ? Infinity : aDate || Date.now();
}

Comment 5

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

The Infinity there is because the selected tab for window should always sort to the top. These lastAccessed dates and the whole way this is set up is really designed for the tabstrip case and the harder I look, doesn't seem useful for a tabs-from-all-windows, by-recency sorted list. I think we want a data provider module (.sys.mjs) to populate a single, recency-sorted "table" for the lifetime of the session. Then firefox view can query that to get the top 5, or the by-window breakdown. It should be a system module because the open tabs list we are creating is for all open windows; it really doesn't make sense to build and maintain a separate but entirely parallel data structure for each instance of Firefox View (we have at least one per browser window).

I don't think that exists today. There's some overlap with the urlbar tab provider, but not enough that it would make sense to try and extend it to cover this case. And of course, there's overlap with the web extension tabs API. Again, without some serious refactoring/restructuring, its probably not a useful starting point.

Implementing this module doesn't seem like it would be particularly hard - modulo the edge cases I'm not thinking of, of course - but it is a quite different approach to the one we currently take in the opentabs.mjs. I imagine it will boil down to a WeakMap of tab => lastAccessed timestamp, and all the gubbins to observe all the various window and tab events/notifications to keep that current. And provide the necessary accessors for the consumers.

Comment 6

[:Gijs (he/him)]

Bug 1756851 has a conundrum caused by the same Infinity use and has an outline for a solution.

I'm not sure that I agree that moving things to a sys.mjs rather than keeping the data associated to individual tabs would be an improvement.

Comment 7

[Sarah Clements [:sclements]]

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

The Infinity there is because the selected tab for window should always sort to the top. These lastAccessed dates and the whole way this is set up is really designed for the tabstrip case and the harder I look, doesn't seem useful for a tabs-from-all-windows, by-recency sorted list. I think we want a data provider module (.sys.mjs) to populate a single, recency-sorted "table" for the lifetime of the session. Then firefox view can query that to get the top 5, or the by-window breakdown. It should be a system module because the open tabs list we are creating is for all open windows; it really doesn't make sense to build and maintain a separate but entirely parallel data structure for each instance of Firefox View (we have at least one per browser window).

I don't think that exists today. There's some overlap with the urlbar tab provider, but not enough that it would make sense to try and extend it to cover this case. And of course, there's overlap with the web extension tabs API. Again, without some serious refactoring/restructuring, its probably not a useful starting point.

Implementing this module doesn't seem like it would be particularly hard - modulo the edge cases I'm not thinking of, of course - but it is a quite different approach to the one we currently take in the opentabs.mjs. I imagine it will boil down to a WeakMap of tab => lastAccessed timestamp, and all the gubbins to observe all the various window and tab events/notifications to keep that current. And provide the necessary accessors for the consumers.

Is there not a simpler solution for the more immediate future, with a look at re-implementation for a later release?

Comment 8

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

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

Is there not a simpler solution for the more immediate future, with a look at re-implementation for a later release?

I think Gijs' point was that we shouldn't create another module and another source of truth for this - tabs have a lastAccessed property which is maintained during a session and is even persisted when we save a session so it can be restored next time. The tab is the definitive source for this information. As lastAccessed isn't quite what we need and changing it would introduce risk we don't have time to address, we'll create another property to sit next to it and use that for our sorting.
We still need firefox view to be notified when a change has occured, so we'll still need to watch all the windows for tab events, and I think window activate events. But the data will live on the tab. I'm not sure this is more or less involved to implement, but its simpler in terms of lines of code written I guess.

Comment 9

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

Attachment: Bug 1852622 - Add a test-only firefoxview-entered notification, and modify the test helpers to use it. r?sclements

Comment 10

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

Attachment: Bug 1852622 - Track a lastSetActive property on each tab and use that when sorting open tabs for recency in firefox view. r?jsudiaman,gijs

Depends on D189443

Comment 12

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

Attachment: Bug 1852622 - Fix an issue in PlacesQuery where it tries to re-finalizes the deferred task. r?jsudiaman

Depends on D189443

Comment 13

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

Comment on attachment 9355448 [details]
Bug 1852622 - Track a lastSetActive property on each tab and use that when sorting open tabs for recency in firefox view. r?jsudiaman,gijs

Beta/Release Uplift Approval Request

• User impact if declined: The order of open tabs in the "Recent Browsing" section of Firefox View isn't always accurate.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: There's pretty thorough automated test coverage, but see comment #0 for STR if we want to manually test to verify
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): We're tracking and making use of a new "last seen" property on browser tabs rather than changing existing behavior to the lastAccessed property.
• String changes made/needed: None
• Is Android affected?: No

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb6b38500c1f
Add a test-only firefoxview-entered notification, and modify the test helpers to use it. r=sclements,fxview-reviewers
https://hg.mozilla.org/integration/autoland/rev/f272f049ead4
Fix an issue in PlacesQuery where it tries to re-finalizes the deferred task. r=jsudiaman,places-reviewers
https://hg.mozilla.org/integration/autoland/rev/2a4ab75fb0d7
Track a lastSetActive property on each tab and use that when sorting open tabs for recency in firefox view. r=jsudiaman,Gijs,fxview-reviewers,tabbrowser-reviewers,mak,sclements

Comment 15

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/bb6b38500c1f
https://hg.mozilla.org/mozilla-central/rev/f272f049ead4
https://hg.mozilla.org/mozilla-central/rev/2a4ab75fb0d7

Comment 16

[Donal Meehan [:dmeehan]]

Comment on attachment 9355447 [details]
Bug 1852622 - Add a test-only firefoxview-entered notification, and modify the test helpers to use it. r?sclements

Approved for 119.0b7

Comment 17

[Donal Meehan [:dmeehan]]

Comment on attachment 9355448 [details]
Bug 1852622 - Track a lastSetActive property on each tab and use that when sorting open tabs for recency in firefox view. r?jsudiaman,gijs

Approved for 119.0b7

Comment 18

[Donal Meehan [:dmeehan]]

Comment on attachment 9356256 [details]
Bug 1852622 - Fix an issue in PlacesQuery where it tries to re-finalizes the deferred task. r?jsudiaman

Approved for 119.0b7

Comment 19

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/5b7cd277830d
https://hg.mozilla.org/releases/mozilla-beta/rev/a105ff5e54c3
https://hg.mozilla.org/releases/mozilla-beta/rev/b3368ac5d680

Comment 20

[Anca Soncutean, Desktop QA]

Reproduced with Fx 119.0a1 (2023-09-11) on Windows 10.
Verified fixed with Fx 120.0a1 (2023-10-10) and Fx 119.0b7 on Windows 10, macOS 13 and Ubuntu 22.