Fix feedback for pinning app under RTL

Basically two elements are required by View::ScrollRectToVisible:
the rectangle to show and the bounds of the visible space. However,
in ScrollableShelfView::ScrollRectToVisible, the rectangle to show
is not mirrored under RTL while the visible space is mirrored.

This CL mirrors the visible space from RTL to LTR when RTL is enabled.
A test case is also created for this scenario.

Bug: 1052553
Change-Id: I3a051c40a87f93f612edf558944479aad61d6ca2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2056938Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Andrew Xu <andrewxu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#742221}

ash/shelf/scrollable_shelf_view.cc

@@ -990,9 +990,14 @@ void ScrollableShelfView::ScrollRectToVisible(const gfx::Rect& rect) {
   else
     rect_after_adjustment.Offset(0, -scroll_offset_.y());
 
+  // Notes that |rect| is not mirrored under RTL while |visible_space_| has been
+  // mirrored. It is easier for coding if we mirror |visible_space_| back and
+  // then do the calculation.
+  const gfx::Rect visible_space_without_RTL = GetMirroredRect(visible_space_);
+
   // |rect_after_adjustment| is already shown completely. So scroll is not
   // needed.
-  if (visible_space_.Contains(rect_after_adjustment)) {
+  if (visible_space_without_RTL.Contains(rect_after_adjustment)) {
     AdjustOffset();
     return;
   }
@@ -1002,8 +1007,8 @@ void ScrollableShelfView::ScrollRectToVisible(const gfx::Rect& rect) {
   // |forward| indicates the scroll direction.
   const bool forward =
       is_horizontal_alignment
-          ? rect_after_adjustment.right() > visible_space_.right()
-          : rect_after_adjustment.bottom() > visible_space_.bottom();
+          ? rect_after_adjustment.right() > visible_space_without_RTL.right()
+          : rect_after_adjustment.bottom() > visible_space_without_RTL.bottom();
 
   // Scrolling |shelf_view_| has the following side-effects:
   // (1) May change the layout strategy.
@@ -1013,7 +1018,7 @@ void ScrollableShelfView::ScrollRectToVisible(const gfx::Rect& rect) {
   // scroll.
   LayoutStrategy layout_strategy_after_scroll = layout_strategy_;
   float main_axis_offset_after_scroll = original_offset;
-  gfx::Rect visible_space_after_scroll = visible_space_;
+  gfx::Rect visible_space_after_scroll = visible_space_without_RTL;
   gfx::Rect rect_after_scroll = rect_after_adjustment;
 
   // In each iteration, it scrolls |shelf_view_| to the neighboring page.
@@ -1038,8 +1043,7 @@ void ScrollableShelfView::ScrollRectToVisible(const gfx::Rect& rect) {
     main_axis_offset_after_scroll = CalculateScrollDistanceAfterAdjustment(
         main_axis_offset_after_scroll, layout_strategy_after_scroll);
     visible_space_after_scroll =
-        CalculateVisibleSpace(layout_strategy_after_scroll);
-
+        GetMirroredRect(CalculateVisibleSpace(layout_strategy_after_scroll));
     rect_after_scroll = rect_after_adjustment;
     const int offset_delta = main_axis_offset_after_scroll - original_offset;
     if (is_horizontal_alignment)

ash/shelf/scrollable_shelf_view_unittest.cc

@@ -17,6 +17,7 @@
 #include "ash/shell.h"
 #include "ash/test/ash_test_base.h"
 #include "ash/wm/tablet_mode/tablet_mode_controller.h"
+#include "base/test/icu_test_util.h"
 #include "base/test/scoped_feature_list.h"
 #include "chromeos/constants/chromeos_features.h"
 #include "ui/display/manager/display_manager.h"
@@ -680,35 +681,6 @@ TEST_F(ScrollableShelfViewTest, ScrollWithMouseWheel) {
             scrollable_shelf_view_->layout_strategy_for_test());
 }
 
-// Verifies that the shelf is scrolled to show the pinned app after pinning.
-TEST_F(ScrollableShelfViewTest, FeedbackForAppPinning) {
-  AddAppShortcutsUntilOverflow();
-  ASSERT_EQ(ScrollableShelfView::kShowRightArrowButton,
-            scrollable_shelf_view_->layout_strategy_for_test());
-
-  // Pins the icons of running apps to the shelf.
-  const int num = shelf_view_->view_model()->view_size();
-  for (int i = 0; i < 2 * num; i++)
-    AddAppShortcut(ShelfItemType::TYPE_APP);
-
-  // Emulates the process that user pins another app icon. Icons of pinned apps
-  // are placed before those of running apps. So the icon should be located on
-  // the second page.
-  ShelfModel::ScopedUserTriggeredMutation user_triggered(
-      scrollable_shelf_view_->shelf_view()->model());
-  ShelfID shelf_id = AddAppShortcut();
-  const int view_index =
-      shelf_view_->model()->ItemIndexByAppID(shelf_id.app_id);
-  ASSERT_EQ(view_index, num);
-
-  // Scrolls the shelf to show the pinned app. Expects that the shelf is
-  // scrolled to the correct page.
-  EXPECT_LT(view_index, scrollable_shelf_view_->last_tappable_app_index());
-  EXPECT_GT(view_index, scrollable_shelf_view_->first_tappable_app_index());
-  EXPECT_EQ(ScrollableShelfView::kShowButtons,
-            scrollable_shelf_view_->layout_strategy_for_test());
-}
-
 // Verifies that removing a shelf icon by mouse works as expected on scrollable
 // shelf (see https://crbug.com/1033967).
 TEST_F(ScrollableShelfViewTest, RipOffShelfItem) {
@@ -806,4 +778,66 @@ TEST_F(ScrollableShelfViewTest, VerifyScrollEvent) {
             scrollable_shelf_view_->layout_strategy_for_test());
 }
 
+// Tests scrollable shelf's features under both LTR and RTL.
+class ScrollableShelfViewRTLTest : public ScrollableShelfViewTest,
+                                   public testing::WithParamInterface<bool> {
+ public:
+  ScrollableShelfViewRTLTest() : scoped_locale_(GetParam() ? "ar" : "") {}
+  ~ScrollableShelfViewRTLTest() override = default;
+
+ private:
+  base::test::ScopedRestoreICUDefaultLocale scoped_locale_;
+};
+
+INSTANTIATE_TEST_SUITE_P(LtrRTL, ScrollableShelfViewRTLTest, testing::Bool());
+
+// Verifies that the shelf is scrolled to show the pinned app after pinning.
+TEST_P(ScrollableShelfViewRTLTest, FeedbackForAppPinning) {
+  AddAppShortcutsUntilOverflow();
+  ASSERT_EQ(ScrollableShelfView::kShowRightArrowButton,
+            scrollable_shelf_view_->layout_strategy_for_test());
+
+  // |num| is the minimum of app icons to enter overflow mode.
+  const int num = shelf_view_->view_model()->view_size();
+
+  ShelfModel::ScopedUserTriggeredMutation user_triggered(
+      scrollable_shelf_view_->shelf_view()->model());
+
+  {
+    ShelfID shelf_id = AddAppShortcut();
+    const int view_index =
+        shelf_view_->model()->ItemIndexByAppID(shelf_id.app_id);
+    ASSERT_EQ(view_index, num);
+
+    // When shelf only contains pinned apps, expects that the shelf is scrolled
+    // to the last page to show the latest pinned app.
+    EXPECT_EQ(ScrollableShelfView::kShowLeftArrowButton,
+              scrollable_shelf_view_->layout_strategy_for_test());
+  }
+
+  GetEventGenerator()->GestureTapAt(
+      scrollable_shelf_view_->left_arrow()->GetBoundsInScreen().CenterPoint());
+  ASSERT_EQ(ScrollableShelfView::kShowRightArrowButton,
+            scrollable_shelf_view_->layout_strategy_for_test());
+
+  // Pins the icons of running apps to the shelf.
+  for (int i = 0; i < 2 * num; i++)
+    AddAppShortcut(ShelfItemType::TYPE_APP);
+
+  {
+    ShelfID shelf_id = AddAppShortcut();
+    const int view_index =
+        shelf_view_->model()->ItemIndexByAppID(shelf_id.app_id);
+    ASSERT_EQ(view_index, num + 1);
+
+    // Scrolls the shelf to show the pinned app. Expects that the shelf is
+    // scrolled to the correct page. Notes that the pinned app should be placed
+    // ahead of running apps.
+    EXPECT_LT(view_index, scrollable_shelf_view_->last_tappable_app_index());
+    EXPECT_GT(view_index, scrollable_shelf_view_->first_tappable_app_index());
+    EXPECT_EQ(ScrollableShelfView::kShowButtons,
+              scrollable_shelf_view_->layout_strategy_for_test());
+  }
+}
+
 }  // namespace ash