588265 - Closing a tab when in Tab Candy should cause the containing group to become focused

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P2)

Description

[Aza Raskin [:aza]]

It doesn't currently, but dragging a tab into a group does.

We need this for correct behavior as we start implementing undo close tab.

Comment 1

[Raymond Lee [:raymondlee]]

Do you mean closing a tab without the blue borders and the group of that tab should become focus?

Would it be odd that if you have a selected tab (with blue borders) is in group A (has focus) and you close a tab in group B?  The group B would become focused. Should the selected tab in group A still have the blue borders?

Comment 2

[Aza Raskin [:aza]]

I believe so.

To be clear, if group A has the selected (blue-bordered) tab and is focused, and then you close a tab in group B, the selected tab should remain in A but group B should gain focus.

So yes, you can have group and tab focus separately.

What this means is that if you create a new tab via command-t, it will appear in the last group you interacted with (i.e. B), but if you hit return it will activate the selected tab in A.

Comment 3

[Ian Gilman [:iangilman]]

Sounds confusing. Why do we need this?

Comment 4

[Aza Raskin [:aza]]

Because I've found myself in the situation that I close a tab or two in another group and then use command-t to open a new tab. Even though my last locus of attention was on the group in-which I closed the tab, command-t opens the tab in some other group!

Comment 5

[Ian Gilman [:iangilman]]

