307126 - Closing tabs should return to 'parent' tab.

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, enhancement, P1)

Description

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

I filed this because bug 105722 has too many comments.

Don't comment in this bug about:
* How it should work (or how any other implementations work)
* Firefox
* Your two cents

I'm implementing bug 105722 comment 136.

Comment 1

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch

Comment 2

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 194924 [details] [diff] [review]
patch

Given the situations in which it will/won't return to the parent tab, I don't
think this needs a pref.

Comment 3

[[e-mail]]

FYI it takes about 16 lines of tabbrowser.xml code so that when you close the
current tab the tab you had open previously (if it still exists) is selected.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

One potential refinement to the heuristic I proposed in bug 105722 comment 16 is
to intepret any loss-of-focus action (including switching to another
application) as "switching to another tab".

In other words, go back to the parent tab when the new tab was opened, took
focus, used and then disposed without the user switching focus from that new tab.

Comment 5

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #4)
> One potential refinement to the heuristic I proposed in bug 105722 comment 16 is
> to intepret any loss-of-focus action (including switching to another
> application) as "switching to another tab".
> 
> In other words, go back to the parent tab when the new tab was opened, took
> focus, used and then disposed without the user switching focus from that new tab.

I can think of too many reasons that would be a confusing behavior.  I
understand the reasoning, but don't think it would work in the real world.

Comment 6

[Daniel Brooks [:db48x]]

Comment on attachment 194924 [details] [diff] [review]
patch

>+      <field name="mParentTab">
>+        null
>+      </field>
>+


I kinda want a comment stating what this is for, when it's set, when it's used,
etc. Or maybe you could change the name to mPreviousTab, or something. Just to
indicate that this isn't part of some method of defining a visual heirarchy of
tabs. Maybe you could call it mTabToFocusAfterClosingThisTab ;)

otherwise it looks good, r=db48x

Comment 7

[[e-mail]]

Is this only supposed to apply to new foreground tabs opened from links?

Comment 8

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #7)
> Is this only supposed to apply to new foreground tabs opened from links?

Yes.

Comment 9

[[e-mail]]

Comment on attachment 194924 [details] [diff] [review]
patch

By my reading of the code this will do nasty things if you open a link in a new
tab then context-close the original tab...

Comment 10

[Daniel Brooks [:db48x]]

yea, it gets a little confused when you close the parent tab, either the way
Neil mentioned or by using Close Other Tabs from the context menu.

Comment 11

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: better patch

Renamed mParentTab to mPreviousTab.  Made addTab clear mPreviousTab if it
doesn't set it to the current tab (I don't think this is absolutely necessary
due to the onlocationchange code, but maybe it helps if you open a new tab in
the background).  Made removeTab check if it's removing the current tab before
deciding to select the parent tab, and null mPreviousTab if not.

Comment 12

[[e-mail]]

Comment on attachment 198454 [details] [diff] [review]
better patch

>+            var isCurrentTab = (aTab == this.mCurrentTab);
Wrong place to put this, and you don't need a variable for it anyway,
mCurrentTab should stay valid until you select another tab. Oh, and for some
reason aTab is known as oldTab later on, so change it when you move the
condition.

>+            if (!this.mPreviousTab || !isCurrentTab) {
>+              this.mTabContainer.selectedIndex = newIndex;
>+
>+              // When removing a tab to the left of the current tab
>+              // fix up the panel index without firing any events
>+              this.mPanelContainer.selectedIndex = newIndex;
>+
>+              this.mPreviousTab = null;
>+            }
>+            else {
>+              this.selectedTab = this.mPreviousTab;
>+              this.mPreviousTab = null;
>+            }
You don't need to set mPreviousTab to null separately in each half of the if.
In fact, the second case is unnecessary as updateCurrentBrowser already clears
the previous tab. But to be sure, comment all the times you clear the previous
tab to explain why.

Comment 13

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch

Comment 14

[[e-mail]]

Comment on attachment 198794 [details] [diff] [review]
patch

>+      <field name="mPreviousTab">
>+        null
>+      </field>
Probably belongs next to mCurrentTab.

>+            if (!this.mPreviousTab || (oldTab != this.mCurrentTab)) {
This condition looks clumsy this way around. I think it would be more obvious
to have the "switch to previous tab" as the "true" action instead.

>+            else {
>+              this.selectedTab = this.mPreviousTab;
>+            }
Inconsistent bracing - earlier you didn't use {}s (somewhere in addTab iirc).

Comment 15

[Daniel Brooks [:db48x]]

Comment on attachment 198794 [details] [diff] [review]
patch

looks good, and didn't break in testing either. r=db48x

Comment 16

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 198794 [details] [diff] [review]
patch

SeaMonkey-only patch

Comment 17

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: what i checked in on trunk

Comment 18

[jag (Peter Annema)]

Comment on attachment 200311 [details] [diff] [review]
what i checked in on trunk

I like this change, it makes sense, and it won't bug me 'coz I always open tabs
in the background :-) Now, to pick some nits...

