Fix the issue that focus ring does not work well for scrollable shelf

Due to my previous CL (crrev.com/c/1849096), |first_tappable_index_|
and |last_tappable_index_| are calculated in the wrong way. This CL
fixes such an issue.

Bug: 1013811
Change-Id: I2723070b8a0ecfef31e82433148d1df84937e23f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1857043
Commit-Queue: Andrew Xu <andrewxu@chromium.org>
Reviewed-by: Manu Cornet <manucornet@chromium.org>
Cr-Commit-Position: refs/heads/master@{#705788}

ash/shelf/scrollable_shelf_view.cc

@@ -1193,25 +1193,30 @@ void ScrollableShelfView::UpdateTappableIconIndices() {
     return;
   }
 
-  int actual_scroll_distance = GetActualScrollOffset();
-  const gfx::Insets ripple_insets = CalculateRipplePaddingInsets();
-  const int ripple_padding_sum = GetShelf()->IsHorizontalAlignment()
-                                     ? ripple_insets.width()
-                                     : ripple_insets.height();
-  int shelf_container_available_space =
-      (GetShelf()->IsHorizontalAlignment() ? visible_space_.width()
-                                           : visible_space_.height()) -
-      ripple_padding_sum;
+  const int scroll_distance_on_main_axis = GetShelf()->IsHorizontalAlignment()
+                                               ? scroll_offset_.x()
+                                               : scroll_offset_.y();
+  const int visible_size = GetShelf()->IsHorizontalAlignment()
+                               ? visible_space_.width()
+                               : visible_space_.height();
+
   if (layout_strategy_ == kShowRightArrowButton ||
       layout_strategy_ == kShowButtons) {
-    first_tappable_app_index_ = actual_scroll_distance / GetUnit();
+    first_tappable_app_index_ = scroll_distance_on_main_axis / GetUnit() +
+                                (layout_strategy_ == kShowButtons ? 1 : 0);
     last_tappable_app_index_ =
-        first_tappable_app_index_ + shelf_container_available_space / GetUnit();
+        first_tappable_app_index_ + visible_size / GetUnit();
+
+    const int end_of_last_tappable_app = last_tappable_app_index_ * GetUnit() +
+                                         ShelfConfig::Get()->button_size() -
+                                         scroll_distance_on_main_axis;
+    if (end_of_last_tappable_app > visible_size)
+      last_tappable_app_index_--;
   } else {
     DCHECK_EQ(layout_strategy_, kShowLeftArrowButton);
     last_tappable_app_index_ = shelf_view_->last_visible_index();
     first_tappable_app_index_ =
-        last_tappable_app_index_ - shelf_container_available_space / GetUnit();
+        last_tappable_app_index_ - visible_size / GetUnit() + 1;
   }
 }
 

ash/shelf/scrollable_shelf_view_unittest.cc

@@ -62,12 +62,43 @@ class ScrollableShelfViewTest : public AshTestBase {
     return item.id;
   }
 
