CrOS Shelf: Change icon centering strategy

The main change here is that icons are first centered on the whole
screen, then, when space becomes tighter, they are centered over the
available area on the shelf.

A few changes to make this work properly:

* Fix an issue in |GetDimensionOfCenteredShelfItems| which was taking
  into account all items, even the ones within overflow, making the
  calculations incorrect.
* Another issue was that the code calculating ideal bounds would
  assume control buttons (app list, back, overflow) were the same size
  as app buttons (56) while they're actually a little smaller (40).
  Giving control buttons their "real" size in the layout code allows
  for the removal of some overrides at each button class level.
* Split out a small chunk from |CalculateIdealBounds| which is getting
  too long. More of that to come in future changes.
* Make the special spacing between the app list button and the first
  app icon also there on the other side for more symmetry.

Also add some extensive test coverage.

Bug: 891080, 933291
Change-Id: If4bb69aa6e182cd29c02d467039b5d839d09f493
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1512299
Commit-Queue: Manu Cornet <manucornet@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#642245}

ash/shelf/app_list_button.cc

@@ -188,19 +188,6 @@ const char* AppListButton::GetClassName() const {
   return kViewClassName;
 }
 
-std::unique_ptr<views::InkDropRipple> AppListButton::CreateInkDropRipple()
-    const {
-  const int app_list_button_radius = ShelfConstants::control_border_radius();
-  gfx::Point center = GetCenterPoint();
-  gfx::Rect bounds(center.x() - app_list_button_radius,
-                   center.y() - app_list_button_radius,
-                   2 * app_list_button_radius, 2 * app_list_button_radius);
-  return std::make_unique<views::FloodFillInkDropRipple>(
-      size(), GetLocalBounds().InsetsFrom(bounds),
-      GetInkDropCenterBasedOnLastEvent(), GetInkDropBaseColor(),
-      ink_drop_visible_opacity());
-}
-
 void AppListButton::PaintButtonContents(gfx::Canvas* canvas) {
   gfx::PointF circle_center(GetCenterPoint());
 

ash/shelf/app_list_button.h

@@ -50,7 +50,6 @@ class ASH_EXPORT AppListButton : public ShelfControlButton,
 
  protected:
   // views::Button:
-  std::unique_ptr<views::InkDropRipple> CreateInkDropRipple() const override;
   void PaintButtonContents(gfx::Canvas* canvas) override;
 
  private:

ash/shelf/app_list_button_unittest.cc

@@ -16,6 +16,7 @@
 #include "ash/root_window_controller.h"
 #include "ash/session/session_controller.h"
 #include "ash/shelf/shelf.h"
+#include "ash/shelf/shelf_constants.h"
 #include "ash/shelf/shelf_view.h"
 #include "ash/shelf/shelf_view_test_api.h"
 #include "ash/shell.h"
@@ -153,7 +154,7 @@ TEST_F(AppListButtonTest, ButtonPositionInTabletMode) {
 
   Shell::Get()->tablet_mode_controller()->EnableTabletModeWindowManager(false);
   test_api.RunMessageLoopUntilAnimationsDone();
-  EXPECT_EQ(0, app_list_button()->bounds().x());
+  EXPECT_EQ(ShelfConstants::button_spacing(), app_list_button()->bounds().x());
 }
 
 class VoiceInteractionAppListButtonTest : public AppListButtonTest {

ash/shelf/overflow_bubble_view.cc

@@ -179,6 +179,8 @@ void OverflowBubbleView::OnScrollEvent(ui::ScrollEvent* event) {
 gfx::Rect OverflowBubbleView::GetBubbleBounds() {
   const gfx::Size content_size = GetPreferredSize();
   const gfx::Rect anchor_rect = GetAnchorRect();
+  const int distance_to_overflow_button =
+      kDistanceToMainShelf + (kShelfSize - kShelfControlSize) / 2;
   gfx::Rect monitor_rect =
       display::Screen::GetScreen()
           ->GetDisplayNearestPoint(anchor_rect.CenterPoint())
@@ -191,7 +193,7 @@ gfx::Rect OverflowBubbleView::GetBubbleBounds() {
         base::i18n::IsRTL()
             ? anchor_rect.x() - kEndPadding
             : anchor_rect.right() - content_size.width() - kEndPadding,
-        anchor_rect.y() - kDistanceToMainShelf - content_size.height(),
+        anchor_rect.y() - distance_to_overflow_button - content_size.height(),
         content_size.width() + 2 * kEndPadding, content_size.height());
     if (bounds.x() < monitor_rect.x())
       bounds.Offset(monitor_rect.x() - bounds.x(), 0);
@@ -203,9 +205,10 @@ gfx::Rect OverflowBubbleView::GetBubbleBounds() {
       0, anchor_rect.bottom() - content_size.height() - kEndPadding,
       content_size.width(), content_size.height() + 2 * kEndPadding);
   if (shelf_->alignment() == SHELF_ALIGNMENT_LEFT)
-    bounds.set_x(anchor_rect.right() + kDistanceToMainShelf);
+    bounds.set_x(anchor_rect.right() + distance_to_overflow_button);
   else
-    bounds.set_x(anchor_rect.x() - kDistanceToMainShelf - content_size.width());
+    bounds.set_x(anchor_rect.x() - distance_to_overflow_button -
+                 content_size.width());
   if (bounds.y() < monitor_rect.y())
     bounds.Offset(0, monitor_rect.y() - bounds.y());
   if (bounds.bottom() > monitor_rect.bottom())

ash/shelf/shelf_control_button.cc

@@ -22,21 +22,22 @@ namespace ash {
 ShelfControlButton::ShelfControlButton(ShelfView* shelf_view)
     : ShelfButton(shelf_view), shelf_(shelf_view->shelf()) {
   set_has_ink_drop_action_on_click(true);
-  SetSize(gfx::Size(kShelfControlSize, kShelfControlSize));
 }
 
 ShelfControlButton::~ShelfControlButton() = default;
 
 gfx::Point ShelfControlButton::GetCenterPoint() const {
-  return gfx::Point(width() / 2.f, width() / 2.f);
+  return gfx::Point(width() / 2.f, height() / 2.f);
 }
 
 std::unique_ptr<views::InkDropRipple> ShelfControlButton::CreateInkDropRipple()
     const {
+  const int button_radius = ShelfConstants::control_border_radius();
+  gfx::Point center = GetCenterPoint();
+  gfx::Rect bounds(center.x() - button_radius, center.y() - button_radius,
+                   2 * button_radius, 2 * button_radius);
   return std::make_unique<views::FloodFillInkDropRipple>(
-      size(),
-      gfx::Insets(ShelfConstants::button_size() / 2 -
-                  ShelfConstants::control_border_radius()),
+      size(), GetLocalBounds().InsetsFrom(bounds),
       GetInkDropCenterBasedOnLastEvent(), GetInkDropBaseColor(),
       ink_drop_visible_opacity());
 }
@@ -51,21 +52,8 @@ const char* ShelfControlButton::GetClassName() const {
   return "ash/ShelfControlButton";
 }
 
-gfx::Rect ShelfControlButton::CalculateButtonBounds() const {
-  ShelfAlignment alignment = shelf_->alignment();
-  gfx::Rect content_bounds = GetContentsBounds();
-  // Align the button to the top of a bottom-aligned shelf, to the right edge
-  // a left-aligned shelf, and to the left edge of a right-aligned shelf.
-  const int inset = (ShelfConstants::shelf_size() - kShelfControlSize) / 2;
-  const int x = alignment == SHELF_ALIGNMENT_LEFT
-                    ? content_bounds.right() - inset - kShelfControlSize
-                    : content_bounds.x() + inset;
-  return gfx::Rect(x, content_bounds.y() + inset, kShelfControlSize,
-                   kShelfControlSize);
-}
-
 void ShelfControlButton::PaintButtonContents(gfx::Canvas* canvas) {
-  PaintBackground(canvas, CalculateButtonBounds());
+  PaintBackground(canvas, GetContentsBounds());
 }
 
 void ShelfControlButton::PaintBackground(gfx::Canvas* canvas,

ash/shelf/shelf_control_button.h

@@ -37,9 +37,6 @@ class ASH_EXPORT ShelfControlButton : public ShelfButton {
   void PaintButtonContents(gfx::Canvas* canvas) override;
 
  private:
-  // Calculates the bounds of the control button based on the shelf alignment.
-  gfx::Rect CalculateButtonBounds() const;
-
   Shelf* shelf_;
 
   DISALLOW_COPY_AND_ASSIGN(ShelfControlButton);

ash/shelf/shelf_view.h

@@ -247,9 +247,10 @@ class ASH_EXPORT ShelfView : public views::View,
   // Returns whether |item| should belong in the pinned section of the shelf.
   bool IsItemPinned(const ShelfItem& item) const;
 
-  // Enumerates the shelf items that are centered in the new UI and returns
-  // the total size they occupy.
-  int GetDimensionOfCenteredShelfItems() const;
+  // Enumerates the shelf apps and returns the total size they occupy,
+  // accounting for all apps or, if the total size is greater than |max_size|,
+  // the size of however many app can fit without exceeding |max_size|.
+  int GetDimensionOfAppIcons(int max_size) const;
 
   // Returns the index of the item after which the separator should be shown,
   // or -1 if no separator is required.
@@ -343,7 +344,9 @@ class ASH_EXPORT ShelfView : public views::View,
 
   // Calculates the ideal bounds. The bounds of each button corresponding to an
   // item in the model is set in |view_model_|.
-  void CalculateIdealBounds(gfx::Rect* overflow_bounds) const;
+  void CalculateIdealBounds() const;
+
+  void LayoutOverflowButton() const;
 
   // Returns the index of the last view whose max primary axis coordinate is
   // less than |max_value|. Returns -1 if nothing fits, or there are no views.

ash/shelf/shelf_view_unittest.cc

@@ -12,6 +12,7 @@
 #include "ash/app_list/test/app_list_test_helper.h"
 #include "ash/app_list/views/app_list_view.h"
 #include "ash/focus_cycler.h"
+#include "ash/ime/ime_controller.h"
 #include "ash/public/cpp/shelf_item_delegate.h"
 #include "ash/public/cpp/shelf_model.h"
 #include "ash/public/cpp/shelf_prefs.h"
@@ -2281,6 +2282,122 @@ TEST_F(ShelfViewTest, AppListButtonDoesShowContextMenu) {
   EXPECT_TRUE(test_api_->CloseMenu());
 }
 
+void ExpectWithinOnePixel(int a, int b) {
+  EXPECT_TRUE(abs(a - b) <= 1) << "Values " << a << " and " << b
+                               << " should have a difference no greater than 1";
+}
+
+TEST_F(ShelfViewTest, IconCenteringTest) {
+  const display::Display display =
+      display::Screen::GetScreen()->GetPrimaryDisplay();
+  const int screen_width = display.bounds().width();
+  const int screen_center = screen_width / 2;
+
+  // Show the IME panel, to introduce for asymettry with a larger status area.
+  Shell::Get()->ime_controller()->ShowImeMenuOnShelf(true);
+
+  // At the start, we have exactly one app icon for the browser. That should
+  // be centered on the screen.
+  const ShelfAppButton* button1 = GetButtonByID(model_->items()[2].id);
+  ExpectWithinOnePixel(screen_center,
+                       button1->GetBoundsInScreen().CenterPoint().x());
+  // Also check that the distance between the icon edge and the screen edge is
+  // the same on both sides.
+  ExpectWithinOnePixel(button1->GetBoundsInScreen().x(),
+                       screen_width - button1->GetBoundsInScreen().right());
+
+  const int apps_that_can_fit_at_center_of_screen = 8;
+  std::vector<ShelfAppButton*> app_buttons;
+  // Start with just the browser app button.
+  app_buttons.push_back(GetButtonByID(model_->items()[2].id));
+  int n_buttons = 1;
+
+  // Now repeat the same process by adding apps until they can't fit at the
+  // center of the screen.
+  for (int i = 1; i < apps_that_can_fit_at_center_of_screen; ++i) {
+    // Add a new app and add its button to our list.
+    app_buttons.push_back(GetButtonByID(AddApp()));
+    n_buttons = app_buttons.size();
+    if (n_buttons % 2 == 1) {
+      // Odd number of apps. Check that the middle app is exactly at the center
+      // of the screen.
+      ExpectWithinOnePixel(
+          screen_center,
+          app_buttons[n_buttons / 2]->GetBoundsInScreen().CenterPoint().x());
+    }
+    // Also check that the first icon is at the same distance from the left
+    // screen edge as the last icon is from the right screen edge.
+    ExpectWithinOnePixel(
+        app_buttons[0]->GetBoundsInScreen().x(),
+        screen_width - app_buttons[n_buttons - 1]->GetBoundsInScreen().right());
+  }
+
+  // Add one more app. Now the block of apps should be at the center of the
+  // shelf part of the shelf widget (not including the status area) as opposed
+  // to at the center of the whole screen. But we're not overflowing yet.
+  app_buttons.push_back(GetButtonByID(AddApp()));
+  n_buttons = app_buttons.size();
+  EXPECT_FALSE(shelf_view_->GetOverflowButton()->visible());
+  // Icons at either end should also be at the same distance from the app list
+  // button on the left, and the status area on the right.
+  gfx::NativeWindow window = shelf_view_->shelf_widget()->GetNativeWindow();
+  views::View* status_area_view = RootWindowController::ForWindow(window)
+                                      ->GetStatusAreaWidget()
+                                      ->GetContentsView();
+  const int status_area_left = status_area_view->GetBoundsInScreen().x();
+  const int app_list_button_right =
+      shelf_view_->GetAppListButton()->GetBoundsInScreen().right();
+  ExpectWithinOnePixel(
+      app_buttons[0]->GetBoundsInScreen().x() - app_list_button_right,
+      status_area_left -
+          app_buttons[n_buttons - 1]->GetBoundsInScreen().right());
+
+  // Add another app. The overflow button should now appear.
+  app_buttons.push_back(GetButtonByID(AddApp()));
+  n_buttons = app_buttons.size();
+  EXPECT_TRUE(shelf_view_->GetOverflowButton()->visible());
+}
+
+TEST_F(ShelfViewTest, FirstAndLastVisibleIndex) {
+  // At the start, the only things visible on the shelf are the app list button
+  // (index 1) and the browser app button (index 2).
+  EXPECT_EQ(1, shelf_view_->first_visible_index());
+  EXPECT_EQ(2, shelf_view_->last_visible_index());
+  // By enabling tablet mode, the back button (index 0) should become visible.
+  Shell::Get()->tablet_mode_controller()->EnableTabletModeWindowManager(true);
+  EXPECT_EQ(0, shelf_view_->first_visible_index());
+  EXPECT_EQ(2, shelf_view_->last_visible_index());
+  // And things should return back to the previous state once tablet mode is off
+  // again.
+  Shell::Get()->tablet_mode_controller()->EnableTabletModeWindowManager(false);
+  EXPECT_EQ(1, shelf_view_->first_visible_index());
+  EXPECT_EQ(2, shelf_view_->last_visible_index());
+  // Now let's add some apps until the overflow button shows up, each time
+  // checking the first and last visible indices are what we expect.
+  int last_visible_index = 2;
+  int last_visible_index_before_overflow;
+  ShelfID last_added_item_id;
+  while (true) {
+    last_added_item_id = AddApp();
+    if (shelf_view_->GetOverflowButton()->visible()) {
+      last_visible_index_before_overflow = last_visible_index;
+      break;
+    }
+    last_visible_index++;
+    EXPECT_EQ(1, shelf_view_->first_visible_index());
+    EXPECT_EQ(last_visible_index, shelf_view_->last_visible_index());
+  }
+  // Now remove the last item we just added. That should get rid of the
+  // overflow button, and get back to the previous state.
+  RemoveByID(last_added_item_id);
+  EXPECT_EQ(1, shelf_view_->first_visible_index());
+  EXPECT_EQ(last_visible_index_before_overflow,
+            shelf_view_->last_visible_index());
+  // Adding another app should let the overflow button appear again.
+  AddApp();
+  EXPECT_TRUE(shelf_view_->GetOverflowButton()->visible());
+}
+
 // Test class that tests both context and application menus.
 class ShelfViewMenuTest : public ShelfViewTest,
                           public testing::WithParamInterface<bool> {