(In reply to comment #4)
> Because I've found myself in the situation that I close a tab or two in another
> group and then use command-t to open a new tab. Even though my last locus of
> attention was on the group in-which I closed the tab, command-t opens the tab
> in some other group!

That makes sense, but why keep the tab selection in another group at that point? I would think the selected tab should always be in the selected group.

Comment 6

[Aza Raskin [:aza]]

Why?

Comment 7

[Ian Gilman [:iangilman]]

I don't know, just a gut reaction I guess. It feels to me like having a tab selected in a different group than the selected group breaks the hierarchy. It's setting up a situation where you really have two groups selected, and depending on which action you choose, one or the other of them will be affected. 

A more consistent UI would only allow for a single group to be selected in any way (either as a whole or part of it). Each group would have their own selected tab, but only the selected group's selected tab would be visibly so. Clicking from one group to another would highlight the selected tab for that group and un-highlight the selected tab for the previous group.

Comment 8

[Raymond Lee [:raymondlee]]

What's the final behaviour we should implement for this?

Comment 9

[Aza Raskin [:aza]]

Executive decision: We are going with what I stated in comment #2

Comment 10

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 11

[Ian Gilman [:iangilman]]

Comment on attachment 478769 [details] [diff] [review]
v1

-
+      
       item.setParent(null)

Adding trailing white space; please remove. 

+    EventUtils.synthesizeKey("t", { accelKey: true });
+    is(groupItemOne.getChildren().length, 2, 
+       "The num of childen in group one is 2");
+
+    let onTabViewShown = function() {
+      window.removeEventListener("tabviewshown", onTabViewShown, false);
+      ok(TabView.isVisible(), "Tab View is visible");
+
+      testTwo(groupItemOne, groupItemTwo, contentWindow);
+    };
+    window.addEventListener("tabviewshown", onTabViewShown, false);
+    TabView.toggle();

Shouldn't there be an event handler for the "tabviewhidden" event from doing the command+t? 

It seems to me the test is more complicated than it needs to be, and could perhaps make use of some helper routines to make it more DRY, but other than that, looks good.

Comment 12

[Raymond Lee [:raymondlee]]

Attachment: v1.1

Cleaned up the tests.

Comment 13

[Raymond Lee [:raymondlee]]

Pass(In reply to comment #12)
> Created attachment 478952 [details] [diff] [review]
> v1.1
> 
> Cleaned up the tests.

Passed Try!

Comment 14

[Justin Dolske [:Dolske]]

Comment on attachment 478952 [details] [diff] [review]
v1.1

r+ for technical changes, but flagging faaborg for a second UI opinion... I have the same concern Ian did; it seems like it would be confusing to have a focus/selection split, such that some operations do something with Group A and some do something with Group B instead.

I would expect that closing a tab in another group either (1) moves neither the focus nor selection or (2) moves both.

Comment 15

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 478952 [details] [diff] [review]
v1.1

Focus and selection need to be tied together, if the user sees that something is visually selected, it shouldn't be the case that the focus is actually elsewhere. Outside of panorama, tab closure has no effect on focus.  However, I think Aza has a point that after closing several tabs in a particular group, it is strange for command-t or the tab key to start acting on the previously selected group (which might have been selected awhile ago).

>That makes sense, but why keep the tab selection in another group at that
>point? I would think the selected tab should always be in the selected group.

I agree that selected tabs should only display in the selected group (and that performing an action like closing a tab should cause that group to be focused/selected).  Is there any reason why we would want to have a difference between selection and focus?  Aza's comment #9 didn't provide any additional information, but I'm possibly missing something here.

Comment 16

[Ian Gilman [:iangilman]]

Aza, Alex, can you guys make a determination?

Comment 17

[Aza Raskin [:aza]]

Im confused as to why group focus and tab focus have to be correlated. This is simple. When you hit return/esc you should go back to the last tab you zoomed out from, when you hit command-t the tab should open in the last group you interacted with. Actions can be at the tab or the group level, which is why tab focus and group focus can be separate (although normally together).

Thus still with the behavior in #2

Comment 18

[Ian Gilman [:iangilman]]

(In reply to comment #17)
> Im confused as to why group focus and tab focus have to be correlated. 

I feel they have to be correllated because there's a clear hierarchy; tabs are contained inside of groups. Hierarchies in computer interfaces generally work that way; you can't have parent A focused at the same time as the child of parent B. You can't have one application focused, but also the window from another application focused, to site an extreme example. 

> When you hit return/esc you should go back to the last tab you zoomed
> out from

When you hit return/escape you don't necessarily go back to the last tab you zoomed out from; you go to whatever tab is currently selected. It just so happens that we automatically select the tab you zoomed out from, as well as its parent group. Pretty much anything you do afterwards, however, changes your selection, at which point return/escape no longer takes you back to the tab you zoomed out from. 

As a side note, it seems this is exactly how return should behave, but it would make sense for escape to behave differently, to actually take you back to the tab you had zoomed out of (unless the tab doesn't exist anymore). 

> when you hit command-t the tab should open in the last group you
> interacted with.

I think we all agree on this. I (and I think Justin and Alex) am just suggesting that the tab selection should also be in the last group you interacted with, and that selecting a group's tab should count as "interacting with a group".

Comment 19

[Aza Raskin [:aza]]

>I think we all agree on this. I (and I think Justin and Alex) am just
> suggesting that the tab selection should also be in the last group you
> interacted with, and that selecting a group's tab should count as "interacting
> with a group".

So then you would have to focus a random (probably the first) tab from the group?

I still think that the user will never notice that they group and the focused tab are disassociated because each operation by itself always does the expected thing.

Comment 20

[Ian Gilman [:iangilman]]

(In reply to comment #19)
> So then you would have to focus a random (probably the first) tab from the
> group?

We already keep track of which was the last selected tab in a group, so we can reselect that when the group is reselected. Other options include first tab, closest tab to action, and sure, random. :)

> I still think that the user will never notice that they group and the focused
> tab are disassociated because each operation by itself always does the expected
> thing.

Actually this is another problem... the group selection highlighting is so subtle I think most people don't even notice it. By having the selected tab always in the selected group, that's another way to make it clear which is the selected group (and therefore where a new tab will show up if you hit command + t). 

We're not doing a good job of keeping tab and group selection together at the moment, so, for instance, it's possible to use the arrow keys to move the tab selection out of the selected group. If you arrowed your way over to a different group and then hit command + t, where would you expect the new tab to appear? It's odd that it appears in the last place you touched with a mouse, rather than where your attention has moved to with the keyboard.

Comment 21

[Raymond Lee [:raymondlee]]

I agree with the comment#20.  @Aza: do you have more suggestions?

Comment 22

[Kevin Hanes]

Moving to b9

Comment 23

[Aza Raskin [:aza]]

I guess comment#20 won me over. We should also file a bug to make the highlight more visible.

Comment 24

[Ian Gilman [:iangilman]]

(In reply to comment #23)
> I guess comment#20 won me over. We should also file a bug to make the highlight
> more visible.

Ok, Raymond, you know what to do?

Also filed follow up bug 618854 (highlight more visible).

Comment 25

[Raymond Lee [:raymondlee]]

Attachment: v2

This patch is based on the comment#20.  When a tab item is closed in a group, the group would get the focus and last active tab item would be selected.  If there isn't a last active tab item, the first tab item would be selected.

Comment 26

[Ian Gilman [:iangilman]]

Comment on attachment 497454 [details] [diff] [review]
v2

* I believe this would be cleaner if you did it in the child "close" subscription in GroupItem.add, rather than doing it in GroupItem.remove. For one thing, this would allow you to not need options.childDropOut. 

* I think it's a good principle at the end of a test to verify that the clean up went properly; that way if a succeeding test fails, you've got an idea what caused it. Please verify that the groups and tabs you created in the test have been removed.

Otherwise looking good.

Comment 27

[Raymond Lee [:raymondlee]]

Attachment: v2

Comment 28

[Ian Gilman [:iangilman]]

Comment on attachment 497729 [details] [diff] [review]
v2

>+     // clean up and finish
>+      groupItemTwo.addSubscriber(groupItemTwo, "close", function() {
>+        groupItemTwo.removeSubscriber(groupItemTwo, "close");
>+
>+        gBrowser.removeTab(groupItemOne.getChild(1).tab);
>+        is(contentWindow.GroupItems.groupItems.length, 1, "Has only one group");
>+        is(groupItemOne.getChildren().length, 1, 
>+           "The num of childen in group one is 1");
>+
>+        finish();

I would also check that there's only one tab in the browser (in case of orphans). 

Otherwise looks great!

Comment 29

[Raymond Lee [:raymondlee]]

Attachment: v2

r=ian

Comment 30

[Aza Raskin [:aza]]

Whose giving a+ here?

Comment 31

[Raymond Lee [:raymondlee]]

It would be great if someone can give approval2.0 to this.

Comment 32

[Kevin Hanes]

bugspam (moving b9 to b10)

Comment 33

[Kevin Hanes]

bugspam (removing b9)

Comment 34

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

Comment on attachment 498006 [details] [diff] [review]
v2

a=beltzner

Comment 35

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Sent it to try again since this patch was created a while ago

Comment 36

[Raymond Lee [:raymondlee]]

(In reply to comment #35)
> Created attachment 503059 [details] [diff] [review]
> Patch for check-in
> 
> Sent it to try again since this patch was created a while ago

Passed Try

Comment 37

[Ian Gilman [:iangilman]]

http://hg.mozilla.org/mozilla-central/rev/798e7a45f2c4

Comment 38

[Tracy Walker [:tracy]]

verified with nightly build of minefield.