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

VERIFIED FIXED in Firefox 4.0

Status

defect
P2
normal
VERIFIED FIXED
9 years ago
3 years ago

People

(Reporter: aza, Assigned: raymondlee)

Tracking

Trunk
Firefox 4.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [b8][interaction][good first bug])

Attachments

(1 attachment, 5 obsolete attachments)

Reporter

Description

9 years ago
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.
Reporter

Updated

9 years ago
Target Milestone: --- → Firefox 4.0b5
Assignee

Comment 1

9 years ago
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?
Priority: -- → P4
Reporter

Comment 2

9 years ago
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.
Target Milestone: Firefox 4.0b5 → Firefox 4.0
Reporter

Updated

9 years ago
Blocks: 597043
Reporter

Updated

9 years ago
Priority: P4 → P2
Reporter

Updated

9 years ago
Whiteboard: [b8][interaction][good first bug]
Assignee

Updated

9 years ago
Assignee: nobody → raymond
Sounds confusing. Why do we need this?
Reporter

Comment 4

9 years ago
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!
(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.
Reporter

Comment 6

9 years ago
Why?
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.
Assignee

Updated

9 years ago
OS: Mac OS X → All
Hardware: x86 → All
Assignee

Comment 8

9 years ago
What's the final behaviour we should implement for this?
Reporter

Comment 9

9 years ago
Executive decision: We are going with what I stated in comment #2
Assignee

Updated

9 years ago
Status: NEW → ASSIGNED
Assignee

Comment 10

9 years ago
Posted patch v1 (obsolete) — Splinter Review
Attachment #478769 - Flags: feedback?(ian)
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.
Attachment #478769 - Flags: feedback?(ian) → feedback+
Assignee

Comment 12

9 years ago
Posted patch v1.1 (obsolete) — Splinter Review
Cleaned up the tests.
Attachment #478769 - Attachment is obsolete: true
Attachment #478952 - Flags: review?(dolske)
Assignee

Comment 13

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

Passed Try!
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.
Attachment #478952 - Flags: ui-review?(faaborg)
Attachment #478952 - Flags: review?(dolske)
Attachment #478952 - Flags: review+
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.
Attachment #478952 - Flags: ui-review?(faaborg) → ui-review-
Assignee

Updated

9 years ago
Assignee: raymond → aza
Aza, Alex, can you guys make a determination?
Reporter

Comment 17

9 years ago
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
(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".
Reporter

Comment 19

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

Comment 21

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

Comment 22

9 years ago
Moving to b9
Blocks: 598154

Updated

9 years ago
No longer blocks: 597043
Reporter

Comment 23

9 years ago
I guess comment#20 won me over. We should also file a bug to make the highlight more visible.
(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).
Assignee

Comment 25

9 years ago
Posted patch v2 (obsolete) — Splinter Review
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.
Attachment #478952 - Attachment is obsolete: true
Attachment #497454 - Flags: review?(ian)
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.
Attachment #497454 - Flags: review?(ian) → review-
Assignee

Comment 27

9 years ago
Posted patch v2 (obsolete) — Splinter Review
Attachment #497454 - Attachment is obsolete: true
Attachment #497729 - Flags: review?(ian)
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!
Attachment #497729 - Flags: review?(ian) → review+
Assignee

Comment 29

9 years ago
Posted patch v2 (obsolete) — Splinter Review
r=ian
Attachment #497729 - Attachment is obsolete: true
Attachment #498006 - Flags: approval2.0?
Reporter

Updated

9 years ago
Assignee: aza → raymond
Reporter

Comment 30

9 years ago
Whose giving a+ here?
Assignee

Comment 31

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

Comment 32

9 years ago
bugspam (moving b9 to b10)
Blocks: 608028

Comment 33

9 years ago
bugspam (removing b9)
No longer blocks: 598154
Comment on attachment 498006 [details] [diff] [review]
v2

a=beltzner
Attachment #498006 - Flags: approval2.0? → approval2.0+
Assignee

Comment 35

9 years ago
Sent it to try again since this patch was created a while ago
Attachment #498006 - Attachment is obsolete: true
Assignee

Comment 36

9 years ago
(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
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/798e7a45f2c4
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
verified with nightly build of minefield.
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.