Closed Bug 327873 Opened 15 years ago Closed 15 years ago

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

Categories

(SeaMonkey :: General, defect)

x86
Windows XP
defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: csthomas, Assigned: csthomas)

References

()

Details

(Keywords: fixed-seamonkey1.1a)

Attachments

(1 file, 2 obsolete files)

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.
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.
Attached patch patch (obsolete) — Splinter Review
Unfortunately I can't test this due to local tree bustage.
Assignee: general → cst
Status: NEW → ASSIGNED
Attachment #212434 - Flags: review?(jag)
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).
Attachment #212434 - Flags: review?(jag) → review-
(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.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #212434 - Attachment is obsolete: true
Attachment #223184 - Flags: superreview?(neil)
Attachment #223184 - Flags: review?(jag)
Attachment #223184 - Flags: review?(jag) → review+
Attachment #223184 - Attachment is obsolete: true
Attachment #223190 - Flags: superreview?(neil)
Attachment #223190 - Flags: review?(jag)
Attachment #223184 - Flags: superreview?(neil)
Attachment #223190 - Flags: review?(jag) → review+
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?
Attachment #223190 - Flags: superreview?(neil) → superreview+
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
Attachment #223190 - Flags: approval-seamonkey1.1a+
Nit fixed, checked in on trunk and 1.8.1
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
I backed the checkin on 1.8 branch out shortly after checking it in because I was confused about 1.8 branch tree status.
Whiteboard: [cst: needs to land on branch]
Whiteboard: [cst: needs to land on branch]
You need to log in before you can comment on or make changes to this bug.