cros: Fix menu positioning bug

Fixes menu positioning miscalculations in ShelfView,
also refactors GetMenuAnchorRect so its more readable.

Bug: 852153
Change-Id: Ie8c377de9b2191c3ae739f5adec7f16fc8003bd7
Reviewed-on: https://chromium-review.googlesource.com/1102028
Commit-Queue: Alex Newcomer <newcomer@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569816}

ash/shelf/shelf_view.cc

@@ -1488,39 +1488,32 @@ void ShelfView::UpdateOverflowRange(ShelfView* overflow_view) const {
 }
 
 gfx::Rect ShelfView::GetMenuAnchorRect(const views::View& source,
-                                       const gfx::Point& location) const {
+                                       const gfx::Point& location,
+                                       bool context_menu) const {
   const bool for_item = ShelfItemForView(&source);
-  const gfx::Rect shelf_bounds =
+  const gfx::Rect shelf_bounds_in_screen =
       is_overflow_mode()
           ? owner_overflow_bubble_->bubble_view()->GetBubbleBounds()
-          : screen_util::GetDisplayBoundsWithShelf(
-                shelf_widget_->GetNativeWindow());
+          : GetBoundsInScreen();
   const gfx::Rect& source_bounds_in_screen = source.GetBoundsInScreen();
+  // Application menus and touchable menus for items are anchored on the icon
+  // bounds.
+  if ((features::IsTouchableAppContextMenuEnabled() && for_item) ||
+      !context_menu) {
+    return source_bounds_in_screen;
+  }
+
   gfx::Point origin;
   switch (shelf_->alignment()) {
     case SHELF_ALIGNMENT_BOTTOM:
     case SHELF_ALIGNMENT_BOTTOM_LOCKED:
-      origin =
-          gfx::Point((features::IsTouchableAppContextMenuEnabled() && for_item)
-                         ? source_bounds_in_screen.x()
-                         : location.x(),
-                     shelf_bounds.bottom() - kShelfSize);
+      origin = gfx::Point(location.x(), shelf_bounds_in_screen.y());
       break;
     case SHELF_ALIGNMENT_LEFT:
-      if (features::IsTouchableAppContextMenuEnabled()) {
-        origin =
-            gfx::Point(shelf_bounds.x(),
-                       for_item ? source_bounds_in_screen.y() : location.y());
-      } else {
-        origin = gfx::Point(shelf_bounds.x() + kShelfSize, location.y());
-      }
+      origin = gfx::Point(shelf_bounds_in_screen.right(), location.y());
       break;
     case SHELF_ALIGNMENT_RIGHT:
-      origin =
-          gfx::Point(shelf_bounds.right() - kShelfSize,
-                     (features::IsTouchableAppContextMenuEnabled() && for_item)
-                         ? source_bounds_in_screen.y()
-                         : location.y());
+      origin = gfx::Point(shelf_bounds_in_screen.x(), location.y());
       break;
   }
   return gfx::Rect(origin,
@@ -1992,9 +1985,9 @@ void ShelfView::ShowMenu(std::unique_ptr<ui::SimpleMenuModel> menu_model,
       item ? item->id.app_id : std::string(), std::move(menu_model), source,
       source_type,
       base::BindOnce(&ShelfView::OnMenuClosed, base::Unretained(this), source));
-  shelf_menu_model_adapter_->Run(GetMenuAnchorRect(*source, click_point),
-                                 GetMenuAnchorPosition(item, context_menu),
-                                 run_types);
+  shelf_menu_model_adapter_->Run(
+      GetMenuAnchorRect(*source, click_point, context_menu),
+      GetMenuAnchorPosition(item, context_menu), run_types);
 }
 
 void ShelfView::OnMenuClosed(views::View* source) {

ash/shelf/shelf_view.h

@@ -306,9 +306,11 @@ class ASH_EXPORT ShelfView : public views::View,
   void UpdateOverflowRange(ShelfView* overflow_view) const;
 
   // Gets the menu anchor rect for menus. |source| is the view that is
-  // asking for a menu, |location| is the location of the event.
+  // asking for a menu, |location| is the location of the event, |context_menu|
+  // is whether the menu is for a context or application menu.
   gfx::Rect GetMenuAnchorRect(const views::View& source,
-                              const gfx::Point& location) const;
+                              const gfx::Point& location,
+                              bool context_menu) const;
 
   // Gets the menu anchor position for a menu. |for_item| is true if the menu is
   // for an item on the shelf, or false if the menu is for the shelf view

ash/shelf/shelf_view_test_api.cc

@@ -117,10 +117,10 @@ void ShelfViewTestAPI::RunMessageLoopUntilAnimationsDone() {
   shelf_view_->bounds_animator_->RemoveObserver(observer.get());
 }
 
-gfx::Rect ShelfViewTestAPI::GetMenuAnchorRect(
-    const views::View& source,
-    const gfx::Point& location) const {
-  return shelf_view_->GetMenuAnchorRect(source, location);
+gfx::Rect ShelfViewTestAPI::GetMenuAnchorRect(const views::View& source,
+                                              const gfx::Point& location,
+                                              bool context_menu) const {
+  return shelf_view_->GetMenuAnchorRect(source, location, context_menu);
 }
 
 bool ShelfViewTestAPI::CloseMenu() {

ash/shelf/shelf_view_test_api.h

@@ -76,7 +76,8 @@ class ShelfViewTestAPI {
   // Gets the anchor point that would be used for a context menu with these
   // parameters.
   gfx::Rect GetMenuAnchorRect(const views::View& source,
-                              const gfx::Point& location) const;
+                              const gfx::Point& location,
+                              bool context_menu) const;
 
   // Close any open app list or context menu; returns true if a menu was closed.
   bool CloseMenu();

ash/shelf/shelf_view_unittest.cc

@@ -2083,16 +2083,36 @@ TEST_F(ShelfViewTest, ShelfDragViewAndContextMenu) {
   EXPECT_FALSE(shelf_view_->drag_view());
 }
 
-class ShelfViewTouchableContextMenuTest : public ShelfViewTest {
+struct TouchableAppContextMenuTestParams {
+  TouchableAppContextMenuTestParams(bool enable_touchable_app_context_menu,
+                                    bool context_menu)
+      : enable_touchable_app_context_menu(enable_touchable_app_context_menu),
+        context_menu(context_menu) {}
+  // Whether to enable the touchable app context menu feature.
+  bool enable_touchable_app_context_menu;
+  // Whether the menu is shown as an application or context menu.
+  bool context_menu;
+};
+
+class ShelfViewTouchableContextMenuTest
+    : public ShelfViewTest,
+      public testing::WithParamInterface<TouchableAppContextMenuTestParams> {
  public:
   ShelfViewTouchableContextMenuTest() = default;
   ~ShelfViewTouchableContextMenuTest() override = default;
 
   void SetUp() override {
-    scoped_feature_list_.InitWithFeatures(
-        {features::kTouchableAppContextMenu, features::kNotificationIndicator},
-        {});
-
+    // If the test is parameterized, respect the parameter. Otherwise enable
+    // touchable app context menus by default.
+    const bool enable_touchable_app_context_menu =
+        testing::UnitTest::GetInstance()->current_test_info()->value_param()
+            ? GetParam().enable_touchable_app_context_menu
+            : true;
+    std::vector<base::Feature> enabled_features = {
+        features::kNotificationIndicator};
+    if (enable_touchable_app_context_menu)
+      enabled_features.push_back(features::kTouchableAppContextMenu);
+    scoped_feature_list_.InitWithFeatures(enabled_features, {});
     ShelfViewTest::SetUp();
   }
 
@@ -2102,27 +2122,59 @@ class ShelfViewTouchableContextMenuTest : public ShelfViewTest {
   DISALLOW_COPY_AND_ASSIGN(ShelfViewTouchableContextMenuTest);
 };
 
+INSTANTIATE_TEST_CASE_P(
+    TouchableDisabledAppMenu,
+    ShelfViewTouchableContextMenuTest,
+    ::testing::Values(TouchableAppContextMenuTestParams(false, false)));
+
+INSTANTIATE_TEST_CASE_P(
+    TouchableEnabledAppMenu,
+    ShelfViewTouchableContextMenuTest,
+    ::testing::Values(TouchableAppContextMenuTestParams(true, false)));
+
+INSTANTIATE_TEST_CASE_P(
+    TouchableDisabledContextMenu,
+    ShelfViewTouchableContextMenuTest,
+    ::testing::Values(TouchableAppContextMenuTestParams(false, true)));
+
+INSTANTIATE_TEST_CASE_P(
+    TouchableEnabledContextMenu,
+    ShelfViewTouchableContextMenuTest,
+    ::testing::Values(TouchableAppContextMenuTestParams(true, true)));
+
 // Tests that menu anchor points are aligned with the shelf button bounds.
-TEST_F(ShelfViewTouchableContextMenuTest, ShelfViewMenuAnchorPoint) {
+TEST_P(ShelfViewTouchableContextMenuTest, ShelfViewMenuAnchorPoint) {
   const ShelfButton* shelf_button = GetButtonByID(AddApp());
+  const bool context_menu = GetParam().context_menu;
   EXPECT_EQ(ash::ShelfAlignment::SHELF_ALIGNMENT_BOTTOM,
             GetPrimaryShelf()->alignment());
 
   // Test for bottom shelf.
-  EXPECT_EQ(shelf_button->GetBoundsInScreen().y(),
-            test_api_->GetMenuAnchorRect(*shelf_button, gfx::Point()).y());
+  EXPECT_EQ(
+      shelf_button->GetBoundsInScreen().y(),
+      test_api_->GetMenuAnchorRect(*shelf_button, gfx::Point(), context_menu)
+          .y());
 
   // Test for left shelf.
   GetPrimaryShelf()->SetAlignment(ash::ShelfAlignment::SHELF_ALIGNMENT_LEFT);
 
-  EXPECT_EQ(shelf_button->GetBoundsInScreen().x(),
-            test_api_->GetMenuAnchorRect(*shelf_button, gfx::Point()).x());
+  int expected_x = shelf_button->GetBoundsInScreen().x();
+  // Left shelf context menus when TouchableAppContextMenu is disabled anchor
+  // off of the right edge of the shelf.
+  if (context_menu && !features::IsTouchableAppContextMenuEnabled())
+    expected_x = shelf_button->GetBoundsInScreen().right();
+  EXPECT_EQ(
+      expected_x,
+      test_api_->GetMenuAnchorRect(*shelf_button, gfx::Point(), context_menu)
+          .x());
 
   // Test for right shelf.
   GetPrimaryShelf()->SetAlignment(ash::ShelfAlignment::SHELF_ALIGNMENT_RIGHT);
 
-  EXPECT_EQ(shelf_button->GetBoundsInScreen().x(),
-            test_api_->GetMenuAnchorRect(*shelf_button, gfx::Point()).x());
+  EXPECT_EQ(
+      shelf_button->GetBoundsInScreen().x(),
+      test_api_->GetMenuAnchorRect(*shelf_button, gfx::Point(), context_menu)
+          .x());
 }
 
 // Tests that the app list button does not show a context menu on right click