Per the document in the URL field above, we should experiment with some tabbed browsing usability enhancements for 1.5. Attached is a patch that implements most of them. Outstanding issue: - what to call the "select new tabs opened from links" preference.

replaces toolkit/themes/winstripe/global/icons/close.png (courtesy of brettw)

Todo: apply patch on MacOS X and update stylesheets there.

Why are the toolbar borders being removed here? I'm not sure I dislike this personally, but its a little strange.

sorry, I was just doing a code drop... was experimenting in the same tree. I have a couple more changes I need to make to this patch before it's finalized.

includes theme material for pinstripe, excluding new close icons without backgrounds.

- dragging from the close button meant the same as dragging from a tab, should do nothing - dragging a link to a tab should replace the content of that tab with the link, not open a new tab with that link (originalTarget vs. target confusion)

Ben, while we're looking at the tab prefs, you might want to consider folding in the patch from bug 309011 as well, which would make the prefs section look like: Links that open new windows should be opened in: (*) new windows ( ) new tabs in the most recent window [ ] Always select new tabs [1] [x] Hide the tab bar when only one web site is open [x] Warn when closing multiple tabs [1] is a proposed renaming

Ben, I applied this patch today on w32 using a build from yesterday's (20050919) branch CVS code, and am getting the following errors: - back button on tabs opened by new window links is broken until I switch focus away and back again - I'm having trouble closing tabs, getting a lot of the following in the JS console ..: Error: uncaught exception: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]" nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)" location: "JS frame :: chrome://global/content/bindings/tabbrowser.xml :: removeTab :: line 1256" data: no] - when I do manage to close a tab, clicking another link that opens a new window ends up recreating the tab I just closed This is with a brand new profile.

Oops. My bad. I forgot to make in /browser. Sorry.

> - what to call the "select new tabs opened from links" preference. Immediately [view|switch to] new tabs Immediately [view|switch to] newly opened tabs Immediately switch to view the most recently opened tab

This one: - shows close buttons on tabs only until a minimum width is set (controlled by a pref) at which point only the active tab shows a close button - disables close buttons on background tabs (controlled by a pref) activating them only when the tab is selected to prevent accidental closures. Both of these prefs are hidden (use about:config): browser.tabs.disableBackgroundClose = true (disables close buttons on background tabs) browser.tabs.minTabWidth = 140 (minimum pixel width before bg tabs hide their close buttons and only show on the active)

Some comments from having used this for the past couple of days: 1. Having the close buttons on background tabs not respond to clicks prevents the accidental closing, but ends up creating useless-UI that just adds clutter without value. 2. I understand why tabs are widened upon select when a closebutton needs to be added (ie: the case where closebuttons have disappeared due to tabstrip clutter) but I don't understand why this is the case at all times. It causes the tab headers to jump around as a user selects tabs, and further reduces the selection target size of adjescent tabs. 3. the browser.tabs.minTabWidth requires restart to take effect. Dunno if that's on purpose or not.

I'll tweak the style rules a little more to make sure the tab headers behave appropriately. The minTabWidth thing is a hidden pref designed to let us configure the feature for optimum usability. Making it take effect immediately adds complexity to the code I didn't think was necessary. Changes to this pref apply to all new windows.

For your testing pleasure a new windows build will show up here: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/experimental/tabs/ pretty soon.

