588593 - Can't right-click-move tab to un-named tab group

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect, P3)

Description

[Mike Shaver (:shaver emeritus)]

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]]

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]

I have to concur. I was in the process of submitting a similar report when I found this one.

Comment 3

[Andres Hernandez [:andreshm]]

Attachment: 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] (inactive)]

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]]

Attachment: Updated patch

Added improvements to handle empty groups or groups with only one tab.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

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]]

Attachment: Updated patch

Added suggested fixes.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

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]]

Attachment: Updated patch

This patch includes the latest suggestions.

Comment 10

[Dão Gottwald [:dao]]

(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] (inactive)]

(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]]

Attachment: Updated patch

Added the css styles.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

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]]

Attachment: Updated patch

Applied changes.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

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]]

Attachment: Screenshots

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Screenshot 1

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Screenshot 2

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

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]

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]]

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]]

Attachment: Updated patch

Applied UX changes.

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

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]]

Attachment: Updated patch

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

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]]

Attachment: Patch for checkin

Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=9c3e76b710e3

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/1c5f48f57ef1

Comment 28

[Dave Camp (:dcamp)]

https://hg.mozilla.org/mozilla-central/rev/1c5f48f57ef1