1623054 - SeaMonkey 2.53.1 doesn't jump back to calling tab if a tab has been closed

Status: RESOLVED FIXED

(SeaMonkey :: Tabbed Browser, defect)

Description

[Lars Wendler]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0 SeaMonkey/2.53.1

Steps to reproduce:

The tab numbering is just to make my issue a bit better understandable:

Imagine, you have three open tabs numbered one to three from left to right. Now when I want a link in tab one being opened as new tab, it becomes tab four on the right side of the other tabs (I have the option "open related tabs after current tabs" disabled).

Actual results:

When I close tab four, seamonkey switches to tab three.

Expected results:

Seamonkey should switch back to tab one where I had called tab four from.

Comment 1

[Firefox Bug Husbandry Bot]

Because this bug's Severity has not been changed from the default since it was filed, and it's Priority is -- (Backlog,) indicating it has has not been previously triaged, the bug's Severity is being updated to -- (default, untriaged.)

Comment 2

[Firefox Bug Husbandry Bot]

Because this bug's Severity has not been changed from the default since it was filed, and it's Priority is -- (Backlog,) indicating it has has not been previously triaged, the bug's Severity is being updated to -- (default, untriaged.)

Comment 3

[Firefox Bug Husbandry Bot]

Because this bug's Severity has not been changed from the default since it was filed, and it's Priority is -- (Backlog,) indicating it has has not been previously triaged, the bug's Severity is being updated to -- (default, untriaged.)

Comment 4

[Firefox Bug Husbandry Bot]

The severity of these bugs was changed, mistakenly, from normal to S3.

Because these bugs have a priority of --, indicating that they have not been previously triaged, these bugs should be changed to Severity of --.

Comment 5

[Dmitry Butskoy (:buc)]

Just quick view:

The correspond Fx code respects browser.tabs.selectOwnerOnClose , where owner seems to be set by openerTab .

SM code has just one (seems unused) mention of ownerTab, ie. the implementation is not complete.

To return the feature, a proper changes needed at least inside SM's removeTab, similar to Fx's one .

Should be easy for those who implement tabbrowser.xml for SeaMonkey.

Comment 7

[Dmitry Butskoy (:buc)]

Attachment: seamonkey-2.53.4-ownertab.patch

Some preliminary patch. Needs careful testing.

Note: Fx determines a tab owner before the addTab() call (and passes it as a parameter). Because SM has several addTab() calls in various places (suite/browser/navigator.js and suite/browser/tabbrowser.xml), it looks much easier to implement things immediately in addTab().

Comment 8

[Dmitry Butskoy (:buc)]

There is a report that tab restore is broken with the patch above.
Probably addTab() shouldn't set an owner unconditionally. Instead, a proper parameter must be passed to addTab() calls in suite/browser/navigator.js .

Comment 10

[Dmitry Butskoy (:buc)]

Attachment: seamonkey-2.53.7-ownertab.patch

Fixed one (tab restoring should work now).

Comment 11

[Dmitry Butskoy (:buc)]

Lars,

Could you pls test it properly (in all the possible cases etc.)?

Comparing to Firefox, this implementation differs a bit. It always set an owner for the tab, whereas Fx seems do not set it in every case -- fe. for tabs opened in background.

Could you pls also compare the behaviour of 2.49.5, the current behaviour with the patch applied, and the behaviour of Firefox? What is better?
There is a bit mess here...

Probably strong following of Fx way is not what SM users want (and even not what was before in 2.49.5). It can be yet another difference required between Fx and Suite. So let's test it in particular user cases to deside what to implement exactly.

Comment 12

[Lars Wendler]

Thank you Dimitry.

Tab restore now works. Returning to the calling tab now also works. But with this patch there is now another inconsistency compared to 2.49.5 behavior. I am going to re-use my example from my initial post:

Imagine, you have five open tabs numbered one to five from left to right. I select tab four so that its content can be seen. Now I press the close button on the very right side of the tab bar. seamonkey now switches to tab one but original behavior would switch to tab three (the tab next left to the tab I have closed).

So this patch is an improvement but does not restore original tab behavior of before this regression was introduced.

Comment 13

[Dmitry Butskoy (:buc)]

It seems that you have both "browser.tabs.insertRelatedAfterCurrent" and "browser.tabs.loadInBackground" set to false.

Then in both 2.49.5 and Firefox >= 56, when you close the LAST opened tab, you are jumped to the opener tab.

Whether it is the only case which should be supported/implemented?

Comment 14

[Dmitry Butskoy (:buc)]

Attachment: seamonkey-2.53.7-ownertab-v1.patch

Patch v1.

An alternate version, most closely to the current Fx (and old SM) behaviour.

Since the backport was not entirely trivial, it would be good to independent verify and properly test it.

Comment 15

[Frank-Rainer Grahl (:frg)]

Attachment: WIP-1623054-ownertab.patch

Had some time today and added the latest patch as a wip patch to Bills builds. The next 2.53.10b1 pre should contain it. Please test. It looks ok on first glance but I am not sure if the sessionstore / restores doesn't need to be patches too for it. Well lets see first how it does.

Comment 16

[Lars Wendler]

I am using buc's seamonkey-2.53.7-ownertab-v1.patch successfully for several weeks already. Never had issues with session restore.

Comment 17

[Frank-Rainer Grahl (:frg)]

Never had issues with session restore.

I am not 100% sure the owner is saved and retained after a restore. Other than that suspect no problems.

Comment 18

[Frank-Rainer Grahl (:frg)]

Attachment: 1623054-ownertab-25311.patch

Patch from buc.

