Last Comment Bug 588593 - Can't right-click-move tab to un-named tab group
: Can't right-click-move tab to un-named tab group
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: P3 normal
: Firefox 15
Assigned To: Andres Hernandez [:andreshm]
:
Mentors:
Depends on: 576409 760794 766597
Blocks: 727752 745060
  Show dependency treegraph
 
Reported: 2010-08-18 15:42 PDT by Mike Shaver (:shaver -- probably not reading bugmail closely)
Modified: 2016-04-12 14:00 PDT (History)
11 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (1.97 KB, patch)
2012-03-14 09:28 PDT, Andres Hernandez [:andreshm]
ttaubert: feedback-
Details | Diff | Splinter Review
Updated patch (3.13 KB, patch)
2012-03-30 14:42 PDT, Andres Hernandez [:andreshm]
no flags Details | Diff | Splinter Review
Updated patch (3.02 KB, patch)
2012-03-30 15:27 PDT, Andres Hernandez [:andreshm]
no flags Details | Diff | Splinter Review
Updated patch (3.12 KB, patch)
2012-04-02 10:37 PDT, Andres Hernandez [:andreshm]
no flags Details | Diff | Splinter Review
Updated patch (4.76 KB, patch)
2012-04-03 10:34 PDT, Andres Hernandez [:andreshm]
ttaubert: feedback+
Details | Diff | Splinter Review
Updated patch (4.35 KB, patch)
2012-04-09 15:18 PDT, Andres Hernandez [:andreshm]
ttaubert: review+
Details | Diff | Splinter Review
Screenshots (345.98 KB, application/zip)
2012-04-13 10:47 PDT, Andres Hernandez [:andreshm]
no flags Details
Screenshot 1 (90.06 KB, image/png)
2012-05-08 02:01 PDT, Tim Taubert [:ttaubert]
limi: ui‑review+
Details
Screenshot 2 (202.74 KB, image/png)
2012-05-08 02:02 PDT, Tim Taubert [:ttaubert]
no flags Details
Updated patch (18.12 KB, patch)
2012-05-14 10:29 PDT, Andres Hernandez [:andreshm]
ttaubert: review-
Details | Diff | Splinter Review
Updated patch (3.63 KB, patch)
2012-05-22 21:02 PDT, Raymond Lee [:raymondlee]
ttaubert: review+
Details | Diff | Splinter Review
Patch for checkin (3.92 KB, patch)
2012-05-29 20:21 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review

Description Mike Shaver (:shaver -- probably not reading bugmail closely) 2010-08-18 15:42:02 PDT
I have two groups, one has a name ("distractions").  When right clicking on a tab in the distractions group, I am only offered a way to create new groups, not a way to move it to my unnamed (default, other, etc.) group.