Ben, I would expect the owner thing to be disabled after changing location in the new tab. We may need to expose a method to set a owner on the tab (if you've not already) and set it from onLocationChange.

I don't think owner needs to be reset in the new tab when the location changes, only when the user diverts their attention away from the tab. We have been testing this way for the past week and it feels natural.

It might feel natural in cases where the location has been changed by hitting a link (or, submiting a form, etc). However, isn't it counterintuitive for more explicit/intended location changes (e.g. openings bookmarks, the home command) which default to open in the current tab? That said, I couldn't test the latest patch (doesn't apply), so i might point out to already-addressed issues.

(In reply to comment #24) > I don't think owner needs to be reset in the new tab when the location changes, > only when the user diverts their attention away from the tab. We have been > testing this way for the past week and it feels natural. Well, the goal of the change was to successfully determine when a user was dealing with a side-task or "interrupt" that temporarily took their attention away from some primary browsing task (eg: opening a link sent in email/gmail, opening a link that was set with target="blank_" on Fark.com). When a user diverts the location of a tab using some direct interaction with the browser UI instead of the content UI (ex: typing in the URL bar, running a search from the search box, loading a bookmark) I don't know if we can safely say that they're still dealing with that interrupt as opposed to merely re-using a tab. mconnor and I have had lengthy conversations about how we need to ensure that this behavioural change does the right thing or does nothing. Users need to be able to predict where focus will go when a tab is closed.

The l10n impact of this patch alone looks controllable so far, but how huge is the corresponding help change? Tabbed browsing is *the* key feature if firefox, I don't like the idea of having a considerable amount of our l10n help being incompatible with the UI there.

This bug does the following: - tweaks tab css styles - sets browser.tabs.minTabWidth to a large value so that the close button is only shown on the active tab, reducing the usability impact (accidental closures) of this change until after usability testing results are in.

attach the correct patch

I suggest not including the "always" in the renaming of the pref to "Always bring new tabs to the front" (as suggested in the wiki) because the always is redundant. I, of course, want a setting to _always_ do what I tell it to do. I actually really like "bring new tabs to the front" or a derivation thereof. I also don't like the text starting with "Immediately..." What is the opposite of immediately? Slowly? After a brief pause? We need something that will be easy for users to negate and get the proper uncheck behavior.

(In reply to comment #29) Section 3.4 of the wiki in the URL field implies that attachment 197641 [details] [diff] [review] (or one very much like it) will be checked into Firefox 1.5 before it is released. Is this assumption true or false?

(In reply to comment #31) > very much like it) will be checked into Firefox 1.5 before it is released. Is > this assumption true or false? The assumption is false. We'll tackle this for a future release. In the meantime, we're collecting some usability data and dealing with some dependencies such as bug 121377 and bug 210986.

Looking at bug #037877 I'm concerned that the issue with the padding around the close.png icon will cause transparent overlap of elements in linux. I see that there is still "padding: 0px 4px 2px 0px;" set for that element. The similar "padding: 4px 2px;" creates some troubles in linux builds. A solution for this is to remove the image padding, however it apears that the padding is being used to keep the tabs a fixed height. Anyways, just wondering if the two bugs need to be merged.

I suggest to move the code that selects the index for the new tab in removeTab() to a separate method. This way, if an extension wants to change the default behaviour, only has to override this method, not the entire removeTab() method. For instance, I might prefer to select the tab I previously visited, not the "owner" tab.

I was thinking of opening this exact type of "tracking bug" a few weeks ago. I was just this past weekend pointed here. Please add the following few dependencies which seem appropriate. Bugzilla Bug 184994 Bugzilla Bug 255696 Bugzilla Bug 74143 Bugzilla Bug 152391 Bugzilla Bug 226826 Bugzilla Bug 263553 Bugzilla Bug 266510 Bugzilla Bug 313300

I think this is a great idea. I used to have TabX extension, which isn't working so hot lately. Other tab related extensions are Duplicate Tab, SingleWindow, Focus Last Selected Tab, Disable Targets for Downloads, Hash Colored Tabs (not currently compatible), Last Tab, Ook, Session Saver, Tab Clicking Options, UndoCloseTab, and maybe one or two others which I don't currently use because of compatibility. If any or all of these could have their functions built in, all the better. Especially important are TabX, Duplicate Tab, SingleWindow (This one is STILL not entirely replaced in Firefox!), Focus Last Selected Tab, Disable Targets for Downloads, UndoCloseTab. Thanks for doing this much needed fixing of the tabbed interface.

Noticing you dropped the 1.5 from the description, are there any of these which WILL go into 1.5? I imagine maybe a few of the simplest would stand a chance, correct?

(In reply to comment #38) AFAIK, the only change kinda resulting from this bug which is in 1.5 (which is pretty much done, since its at *RC2*) is Bug 313300. Everything else will wait for the next release.

does anyone know if this is going into 1.5? There are some bugs remaining (# 307877) that aren't being addressed becuase they are trivial and should be invalidated with this update. It would be nice to hear if this is or is not going into 1.5 so we can get some work done to clean up so issues or otherwise let go. ~Anders

(In reply to comment #40) > does anyone know if this is going into 1.5? There are some bugs remaining (# > 307877) that aren't being addressed becuase they are trivial and should be > invalidated with this update. It would be nice to hear if this is or is not > going into 1.5 so we can get some work done to clean up so issues or otherwise > let go. > > ~Anders > Please see comment #39.

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

Will there be a possibility for the user to keep current interface with only one closing button on the right? I would suggest so as there are people who prefer old style. For "owner tab" there is a config option to switch this new behavior off, could it be possible to do the same for closing buttons?

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

Here is the final cut of v1.0 of this patch. Several of us have been testing it for several months and it is stable and usability wise represents a significant improvement over what exists now. I would like to get this reviewed and landed as soon as possible so that ongoing additional tabbed browsing improvements (organized by Mike Connor and documented here: http://wiki.mozilla.org/Tabbed_Browsing ) can proceed in an incremental fashion. Because of the centrality of this feature to browser usability and because this particular patch and its effects have been well known for some time, I believe it best to work in chunks like this. What this patch does NOT do: - enable single window mode by default (this is still blocked by the bug about window vs. tab naming) - feature final values for tweakable preferences like minTabWidth. It's set at a fairly conservative 140 now. I run with 70, but I'm me. This is worth investigating. Important Note: Those applying the patch may not see close buttons. This is because you forgot to add the new close.png (second attachment) into your build. If you see this, save yourself the headache (I have tripped on this several times) and perform this step first!

Comment on attachment 209055 [details] [diff] [review] final patch for trunk, merged as of 1/19/2006 r=me, with comments below. Thanks for all of your hard work on this. > // handle external links > // 0=default window, 1=current window/tab, 2=new window, 3=new tab in most recent window >-pref("browser.link.open_external", 3); >+pref("browser.link.open_external", 2); I don't really want to switch this right now, but if you don't the prefwindow syncs them anyway. I'll make this a single pref and migrate once bz is done fixing the targeting bug. >+pref("browser.tabs.minTabWidth", 140); We'll want to call this pref something else. Once we have a viable overflow solution, I want to be able to set a minimum tab width. >-tabs[closebutton="true"] { >- -moz-binding: url("chrome://global/content/bindings/tabbox.xml#tabs-closebutton"); >-} >- We shouldn't remove this binding (especially on a fairly stable branch) just because tabbrowser is changing its implementation. Other consumers of tabbox may be using this as well. >Index: toolkit/content/widgets/tabbox.xml >=================================================================== > >- <binding id="tabs-closebutton" >- extends="chrome://global/content/bindings/tabbox.xml#tabs"> >- <content> >- <xul:hbox flex="1" style="min-width: 1px; overflow: -moz-hidden-unscrollable;"> >- <children/> >- <xul:spacer class="tabs-right" flex="1"/> >- </xul:hbox> >- <xul:stack> >- <xul:spacer class="tabs-right"/> >- <xul:hbox class="tabs-closebutton-box" align="center" pack="end"> >- <xul:toolbarbutton ondblclick="event.preventBubble();" class="tabs-closebutton close-button" xbl:inherits="disabled=disableclose,oncommand=onclosetab"/> >- </xul:hbox> >- </xul:stack> >- </content> >- </binding> >- See previous comment, just leave it intact and unused (by Firefox) > // Set newly selected tab after quick timeout, otherwise hideous focus problems > // can occur when "browser.tabs.loadInBackground" is false and presshell is not ready >- if (!bgLoad) { >- function selectNewForegroundTab(browser, tab) { >- browser.selectedTab = tab; >- } >- setTimeout(selectNewForegroundTab, 0, getBrowser(), tab); >- } >+ if (!bgLoad) >+ this.selectedTab = tab; no more hideous focus problems? (does updateCurrentBrowser handle this better now?) >+ for (var i = 1; i < aURIs.length; ++i) >+ gBrowser.addTab(aURIs[i]); >+ if (!aLoadInBackground) { >+ if (firstTabAdded) >+ this.selectedTab = firstTabAdded; >+ window.content.focus(); You shouldn't need to manually focus here, if you do in the !firstTabAdded case then just do it as an else (since updateCurrentBrowser gets called onselect) >- <method name="onTabBarDblClick"> >- <parameter name="aEvent"/> >- <body> >- <![CDATA[ >- if (aEvent.button == 0 && >- // Only capture clicks on tabbox.xml's <spacer> >- aEvent.originalTarget.localName == "spacer") { >- var e = document.createEvent("Events"); >- e.initEvent("NewTab", false, true); >- this.dispatchEvent(e); >- } >- ]]> >- </body> >- </method> >- why are we removing this feature? Since we changed to use the spacer thsi shouldn't have any problems/conflicts. (though we can fix the comment) >+ >+ <binding id="tabbrowser-tabs" >+ extends="chrome://global/content/bindings/tabbox.xml#tabs"> >+ <content> >+ <xul:hbox flex="1" style="min-width: 1px;"> >+ <children includes="tab"/> >+ <xul:spacer class="tabs-right" flex="1"/> >+ </xul:hbox> >+ </content> >+ <implementation implements="nsIObserver"> >+ <constructor> >+ var prefs = >+ Components.classes['@mozilla.org/preferences-service;1']. >+ getService(Components.interfaces.nsIPrefService). >+ getBranch(null); just do this and you can save the QI later in the constructor var prefs = Components.classes['@mozilla.org/preferences-service;1'] .getService(Components.interfaces.nsIPrefBranch2); >+ try { >+ this.mMinTabWidth = prefs.getIntPref("browser.tabs.minTabWidth"); >+ } >+ catch (e) { >+ this.mMinTabWidth = 140; >+ } you're already defaulting to 140 in the <field> so you can skip setting it in the catch. >+ <method name="_updateDisableBackgroundClose"> >+ <body><![CDATA[ >+ var prefs = >+ Components.classes['@mozilla.org/preferences-service;1']. >+ getService(Components.interfaces.nsIPrefService). >+ getBranch(null); same thing with nsIPrefBranch instead of nsIPrefService::getBranch >+ <method name="adjustCloseButtons"> >+ <parameter name="numTabs"/> let's be consistent here with the aFoo style. Also, please document the usage of the arg (I had to work backwards a bit) I didn't really worry about the CSS/images bits, we can tweak and refine the look after the fact (and I don't have a Mac build at this moment in time).

(In reply to comment #46) > (From update of attachment 209055 [details] [diff] [review] [edit]) > r=me, with comments below. Thanks for all of your hard work on this. > > I don't really want to switch this right now, but if you don't the prefwindow > syncs them anyway. I'll make this a single pref and migrate once bz is done > fixing the targeting bug. I filed 324164 on you for this. > >+pref("browser.tabs.minTabWidth", 140); > > We'll want to call this pref something else. Once we have a viable overflow > solution, I want to be able to set a minimum tab width. I will rename it to "browser.tabs.closeButtonClipWidth" > > >-tabs[closebutton="true"] { > >- -moz-binding: url("chrome://global/content/bindings/tabbox.xml#tabs-closebutton"); > >-} > >- > > We shouldn't remove this binding (especially on a fairly stable branch) just > because tabbrowser is changing its implementation. Other consumers of tabbox > may be using this as well. What for? XUL-in-the-esoteric (which this definitely is) isn't an api we promise to support. I'd go so far as to even say that tabbrowser is more of a mozilla/browser thing, not a mozilla/toolkit thing, but that's another debate. Extension versioning should accomodate for this. > > // Set newly selected tab after quick timeout, otherwise hideous focus problems > > // can occur when "browser.tabs.loadInBackground" is false and presshell is not ready > >- if (!bgLoad) { > >- function selectNewForegroundTab(browser, tab) { > >- browser.selectedTab = tab; > >- } > >- setTimeout(selectNewForegroundTab, 0, getBrowser(), tab); > >- } > >+ if (!bgLoad) > >+ this.selectedTab = tab; > > no more hideous focus problems? (does updateCurrentBrowser handle this better > now?) Let me re-check this. It may be a bad merge.

this patch: - leaves the open_external pref alone for now (other bug filed, see above) - renames minTabWidth to tabClipWidth - restores onTabBarDblClick - fixes the assorted prefs getService QIs - documents the aNumTabs parameter to adjustCloseButtons I have one remaining question - about window.content.focus() - why do you say it's not necessary except in an |else| case ? I didn't understand that.

(In reply to comment #48) > I have one remaining question - about window.content.focus() - why do you say > it's not necessary except in an |else| case ? I didn't understand that. when you set this.selectedTab = firstTabAdded, the onselect handler calls updateCurrentBrowser, which focuses window.content in the new tab case, so its a redundant call. (In reply to comment #47) > > >-tabs[closebutton="true"] { > > >- -moz-binding: url("chrome://global/content/bindings/tabbox.xml#tabs-closebutton"); > > >-} > > >- > > > > We shouldn't remove this binding (especially on a fairly stable branch) just > > because tabbrowser is changing its implementation. Other consumers of tabbox > > may be using this as well. > > What for? XUL-in-the-esoteric (which this definitely is) isn't an api we > promise to support. I'd go so far as to even say that tabbrowser is more of a > mozilla/browser thing, not a mozilla/toolkit thing, but that's another debate. > Extension versioning should accomodate for this. Tabbrowser really is a mozilla/browser thing, I agree (and still has at least one dependency in that direction). I'm 95% sure I'll actually move it out of toolkit in the near term. Tabbox, on the other hand, is something that's generic and longstanding enough that we don't know what people may be using it for. We're trying to make a concerted effort to minimize embeddor bustage from toolkit down, so without a tangible benefit to removing it, brendan's "conserve where possible" dictum makes sense. Its been in tabbox since late 2001, so there's certainly a nonzero chance that some XUL apps are using it. If you really feel strongly about it, let's spin off removing that binding into another bug and try to get some embeddor/extension author feedback, and just get this patch in today.

Clarity! OK, I'll have a new patch for checkin ready in a few mins. Thanks for the fast turnaround Mike!

this leaves the close box binding but adds comments nearby, and fixes the content area focusing.

landed. marking FIXED. Thanks to everyone who contributed to this, including Mike Connor, Mike Beltzner, Brian Rakowski and Andrea Knight!

Were the pinstripe image changes also landed? This is what I'm seeing on OSX right now with a trunk build, and I'm not sure if I should be applying the images (active-middletab.png?) manually or not as per comment 45 ...

I just checked in the mac image. sorry!

Thanks, Ben. Further discussion about the changes introduced by this bug should be held over in the MozillaZine forums at: http://forums.mozillazine.org/viewtopic.php?p=2033085 We'd love to hear from you there.

I would like to merge these changes over to the branch before 2.0a1 so that we can get larger scale testing feedback. I will produce a branch patch shortly.

I know this bug is "Resolved Fixed" and it's possible no one will see my comment but for the record, I much preferred having a single tab-close button instead of buttons on all the tabs. I tend to open a lot of tabs and then sort them. It's much too easy to accidentally close a tab, even with the buttons disabled on background tabs (although that setting helps a lot). Is there a setting that would let me have the old behavior, with just a single tab-close button? If not, I suppose this is an enhancement request that should stand on it's own.

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

A side-effect of this that I have run into is that I frequently accidentally close tabs when switching between tabs. Since I don't actually look up at the tab to verify the location of my mouse pointer it sometimes lands on the close button.

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

(In reply to comment #59) > A side-effect of this that I have run into is that I frequently accidentally > close tabs when switching between tabs. Since I don't actually look up at the > tab to verify the location of my mouse pointer it sometimes lands on the close > button. > For those who liked the "old way" of closing tabs, there is an extension for the 1.6a1 nightlies called Tab No x (https://addons.mozilla.org/extensions/moreinfo.php?id=1919&application=firefox) that undoes these changes and restores that behavior.

after this bug fix the functions openUILinkIn and BrowserHomeClick don't use "tabshifted" any more. this fix the bug in both function - var loadInBackground = getBoolPref("browser.tabs.loadBookmarksInBackground", false); + var loadInBackground = ((where == "tab") ^ getBoolPref("browser.tabs.loadBookmarksInBackground", false)) ? false : true;

should bug 117077 and bug 156025 be closed given the above changes?

This was fixed on the branch by bug 329054. Marking so.

(In reply to comment #64) > This was fixed on the branch by bug 329054. Marking so. > Did, can, or will any of these go on the 1.8.0.x branch; or is that security only now?

(In reply to comment #65) > Did, can, or will any of these go on the 1.8.0.x branch; or is that security > only now? The 1.8.0 branch has been security and regression-fix only since it's creation. Please feel free to email me directly if you have future questions, instead of asking these questions in a bug.

Could you not simply replace the following in gBrowser.removeTab: + if ("owner" in oldTab && oldTab.owner && + this.mPrefs.getBoolPref("browser.tabs.selectOwnerOnClose")) { + for (i = 0; i < this.mTabContainer.childNodes.length; ++i) { + tab = this.mTabContainer.childNodes[i]; + if (tab == oldTab.owner) { + newIndex = i; + break; + } + } + } with: + if ("owner" in oldTab && oldTab.owner && + this.mPrefs.getBoolPref("browser.tabs.selectOwnerOnClose")) { + newIndex = oldTab.owner._tPos; + }

Is there a good reason to not to have the X "close tab" on the only tab, for example if one goes from multiple tabs down to one? This is not consistent behavior, in that a tab which previously had an X no longer has it. Furthermore, you're forced to go the window's top right to close the tab/window. (the other improvements are great)

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

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

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

its look to me that there is a bug in addTab methot var self = this; function attrChanged(event) { if (event.attrName == "selectedIndex" && event.prevValue != event.newValue) self.resetOwner(parseInt(event.prevValue)); } if (!this.mTabChangedListenerAdded) { this.mTabBox.addEventListener("DOMAttrModified", attrChanged, false); this.mTabChangedListenerAdded = true; } as i understand it event.prevValue and event.newValue aren't prev and new tab index thay are prev and new panel index in mPanelContainer. after tabs are moved this indexes aren't the same so we end up reset owner for the wrong tab this patch fix the bug <method name="resetOwner"> <parameter name="oldIndex"/> <body> <![CDATA[ // Reset the owner property, since we're leaving the modally opened // tab for another. if (oldIndex < this.mTabContainer.childNodes.length) { + var oldBrowser = this.mPanelContainer.childNodes[oldIndex].firstChild; + oldIndex = this.getBrowserIndexForDocument(oldBrowser.contentDocument); + if (oldIndex < 0) + return; var tab = this.mTabContainer.childNodes[oldIndex]; tab.owner = null; } ]]> </body> </method> i filled new bug #358362 - tab owner not always reset after tabs moved https://bugzilla.mozilla.org/show_bug.cgi?id=358362