no option to move a tab to a group if the folder is empty

RESOLVED FIXED in Firefox 16

Status

Firefox Graveyard
Panorama
--
minor
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: tchung, Assigned: andreshm)

Tracking

({regression})

Trunk
Firefox 16
regression

Details

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

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 634944 [details]
screenshot 2

See screenshots.   If the folder is empty in the panorama dashboard, there is no content menu option to move a tab there.  

Repro
1) install nightly 6-19-2012, mac osx 
2) create an empty folder and name it in panorama dashboard
3) open a tab, and context click > move to group
4) verify the folder you created (empty) isnt there on the list

Expected:
- creating an empty folder should still appear in the context menu > Move to group

Actual:
- folder is missing
(Reporter)

Comment 1

5 years ago
Created attachment 634945 [details]
screenshot 1
Regression from bug 588593.

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-tabview.js#295

The condition here needs to be:

> if (!groupItem.hidden && (groupItem.getTitle().trim() || groupItem.getChildren().length) &&
Blocks: 588593
Keywords: regression
OS: Mac OS X → All
Hardware: x86 → All
Assignee: nobody → andres
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 3

5 years ago
Created attachment 635343 [details] [diff] [review]
Patch v1

I'll create a test in a separate patch.
Attachment #635343 - Flags: review?(ttaubert)
(Assignee)

Comment 4

5 years ago
Created attachment 635381 [details] [diff] [review]
Patch test
Attachment #635381 - Flags: review?(ttaubert)
Comment on attachment 635343 [details] [diff] [review]
Patch v1

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

Thanks!
Attachment #635343 - Flags: review?(ttaubert) → review+
Comment on attachment 635381 [details] [diff] [review]
Patch test

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

Looks good but the code about opening the context menu needs some corrections.

::: browser/components/tabview/test/browser_tabview_bug766597.js
@@ +16,5 @@
> +
> +  contentWindow.UI.setActive(group1);
> +  group2.setTitle("Group 2");
> +
> +  var checkGroupCount = function() {

function checkGroupCount() {

@@ +20,5 @@
> +  var checkGroupCount = function() {
> +    // first test with an empty untitled group.
> +    checkContextMenu(tab, 1, function() {
> +      // then test with an empty but titled group.
> +      group3.setTitle("E");

group3.setTitle("empty group with title") would be a little more self-explanatory.

@@ +24,5 @@
> +      group3.setTitle("E");
> +      checkContextMenu(tab, 2, clean);
> +    });
> +  };
> +  var clean = function() {

function clean() {

@@ +44,5 @@
> +
> +  tabViewMenuPopup.dispatchEvent(tabContextMenuEvent);
> +  tabContextMenu.openPopup(aTab, "end_after", 0, 0, true, false, tabContextMenuEvent);
> +  // Forze to fill the context menu.
> +  TabView.updateContextMenu(aTab, tabViewMenuPopup);

We don't need to do this. Please have a look at browser_tabview_bug641802.js for how you can trigger Panorama's submenu.

@@ +45,5 @@
> +  tabViewMenuPopup.dispatchEvent(tabContextMenuEvent);
> +  tabContextMenu.openPopup(aTab, "end_after", 0, 0, true, false, tabContextMenuEvent);
> +  // Forze to fill the context menu.
> +  TabView.updateContextMenu(aTab, tabViewMenuPopup);
> +  window.setTimeout(function() {

Please don't use setTimeout() that's very unreliable. I don't think we need this here at all, do we?
Attachment #635381 - Flags: review?(ttaubert) → feedback+
(Assignee)

Comment 7

5 years ago
Created attachment 636499 [details] [diff] [review]
Patch test v2

Updated test.
Attachment #635381 - Attachment is obsolete: true
Attachment #636499 - Flags: review?(ttaubert)
Comment on attachment 636499 [details] [diff] [review]
Patch test v2

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

This is definitely good to go with the little fixes below. Thanks, Andres!

::: browser/components/tabview/test/browser_tabview_bug766597.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Totally overlooked that you're using MPL. Please use public domain here.

/* Any copyright is dedicated to the Public Domain.
 * http://creativecommons.org/publicdomain/zero/1.0/ */

@@ +34,5 @@
> +    group2 = createEmptyGroupItem(cw, 200, 200, 20);
> +    cw.UI.setActive(cw.GroupItems.groupItems[0]);
> +
> +    // Check the group count.
> +    is(cw.GroupItems.groupItems.length, 3, "Invalid group count in tab view.");

As a general comment, the message you're passing here is not an error message as it's always displayed, no matter if the check is successful or not. It should rather be a short description of what you're checking here.
Attachment #636499 - Flags: review?(ttaubert) → feedback+
(Assignee)

Comment 9

5 years ago
Created attachment 636738 [details] [diff] [review]
Patch test v3

Applied changes.
Attachment #636499 - Attachment is obsolete: true
Attachment #636738 - Flags: review?(ttaubert)
(Assignee)

Comment 10

5 years ago
(In reply to Tim Taubert [:ttaubert] from comment #8)
>
> > +
> > +    // Check the group count.
> > +    is(cw.GroupItems.groupItems.length, 3, "Invalid group count in tab view.");
> 
> As a general comment, the message you're passing here is not an error
> message as it's always displayed, no matter if the check is successful or
> not. It should rather be a short description of what you're checking here.

Ok, thanks for clarifying. Actually it make more sense, but in the doc says that is an "Error Message". See: https://developer.mozilla.org/en/Mochitest  

is(actualValue, expectedValue, "Error message") -- compares two values (using ==, not ===)
Comment on attachment 636738 [details] [diff] [review]
Patch test v3

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

Nice!
Attachment #636738 - Flags: review?(ttaubert) → review+
https://hg.mozilla.org/integration/fx-team/rev/5393fad896d1
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 16
https://hg.mozilla.org/mozilla-central/rev/5393fad896d1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.