Closed
Bug 588265
Opened 15 years ago
Closed 14 years ago
Closing a tab when in Tab Candy should cause the containing group to become focused
Categories
(Firefox Graveyard :: Panorama, defect, P2)
Firefox Graveyard
Panorama
Tracking
(Not tracked)
VERIFIED
FIXED
Firefox 4.0
People
(Reporter: aza, Assigned: raymondlee)
References
Details
(Whiteboard: [b8][interaction][good first bug])
Attachments
(1 file, 5 obsolete files)
11.23 KB,
patch
|
Details | Diff | Splinter Review |
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•15 years ago
|
Target Milestone: --- → Firefox 4.0b5
Assignee | ||
Comment 1•15 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?
Updated•15 years ago
|
Priority: -- → P4
Reporter | ||
Comment 2•15 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•15 years ago
|
Priority: P4 → P2
Reporter | ||
Updated•15 years ago
|
Whiteboard: [b8][interaction][good first bug]
Assignee | ||
Updated•15 years ago
|
Assignee: nobody → raymond
Comment 3•15 years ago
|
||
Sounds confusing. Why do we need this?
Reporter | ||
Comment 4•15 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!
Comment 5•15 years ago
|
||
(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•15 years ago
|
||
Why?
Comment 7•15 years ago
|
||
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•15 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Comment 8•15 years ago
|
||
What's the final behaviour we should implement for this?
Reporter | ||
Comment 9•15 years ago
|
||
Executive decision: We are going with what I stated in comment #2
Assignee | ||
Updated•15 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 10•15 years ago
|
||
Attachment #478769 -
Flags: feedback?(ian)
Comment 11•15 years ago
|
||
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•15 years ago
|
||
Cleaned up the tests.
Attachment #478769 -
Attachment is obsolete: true
Attachment #478952 -
Flags: review?(dolske)
Assignee | ||
Comment 13•15 years ago
|
||
Pass(In reply to comment #12)
> Created attachment 478952 [details] [diff] [review]
> v1.1
>
> Cleaned up the tests.
Passed Try!
Comment 14•15 years ago
|
||
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 15•15 years ago
|
||
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•15 years ago
|
Assignee: raymond → aza
Comment 16•15 years ago
|
||
Aza, Alex, can you guys make a determination?
Reporter | ||
Comment 17•15 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
Comment 18•15 years ago
|
||
(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•15 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.
Comment 20•15 years ago
|
||
(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•14 years ago
|
||
I agree with the comment#20. @Aza: do you have more suggestions?
Reporter | ||
Comment 23•14 years ago
|
||
I guess comment#20 won me over. We should also file a bug to make the highlight more visible.
Comment 24•14 years ago
|
||
(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•14 years ago
|
||
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 26•14 years ago
|
||
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•14 years ago
|
||
Attachment #497454 -
Attachment is obsolete: true
Attachment #497729 -
Flags: review?(ian)
Comment 28•14 years ago
|
||
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•14 years ago
|
||
r=ian
Attachment #497729 -
Attachment is obsolete: true
Attachment #498006 -
Flags: approval2.0?
Reporter | ||
Updated•14 years ago
|
Assignee: aza → raymond
Reporter | ||
Comment 30•14 years ago
|
||
Whose giving a+ here?
Assignee | ||
Comment 31•14 years ago
|
||
It would be great if someone can give approval2.0 to this.
Comment 34•14 years ago
|
||
Comment on attachment 498006 [details] [diff] [review]
v2
a=beltzner
Attachment #498006 -
Flags: approval2.0? → approval2.0+
Assignee | ||
Comment 35•14 years ago
|
||
Sent it to try again since this patch was created a while ago
Attachment #498006 -
Attachment is obsolete: true
Assignee | ||
Comment 36•14 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
Comment 37•14 years ago
|
||
Updated•9 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•