UE fixes for tabbed browsing

VERIFIED FIXED in Firefox 2 alpha1

Status

()

defect
VERIFIED FIXED
14 years ago
2 years ago

People

(Reporter: bugs, Assigned: bugs)

Tracking

({verified1.8.1})

unspecified
Firefox 2 alpha1
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b5 -
blocking-firefox2 +

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(6 attachments, 13 obsolete attachments)

2.07 KB, image/png
Details
362 bytes, image/png
Details
86.53 KB, image/png
Details
124.53 KB, image/png
Details
58.08 KB, patch
Details | Diff | Splinter Review
7.93 KB, image/png
Details
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.
Posted patch patch (obsolete) — Splinter Review
Posted image new close.png
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.

Updated

14 years ago
Blocks: 307877
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. 
Posted patch more progress (obsolete) — Splinter Review
includes theme material for pinstripe, excluding new close icons without
backgrounds.
Attachment #195969 - Attachment is obsolete: true
Posted patch fix D&D issues (obsolete) — Splinter Review
- 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)
Attachment #196064 - Attachment is obsolete: true
Posted patch better small-tab handling (obsolete) — Splinter Review
Attachment #196076 - Attachment is obsolete: true
Posted patch make mac look nice (obsolete) — Splinter Review
Attachment #196114 - Attachment is obsolete: true
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

Updated

14 years ago
Flags: blocking1.8b5?
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.

Comment 13

14 years ago
> - 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
Posted patch newer patch (obsolete) — Splinter Review
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)
Attachment #196224 - Attachment is obsolete: true
Attachment #196971 - Attachment is obsolete: true

Updated

14 years ago
Flags: blocking1.8b5? → blocking1.8b5+
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. 
Assignee: bugs → nobody
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.
Posted patch latest patch (obsolete) — Splinter Review
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.
Assignee: nobody → bugs
Attachment #197080 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Posted patch latest patch (obsolete) — Splinter Review
attach the correct patch
Attachment #197639 - Attachment is obsolete: true

Comment 30

14 years ago
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.

Updated

14 years ago
Flags: blocking1.8b5+ → blocking1.8b5-
(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.
Depends on: 121377, 210986
Target Milestone: --- → Future

Updated

14 years ago
Flags: blocking1.9a1?

Comment 33

14 years ago
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.

Comment 34

14 years ago
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.

Comment 35

14 years ago
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

Comment 36

14 years ago
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. 
Summary: UE fixes for tabbed browsing for 1.5 → UE fixes for tabbed browsing
Attachment #197641 - Attachment is obsolete: true

Comment 38

14 years ago
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?

Comment 39

14 years ago
(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.

Comment 40

14 years ago
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

Comment 41

14 years ago
(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.
Flags: blocking-aviary2?
Flags: blocking1.9a1?
Flags: blocking-aviary2?
Flags: blocking-aviary2+
*** Bug 177500 has been marked as a duplicate of this bug. ***

Comment 43

14 years ago
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!
Attachment #202171 - Attachment is obsolete: true
Attachment #209055 - Flags: review?
Attachment #209055 - Flags: review? → review?(mconnor)
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).
Attachment #209055 - Flags: review?(mconnor) → review+
(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. 
Posted patch patch (obsolete) — Splinter Review
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.
Attachment #209055 - Attachment is obsolete: true
(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.
Attachment #209124 - Attachment is obsolete: true
landed. marking FIXED. 

Thanks to everyone who contributed to this, including Mike Connor, Mike Beltzner, Brian Rakowski and Andrea Knight!
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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.

Updated

14 years ago
Depends on: 324209
Depends on: 324230

Updated

14 years ago
Blocks: 324256

Updated

14 years ago
Depends on: 324237

Updated

14 years ago
Depends on: 324259

Updated

14 years ago
Depends on: 324396
Depends on: 324449

Updated

14 years ago
Depends on: 324470
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. 
Depends on: 324512

Comment 57

14 years ago
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.
No longer depends on: 324230

Comment 58

14 years ago
*** Bug 279574 has been marked as a duplicate of this bug. ***
No longer depends on: 279574

Comment 59

14 years ago
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.

Comment 60

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

Comment 61

14 years ago
(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.
Depends on: 329054

Comment 62

14 years ago
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;

Updated

13 years ago
Depends on: 333000
No longer depends on: 333000
should bug 117077 and bug 156025 be closed given the above changes?

Comment 64

13 years ago
This was fixed on the branch by bug 329054. Marking so.
Keywords: fixed1.8.1
Target Milestone: Future → Firefox 2 alpha1

Comment 65

13 years ago
(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. ***

Comment 70

13 years ago
*** Bug 342023 has been marked as a duplicate of this bug. ***
Depends on: 344155
Status: RESOLVED → VERIFIED
*** Bug 344782 has been marked as a duplicate of this bug. ***

Updated

13 years ago
Depends on: 345028
Depends on: 305267

Comment 72

13 years ago
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
Depends on: 416710

Updated

11 years ago
Duplicate of this bug: 279574
You need to log in before you can comment on or make changes to this bug.