When I open a new blank tab via File->New Tab or the ctrl/command-t shortcut, and then I close that tab, Firefox returns me to the tab I was on when I initially opened the new tab. This is the "focus parent tab on child tab close" optimization recently added to Firefox, but while it can be useful when opening tabs via link clicks (especially if I remain conscious of still being/working in the parent tab while browsing the child tab), it's not very useful when opening new blank tabs, since the content I load in the new blank tab is rarely related to the "parent" tab, especially not with a parent-child relationship, and I rarely stay conscious of being in the "parent" tab. I suspect most other users who open new blank tabs are in the same boat, and we should turn off this optimization for those tabs.

load-balancing tabbrowser stuff to sspitzer

gah. s/.org/.com/!

accepting (working on an easier one as I start to learn the tab browser code)

this tab browsing code and behaviour, in addition to being shared by the suite and firefox, is something that other things (like extensions) depend on. one idea is to add a new param to the loadOneTab method and pass in "owner" instead determining it inside the method ("var owner = aLoadInBackground ? null : this.selectedTab;") this way, from the loadOneTab callers (in mozilla/browser, see http://lxr.mozilla.org/mozilla1.8/search?string=loadonetab) I could do the override to set owner to be null (to fix this bug.) but this might be over-kill. gavin/neil/jag/mconnor, comments?

(In reply to comment #5) > one idea is to add a new param to the loadOneTab method and pass in "owner" > instead determining it inside the method Does it really need to pass the owner itself? Seems like that would put a bigger burden on the caller in a lot of cases. Wouldn't a "aDontSetOwner" (or similar) boolean parameter be sufficient?

> Does it really need to pass the owner itself? Seems like that would put a > bigger burden on the caller in a lot of cases. Wouldn't a "aDontSetOwner" (or > similar) boolean parameter be sufficient? Gavin, good point. Your solution is definitely simpler and cleaner. I've got a patch that adds the aSetOwner boolean parameter to loadOneTab and fixes the 9 callers (see http://lxr.mozilla.org/mozilla1.8/search?string=loadOneTab) to use it. Only one case (see http://lxr.mozilla.org/mozilla1.8/source/browser/base/content/browser.js#1825) do I pass in false for aSetOwner, the other 8 I pass in true. I'll attach the patch as soon as I'm done testing.

for completeness, here's a summary of a discussion I had with gavin over IRC concerning this code: I was concerned about how this change to mozilla/toolkit would affect seamonkey or tbird. gavin told me that all this code is forked you shouldn't need to worry about seamonkey. toolkit is not used by seamonkey (yet). gavin mentioned that mconnor (see http://steelgryphon.com/blog/?p=74) wanted to move the toolkit tabbrowser out of the toolkit and into browser. This sounds like a good idea to me, too. My current understanding is: even if there are apps that might add some sort of tab browsing (like tbird, I think myk was interested in doing this), the tabbrowser.xml in toolkit is very intertwined to mozilla/browser, and would need to be forked (like it is forked for seamonkey) if there isn't a bug for moving tabbrowser.xml into mozilla/browser (on the trunk) I'll log one.

> Only one case (see > http://lxr.mozilla.org/mozilla1.8/source/browser/base/content/browser.js#1825) > do I pass in false for aSetOwner, the other 8 I pass in true. correction, there is another place I pass in false for aSetOwner. See http://lxr.mozilla.org/mozilla1.8/source/browser/base/content/browser.js#1848 This is the code that gets called when you do "File | Open Location..." and the navigation toolbar is hidden. You get the "Open Web Location" dialog, and if you choose "New Tab" as the target, we call loadOneTab(). (See http://lxr.mozilla.org/mozilla1.8/source/browser/base/content/openLocation.js#116) In this scenario, we don't want the parent to be the selected tab as the selected tab is not related to the new tab. I'll attach the patch now.

gavin already caught a slip-up: - var newTab = gBrowser.loadOneTab("about:blank", null, null, null, loadInBackground, false); + var newTab = gBrowser.loadOneTab("about:blank", null, null, false, loadInBackground, true); I changed the null to false as that param is supposed to be a bool, but I forgot to add the new "true" parameter. thanks gavin! new patch coming...

here's the correction: case nsCI.nsIBrowserDOMWindow.OPEN_NEWTAB : var loadInBackground = gPrefService.getBoolPref("browser.tabs.loadDivertedInBackground"); - var newTab = gBrowser.loadOneTab("about:blank", null, null, null, loadInBackground, false); + var newTab = gBrowser.loadOneTab("about:blank", null, null, false, loadInBackground, false, true);

Hmm, do we really want the BrowserLoadURL (called from handleURLBarCommand) cases to set an owner? What about the openURI (link from external application) case? I guess that's what happens now, so changes in behavior can be discussed separately. I thought I remembered bugs about changing the behavior in those two cases, but I can't find any now. The patch looks OK to me.

> My current understanding is: even if there are apps that might add some sort > of tab browsing (like tbird, I think myk was interested in doing this), the > tabbrowser.xml in toolkit is very intertwined to mozilla/browser, and would > need to be forked (like it is forked for seamonkey) FWIW, I had to make very few changes to tabbrowser.xml to get it working in my Thunderbird message tabs prototype, and the changes were all Firefox-compatible: https://bugzilla.mozilla.org/attachment.cgi?id=205098&action=diff#mozilla/directtoolkit/content/widgets/tabbrowser.xml_sec1 Basically, they consisted of: 1. Moving the <children/> tag, which Firefox doesn't use. 2. Adding the ability for code to register as a tab switch listener (this was probably unnecessary, as onLocationChange would suffice). 3. Only initializing and updating type-ahead find if it's available. Presumably a complete implementation would need more changes, though, and perhaps future Firefox plans for the widget include additional breaking changes.

> Hmm, do we really want the BrowserLoadURL (called from handleURLBarCommand) > cases to set an owner? Good point. I think we want to pass in false for aSetOwner in this case, since in this case, if you enter a url in the location bar and then hit alt before pressing enter, we weill get a new tab. and in that scenario, the new tab doesn't have anything to do with the selected tab, so the owner should be null. >What about the openURI (link from external application) case? another good point. This scenario, like the handleURLBarCommand case, the url we are loading into a new tab is not related to the current tab, so owner should be null. I'll fix these two cases and attach a new patch for review.

>>What about the openURI (link from external application) case? > >another good point. This scenario, like the handleURLBarCommand case, the url >we are loading into a new tab is not related to the current tab, so owner >should be null. I'm not so sure now about the openURI case, as this code path may not just be called in the "link from external application" scenario.

Comment on attachment 224127 [details] [diff] [review] revised patch, with two additional changes per gavin re-requesting review from mconnor

Comment on attachment 224127 [details] [diff] [review] revised patch, with two additional changes per gavin spreading the reviewing around, following the reviewer list from http://www.mozilla.org/projects/firefox/review.html,

I would say this is not a bug. I understand there's differences in the relationship, but I think consistency with stacking order of windows should be preserved wherever possible: - If we were still using windows and hit Ctrl+N/File->New Window, a new window would open on top of the current document - If we then hit close, the window would disappear and we would see the document we were just at, not some other document in the window stack order.

(Note that I did consider this when I implemented the close heuristic, and felt that the way that it's checked in now "feels" better too).

I'm glad I asked for ben's review. I didn't realize the current behavior was by design. See bug #308396 for details. ben's comment #21 about the new / close window metaphor makes sense to me. Question for beltzner: Do you agree that the current behavior is "as designed" so this is wontfix or do you feel that there are some scenarios where we want to switch to setting "owner" to null so that on close, we don't restore the selected tab?

*** Bug 334056 has been marked as a duplicate of this bug. ***

robert (who logged #334056 which is a dup) has an interesting argument about why we should consider switching the behavior to what the patch implements. I'll paraphrase his comments next...

> I'll paraphrase his comments next... after discussing bug #334056 with robert strong, here's what he was getting at: if you have a bunch of tabs, he expects that the one we select after closing a tab should always be predictable. one way to do this is always select the tab to the left (regardless of the "owner") when closing a tab. the scenario that he commented on was this: imagine I have a window with 5 tabs: site #1, site #2, blank tab, blank tab, site #3. I select the site #3 tab and now I want to close it and the two blank tabs, and get to the state where I have just two tabs: site #1, site #2. My intuition would be that if I do ctrl+w 3 times, that would get me into my desired state. But depending on the the "opener" of the tabs, this might not do what I expect, and instead it could close the tabs I wanted to keep open.

> consistency with stacking order of windows should be preserved > wherever possible But tabs don't employ the stack metaphor. When a user focuses a window, the user's windows reorder themselves visually as if the user had pulled that window out of the middle of a stack and placed it on top of the stack. But when a user focuses a tab, the user's tabs don't reorder themselves visually; they retain the order they had before. Tabs thus appear more like a sequence of lines in a document than a stack of windows, with a strong sense of order uninfluenced by focus. And their behavior in 1.5, where closing the focused tab always focuses the next one, seems more intuitive than making them obey the laws of window stacking order. It also has the advantage of consistency (both internally and with other tabbed interfaces). Nevertheless, I do see the value of sacrificing that consistency for an even greater measure of efficiency when we can be pretty sure the user wants to return to the parent tab. But while the "open link in new tab" case arguably meets that standard, I wager that the "open new blank tab" case does not.

As I see it, using an "owner" tab requires the user to think about what closing a tab will do when using the keyboard otherwise tabs that are meant to be closed will at times be closed. Actually, even when thinking about what it will end up doing it isn't obvious what will happen. The end result is only discoverable by pausing between each action and visually inspecting the result which defeats one of the main reasons for using the keyboard instead of a mouse (e.g. repeating actions quickly). The comparison to a sequence of lines applies to how tabs can be used fairly closely in that when deleting a word the caret will never move to a new location outside of the area that is being modified. It seems as if we are trying to emulate an undo stack and when reading web pages the order of tabs on a stack is not something people are going to remember... I certainly don't and I can't imagine very many people will. In some ways it seems as if this change was made for interacting with the ui using the mouse. Mouse navigation is typically slower and speeding it up is a good thing IMO but not by sacrificing the ability to quickly interact with the ui when using the keyboard.

(In reply to comment #23) > Question for beltzner: > > Do you agree that the current behavior is "as designed" so this is wontfix or > do you feel that there are some scenarios where we want to switch to setting > "owner" to null so that on close, we don't restore the selected tab? This think that the heuristic as currently implemented is wrong, and support this bug. Here's why (with advance apologies for what promises to be a long bug comment): The return-to-parent heuristic was meant to apply to the case where we diverted a window.open() or target="_blank" action into a new tab instead of a new window. There was a reasonable user expectation here for tabs to behave like new windows; specifically, were those links to have been opened in new windows, and were users to have closed that newly opened window, they'd be back where they were. There was a well established behavioural pattern of "Read - click -> read newly opened content -> close & return to parent" which we wanted to support. So, after hashing things out a lot with ben and brianr, I'd originally described the desired behaviour like so: "When the user clicks a link which opens a new tab *with focus*, if the user does not subsequently switch tabs, then close tab should return to parent." There are two nuggets in there -- first, the tab must open with focus, meaning that the action must take the user away from the parent such that closing the tab immediately returns there. Second, if the user does anything that indicates that they're in full control and mastery of their tabbed behaviour, then we drop the memory because we can no longer accurately predict what they're doing. The Accel-T/Open New Tab action seems to to fit into that second category. It is not always a sure thing that the content destined for that freshly minted tab will be related, task-wise, content-wise or any-other-wise to its parent, and thus we should not be making that assumption. Ben argues for consistency with Accel-N/New Window, but I disagree that we should be looking to be completely consistent. A user opening a new tab is looking for a different action than one who opens a new window, and has certain expectations for that action. The use case we should be making consistent is the user who clicks on a link and has that action spawn a new content area -- that behaviour should be (and is) consistent whether the new content area lands in a window or a tab. Put plainly: I think this heuristic is a great optimization for links we've diverted into new tabs (ie: without the express say-so of users) but as reported in comment 0, it falls down when it's the user asking to open the new tab.

With the background beltzner provided the reason it is the way it is today makes more sense. For the operation of a single diverted window to tab as described I believe this new behavior does make sense but as demonstrated by this bug the new behavior does indeed fall down in regards to operations other than that one type of operation. Previously I wasn't completely convinced that I shouldn't have to learn a new way to interact with tabs but after learning the reason for this change in behavior and the model chosen I'm convinced the new behavior is incorrect at least in part. The window model could never match the tab model 1 for 1 regarding the behavior that was changed and still provide predictability of behavior to the user.

FYI SeaMonkey does this because Mike has gone though it before with CTho ;-)

(In reply to comment #29) > (In reply to comment #23) > This think that the heuristic as currently implemented is wrong, and support I am not sure how I typo'd "This" for "I", but apparently I did. Sorry 'bout that.

An interesting discussion. Despite the fact that I think the behavior will be weird, I'd be interested in experimenting with the other mode, but not for Firefox 2 since I believe it's too risky to monkey with this sort of functionality now, after 9+ months of testing of the current setup. Can we get a test build with this functionality?

(In reply to comment #33) > An interesting discussion. Despite the fact that I think the behavior will be > weird, I'd be interested in experimenting with the other mode, but not for > Firefox 2 since I believe it's too risky to monkey with this sort of > functionality now, after 9+ months of testing of the current setup. Can we get > a test build with this functionality? > SeaMonkey builds do it.

I'd really like to try this on beta1 and see the response. The current behaviour has been the one I've heard the most complaints about, anecdotally, and it should be an easy fix to back out if it turns out that this new optimization makes it weirder.

Mike, I agree we need to test this, but I think that given how difficult it can be to make substantial changes after b1, and given the nature of this change, something slightly different should be done. I would like to see a test build posted for this that can be used by folk before this gets checked in anywhere, just like we did with the last set of changes like this. Seth, can you run with these things in your .mozconfig: mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-opt ac_add_options --enable-optimize ac_add_options --disable-shared ac_add_options --enable-static then build the installer (it used to be: cd browser/installer make installer ... but check with rob) - that's for windows only too. Get the build it spits out staged in the experimental directory of ftp.mozilla.org or somewhere else, and we can test this. Please report the URIs where the builds can be found in this bug, when you're done.

Ben, If the response to the testing build is universally positive would you approve of the change for inclusion in b2? If so wouldn't it make more sense to get this change into b1 and get a wider audience for feedback. If the feedback is negative we back-out the code - if it is positive we leave it in. Either way we get better testing of the code path that will ship in the final release than if we try and land this between b1-b2. (In reply to comment #36) > Mike, I agree we need to test this, but I think that given how difficult it can > be to make substantial changes after b1, and given the nature of this change, > something slightly different should be done. I would like to see a test build > posted for this that can be used by folk before this gets checked in anywhere, > just like we did with the last set of changes like this. Seth, can you run with > these things in your .mozconfig: > > mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-opt > ac_add_options --enable-optimize > ac_add_options --disable-shared > ac_add_options --enable-static > > then build the installer (it used to be: > > cd browser/installer > make installer > > ... but check with rob) - that's for windows only too. > > Get the build it spits out staged in the experimental directory of > ftp.mozilla.org or somewhere else, and we can test this. Please report the URIs > where the builds can be found in this bug, when you're done. >

It was decided that closing bookmarks should automatically focus the parent (in this context) -- see <https://bugzilla.mozilla.org/show_bug.cgi?id=324512#c4>. So why shouldn't new blank tabs? I would expect that the user is actually more likely to want to focus the parent when it's a new blank tab than when it's a bookmark. If I'm right, then if closing bookmarks should automatically focus the parent, closing new tabs should as well.

Comment on attachment 224127 [details] [diff] [review] revised patch, with two additional changes per gavin -> Mano for review, since Ben's been sitting on this for about six weeks discussed with beltzner earlier, and this is something we should fix.

> Mano for review, since Ben's been sitting on this for about six weeks oops, I am to blame. Ben asked me for a test build, and I've failed to give him one. this patch may also need to be updated to match the current trunk / branch. (I haven't looked at it in a while.)

Comment on attachment 224127 [details] [diff] [review] revised patch, with two additional changes per gavin Seth, First of, please attach an updated version of this patch. I would like to test this a bit. Some questions: * Why do you pass |false| in the nsCI.nsIBrowserDOMWindow.OPEN_NEWTAB case? I'm pretty sure this would break closing a |window.open|ed window diverted to a new tab (please check both trunk and branch). * Same question for delayedOpenTab. AFAIR, alt+enter in the location bar does set the owner. * Should we make aSetOwner default to true?

Moving to Fx3a1, we'll revisit this issue when we have more time.

sorry for the bug spam, re-assigning bugs back to default owner if I'm not working actively on them.

Is this still targeted for Firefox 3?

is this a WONTFIX?

I think so. I find this behavior actually useful, because I often open a new tab "temporarily" to look something up, and then want to go back to my original tab once I'm done with the quick digression. But more importantly, we've shipped with this behavior for a long time, so the cost to switching back now probably outweighs the benefits to making the change.