Last Comment Bug 766597 - no option to move a tab to a group if the folder is empty
: no option to move a tab to a group if the folder is empty
Status: RESOLVED FIXED
[fixed-in-fx-team]
: regression
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- minor
: Firefox 16
Assigned To: Andres Hernandez [:andreshm]
:
Mentors:
Depends on:
Blocks: 588593
  Show dependency treegraph
 
Reported: 2012-06-20 09:28 PDT by Tony Chung [:tchung]
Modified: 2016-04-12 14:00 PDT (History)
2 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
screenshot 2 (55.35 KB, image/png)
2012-06-20 09:28 PDT, Tony Chung [:tchung]
no flags Details
screenshot 1 (25.60 KB, image/png)
2012-06-20 09:28 PDT, Tony Chung [:tchung]
no flags Details
Patch v1 (1.32 KB, patch)
2012-06-21 08:56 PDT, Andres Hernandez [:andreshm]
ttaubert: review+
Details | Diff | Review
Patch test (3.62 KB, patch)
2012-06-21 11:19 PDT, Andres Hernandez [:andreshm]
ttaubert: feedback+
Details | Diff | Review
Patch test v2 (3.71 KB, patch)
2012-06-25 15:00 PDT, Andres Hernandez [:andreshm]
ttaubert: feedback+
Details | Diff | Review
Patch test v3 (3.60 KB, patch)
2012-06-26 09:29 PDT, Andres Hernandez [:andreshm]
ttaubert: review+
Details | Diff | Review

Description Tony Chung [:tchung] 2012-06-20 09:28:20 PDT
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
Comment 1 Tony Chung [:tchung] 2012-06-20 09:28:55 PDT
Created attachment 634945 [details]
screenshot 1
Comment 2 Tim Taubert [:ttaubert] 2012-06-21 05:50:34 PDT
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] 2012-06-21 08:56:35 PDT
Created attachment 635343 [details] [diff] [review]
Patch v1

I'll create a test in a separate patch.
Comment 4 Andres Hernandez [:andreshm] 2012-06-21 11:19:54 PDT
Created attachment 635381 [details] [diff] [review]
Patch test
Comment 5 Tim Taubert [:ttaubert] 2012-06-23 07:29:05 PDT
Comment on attachment 635343 [details] [diff] [review]
Patch v1

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

Thanks!
Comment 6 Tim Taubert [:ttaubert] 2012-06-23 07:48:50 PDT
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] 2012-06-25 15:00:26 PDT
Created attachment 636499 [details] [diff] [review]
Patch test v2

Updated test.
Comment 8 Tim Taubert [:ttaubert] 2012-06-26 06:18:38 PDT
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] 2012-06-26 09:29:32 PDT
Created attachment 636738 [details] [diff] [review]
Patch test v3

Applied changes.
Comment 10 Andres Hernandez [:andreshm] 2012-06-26 09:33:55 PDT
(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] 2012-06-26 10:51:18 PDT
Comment on attachment 636738 [details] [diff] [review]
Patch test v3

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

Nice!
Comment 12 Tim Taubert [:ttaubert] 2012-06-26 11:14:20 PDT
https://hg.mozilla.org/integration/fx-team/rev/5393fad896d1
Comment 13 Tim Taubert [:ttaubert] 2012-06-30 15:17:09 PDT
https://hg.mozilla.org/mozilla-central/rev/5393fad896d1

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