[Approval Request Comment]
Regression caused by (bug #): various
User impact if declined: Wrong tab selected after tab close
Testing completed (on m-c, etc.): 2.53.11b1 pre
Risk to taking this patch (and alternatives if risky): For some time now in Bills builds and Linux distributions so low risk.
String changes made by this patch: --

Might need a followup for the sessionstore but imho won't regress anything. We could also expose the pref in the ui but we have a bunch already and I think generally true is the right thing here. r+ from me but needs another set of eyes.

Comment 19

[Ian Neal]

Comment on attachment 9256084 [details] [diff] [review]
1623054-ownertab-25311.patch

>+pref("browser.tabs.selectOwnerOnClose", true);
It would be useful to expose this pref in the UI.

>+++ b/suite/base/content/utilityOverlay.js
>   case "tab":
>     var browser = w.getBrowser();
>+    var owner = loadInBackground ? null : browser.selectedTab;
>     var tab = browser.addTab(url, {
>                 referrerURI: aReferrerURI,
>                 referrerPolicy: aReferrerPolicy,
>                 charset: aCharset,
>                 postData: aPostData,
>+                ownerTab: owner,
This could be inlined unless there is a plan to use it elsewhere in a follow-up patch, so:
>+                ownerTab: !loadInBackground ? browser.selectedTab : null,

>+++ b/suite/browser/tabbrowser.xml
>@@ -1151,16 +1151,28 @@
>+            var oldTab = this.mCurrentTab;
>+
>+            // Preview mode should not reset the owner
Nit: should end with .
>+            if (!this._previewMode && !oldTab.selected)
>+              oldTab.owner = null;
>+
>+            let lastRelatedTab = this.mLastRelatedIndex ? this.tabs[this.mLastRelatedIndex] : null;
>+            if (lastRelatedTab) {
>+              if (!lastRelatedTab.selected)
Nit: make into a single if statement:
>+            if (lastRelatedTab && !lastRelatedTab.selected) {

>+                lastRelatedTab.owner = null;
>+            }

>@@ -1556,43 +1571,49 @@
>+            // if we're adding tabs, we're past interrupt mode, ditch the owner
Nit: Start comment with capital I and end with .

>@@ -1640,16 +1661,20 @@
>+            // If this new tab is owned by another, assert that relationship
Nit: end comment with .
>+            if (aOwner)
>+              t.owner = aOwner;

>@@ -1689,16 +1714,20 @@
>+              if (this.mLastRelatedIndex)
>+                  this.tabs[this.mLastRelatedIndex].owner = null;
>+              else
>+                  t.owner = this.selectedTab;
Nit: indentation is wrong.

>@@ -2007,16 +2036,23 @@
>+            // Remove this tab as the owner of any other tabs, since it's going away.
>+            for (let tab of this.tabs) {
>+              if ("owner" in tab && tab.owner == aTab)
>+                // |tab| is a child of the tab we're removing, make it an orphan
Nit: end comment with .

>@@ -2027,23 +2063,28 @@
>-            // Clean up before/afterselected attributes before removing the tab
>+            // Clean up before/after selected attributes before removing the
>+            // tab.
>             aTab._selected = false;
>             aTab.remove();
> 
>             // When the current tab is removed select a new tab
>             // and fire select events on tabpanels and tabs
>-            if (this.mPreviousTab && (aTab == this.mCurrentTab))
>+            if (aTab.owner && !aTab.owner.hidden && !aTab.owner.closing &&
With testing the patch seems to fail if no breakpoints in but works if there is a breakpoint put here and continue immediately, seems like some sort of timing / race issue.
>+                Services.prefs.getBoolPref("browser.tabs.selectOwnerOnClose")) {
>+              this.selectedTab = aTab.owner;
>+            }
If the following changes are made:
+            let owner = aTab.owner;
+
             // Clean up before/after selected attributes before removing the
             // tab.
             aTab._selected = false;
             aTab.remove();
 
             // When the current tab is removed select a new tab
             // and fire select events on tabpanels and tabs
-            if (aTab.owner && !aTab.owner.hidden && !aTab.owner.closing &&
+            if (owner && !owner.hidden && !owner.closing &&
                 Services.prefs.getBoolPref("browser.tabs.selectOwnerOnClose")) {
-              this.selectedTab = aTab.owner;
+              this.selectedTab = owner;

It seems to fix the issue for me. Perhaps buc has insight into this?

Comment 20

[Ian Neal]

Perhaps safer to use instead:
let owner = ("owner" in aTab) ? aTab.owner : null;

Comment 21

[Frank-Rainer Grahl (:frg)]

Attachment: 1623054-ownertab-v1_1-25311.patch

New version with nits fixed. I added the proposed fix for the possible race condition. Will be in the next WG9s build.

Wonder if the remove triggers an async operation which sets flags but then it should always/mostly fail breakpoint or no breakpoint.

Letting it bake a little before checking in for 2.53.11b1 in case we need to change it again.

For the prefs I would do a followup bug.

Comment 22

[Dmitry Butskoy (:buc)]

Comment on attachment 9258250 [details] [diff] [review]
1623054-ownertab-v1_1-25311.patch

Review of attachment 9258250 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 23

[Dmitry Butskoy (:buc)]

It would be useful to expose this pref in the UI.

Does Firefox expose this pref in its UI as well?..

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/162b292ad652
Set owner tab for proper tab focus handling after close. r=IanN

Comment 25

[Frank-Rainer Grahl (:frg)]

Does Firefox expose this pref in its UI as well?..

No they only retain some very simple/basic customization options in the ui.

Comment 26

[Frank-Rainer Grahl (:frg)]

https://gitlab.com/seamonkey-project/seamonkey-2.53-comm/-/commit/654d4add54283e39db311a8a9a8a49c2856d6f03
Set owner tab for proper tab focus handling after close. r=IanN a=IanN