Change menus on menu button to use more space

CL:1549297 adds scrolling to the top level menus hanging off a menu
button. The calculation for limiting the height of the menu allowed the
menu to sit inside the viewport with a large number of pixels gap
resulting in a scroll-bar and scrollable region when the menu could
fit inside the viewport without scrolling.

Changes the calculation of positioning the top level menu so that more
vertical space is used - limiting the height to 2 pixels less than the
viewport. The 2 pixel gap at the bottom of the menu serves as a visual
indicator for the user so they can see the menu ends there, rather than
it appearing to be clipped.

Bug: 1040058
Tests: browser_tests --gtest_filter="FileManagerJsTest.MultiMenu"
Change-Id: Idb7d1199095d5d5e7482cc5b8eaa228439202bf0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2018364Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Commit-Queue: Alex Danilo <adanilo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#735659}

ui/file_manager/file_manager/foreground/js/ui/multi_menu.js

@@ -586,29 +586,24 @@ cr.define('cr.ui', () => {
      * @private
      */
     positionMenu_() {
+      const style = this.menu.style;
+      // Clear any maxHeight we've set from previous calls into here.
+      style.maxHeight = 'none';
       cr.ui.positionPopupAroundElement(
           this, this.menu, this.anchorType, this.invertLeftRight);
-      // Check if menu is larger than the viewport and adjust its height
-      // and enable scrolling if so. Note: style.bottom would have been set to
-      // 0.
+      // Check if menu is larger than the viewport and adjust its height to
+      // enable scrolling if so. Note: style.bottom would have been set to 0.
       const viewportHeight = window.innerHeight;
       const menuRect = this.menu.getBoundingClientRect();
-      const style = this.menu.style;
-      // Make sure the top of the menu is in the viewport.
-      let top = menuRect.top;
-      if (top < 0) {
-        top = 0;
-      }
       // Limit the height to fit in the viewport.
-      style.maxHeight = (viewportHeight - top - this.menuEndGap_) + 'px';
-      // Make the menu scrollable if needed.
-      if ((top + menuRect.height + this.menuEndGap_) > viewportHeight) {
-        style.overflowY = 'scroll';
-        style.top = '0';
-        style.bottom = 'auto';
-      } else {
-        style.overflowY = 'auto';
+      style.maxHeight = (viewportHeight - this.menuEndGap_) + 'px';
+      // If the menu is too tall, position 2px from the bottom of the viewport
+      // so users can see the end of the menu (helps when scroll is needed).
+      if ((menuRect.height + this.menuEndGap_) > viewportHeight) {
+        style.bottom = '2px';
       }
+      // Let the browser deal with scroll bar generation.
+      style.overflowY = 'auto';
     }
 
     /**

ui/file_manager/file_manager/foreground/js/ui/multi_menu_unittest.js

@@ -272,7 +272,7 @@ function testGrowWindowSizesSubMenu() {
   // Test that the height of the sub-menu is the same as
   // the height at the start of this test (before we
   // deliberately shrank it).
-  assertTrue(grownPosition.bottom === subMenuPosition.bottom);
+  assertTrue(grownPosition.height === subMenuPosition.height);
 }
 
 /**
@@ -359,12 +359,12 @@ function testShrinkWindowSizesTopMenu() {
   menubutton.showMenu(true);
   const menuPosition = topMenu.getBoundingClientRect();
   // Reduce window innerHeight so the menu won't fit.
-  window.innerHeight = menuPosition.bottom - 10;
+  window.innerHeight = menuPosition.height - 10;
   // Call showMenu() which will first hide it, then re-open
   // it to force the resizing behavior.
   menubutton.showMenu(true);
   const shrunkPosition = topMenu.getBoundingClientRect();
-  assertTrue(shrunkPosition.bottom < window.innerHeight);
+  assertTrue(shrunkPosition.height == (window.innerHeight - 2));
 }
 
 /**