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

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Tony Chung [:tchung]]

Attachment: 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

Comment 1

[Tony Chung [:tchung]]

Attachment: screenshot 1

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

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) &&

Comment 3

[Andres Hernandez [:andreshm]]

Attachment: Patch v1

I'll create a test in a separate patch.

Comment 4

[Andres Hernandez [:andreshm]]

Attachment: Patch test

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 635343 [details] [diff] [review]
Patch v1

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

Thanks!

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

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?

Comment 7

[Andres Hernandez [:andreshm]]

Attachment: Patch test v2

Updated test.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 9

[Andres Hernandez [:andreshm]]

Attachment: Patch test v3

Applied changes.

Comment 10

[Andres Hernandez [:andreshm]]

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

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 636738 [details] [diff] [review]
Patch test v3

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

Nice!

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/5393fad896d1

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/5393fad896d1