-  void AddAppShortcutUntilOverflow() {
+  void AddAppShortcutsUntilOverflow() {
     while (scrollable_shelf_view_->layout_strategy_for_test() ==
            ScrollableShelfView::kNotShowArrowButtons)
       AddAppShortcut();
   }
 
+  void AddAppShortcutsUntilRightArrowIsShown() {
+    ASSERT_FALSE(scrollable_shelf_view_->right_arrow()->GetVisible());
+
+    while (!scrollable_shelf_view_->right_arrow()->GetVisible())
+      AddAppShortcut();
+  }
+
+  void CheckFirstAndLastTappableIconsBounds() {
+    views::ViewModel* view_model = shelf_view_->view_model();
+
+    gfx::Rect visible_space_in_screen = scrollable_shelf_view_->visible_space();
+    views::View::ConvertRectToScreen(scrollable_shelf_view_,
+                                     &visible_space_in_screen);
+
+    views::View* last_tappable_icon =
+        view_model->view_at(scrollable_shelf_view_->last_tappable_app_index());
+    const gfx::Rect last_tappable_icon_bounds =
+        last_tappable_icon->GetBoundsInScreen();
+
+    // Expects that the last tappable icon is fully shown.
+    EXPECT_TRUE(visible_space_in_screen.Contains(last_tappable_icon_bounds));
+
+    views::View* first_tappable_icon =
+        view_model->view_at(scrollable_shelf_view_->first_tappable_app_index());
+    const gfx::Rect first_tappable_icon_bounds =
+        first_tappable_icon->GetBoundsInScreen();
+
+    // Expects that the first tappable icon is fully shown.
+    EXPECT_TRUE(visible_space_in_screen.Contains(first_tappable_icon_bounds));
+  }
+
   ScrollableShelfView* scrollable_shelf_view_ = nullptr;
   ShelfView* shelf_view_ = nullptr;
   std::unique_ptr<ShelfViewTestAPI> test_api_;
@@ -142,7 +173,7 @@ TEST_F(ScrollableShelfViewTest, CorrectUIAfterDisplayRotationLongToShort) {
   UpdateDisplay("600x300");
 
   display::Display display = GetPrimaryDisplay();
-  AddAppShortcutUntilOverflow();
+  AddAppShortcutsUntilOverflow();
 
   // Presses the right arrow until reaching the last page of shelf icons.
   const views::View* right_arrow = scrollable_shelf_view_->right_arrow();
@@ -171,7 +202,7 @@ TEST_F(ScrollableShelfViewTest, CorrectUIAfterDisplayRotationLongToShort) {
 // When hovering mouse on a shelf icon, the tooltip only shows for the visible
 // icon (see https://crbug.com/997807).
 TEST_F(ScrollableShelfViewTest, NotShowTooltipForHiddenIcons) {
-  AddAppShortcutUntilOverflow();
+  AddAppShortcutsUntilOverflow();
 
   ASSERT_EQ(ScrollableShelfView::kShowRightArrowButton,
             scrollable_shelf_view_->layout_strategy_for_test());
@@ -208,7 +239,7 @@ TEST_F(ScrollableShelfViewTest, NotShowTooltipForHiddenIcons) {
 // Test that tapping near the scroll arrow button triggers scrolling. (see
 // https://crbug.com/1004998)
 TEST_F(ScrollableShelfViewTest, ScrollAfterTappingNearScrollArrow) {
-  AddAppShortcutUntilOverflow();
+  AddAppShortcutsUntilOverflow();
 
   ASSERT_EQ(ScrollableShelfView::kShowRightArrowButton,
             scrollable_shelf_view_->layout_strategy_for_test());
@@ -247,4 +278,33 @@ TEST_F(ScrollableShelfViewTest, ScrollAfterTappingNearScrollArrow) {
             scrollable_shelf_view_->layout_strategy_for_test());
 }
 
+// Verifies that in overflow mode, the app icons indexed by
+// |first_tappable_app_index_| and |last_tappable_app_index_| are completely
+// shown (https://crbug.com/1013811).
+TEST_F(ScrollableShelfViewTest, VerifyTappableAppIndices) {
+  AddAppShortcutsUntilOverflow();
+
+  // Checks bounds when the layout strategy is kShowRightArrowButton.
+  ASSERT_EQ(ScrollableShelfView::kShowRightArrowButton,
+            scrollable_shelf_view_->layout_strategy_for_test());
+  CheckFirstAndLastTappableIconsBounds();
+
+  GetEventGenerator()->GestureTapAt(
+      scrollable_shelf_view_->right_arrow()->GetBoundsInScreen().CenterPoint());
+  AddAppShortcutsUntilRightArrowIsShown();
+
+  // Checks bounds when the layout strategy is kShowButtons.
+  ASSERT_EQ(ScrollableShelfView::kShowButtons,
+            scrollable_shelf_view_->layout_strategy_for_test());
+  CheckFirstAndLastTappableIconsBounds();
+
+  GetEventGenerator()->GestureTapAt(
+      scrollable_shelf_view_->right_arrow()->GetBoundsInScreen().CenterPoint());
+
+  // Checks bounds when the layout strategy is kShowLeftArrowButton.
+  ASSERT_EQ(ScrollableShelfView::kShowLeftArrowButton,
+            scrollable_shelf_view_->layout_strategy_for_test());
+  CheckFirstAndLastTappableIconsBounds();
+}
+
 }  // namespace ash