327873 - Return-to-parent tab doesn't work for tabs opened by target="_blank" links

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Set the pref to force new windows into new tabs.  Open the testcase, open a tab after it (or drag one so it's not the last), go back to the testcase and click the link.  Close the resulting tab.  You *should* get returned to the tab with the testcase, but you end up in the last tab.

Comment 1

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

This actually works in Firefox.
http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/tabbrowser.xml#1353

http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/navigator.js#1270 - I assume we don't want to record the owner tab in this case?  and http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/navigator.js#1292 ?

Patch in a moment that handles all the other callers I found that should be changed (but not the two I mentioned above).  I'm not convinced this is the best way to do it though... maybe someone can suggest an alternative.

Comment 2

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch

Unfortunately I can't test this due to local tree bustage.

Comment 3

[jag (Peter Annema)]

Comment on attachment 212434 [details] [diff] [review]
patch

In tabbrowser.xml:
+                tab = this.addTab(getShortcutOrURI(url, undefined, undefined,
+                                                   this.mCurrentTab != tab && !bgLoad));

One of the ')' is is in the wrong place. And instead of |undefined| I'd just use |null|.

At this point |tab| is null, so |this.mCurrentTab != tab| is not the right test.

Do we always want to go back to the previously selected tab when adding a "foreground" tab? E.g. I ctrl+click a bookmark or drag a URL from another window onto the tabbar (and have "Switch to new tabs opened from links" checked). Or only for links clicked in or dragged out of mCurrentTab? If the latter, you'll need some extra tests in this |else| case, and then we shouldn't make most of the other changes (we do of course wanna make the first change in navigator.js).

Comment 4

[[e-mail]]

(In reply to comment #3)
>Do we always want to go back to the previously selected tab when adding a "foreground" tab?
I think we should only enable it for tabs diverted from window.open or opened from links. Thus for dragdrop you probably need to (null-) check that aDragSession.sourceDocument.defaultView.top == content.

Comment 5

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch v2

Comment 6

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: v3 - address comment 4 and some issues from IRC

Comment 7

[[e-mail]]

Comment on attachment 223190 [details] [diff] [review]
v3 - address comment 4 and some issues from IRC

>+              else if (aDragSession.sourceDocument &&
>+                      aDragSession.sourceDocument.defaultView.top == content) {
Is there room for another space here to line up aDragSession?

Comment 8

[Robert Kaiser]

Comment on attachment 223190 [details] [diff] [review]
v3 - address comment 4 and some issues from IRC

a=me for SeaMonkey 1.1 given that it works fine on trunk

Comment 9

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Nit fixed, checked in on trunk and 1.8.1

Comment 10

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

I backed the checkin on 1.8 branch out shortly after checking it in because I was confused about 1.8 branch tree status.