The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 15

Status

Firefox Graveyard
Panorama
P3
normal
RESOLVED FIXED
7 years ago
a year ago

People

(Reporter: shaver, Assigned: andreshm)

Tracking

Trunk
Firefox 15
Dependency tree / graph

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(3 attachments, 9 obsolete attachments)

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.
Depends on: 576409
Priority: -- → P3

Comment 1

7 years ago
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.
Target Milestone: --- → Future

Comment 2

6 years ago
I have to concur. I was in the process of submitting a similar report when I found this one.
OS: Windows 7 → All
Summary: can't right-click-move tab to un-named tab group → Can't right-click-move tab to un-named tab group
Blocks: 727752
(Assignee)

Comment 3

5 years ago
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.
Attachment #605801 - Flags: review?(ttaubert)
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.
Attachment #605801 - Flags: review?(ttaubert) → feedback-
(Assignee)

Updated

5 years ago
Assignee: nobody → andres
(Assignee)

Comment 5

5 years ago
Created attachment 611047 [details] [diff] [review]
Updated patch

Added improvements to handle empty groups or groups with only one tab.
Attachment #605801 - Attachment is obsolete: true
Attachment #611047 - Flags: review?(ttaubert)
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'.
Attachment #611047 - Flags: review?(ttaubert)
(Assignee)

Comment 7

5 years ago
Created attachment 611056 [details] [diff] [review]
Updated patch

Added suggested fixes.
Attachment #611047 - Attachment is obsolete: true
Attachment #611056 - Flags: review?(ttaubert)
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.
Attachment #611056 - Flags: review?(ttaubert)
(Assignee)

Comment 9

5 years ago
Created attachment 611513 [details] [diff] [review]
Updated patch

This patch includes the latest suggestions.
Attachment #611056 - Attachment is obsolete: true
Attachment #611513 - Flags: review?(ttaubert)
(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.
(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.
Status: NEW → ASSIGNED
Attachment #611513 - Flags: review?(ttaubert)
(Assignee)

Comment 12

5 years ago
Created attachment 611878 [details] [diff] [review]
Updated patch

Added the css styles.
Attachment #611513 - Attachment is obsolete: true
Attachment #611878 - Flags: review?(ttaubert)
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?
Attachment #611878 - Flags: review?(ttaubert) → feedback+
(Assignee)

Comment 14

5 years ago
Created attachment 613412 [details] [diff] [review]
Updated patch

Applied changes.
Attachment #611878 - Attachment is obsolete: true
Attachment #613412 - Flags: review?(ttaubert)
Blocks: 745060
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.
Attachment #613412 - Flags: review?(ttaubert) → review+
(Assignee)

Comment 16

5 years ago
Created attachment 614854 [details]
Screenshots
Created attachment 621916 [details]
Screenshot 1
Attachment #614854 - Attachment is obsolete: true
Created attachment 621917 [details]
Screenshot 2
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.
Attachment #621916 - Flags: ui-review?(ux-review)
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?
Attachment #621916 - Flags: ui-review?(ux-review) → ui-review+
(Assignee)

Comment 21

5 years ago
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?
(Assignee)

Comment 22

5 years ago
Created attachment 623717 [details] [diff] [review]
Updated patch

Applied UX changes.
Attachment #613412 - Attachment is obsolete: true
Attachment #623717 - Flags: review?(ttaubert)
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.
Attachment #623717 - Flags: review?(ttaubert) → review-
Created attachment 626314 [details] [diff] [review]
Updated patch
Attachment #623717 - Attachment is obsolete: true
Attachment #626314 - Flags: review?(ttaubert)
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.
Attachment #626314 - Flags: review?(ttaubert) → review+
Created attachment 628196 [details] [diff] [review]
Patch for checkin

Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=9c3e76b710e3
Attachment #626314 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/1c5f48f57ef1
Hardware: x86 → All
Whiteboard: [fixed-in-fx-team]
Target Milestone: Future → Firefox 15
Version: unspecified → Trunk

Comment 28

5 years ago
https://hg.mozilla.org/mozilla-central/rev/1c5f48f57ef1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 760794
Depends on: 766597
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.