1916188 - Active pinned tab moves to first position after restarting the browser with vertical tabs and session restore

Status: VERIFIED FIXED

(Firefox :: Sidebar, defect, P1)

Description

[Kestrel]

Attachment: Second pinned tab moves to first after restart

STR:

1. Start with sidebar.verticalTabs = true and sidebar.revamp = true.
2. Enable "Open previous windows and tabs".
3. Pin several tabs.
4. Select the second pinned tab.
5. Restart the browser.

Expected:
Pinned tab order does not change.

Actual:
The second pinned tab moves to the first position.

Does not happen when manually restoring the previous session.

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=aa647a9d07534829200207b0e9dbb0049bda0cec&tochange=4baacd30d483bf663babfaf4d87014aa331b9312

Regressed by Bug 1910576.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

:dao, since you are the author of the regressor, bug 1910576, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 2

[Dão Gottwald [:dao]]

Bug 1910576 only fixed a typo in bug 1899336's patch.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1899336

Comment 4

[Sarah Clements [:sclements]]

Thanks for the redirect, I'll look into it.

Comment 5

[Sarah Clements [:sclements]]

I can reproduce this only if a pinned tab is selected and I restart the browser (with session restore automatically enabled) due to this condition.

Because of the work in bug 1899336, we don't want pinTab to be called for vertical tabs because this was set up to handle actually pinning tabs based on user action. For restoring a session we want the positioning of pinned tabs to be handled consistently by positionPinnedTabs regardless of a pinned tabs' "selected" state.

Comment 6

[Sarah Clements [:sclements]]

And looking into this a bit more, the reason we're hitting this condition only with a browser restart seems to be because of this condition in Session Restore restoreWindow where when overwriteTabs is truthy - as it is in this specific restart scenerio selectedTab is assigned a value greater than 0 which is then later passed to tabbrowser.createTabsForSessionRestore.

There were probably historical reasons for doing this, but it ends up forcing a different action for pinning tabs in the tabbrowser code which we'll probably want to address in a more cohesive way in bug 1910097. For now I've added a condition for vertical tabs.

Comment 7

[Sarah Clements [:sclements]]

Attachment: Bug 1916188 - Fix issue with selected pinned tab reordering for vertical tabs r=#tabbrowser-reviewers

• Add a condition in createTabsForSessionRestore to account for pinned tabs that are selected and update test

Comment 8

[Dão Gottwald [:dao]]

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

Because of the work in bug 1899336, we don't want pinTab to be called for vertical tabs because this was set up to handle actually pinning tabs based on user action.

That's not quite true. pinTab is part of the tabbrowser API, and while there may be reasons for setting this up differently for session restore to optimize for speed for instance, calling it also shouldn't break stuff, I don't think.

For restoring a session we want the positioning of pinned tabs to be handled consistently by positionPinnedTabs regardless of a pinned tabs' "selected" state.

This sounds rather sketchy. _positionPinnedTabs is called indirectly from pinTab, one doesn't replace the other. Could you please go into more detail about what pinTab breaks here and why?

Comment 9

[Sarah Clements [:sclements]]

(In reply to Dão Gottwald [:dao] from comment #8)

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

Because of the work in bug 1899336, we don't want pinTab to be called for vertical tabs because this was set up to handle actually pinning tabs based on user action.

That's not quite true. pinTab is part of the tabbrowser API, and while there may be reasons for setting this up differently for session restore to optimize for speed for instance, calling it also shouldn't break stuff, I don't think.

For restoring a session we want the positioning of pinned tabs to be handled consistently by positionPinnedTabs regardless of a pinned tabs' "selected" state.

This sounds rather sketchy. _positionPinnedTabs is called indirectly from pinTab, one doesn't replace the other. Could you please go into more detail about what pinTab breaks here and why?

It seems strange to me that a different code path is taken in gBrowser.createTabsForSessionRestore depending on whether a tab is selected and pinned vs unselected and pinned. In the case of unselected , this._updateTabBarForPinnedTabs is called here which in turn also calls _positionPinnedTabs.

The difference is that in the first case, by the time we hit this condition in _updateVerticalPinnedTabs we already have one pinned tab in the vertical-pinned-tabs-container that was added regardless of its actual position. Ie, pinned tab at index 3 was the selected tab, so now we have that in the pinned tabs container and due to this change [...verticalPinnedTabsContainer.children, ...children] that was made in allTabs, this tab with index 3 becomes index 0.

And now we have a new order for that pinned tab that has nothing to do with what its original order was. I could try addressing this in _updateVerticalPinnedTabs but it needs to be generic enough to handle when switching from horizontal to vertical tabs (moving all the tabs over to pinned tabs container) and for the other session restore cases where pinTab is not called.

This leads me back to whether changing that makes sense vs adding a condition in createTabsForSessionRestore for vertical tabs as I have in the patch (and I suspect when moving horizontal pinned tabs to another container in bug 1910097 that entire condition would likely need to be reevaluated).

If there are important reasons for preserving the existing logic for vertical tabs, explaining why would be helpful.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/45632c16ab58
Fix issue with selected pinned tab reordering for vertical tabs r=mconley

Comment 11

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/45632c16ab58

Comment 12

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Reproduced the issue with Firefox 132.0a1 (2024-09-03) on Windows 10x64 by following steps from comment 0. After restarting Firefox while the second pinned tab was selected, this will move as being the first pinned tab.
The issue is verified fixed with Firefox 132.0b2 on Windows 10x64, macOS 12, and Ubuntu 24.04. After following the steps from comment 0 the second tab will remain in its place after a restart.