Events on shelf items should not dismiss the app list

AppListPresetnerDelegateImpl adds a pre target event handler that
dismisses the app list when the user clicks or taps outside the app list
bounds. Certain areas of the shelf are excluded from handling, but they
do not include shelf app buttons. This means that the app list hides
when the user presses the mouse on a shelf app button, or shelf arrows.

This breaks shelf interactions in HTML fullscreen mode, where the shelf
is only shown with the app list. The app list gets hidden before the
mouse is released, which also hides the shelf (note that auto-hide lock
set by the AppListControllerImpl does not work in this case, as the
shelf transitions to HIDDEN state when the app list is hidden). This
prevents shelf app button clicks from getting handled - ShelfAppButton
ignores click events if the shelf is not visible.

In addition to breaking HTML fullscreen mode, hiding app list when the
user starts interacting with a shelf app button creates other unexpected
interactions. For example, dismissing app list when the user clicks a
scrollable shelf arrow is not intuitive. Note that the app list will get
hidden when an app gets launched from the shelf (if the user clicks a
shelf app button).

BUG=1097131, 1090713

Change-Id: I6b87b17eb9550840aeb0a2a0728baf9e49e96010
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2261260Reviewed-by: Alex Newcomer <newcomer@chromium.org>
Commit-Queue: Toni Baržić <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#781564}

ash/app_list/app_list_presenter_delegate_impl.cc

@@ -19,6 +19,7 @@
 #include "ash/root_window_controller.h"
 #include "ash/shelf/back_button.h"
 #include "ash/shelf/home_button.h"
+#include "ash/shelf/hotseat_widget.h"
 #include "ash/shelf/shelf_layout_manager.h"
 #include "ash/shelf/shelf_navigation_widget.h"
 #include "ash/shelf/shelf_widget.h"
@@ -247,17 +248,21 @@ void AppListPresenterDelegateImpl::ProcessLocatedEvent(
   aura::Window* window = view_->GetWidget()->GetNativeView()->parent();
   if (!window->Contains(target) && !presenter_->HandleCloseOpenFolder() &&
       !switches::ShouldNotDismissOnBlur() && !IsTabletMode()) {
+    // Do not dismiss the app list if the event is targeting shelf area
+    // containing app icons.
+    if (target == shelf->hotseat_widget()->GetNativeWindow() &&
+        shelf->hotseat_widget()->EventTargetsShelfView(*event)) {
+      return;
+    }
+
+    // Don't dismiss the auto-hide shelf if event happened in status area. Then
+    // the event can still be propagated.
+    base::Optional<Shelf::ScopedAutoHideLock> auto_hide_lock;
     const aura::Window* status_window =
         shelf->shelf_widget()->status_area_widget()->GetNativeWindow();
-    const aura::Window* hotseat_window =
-        shelf->hotseat_widget()->GetNativeWindow();
-    // Don't dismiss the auto-hide shelf if event happened in status area or the
-    // hotseat. Then the event can still be propagated.
-    base::Optional<Shelf::ScopedAutoHideLock> auto_hide_lock;
-    if ((status_window && status_window->Contains(target)) ||
-        (hotseat_window && hotseat_window->Contains(target))) {
+    if (status_window && status_window->Contains(target))
       auto_hide_lock.emplace(shelf);
-    }
+
     // Record the current AppListViewState to be used later for metrics. The
     // AppListViewState will change on app launch, so this will record the
     // AppListViewState before the app was launched.

ash/app_list/app_list_presenter_delegate_unittest.cc

@@ -47,10 +47,14 @@
 #include "ash/public/cpp/test/shell_test_api.h"
 #include "ash/root_window_controller.h"
 #include "ash/shelf/home_button.h"
+#include "ash/shelf/scrollable_shelf_view.h"
 #include "ash/shelf/shelf.h"
+#include "ash/shelf/shelf_app_button.h"
 #include "ash/shelf/shelf_layout_manager.h"
 #include "ash/shelf/shelf_navigation_widget.h"
+#include "ash/shelf/shelf_test_util.h"
 #include "ash/shelf/shelf_view.h"
+#include "ash/shelf/shelf_view_test_api.h"
 #include "ash/shell.h"
 #include "ash/system/unified/unified_system_tray.h"
 #include "ash/test/ash_test_base.h"
@@ -2732,10 +2736,85 @@ TEST_F(AppListPresenterDelegateTest, TapAutoHideShelfWithAppListOpened) {
   GetAppListTestHelper()->CheckVisibility(true);
   EXPECT_EQ(SHELF_AUTO_HIDE_SHOWN, shelf->GetAutoHideState());
 
-  // Test that tapping the auto-hidden shelf keeps shelf visible but dismiss the
-  // app list.
+  // Make sure the shelf has at least one item.
+  ShelfItem item =
+      ShelfTestUtil::AddAppShortcut(base::NumberToString(1), TYPE_PINNED_APP);
+
+  // Wait for shelf view's bounds animation to end. Otherwise the scrollable
+  // shelf's bounds are not updated yet.
+  ShelfView* const shelf_view = shelf->GetShelfViewForTesting();
+  ShelfViewTestAPI shelf_view_test_api(shelf_view);
+  shelf_view_test_api.RunMessageLoopUntilAnimationsDone();
+
+  // Test that tapping the auto-hidden shelf dismisses the app list when tapping
+  // part of the shelf that does not contain the apps.
+  generator->GestureTapAt(shelf_view->GetBoundsInScreen().left_center() +
+                          gfx::Vector2d(10, 0));
+  GetAppListTestHelper()->CheckVisibility(false);
+  EXPECT_EQ(SHELF_AUTO_HIDE_HIDDEN, shelf->GetAutoHideState());
+
+  // Show the AppList again.
+  GetAppListTestHelper()->ShowAndRunLoop(GetPrimaryDisplayId());
+  GetAppListTestHelper()->CheckVisibility(true);
+  EXPECT_EQ(SHELF_AUTO_HIDE_SHOWN, shelf->GetAutoHideState());
+
+  // App list should remain visible when tapping on a shelf app button.
+  ASSERT_TRUE(shelf_view_test_api.GetButton(0));
   generator->GestureTapAt(
-      shelf->GetShelfViewForTesting()->GetBoundsInScreen().CenterPoint());
+      shelf_view_test_api.GetButton(0)->GetBoundsInScreen().CenterPoint());
+  GetAppListTestHelper()->CheckVisibility(true);
+  EXPECT_EQ(SHELF_AUTO_HIDE_SHOWN, shelf->GetAutoHideState());
+}
+
+TEST_F(AppListPresenterDelegateTest, ClickingShelfArrowDoesNotHideAppList) {
+  // Add enough shelf items for the shelf to enter overflow.
+  Shelf* const shelf = GetPrimaryShelf();
+  ScrollableShelfView* const scrollable_shelf_view =
+      shelf->hotseat_widget()->scrollable_shelf_view();
+  ShelfView* const shelf_view = shelf->GetShelfViewForTesting();
+  int index = 0;
+  while (scrollable_shelf_view->layout_strategy_for_test() ==
+         ScrollableShelfView::kNotShowArrowButtons) {
+    ShelfItem item = ShelfTestUtil::AddAppShortcut(
+        base::NumberToString(index++), TYPE_PINNED_APP);
+  }
+
+  ShelfViewTestAPI shelf_view_test_api(shelf_view);
+  shelf_view_test_api.RunMessageLoopUntilAnimationsDone();
+
+  shelf->SetAutoHideBehavior(ShelfAutoHideBehavior::kAlways);
+
+  GetAppListTestHelper()->ShowAndRunLoop(GetPrimaryDisplayId());
+  GetAppListTestHelper()->CheckVisibility(true);
+  EXPECT_EQ(SHELF_AUTO_HIDE_SHOWN, shelf->GetAutoHideState());
+
+  // Click right scrollable shelf arrow - verify the the app list remains
+  // visible.
+  const views::View* right_arrow = scrollable_shelf_view->right_arrow();
+  ASSERT_TRUE(right_arrow->GetVisible());
+  GetEventGenerator()->MoveMouseTo(
+      right_arrow->GetBoundsInScreen().CenterPoint());
+  GetEventGenerator()->ClickLeftButton();
+
+  GetAppListTestHelper()->CheckVisibility(true);
+  EXPECT_EQ(SHELF_AUTO_HIDE_SHOWN, shelf->GetAutoHideState());
+
+  // Click left button - verify the app list stays visible.
+  const views::View* left_arrow = scrollable_shelf_view->left_arrow();
+  ASSERT_TRUE(left_arrow->GetVisible());
+  GetEventGenerator()->MoveMouseTo(
+      left_arrow->GetBoundsInScreen().CenterPoint());
+  GetEventGenerator()->ClickLeftButton();
+
+  GetAppListTestHelper()->CheckVisibility(true);
+  EXPECT_EQ(SHELF_AUTO_HIDE_SHOWN, shelf->GetAutoHideState());
+
+  // Click right of the right arrow - verify the app list gets dismissed.
+  ASSERT_TRUE(right_arrow->GetVisible());
+  GetEventGenerator()->MoveMouseTo(
+      right_arrow->GetBoundsInScreen().right_center() + gfx::Vector2d(10, 0));
+  GetEventGenerator()->ClickLeftButton();
+
   GetAppListTestHelper()->CheckVisibility(false);
   EXPECT_EQ(SHELF_AUTO_HIDE_SHOWN, shelf->GetAutoHideState());
 }

ash/shelf/hotseat_widget.cc

@@ -904,6 +904,15 @@ bool HotseatWidget::IsShowingShelfMenu() const {
   return GetShelfView()->IsShowingMenu();
 }
 
+bool HotseatWidget::EventTargetsShelfView(const ui::LocatedEvent& event) const {
+  DCHECK_EQ(event.target(), GetNativeWindow());
+  gfx::Point location_in_shelf_view = event.location();
+  views::View::ConvertPointFromWidget(scrollable_shelf_view_,
+                                      &location_in_shelf_view);
+  return scrollable_shelf_view_->GetHotseatBackgroundBounds().Contains(
+      location_in_shelf_view);
+}
+
 const ShelfView* HotseatWidget::GetShelfView() const {
   return const_cast<const ShelfView*>(
       const_cast<HotseatWidget*>(this)->GetShelfView());

ash/shelf/hotseat_widget.h

@@ -126,6 +126,9 @@ class ASH_EXPORT HotseatWidget : public ShelfComponent,
 
   bool IsShowingShelfMenu() const;
 
+  // Whether the event is located in the hotseat area containing shelf apps.
+  bool EventTargetsShelfView(const ui::LocatedEvent& event) const;
+
   ShelfView* GetShelfView();
   const ShelfView* GetShelfView() const;
 

ash/shelf/shelf_view_unittest.cc

@@ -48,6 +48,7 @@
 #include "ash/wallpaper/wallpaper_controller_impl.h"
 #include "ash/wallpaper/wallpaper_controller_test_api.h"
 #include "ash/wm/tablet_mode/tablet_mode_controller.h"
+#include "ash/wm/window_state.h"
 #include "base/i18n/rtl.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
@@ -1977,6 +1978,79 @@ TEST_F(ShelfViewTest, ReplacingDelegateCancelsContextMenu) {
   EXPECT_FALSE(shelf_view_->IsShowingMenu());
 }
 
+// Verifies that shelf is shown with the app list in fullscreen mode, and that
+// shelf app buttons are clickable.
+TEST_F(ShelfViewTest, ClickItemInFullscreen) {
+  ShelfID app_button_id = AddAppShortcut();
+  auto selection_tracker_owned = std::make_unique<ShelfItemSelectionTracker>();
+  ShelfItemSelectionTracker* selection_tracker = selection_tracker_owned.get();
+  model_->SetShelfItemDelegate(app_button_id,
+                               std::move(selection_tracker_owned));
+
+  // Create a fullscreen widget.
+  std::unique_ptr<views::Widget> widget = CreateTestWidget();
+  widget->SetFullscreen(true);
+  WindowState* window_state = WindowState::Get(widget->GetNativeWindow());
+  window_state->SetHideShelfWhenFullscreen(true);
+
+  Shelf* const shelf = Shelf::ForWindow(Shell::GetPrimaryRootWindow());
+  EXPECT_EQ(SHELF_HIDDEN, shelf->GetVisibilityState());
+
+  // Show app list, which will bring the shelf up.
+  GetAppListTestHelper()->ShowAndRunLoop(GetPrimaryDisplayId());
+
+  EXPECT_EQ(SHELF_AUTO_HIDE, shelf->GetVisibilityState());
+  EXPECT_EQ(SHELF_AUTO_HIDE_SHOWN, shelf->GetAutoHideState());
+
+  // Verify that clicking a shelf button activates it.
+  EXPECT_EQ(0u, selection_tracker->item_selected_count());
+  const ShelfAppButton* shelf_button = GetButtonByID(app_button_id);
+  GetEventGenerator()->MoveMouseTo(
+      shelf_button->GetBoundsInScreen().CenterPoint());
+  GetEventGenerator()->ClickLeftButton();
+  EXPECT_EQ(1u, selection_tracker->item_selected_count());
+
+  // Shelf gets hidden when the app list is dismissed.
+  GetAppListTestHelper()->DismissAndRunLoop();
+  EXPECT_EQ(SHELF_HIDDEN, shelf->GetVisibilityState());
+}
+
+// Verifies that shelf is shown with the app list in fullscreen mode, and that
+// shelf app buttons are tappable.
+TEST_F(ShelfViewTest, TapInFullscreen) {
+  ShelfID app_button_id = AddAppShortcut();
+  auto selection_tracker_owned = std::make_unique<ShelfItemSelectionTracker>();
+  ShelfItemSelectionTracker* selection_tracker = selection_tracker_owned.get();
+  model_->SetShelfItemDelegate(app_button_id,
+                               std::move(selection_tracker_owned));
+
+  // Create a fullscreen widget.
+  std::unique_ptr<views::Widget> widget = CreateTestWidget();
+  widget->SetFullscreen(true);
+  WindowState* window_state = WindowState::Get(widget->GetNativeWindow());
+  window_state->SetHideShelfWhenFullscreen(true);
+
+  Shelf* const shelf = Shelf::ForWindow(Shell::GetPrimaryRootWindow());
+  EXPECT_EQ(SHELF_HIDDEN, shelf->GetVisibilityState());
+
+  // Show app list, which will bring the shelf up.
+  GetAppListTestHelper()->ShowAndRunLoop(GetPrimaryDisplayId());
+
+  EXPECT_EQ(SHELF_AUTO_HIDE, shelf->GetVisibilityState());
+  EXPECT_EQ(SHELF_AUTO_HIDE_SHOWN, shelf->GetAutoHideState());
+
+  // Verify that tapping a shelf button activates it.
+  EXPECT_EQ(0u, selection_tracker->item_selected_count());
+  const ShelfAppButton* shelf_button = GetButtonByID(app_button_id);
+  GetEventGenerator()->GestureTapAt(
+      shelf_button->GetBoundsInScreen().CenterPoint());
+  EXPECT_EQ(1u, selection_tracker->item_selected_count());
+
+  // Shelf gets hidden when the app list is dismissed.
+  GetAppListTestHelper()->DismissAndRunLoop();
+  EXPECT_EQ(SHELF_HIDDEN, shelf->GetVisibilityState());
+}
+
 // Test class that tests both context and application menus.
 class ShelfViewMenuTest : public ShelfViewTest,
                           public testing::WithParamInterface<bool> {