Make new folder visible in menus for all roots that aren't fake entries.

Fix cr-menu to hide superfluous separators as claimed in its comment.

Bug: 875356
Change-Id: Ib4178efe087649558fd49b32cf43d6eba9e0fff7
Reviewed-on: https://chromium-review.googlesource.com/c/1341446Reviewed-by: calamity <calamity@chromium.org>
Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Sam McNally <sammc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609562}

chrome/browser/chromeos/file_manager/file_manager_browsertest.cc

@@ -697,7 +697,8 @@ WRAPPED_INSTANTIATE_TEST_CASE_P(
         TestCase("showSelectAllInCurrentFolder"),
         TestCase("showToggleHiddenAndroidFoldersGearMenuItemsInMyFiles"),
         TestCase("enableToggleHiddenAndroidFoldersShowsHiddenFiles"),
-        TestCase("hideCurrentDirectoryByTogglingHiddenAndroidFolders")));
+        TestCase("hideCurrentDirectoryByTogglingHiddenAndroidFolders"),
+        TestCase("newFolderInDownloads")));
 
 WRAPPED_INSTANTIATE_TEST_CASE_P(
     Crostini, /* crostini.js */

chrome/test/data/webui/menu_test.html

@@ -63,6 +63,93 @@ function testShowViaKeyboardIgnoresMouseUps() {
   assertTrue(mouseUpAt(0, 0));
 }
 
+/**
+ * Tests that if the command attributes are spacified, they are copied to the
+ * corresponding menuitem.
+ */
+function testCommandMenuItem() {
+  // Test 1: The case that the command label is set and other attributes copied.
+  var command = new cr.ui.Command();
+  command.id = 'the-command';
+  command.label = 'CommandLabel';
+  command.disabled = true;
+  command.hidden = true;
+  command.checked = true;
+  document.body.appendChild(command);
+
+  var menuItem = new cr.ui.MenuItem();
+  menuItem.command = '#the-command';
+
+  // Confirms the label is copied from the command.
+  assertEquals('CommandLabel', menuItem.label);
+  // Confirms the attributes are copied from the command.
+  assertEquals(true, menuItem.disabled);
+  assertEquals(true, menuItem.hidden);
+  assertEquals(true, menuItem.checked);
+
+  // Test 2: The case that the command label is not set, and other attributes
+  // have default values.
+  var command2 = new cr.ui.Command();
+  command2.id = 'the-command2';
+  document.body.appendChild(command2);
+
+  var menuItem2 = new cr.ui.MenuItem();
+  menuItem2.label = 'MenuLabel';
+  menuItem2.command = '#the-command2';
+
+  // Confirms the label is not copied, keeping the original label.
+  assertEquals('MenuLabel', menuItem2.label);
+  // Confirms the attributes are copied from the command.
+  assertEquals(false, menuItem2.disabled);
+  assertEquals(false, menuItem2.hidden);
+  assertEquals(false, menuItem2.checked);
+}
+
+/**
+ * Mark all menu items other than |hiddenItems| as visible and check that the
+ * expected number of separators are visible.
+ */
+function runSeparatorTest(items, hiddenItems, expectedSeparators) {
+  for (let item of menu.menuItems) {
+    item.hidden = false;
+  }
+  for (let i of hiddenItems) {
+    items[i].hidden = true;
+  }
+  menu.updateCommands();
+  assertEquals(hiddenItems.length != items.length, menu.hasVisibleItems());
+  assertEquals(expectedSeparators,
+               menu.querySelectorAll('hr:not([hidden])').length);
+
+  // The separators at the ends are always hidden.
+  assertTrue(menu.menuItems[0].hidden);
+  assertTrue(menu.menuItems[6].hidden);
+}
+
+/**
+ * Tests that separators are only displayed when there is a visible
+ * non-separator item on both sides of it. Further, ensure that multiple
+ * separators will not be displayed adjacent to each other.
+ */
+function testSeparators() {
+  const menuItems = [];
+  menu.addSeparator();
+  menuItems.push(menu.addMenuItem({label: 'a'}));
+  menu.addSeparator();
+  menuItems.push(menu.addMenuItem({label: 'b'}));
+  menu.addSeparator();
+  menuItems.push(menu.addMenuItem({label: 'c'}));
+  menu.addSeparator();
+
+  runSeparatorTest(menuItems, [0, 1, 2], 0);
+  runSeparatorTest(menuItems, [0, 1], 0);
+  runSeparatorTest(menuItems, [0, 2], 0);
+  runSeparatorTest(menuItems, [1, 2], 0);
+  runSeparatorTest(menuItems, [0], 1);
+  runSeparatorTest(menuItems, [1], 1);
+  runSeparatorTest(menuItems, [2], 1);
+  runSeparatorTest(menuItems, [], 2);
+}
 </script>
 </body>
 </html>

ui/file_manager/file_manager/foreground/js/file_manager_commands.js

@@ -727,8 +727,7 @@ CommandHandler.COMMANDS_['new-folder'] = (function() {
       var locationInfo = fileManager.volumeManager.getLocationInfo(entry);
       event.canExecute = locationInfo && !locationInfo.isReadOnly &&
           CommandUtil.hasCapability([entry], 'canAddChildren');
-      event.command.setHidden(
-          CommandUtil.isRootEntry(fileManager.volumeManager, entry));
+      event.command.setHidden(false);
     } else {
       var directoryModel = fileManager.directoryModel;
       var directoryEntry = fileManager.getCurrentDirectoryEntry();

ui/file_manager/integration_tests/file_manager/gear_menu.js

@@ -789,3 +789,48 @@ testcase.showSelectAllInCurrentFolder = function() {
     },
   ]);
 };