>       <method name="addTab">
>         <parameter name="aURI"/>
>         <parameter name="aReferrerURI"/>
>         <parameter name="aCharset"/>
>+        <parameter name="aFocusNewTab"/>

I'm not sure I like this name. To me it says that this will take care of
focusing the new tab for me. I was ready to write and submit a patch to make
the other "loadInBackground" places pass in this fourth parameter until I read
the other change to |addTab(...)|. Can't think of a better name for this
parameter though, other than
|aFocusNewTabAndEnableReturnToTheTabTheNewTabWasOpenedFrom|. Suggestions,
anyone? :-)

I'm almost inclined to say that perhaps setting the "previous tab" doesn't
belong here but in the caller that wants it remembered. It'd get rid of the
need for the slightly misleading |aFocusNewTab|.


>+            if (aFocusNewTab) {
>+              var parentTab = this.selectedTab;

You can skip |parentTab| and directly assign into |this.mPreviousTab|.


>+            }
>+            else
>+              // The user opened a background tab, so updateCurrentBrowser
>+              // won't be called.  Explicitly clear the previous tab.
>+              this.mPreviousTab = null;

In my experience, not having braces around this (even though it's only one line
of code and you technically don't need them) is asking for trouble.

Comment 19

[[e-mail]]

(In reply to comment #18)
>>+            if (aFocusNewTab) {
>>+              var parentTab = this.selectedTab;
> You can skip |parentTab| and directly assign into |this.mPreviousTab|.
Not true, because focusing the new tab clears mPreviousTab.

Comment 20

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #18)
> In my experience, not having braces around this (even though it's only one line
> of code and you technically don't need them) is asking for trouble.

Comment 21

[jag (Peter Annema)]

(In reply to comment #19)
> (In reply to comment #18)
> > You can skip |parentTab| and directly assign into |this.mPreviousTab|.
> Not true, because focusing the new tab clears mPreviousTab.
Doh! That's right. Subtle enough that a comment is warranted?

Comment 22

[John A.]

It is my understanding from the SeaMonkey 1.0b release notes that this should be working now. As far as I can tell it is not.

1. I started with two tabs: A and B.
2. I added two more with Ctrl-T and had tabs A, B, C, & D, with D being displayed.
3. I switched to Tab B, then to Tab D and closed it.
4. Tab C was then displayed, rather than Tab B as I would have expected had it functioned as described in the release notes.

Comment 23

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #22)
> It is my understanding from the SeaMonkey 1.0b release notes that this should
> be working now. As far as I can tell it is not.
> 
> 1. I started with two tabs: A and B.
> 2. I added two more with Ctrl-T and had tabs A, B, C, & D, with D being
> displayed.
> 3. I switched to Tab B, then to Tab D and closed it.
> 4. Tab C was then displayed, rather than Tab B as I would have expected had it
> functioned as described in the release notes.
> 

I implemented bug 105722 comment 136, the suggestion recommended by MoCo's usability person (Mike Beltzner).  To return to the parent tab, you have to:
1. Have focus switch to a new tab when you open it
2. No open or close any tabs or switch to other tabs before closing the current tab.

An example situation where this will occur:
Tabs start as:
| google | slashdot | wikipedia | wikipedia |
with focus on the google tab.  You do a search, and middle-click a result (or if your default is to not select new tabs, shift+middle-click).  You get:
| google | slashdot | wikipedia | wikipedia | result1 |
with focus on result1.  If the next thing you do is close the result tab, focus will move back to the google tab.

If instead you opened multiple results in tabs in the background, you would be returned to the tab to the left of the one you closed (i.e. the "normal" behavior).

If you read bug 105772, you'll see it would be very difficult to make everybody happy, so I chose to implement one method that would be most useful to people who don't change the preferences (e.g. me, when I started).  The fact that switching tabs makes it not return to the parent when you close a tab is apparently a usability suggestion (and I see reasons that it does make sense).

Comment 24

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

> If instead you opened multiple results in tabs in the background, you would be
> returned to the tab to the left of the one you closed (i.e. the "normal"
> behavior).
Sorry, I meant the tab to the right.