My very ingrained workflow is to open a new tab and start typing a url. Now that the default action is to switch to the open tab, I end up leaving a

(submitted too early... continuing from comment 0) ... tab open that has no history, etc (just a "blank tab"). Ideally we would close this tab when we switch to an open tab. I'd say we should definitely do this for about:blank, but it get's a little trickier for new tabs with homepages (though I'd say we should probably close those too if no further actions have been taken). For the homepage tabs, that could get a little obnoxious with session restore though - those tabs would show up in the recently closed tabs list, though they really shouldn't.

Precisely so. Saves me filing the bug.

Closes the tab when it's empty (no history entries). No tests (yet... anybody else want to write them?). Also - the case of a new window should be thought about a bit harder. Right now if the tab is the only tab, it closes the window (per browser.tabs.closeWindowWithLastTab), but not sure if that's actually what we want to do. If it is, then this code works, but should probably be optimized for the !closeWindowWithLastTab case (so we don't close an empty tab & then open another)

Now with a test.

The behavior you expected when you opened a new tab and type in a query then selecting the page normally results in a new page being loaded. Except now the existing tab will be selected with any old contents on the page and now you need to refresh the tab to actually get what you want.

(In reply to comment #5) > Except now the existing tab will be selected with any old contents on the page > and now you need to refresh the tab to actually get what you want. We add both the switch-to-tab and the normal entries to the popup, so you can still choose to re-load the page in the new tab (see also bug 555689...)

Comment on attachment 437138 [details] [diff] [review] Patch v0.2 >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js > function switchIfURIInWindow(aWindow) { >+ // Close the tab if it's empty >+ if (gBrowser.selectedBrowser.sessionHistory.count == 0) >+ gBrowser.removeTab(gBrowser.selectedTab) I'm not sure how reliable this check is... we should probably be consistent with the similar code in undoCloseTab(), I guess, which also checks a few other things (URI, document state, etc.). >diff --git a/browser/base/content/test/browser_bug555767.js b/browser/base/content/test/browser_bug555767.js >+function test() { >+ // Open the base tab >+ let baseTab = gBrowser.addTab(testURL); >+ gBrowser.selectedTab = baseTab; >+ baseTab.addEventListener("load", function() { Load events for content don't reach the tab, AFAIK, so I think this is actually relying on the favicon load. This should probably be on the tabbrowser instead. Seems like this test as is will succeed if onTabClose just isn't called - the tabSelect handler should probably check that it was called using a flag.

(In reply to comment #7) > (From update of attachment 437138 [details] [diff] [review]) > >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js > > > function switchIfURIInWindow(aWindow) { > > >+ // Close the tab if it's empty > >+ if (gBrowser.selectedBrowser.sessionHistory.count == 0) > >+ gBrowser.removeTab(gBrowser.selectedTab) > > I'm not sure how reliable this check is... we should probably be consistent > with the similar code in undoCloseTab(), I guess, which also checks a few other > things (URI, document state, etc.). On tab close, sessionstore looks at entries.length > 0 (where entries is pulled almost 1-for-1 from sessionHistory)... http://hg.mozilla.org/mozilla-central/file/49ca6b8821f2/browser/components/sessionstore/src/nsSessionStore.js#l813 Though there is an additional check that can (supposedly) happen where there are no history entries but we're not on about:blank || the page has child nodes... http://hg.mozilla.org/mozilla-central/file/49ca6b8821f2/browser/components/sessionstore/src/nsSessionStore.js#l1193 So I guess I could do this to really mirror what's happening > if (gBrowser.selectedBrowser.sessionHistory.count == 0 && > gBrowser.currentURI.spec == "about:blank" && > !gBrowser.contentDocument.body.hasChildNodes()) > >diff --git a/browser/base/content/test/browser_bug555767.js b/browser/base/content/test/browser_bug555767.js > > >+function test() { > > >+ // Open the base tab > >+ let baseTab = gBrowser.addTab(testURL); > >+ gBrowser.selectedTab = baseTab; > >+ baseTab.addEventListener("load", function() { > > Load events for content don't reach the tab, AFAIK, so I think this is actually > relying on the favicon load. This should probably be on the tabbrowser instead. That or on the tab's browser (which is something I've seen too/more). I'll probably do that _just in case_ there's another tab doing things from a previous test. I know there shouldn't be, but nothing surprises me these days. > Seems like this test as is will succeed if onTabClose just isn't called - the > tabSelect handler should probably check that it was called using a flag. True. I'll make this better.

Shouldn't this close any existing tab that a user switches from? Whether you reload the tab in the current space and close the old tab (that you're switching too) or whether you close whatever tab you're on and move focus to the switched too tab. Either way, the fact that the user has attempted to open a tab that's already open, surely means he's done with whatever tab he was on currently. And thus it's just causing him to have to go back and close the other tab? My apologies for explaining the train of thought so badly.

(In reply to comment #9) > Shouldn't this close any existing tab that a user switches from? Whether you > reload the tab in the current space and close the old tab (that you're > switching too) or whether you close whatever tab you're on and move focus to > the switched too tab. Either way, the fact that the user has attempted to open > a tab that's already open, surely means he's done with whatever tab he was on > currently. And thus it's just causing him to have to go back and close the > other tab? > > My apologies for explaining the train of thought so badly. My appreciation for explaining the train of thought! Having said that, I think we should restrict this behaviour to empty tabs. There are plenty of reasons why use of a current tab could cause me to want to open a new one, without losing the existing one. Examples from today include: - Reading a great article, and wanting to jump over to twitter to tweet about it (don't want to lose the article in the process) - Reading a bug and being reminded of something I want to add to my rememberthemilk todo list (don't want to lose the bug in the process) Creating an empty tab, typing in the location bar, and then ending up switching to an existing tab is a well-understood broken behaviour that we can fix here without risking data loss. Doing something more complete like you propose forces us to tread the line much more carefully, and I think that will slow down progress. If you disagree with my assessment above and still think we should close tabs with content, I think it should be filed as a follow up bug.

So I changed this to use the more exact "tab closed" checking that sessionstore uses. I also moved the closing of the tab below the switch. This avoids an potentially extra TabSelect event from happening, at least when the switch-to tab is in the same window. So I had to rejigger the test a bit to make sure the TabSelect happens before the TabClose

Comment on attachment 446771 [details] [diff] [review] Patch v0.3 Looks pretty good, but I was actually referring to browser.js's "function undoCloseTab(aIndex)", which also has similar checks. I think we should factor them out into a "tabIsBlank(aTab)" helper to share that code.

now with isTabEmpty(aTab)

If the intent of !browser.contentDocument.body.hasChildNodes() is to rule out dependent windows, then I think !browser.contentWindow.opener makes more sense.

It's possible for an extension to use about:blank tabs with generated content, which an opener check wouldn't catch. We could do both tests, I suppose... or just not care about that scenario.

Pushed http://hg.mozilla.org/mozilla-central/rev/2333f6d349d7 Stuck with using !browser.contentDocument.body.hasChildNodes() as reviewed. I'm fine with that, but file a new bug if you feel strongly about using the opener check.

Is this only invoked when using the switch-to-tab feature or more general? Is it supposed to change my example? My example that creates blank tabs (probably always an accident): 1. Open tab 2. Type in address bar: www.google.com 3. Hit "ALT-RETURN" 4. Now blank tab and my new tab are open.

(In reply to comment #17) > Is this only invoked when using the switch-to-tab feature or more general? Is > it supposed to change my example? Just the switch to tab feature. Please file a new bug / find an old one for your example.

Its not something I would worry about; I was just curious. Thank you for a prompt reply.

Verified Fixed on Mozilla/5.0 (Windows; U; Windows NT 6.0; Win64; x64; en-US; rv:1.9.3a6pre) Gecko/20100618 Minefield/3.7a6pre (.NET CLR 3.5.30729) ID:20100618042039

filed bug 595046 on comment 17 case

Bug 573580 is the bug I filed for comment 17. I originally didn't think I needed to but then did anyway.