+
+/**
+ * Tests that new folder appears in the gear menu with Downloads focused in the
+ * directory tree.
+ */
+testcase.newFolderInDownloads = function() {
+  var appId;
+  StepsRunner.run([
+    function() {
+      setupAndWaitUntilReady(null, RootPath.DOWNLOADS, null, [ENTRIES.hello], [
+      ]).then(this.next);
+    },
+    // Focus the directory tree.
+    function(results) {
+      appId = results.windowId;
+
+      remoteCall.callRemoteTestUtil('focus', appId, ['#directory-tree'])
+          .then(this.next);
+    },
+    // Open the gear menu.
+    function() {
+      remoteCall.waitForElement(appId, '#gear-button').then(this.next);
+    },
+    // Open the gear meny by a shortcut (Alt-E).
+    function() {
+      remoteCall.fakeKeyDown(appId, 'body', 'e', false, false, true)
+          .then(this.next);
+    },
+    // Wait for menu to appear.
+    function(result) {
+      chrome.test.assertTrue(result);
+      remoteCall.waitForElement(appId, '#gear-menu').then(this.next);
+    },
+    // Wait for menu to appear, containing new folder.
+    function(result) {
+      remoteCall
+          .waitForElement(
+              appId, '#gear-menu-newfolder:not([disabled]):not([hidden])')
+          .then(this.next);
+    },
+    function() {
+      checkIfNoErrorsOccured(this.next);
+    }
+  ]);
+};

ui/webui/resources/js/cr/ui/menu.js

@@ -202,26 +202,18 @@ cr.define('cr.ui', function() {
     },
 
     /**
-     * Returns whether the menu has any visible items.  Hides any separators
-     * where all items below it until the next separator are hidden.
+     * Returns whether the menu has any visible items.
      * @return {boolean} True if the menu has visible item. Otherwise, false.
      */
     hasVisibleItems: function() {
-      var menuItems = this.menuItems;  // Cache.
-      var result = false;
-      var separatorRequired = false;
       // Inspect items in reverse order to determine if the separator above each
       // set of items is required.
-      for (var i = menuItems.length - 1; i >= 0; i--) {
-        var menuItem = menuItems[i];
-        if (menuItem.isSeparator()) {
-          menuItem.hidden = !separatorRequired;
-          separatorRequired = false;
+      for (let menuItem of this.menuItems) {
+        if (this.isItemVisible_(menuItem)) {
+          return true;
         }
-        if (this.isItemVisible_(menuItem))
-          result = separatorRequired = true;
       }
-      return result;
+      return false;
     },
 
     /**
@@ -320,6 +312,27 @@ cr.define('cr.ui', function() {
         if (!menuItem.isSeparator())
           menuItem.updateCommand(node);
       }
+
+      let separatorRequired = false;
+      let lastSeparator = null;
+      // Hide any separators without a visible item between them and the next
+      // separator or the end of the menu.
+      for (let menuItem of menuItems) {
+        if (menuItem.isSeparator()) {
+          if (separatorRequired) {
+            lastSeparator = menuItem;
+          }
+          menuItem.hidden = true;
+          separatorRequired = false;
+          continue;
+        }
+        if (this.isItemVisible_(menuItem)) {
+          if (lastSeparator) {
+            lastSeparator.hidden = false;
+          }
+          separatorRequired = true;
+        }
+      }
     }
   };
 

ui/webui/resources/js/cr/ui/menu_test.html

-<!doctype html>
-<html>
-<head>
-<script src="https://cdn.rawgit.com/google/closure-library/master/closure/goog/base.js"></script>
-<script src="../../cr.js"></script>
-<script src="../event_target.js"></script>
-<script src="../ui.js"></script>
-<script src="command.js"></script>
-<script src="menu.js"></script>
-<script src="menu_item.js"></script>
-<script>
-
-goog.require('goog.testing.jsunit');
-
-</script>
-
-</head>
-<body>
-
-<script>
-
-/**
- * Tests that if the command attributes are spacified, they are copied to the
- * corresponding menuitem.
- */
-function testCommandMenuItem() {
-  // Test 1: The case that the command label is set and other attributes copied.
-  var command = new cr.ui.Command();
-  command.id = 'the-command';
-  command.label = 'CommandLabel';
-  command.disabled = true;
-  command.hidden = true;
-  command.checked = true;
-  document.body.appendChild(command);
-
-  var menuItem = new cr.ui.MenuItem();
-  menuItem.command = '#the-command';
-
-  // Confirms the label is copied from the command.
-  assertEquals('CommandLabel', menuItem.label);
-  // Confirms the attributes are copied from the command.
-  assertEquals(true, menuItem.disabled);
-  assertEquals(true, menuItem.hidden);
-  assertEquals(true, menuItem.checked);
-
-  // Test 2: The case that the command label is not set, and other attributes
-  // have default values.
-  var command2 = new cr.ui.Command();
-  command2.id = 'the-command2';
-  document.body.appendChild(command2);
-
-  var menuItem2 = new cr.ui.MenuItem();
-  menuItem2.label = 'MenuLabel';
-  menuItem2.command = '#the-command2';
-
-  // Confirms the label is not copied, keeping the original label.
-  assertEquals('MenuLabel', menuItem2.label);
-  // Confirms the attributes are copied from the command.
-  assertEquals(false, menuItem2.disabled);
-  assertEquals(false, menuItem2.hidden);
-  assertEquals(false, menuItem2.checked);
-}
-
-</script>
-
-</body>
-</html>