No name makes it hard, but showing the title of the first tab and the number ("Bug 563876...." and 15 others") would probably suffice, or at least motivate me to name tab sets.
Comment 1 Aza Raskin [:aza] 2010-10-11 23:43:36 PDT
Agreed that we will need a method of talking about unnamed groups. The first tab plus number of others is as good as any (especially if we can show some of the favicons).

Bumping this to the future.
Comment 2 J.K 2011-02-11 05:30:04 PST
I have to concur. I was in the process of submitting a similar report when I found this one.
Comment 3 Andres Hernandez [:andreshm] 2012-03-14 09:28:06 PDT
Created attachment 605801 [details] [diff] [review]
Patch

I'm not sure about the final UX decision, but in case is positive then here is the patch.
Comment 4 Tim Taubert [:ttaubert] 2012-03-19 06:22:11 PDT
Comment on attachment 605801 [details] [diff] [review]
Patch

Review of attachment 605801 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-tabview.js
@@ +313,5 @@
>    _createGroupMenuItem: function TabView__createGroupMenuItem(groupItem) {
> +    let menuItem = document.createElement("menuitem");
> +    let title = groupItem.getTitle();
> +    
> +    if ("" == title) {

Please no yoda conditions: (title == "")

@@ +317,5 @@
> +    if ("" == title) {
> +      title =
> +        gNavigatorBundle.getFormattedString("tabview2.moveToUnnamedGroup.label",
> +          [ groupItem.getTopChild().tab.label,
> +            groupItem.getChildren().length - 1]);

Some groups are empty so there are no tabs to get a title from. We probably can filter them out by adding |groupItem.getChildren().length| to the condition above.
Comment 5 Andres Hernandez [:andreshm] 2012-03-30 14:42:52 PDT
Created attachment 611047 [details] [diff] [review]
Updated patch

Added improvements to handle empty groups or groups with only one tab.
Comment 6 Tim Taubert [:ttaubert] 2012-03-30 15:14:32 PDT
Comment on attachment 611047 [details] [diff] [review]
Updated patch

Review of attachment 611047 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch, Andres. Alas, the behavior is not quite right, yet. We still would see an error for empty groups.

::: browser/base/content/browser-tabview.js
@@ +297,5 @@
>        groupItems.forEach(function(groupItem) {
>          // if group has title, it's not hidden and there is no active group or
>          // the active group id doesn't match the group id, a group menu item
>          // would be added.
> +        if (!groupItem.hidden &&

We should filter out empty groups already here with:

|if (!groupitem.hidden && groupItem.getChildren().length &&|

@@ +321,5 @@
> +            "tabview2.moveToUnnamedGroup.label",
> +            [ groupItem.getTopChild().tab.label,
> +              groupItem.getChildren().length - 1]);
> +      } else {
> +        title = groupItem.getTopChild().tab.label;

In case there's only one tab in the group we should show 'about:foobar and 0 more'.
Comment 7 Andres Hernandez [:andreshm] 2012-03-30 15:27:33 PDT
Created attachment 611056 [details] [diff] [review]
Updated patch

Added suggested fixes.
Comment 8 Tim Taubert [:ttaubert] 2012-04-02 02:35:59 PDT
Comment on attachment 611056 [details] [diff] [review]
Updated patch

Review of attachment 611056 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, just some minor additions.

::: browser/base/content/browser-tabview.js
@@ +321,5 @@
> +            groupItem.getChildren().length - 1]);
> +    }
> +
> +    menuItem.setAttribute("label", title);
> +    menuItem.setAttribute("tooltiptext", title);

Having a website with a very long title this can result in a very wide menuitem. We should add the class "bookmark-item" which restricts the max-width to 32em and set the attribute crop="center" so that the menu item's label is cropped in the middle and "and X more" is still visible.
Comment 9 Andres Hernandez [:andreshm] 2012-04-02 10:37:14 PDT
Created attachment 611513 [details] [diff] [review]
Updated patch

This patch includes the latest suggestions.
Comment 10 Dão Gottwald [:dao] 2012-04-02 10:45:43 PDT
(In reply to Tim Taubert [:ttaubert] from comment #8)
> We should add the class "bookmark-item" which restricts the
> max-width to 32em and set the attribute crop="center" so that the menu
> item's label is cropped in the middle and "and X more" is still visible.

It's not a bookmark item, so don't use that class.
Comment 11 Tim Taubert [:ttaubert] 2012-04-03 05:24:30 PDT
(In reply to Dão Gottwald [:dao] from comment #10)
> (In reply to Tim Taubert [:ttaubert] from comment #8)
> > We should add the class "bookmark-item" which restricts the
> > max-width to 32em and set the attribute crop="center" so that the menu
> > item's label is cropped in the middle and "and X more" is still visible.
> 
> It's not a bookmark item, so don't use that class.

Um, yeah that was a bad advice. So we should create our own CSS class and apply a max-width:32em to winstripe + gnomestripe only.
Comment 12 Andres Hernandez [:andreshm] 2012-04-03 10:34:24 PDT
Created attachment 611878 [details] [diff] [review]
Updated patch

Added the css styles.
Comment 13 Tim Taubert [:ttaubert] 2012-04-05 04:39:22 PDT
Comment on attachment 611878 [details] [diff] [review]
Updated patch

Review of attachment 611878 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-tabview.js
@@ +313,5 @@
>    _createGroupMenuItem: function TabView__createGroupMenuItem(groupItem) {
> +    let menuItem = document.createElement("menuitem");
> +    let title = groupItem.getTitle();
> +
> +    if (title == "") {

We should use |if (!title.trim()) {| here so that group titles consisting of white spaces only are considered empty.

::: browser/themes/gnomestripe/browser.css
@@ +686,5 @@
>    -moz-image-region: rect(0, 64px, 16px, 48px);
>  }
>  
> +/* tabview menus */
> +menuitem.tabview-item {

|.tabview-menuitem {| seems better to me, so we don't need the tag selector and the class name would be rather self-explanatory.

@@ +687,5 @@
>  }
>  
> +/* tabview menus */
> +menuitem.tabview-item {
> +  min-width: 0;

Do we really need the "min-width" style?
Comment 14 Andres Hernandez [:andreshm] 2012-04-09 15:18:11 PDT
Created attachment 613412 [details] [diff] [review]
Updated patch

Applied changes.
Comment 15 Tim Taubert [:ttaubert] 2012-04-13 04:58:06 PDT
Comment on attachment 613412 [details] [diff] [review]
Updated patch

Review of attachment 613412 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you, this looks great!

The next thing we should do is create screenshots of some menuitem combinations (group with one child, group with more than one child, cropped title, etc.) and ask for UX review to get feedback from the UX team. If they like it, we can land it.
Comment 16 Andres Hernandez [:andreshm] 2012-04-13 10:47:25 PDT
Created attachment 614854 [details]
Screenshots
Comment 17 Tim Taubert [:ttaubert] 2012-05-08 02:01:38 PDT
Created attachment 621916 [details]
Screenshot 1
Comment 18 Tim Taubert [:ttaubert] 2012-05-08 02:02:15 PDT
Created attachment 621917 [details]
Screenshot 2
Comment 19 Tim Taubert [:ttaubert] 2012-05-08 02:03:34 PDT
Comment on attachment 621916 [details]
Screenshot 1

Attached two screenshots from the zip file directly (makes reviewing a bit easier). Asking for ui-review for this feature.
Comment 20 Alex Limi (:limi) — Firefox UX Team 2012-05-09 16:24:28 PDT
Comment on attachment 621916 [details]
Screenshot 1

Works for me. 

Two nitpicks:
“…and 0 more” should probably just be dropped. 

The titles seem overly long, and create very wide menus, maybe reduce it by ~20 characters for the max width?
Comment 21 Andres Hernandez [:andreshm] 2012-05-10 14:47:51 PDT
In the current patch, we didn't change it for Mac, we basically have the same styles as the bookmarks menu items. So, should we have the same max width in Mac too?
Comment 22 Andres Hernandez [:andreshm] 2012-05-14 10:29:00 PDT
Created attachment 623717 [details] [diff] [review]
Updated patch

Applied UX changes.
Comment 23 Tim Taubert [:ttaubert] 2012-05-22 04:01:01 PDT
Comment on attachment 623717 [details] [diff] [review]
Updated patch

Review of attachment 623717 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like you accidentally added changes from other bugs to [gnome|win]stripe/browser.css files?

::: browser/base/content/browser-tabview.js
@@ +348,5 @@
> +          gNavigatorBundle.getFormattedString("tabview2.moveToUnnamedGroup.label",
> +            [ groupItem.getTopChild().tab.label,
> +              groupItem.getChildren().length - 1]);
> +      } else {
> +        title = groupItem.getTopChild().tab.label;

Please assign this to a variable (e.g. topChildLabel) because it's used in both branches of the if-statement.
Comment 24 Raymond Lee [:raymondlee] 2012-05-22 21:02:52 PDT
Created attachment 626314 [details] [diff] [review]
Updated patch
Comment 25 Tim Taubert [:ttaubert] 2012-05-29 10:35:27 PDT
Comment on attachment 626314 [details] [diff] [review]
Updated patch

Review of attachment 626314 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you, Raymond, for finishing this!

::: browser/base/content/browser-tabview.js
@@ +314,5 @@
> +      if (groupItem.getChildren().length > 1) {
> +        title =
> +          gNavigatorBundle.getFormattedString("tabview2.moveToUnnamedGroup.label",
> +            [ topChildLabel,
> +              groupItem.getChildren().length - 1]);

Nit: Let's remove the line break here.
Comment 26 Raymond Lee [:raymondlee] 2012-05-29 20:21:44 PDT
Created attachment 628196 [details] [diff] [review]
Patch for checkin

Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=9c3e76b710e3
Comment 27 Tim Taubert [:ttaubert] 2012-05-30 03:03:13 PDT
https://hg.mozilla.org/integration/fx-team/rev/1c5f48f57ef1
Comment 28 Dave Camp (:dcamp) 2012-05-30 19:32:57 PDT
https://hg.mozilla.org/mozilla-central/rev/1c5f48f57ef1

Note You need to log in before you can comment on or make changes to this bug.