Closed Bug 1623054 Opened 5 years ago Closed 3 years ago

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

Categories

(SeaMonkey :: Tabbed Browser, defect)

Product:
SeaMonkey
Component:
Tabbed Browser
Type:
defect

Tracking

(seamonkey2.49esr unaffected, seamonkey2.53+ fixed)

Status:
RESOLVED FIXED
Milestone:
seamonkey 2.95
Tracking Flags:
Tracking Status
seamonkey2.49esr --- unaffected
seamonkey2.53 + fixed

People

(Reporter: polynomial-c, Assigned: frg)

References

(Blocks 2 open bugs)

Details

(Keywords: regression, Whiteboard: SM2.53.11)

Attachments

(1 file, 5 obsolete files)

1623054-ownertab-v1_1-25311.patch
(deleted), patch
frg
: review+
buc
: feedback+
frg
: approval-comm-release+
frg
: approval-comm-esr60+
Details | Diff | Splinter Review

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.

Status: UNCONFIRMED → NEW
status-seamonkey2.49esr: --- → unaffected
status-seamonkey2.53: --- → affected
status-seamonkey2.57esr: --- → affected
Ever confirmed: true
Version: SeaMonkey 2.53 Branch → Trunk
Keywords: regression

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

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

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

Severity: normal → S3

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

Severity: S3 → --

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.

Attached patch seamonkey-2.53.4-ownertab.patch (obsolete) (deleted) — — Splinter Review

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

Flags: needinfo?(frgrahl)

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 .

Blocks: SeaMonkey2.53Bugs
Attached patch seamonkey-2.53.7-ownertab.patch (obsolete) (deleted) — — Splinter Review

Fixed one (tab restoring should work now).

Attachment #9176348 - Attachment is obsolete: true

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.

Flags: needinfo?(polynomial-c)

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.

Flags: needinfo?(polynomial-c)

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?

Flags: needinfo?(polynomial-c)
Attached patch seamonkey-2.53.7-ownertab-v1.patch (obsolete) (deleted) — — Splinter Review

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.

Attached patch WIP-1623054-ownertab.patch (obsolete) (deleted) — — Splinter Review

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.

Assignee: nobody → frgrahl
Attachment #9214275 - Attachment is obsolete: true
Attachment #9214526 - Attachment is obsolete: true
Flags: needinfo?(frgrahl)

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

Flags: needinfo?(polynomial-c)

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.

Attached patch 1623054-ownertab-25311.patch (obsolete) (deleted) — — Splinter Review

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.

Attachment #9236317 - Attachment is obsolete: true
Attachment #9256084 - Flags: review?(iannbugzilla)
Attachment #9256084 - Flags: review?(frgrahl)
Attachment #9256084 - Flags: approval-comm-release?
Attachment #9256084 - Flags: approval-comm-esr60?
Attachment #9256084 - Flags: review?(frgrahl) → review+
Status: NEW → ASSIGNED
tracking-seamonkey2.53: --- → ?
Summary: seamonkey-2.53.1 doesn't jump back to calling tab if a tab has been closed → SeaMonkey 2.53.1 doesn't jump back to calling tab if a tab has been closed
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?
Flags: needinfo?(dmitry)
Attachment #9256084 - Flags: review?(iannbugzilla)
Attachment #9256084 - Flags: review+
Attachment #9256084 - Flags: feedback?(dmitry)
Attachment #9256084 - Flags: approval-comm-release?
Attachment #9256084 - Flags: approval-comm-release+
Attachment #9256084 - Flags: approval-comm-esr60?
Attachment #9256084 - Flags: approval-comm-esr60+

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

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.

Attachment #9256084 - Attachment is obsolete: true
Attachment #9256084 - Flags: feedback?(dmitry)
Attachment #9258250 - Flags: review+
Attachment #9258250 - Flags: feedback?(dmitry)
Attachment #9258250 - Flags: approval-comm-release+
Attachment #9258250 - Flags: approval-comm-esr60+
Comment on attachment 9258250 [details] [diff] [review] 1623054-ownertab-v1_1-25311.patch Review of attachment 9258250 [details] [diff] [review]: ----------------------------------------------------------------- LGTM
Attachment #9258250 - Flags: feedback?(dmitry) → feedback+

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

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

Flags: needinfo?(dmitry)

Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/162b292ad652
Set owner tab for proper tab focus handling after close. r=IanN

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey 2.95

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

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

Blocks: 1750459
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: