CrOS shelf: Hide tooltip between launcher button and 1st shelf item

Shelf items used to be aligned directly after the launcher button,
so it made sense to include everything within bounds where tooltips
should not hide to avoid hiccups.

However, now that we show centered shelf items, this means that a
tooltip can be shown all over a possibly quite large amount of white
space between the launcher button and the first shelf item.

1) It is very strange to show a tooltip over such a potentially large
piece of shelf. 2) On the other hand, when it does happen that the
first shelf item is close to the launcher button, not showing a
tooltip in the space between them does mean a slight stutter when
hovering from one to the next.

The drawbacks of 1) clearly outweigh the ones of 2), so this change
hides tooltips between the launcher button and the first shelf item.

Bug: 896524
Change-Id: I06e666ba0296881f87c2cf3ce5972b38d638c3f7
Reviewed-on: https://chromium-review.googlesource.com/c/1393889
Commit-Queue: Manu Cornet <manucornet@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619727}

ash/shelf/shelf_view.cc

@@ -487,6 +487,11 @@ void ShelfView::UpdateVisibleShelfItemBoundsUnion() {
   visible_shelf_item_bounds_union_.SetRect(0, 0, 0, 0);
   for (int i = first_visible_index_; i <= last_visible_index_; ++i) {
     const views::View* child = view_model_->view_at(i);
+    // Since shelf items are centered, we don't want to include the app list
+    // button, otherwise tooltips will show when hovering a potentially large
+    // amount of white space between the app list button and the first item.
+    if (child == GetAppListButton())
+      continue;
     if (!IsTabletModeEnabled() && child == GetBackButton())
       continue;
     if (ShouldShowTooltipForView(child))
@@ -495,20 +500,22 @@ void ShelfView::UpdateVisibleShelfItemBoundsUnion() {
 }
 
 bool ShelfView::ShouldHideTooltip(const gfx::Point& cursor_location) const {
-  // Hide the tooltip if this is the app list button and the list is showing.
+  // If this is the app list button, only show the tooltip if the app list is
+  // not already showing.
   const AppListButton* app_list_button = GetAppListButton();
   if (app_list_button &&
-      app_list_button->GetMirroredBounds().Contains(cursor_location) &&
-      app_list_button->is_showing_app_list()) {
-    return true;
+      app_list_button->GetMirroredBounds().Contains(cursor_location)) {
+    return app_list_button->is_showing_app_list();
   }
   return !visible_shelf_item_bounds_union_.Contains(cursor_location);
 }
 
 bool ShelfView::ShouldShowTooltipForView(const views::View* view) const {
   // TODO(msw): Push this app list state into ShelfItem::shows_tooltip.
-  if (view == GetAppListButton() && GetAppListButton()->is_showing_app_list())
-    return false;
+  // If this is the app list button, only show the tooltip if the app list is
+  // not already showing.
+  if (view == GetAppListButton())
+    return !GetAppListButton()->is_showing_app_list();
   // Don't show a tooltip for a view that's currently being dragged.
   if (view == drag_view_)
     return false;

ash/shelf/shelf_view_unittest.cc

@@ -1373,20 +1373,46 @@ TEST_F(ShelfViewTest, ShelfAlignmentClosesTooltip) {
 TEST_F(ShelfViewTest, ShouldHideTooltipTest) {
   ShelfID app_button_id = AddAppShortcut();
   ShelfID platform_button_id = AddApp();
+  // TODO(manucornet): It should not be necessary to call this manually. The
+  // |AddItem| call seems to sometimes be missing some re-layout steps. We
+  // should find out what's going on there.
+  shelf_view_->UpdateVisibleShelfItemBoundsUnion();
+  const AppListButton* app_list_button = shelf_view_->GetAppListButton();
+
+  // Make sure we're not showing the app list.
+  EXPECT_FALSE(app_list_button->is_showing_app_list())
+      << "We should not be showing the app list";
 
   // The tooltip shouldn't hide if the mouse is on normal buttons.
   for (int i = 0; i < test_api_->GetButtonCount(); i++) {
     ShelfButton* button = test_api_->GetButton(i);
     if (!button)
       continue;
-
     EXPECT_FALSE(shelf_view_->ShouldHideTooltip(
         button->GetMirroredBounds().CenterPoint()))
         << "ShelfView tries to hide on button " << i;
   }
 
+  // The tooltip should hide if placed in between the app list button and the
+  // first shelf button.
+  const int left = app_list_button->bounds().right();
+  // Find the first shelf button that's to the right of the app list button.
+  int right = 0;
+  for (int i = 0; i < test_api_->GetButtonCount(); ++i) {
+    ShelfButton* button = test_api_->GetButton(i);
+    if (!button)
+      continue;
+    right = button->bounds().x();
+    if (right > left)
+      break;
+  }
+  const int center_x =
+      shelf_view_->GetMirroredXInView(left + (right - left) / 2);
+  EXPECT_TRUE(shelf_view_->ShouldHideTooltip(gfx::Point(
+      center_x, app_list_button->GetMirroredBounds().left_center().y())))
+      << "Tooltip should hide between app list button and first shelf item";
+
   // The tooltip should not hide on the app-list button.
-  AppListButton* app_list_button = shelf_view_->GetAppListButton();
   EXPECT_FALSE(shelf_view_->ShouldHideTooltip(
       app_list_button->GetMirroredBounds().CenterPoint()));
 
@@ -1462,13 +1488,12 @@ TEST_F(ShelfViewTest, ShouldHideTooltipWhenHoveringOnTooltip) {
   EXPECT_TRUE(tooltip_manager->IsVisible());
 
   // Move the mouse cursor slightly to the right of the item. The tooltip should
-  // stay open.
+  // now close.
   generator->MoveMouseBy(bounds.width() / 2 + 5, 0);
-  // Make sure there is no delayed close.
   base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(tooltip_manager->IsVisible());
+  EXPECT_FALSE(tooltip_manager->IsVisible());
 
-  // Move back - it should still stay open.
+  // Move back - it should appear again.
   generator->MoveMouseBy(-(bounds.width() / 2 + 5), 0);
   // Make sure there is no delayed close.
   base::RunLoop().RunUntilIdle();