Separate the edge padding into two parts

Edge padding of scrollable shelf logically consists of two parts:
the minimum gap between scrollable shelf and other components
(such as status area widget) and the gap decided by centering strategy
(display centering, view centering or overflow). This CL separates the
edge padding into those two parts. It improves the code readability.

In addition, currently the shelf view does not notify the parent view
of the added icon. This CL also fixes it.

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

ash/shelf/scrollable_shelf_view.h

@@ -186,11 +186,6 @@ class ASH_EXPORT ScrollableShelfView : public views::AccessiblePaneView,
                                          int space_for_icons,
                                          bool use_target_bounds) const;
 
-  // Returns whether the app icon layout should be centering alignment.
-  // |use_target_bounds| indicates which view bounds are used for
-  // calculation: actual view bounds or target view bounds.
-  bool ShouldApplyDisplayCentering(bool use_target_bounds) const;
-
   Shelf* GetShelf();
   const Shelf* GetShelf() const;
 
@@ -261,13 +256,19 @@ class ASH_EXPORT ScrollableShelfView : public views::AccessiblePaneView,
   bool ShouldShowLeftArrow() const;
   bool ShouldShowRightArrow() const;
 
-  // Returns the padding inset. Different Padding strategies for three scenarios
-  // (1) display centering alignment
-  // (2) scrollable shelf centering alignment
-  // (3) overflow mode
-  // |use_target_bounds| indicates which view bounds are used for
-  // calculation: actual view bounds or target view bounds.
-  gfx::Insets CalculateEdgePadding(bool use_target_bounds) const;
+  // Returns the padding insets which guarantee the minimum gap between
+  // scrollable shelf and other components (like status area widget).
+  gfx::Insets CalculateBaseEdgePadding() const;
+
+  // Returns the extra padding inset which is influenced by the padding
+  // strategy. There are three strategies: (1) display centering alignment (2)
+  // scrollable shelf centering alignment (3) overflow mode
+  // |use_target_bounds| indicates which view bounds are used for calculation:
+  // actual view bounds or target view bounds.
+  gfx::Insets CalculateExtraEdgePadding(bool use_target_bounds) const;
+
+  // Returns the sum of the base padding and the extra padding.
+  gfx::Insets GetTotalEdgePadding() const;
 
   int GetStatusWidgetSizeOnPrimaryAxis(bool use_target_bounds) const;
 
@@ -441,16 +442,23 @@ class ASH_EXPORT ScrollableShelfView : public views::AccessiblePaneView,
   // shelf under RTL.
   gfx::Rect available_space_;
 
-  // Paddings before and after shelf icons, including the app icon group margin.
-  gfx::Insets padding_insets_;
+  ShelfView* shelf_view_ = nullptr;
+
+  // Padding insets based on |base_padding_| and shelf alignment.
+  gfx::Insets base_padding_insets_;
+
+  // Extra insets decided by the current padding strategy.
+  gfx::Insets extra_padding_insets_;
+
+  // Minimum gap between scrollable shelf and other components (like status area
+  // widget) in DIPs.
+  const int base_padding_;
 
   // Visible space of |shelf_container_view| in ScrollableShelfView's local
   // coordinates. Different from |available_space_|, |visible_space_| only
   // contains app icons and is mirrored for horizontal shelf under RTL.
   gfx::Rect visible_space_;
 
-  ShelfView* shelf_view_ = nullptr;
-
   gfx::Vector2dF scroll_offset_;
 
   ScrollStatus scroll_status_ = kNotInScroll;

ash/shelf/scrollable_shelf_view_unittest.cc

@@ -778,6 +778,36 @@ TEST_F(ScrollableShelfViewTest, VerifyScrollEvent) {
             scrollable_shelf_view_->layout_strategy_for_test());
 }
 
+// Verifies that right-click on the last shelf icon should open the icon's
+// context menu instead of the shelf's (https://crbug.com/1041702).
+TEST_F(ScrollableShelfViewTest, ClickAtLastIcon) {
+  AddAppShortcutsUntilOverflow();
+  ASSERT_EQ(ScrollableShelfView::kShowRightArrowButton,
+            scrollable_shelf_view_->layout_strategy_for_test());
+
+  // Taps at the right arrow. Hotseat layout should show the left arrow.
+  gfx::Rect right_arrow =
+      scrollable_shelf_view_->right_arrow()->GetBoundsInScreen();
+  GetEventGenerator()->GestureTapAt(right_arrow.CenterPoint());
+  ASSERT_EQ(ScrollableShelfView::kShowLeftArrowButton,
+            scrollable_shelf_view_->layout_strategy_for_test());
+
+  // Right-click on the edge of the last icon.
+  const views::View* last_icon = shelf_view_->view_model()->view_at(
+      scrollable_shelf_view_->last_tappable_app_index());
+  gfx::Point click_point = last_icon->GetBoundsInScreen().right_center();
+  click_point.Offset(-1, 0);
+  GetEventGenerator()->MoveMouseTo(click_point);
+  GetEventGenerator()->ClickRightButton();
+
+  // Verifies that the context menu of |last_icon| should show.
+  EXPECT_TRUE(shelf_view_->IsShowingMenuForView(last_icon));
+
+  // Verfies that after left-click, the context menu should be closed.
+  GetEventGenerator()->ClickLeftButton();
+  EXPECT_FALSE(shelf_view_->IsShowingMenuForView(last_icon));
+}
+
 // Tests scrollable shelf's features under both LTR and RTL.
 class ScrollableShelfViewRTLTest : public ScrollableShelfViewTest,
                                    public testing::WithParamInterface<bool> {

ash/shelf/shelf_view.cc

@@ -195,11 +195,35 @@ class ShelfFocusSearch : public views::FocusSearch {
   DISALLOW_COPY_AND_ASSIGN(ShelfFocusSearch);
 };
 
+// Returns the id of the display on which |view| is shown.
+int64_t GetDisplayIdForView(const View* view) {
+  aura::Window* window = view->GetWidget()->GetNativeWindow();
+  return display::Screen::GetScreen()->GetDisplayNearestWindow(window).id();
+}
+
+// Whether |item_view| is a ShelfAppButton and its state is STATE_DRAGGING.
+bool ShelfButtonIsInDrag(const ShelfItemType item_type,
+                         const views::View* item_view) {
+  switch (item_type) {
+    case TYPE_PINNED_APP:
+    case TYPE_BROWSER_SHORTCUT:
+    case TYPE_APP:
+      return static_cast<const ShelfAppButton*>(item_view)->state() &
+             ShelfAppButton::STATE_DRAGGING;
+    case TYPE_DIALOG:
+    case TYPE_UNDEFINED:
+      return false;
+  }
+}
+
+}  // namespace
+
 // AnimationDelegate used when inserting a new item. This steadily increases the
 // opacity of the layer as the animation progress.
-class FadeInAnimationDelegate : public gfx::AnimationDelegate {
+class ShelfView::FadeInAnimationDelegate : public gfx::AnimationDelegate {
  public:
-  explicit FadeInAnimationDelegate(views::View* view) : view_(view) {}
+  explicit FadeInAnimationDelegate(ShelfView* host, views::View* view)
+      : shelf_view_(host), view_(view) {}
   ~FadeInAnimationDelegate() override = default;
 
   // AnimationDelegate overrides:
@@ -210,6 +234,7 @@ class FadeInAnimationDelegate : public gfx::AnimationDelegate {
   void AnimationEnded(const Animation* animation) override {
     view_->layer()->SetOpacity(1.0f);
     view_->layer()->ScheduleDraw();
+    shelf_view_->OnFadeInAnimationEnded();
   }
   void AnimationCanceled(const Animation* animation) override {
     view_->layer()->SetOpacity(1.0f);
@@ -217,34 +242,12 @@ class FadeInAnimationDelegate : public gfx::AnimationDelegate {
   }
 
  private:
+  ShelfView* shelf_view_ = nullptr;
   views::View* view_;
 
   DISALLOW_COPY_AND_ASSIGN(FadeInAnimationDelegate);
 };
 
-// Returns the id of the display on which |view| is shown.
-int64_t GetDisplayIdForView(const View* view) {
-  aura::Window* window = view->GetWidget()->GetNativeWindow();
-  return display::Screen::GetScreen()->GetDisplayNearestWindow(window).id();
-}
-
-// Whether |item_view| is a ShelfAppButton and its state is STATE_DRAGGING.
-bool ShelfButtonIsInDrag(const ShelfItemType item_type,
-                         const views::View* item_view) {
-  switch (item_type) {
-    case TYPE_PINNED_APP:
-    case TYPE_BROWSER_SHORTCUT:
-    case TYPE_APP:
-      return static_cast<const ShelfAppButton*>(item_view)->state() &
-             ShelfAppButton::STATE_DRAGGING;
-    case TYPE_DIALOG:
-    case TYPE_UNDEFINED:
-      return false;
-  }
-}
-
-}  // namespace
-
 // AnimationDelegate used when deleting an item. This steadily decreased the
 // opacity of the layer as the animation progress.
 class ShelfView::FadeOutAnimationDelegate : public gfx::AnimationDelegate {
@@ -1568,7 +1571,7 @@ void ShelfView::FadeIn(views::View* view) {
   AnimateToIdealBounds();
   bounds_animator_->SetAnimationDelegate(
       view, std::unique_ptr<gfx::AnimationDelegate>(
-                new FadeInAnimationDelegate(view)));
+                new FadeInAnimationDelegate(this, view)));
 }
 
 void ShelfView::PrepareForDrag(Pointer pointer, const ui::LocatedEvent& event) {
@@ -2067,6 +2070,13 @@ std::pair<int, int> ShelfView::GetDragRange(int index) {
   return std::pair<int, int>(min_index, max_index);
 }
 
+void ShelfView::OnFadeInAnimationEnded() {
+  // Call PreferredSizeChanged() to notify container to re-layout at the end
+  // of fade-in animation.
+  if (chromeos::switches::ShouldShowScrollableShelf())
+    PreferredSizeChanged();
+}
+
 void ShelfView::OnFadeOutAnimationEnded() {
   // Call PreferredSizeChanged() to notify container to re-layout at the end
   // of removal animation.

ash/shelf/shelf_view.h

@@ -362,6 +362,7 @@ class ASH_EXPORT ShelfView : public views::AccessiblePaneView,
  private:
   friend class ShelfViewTestAPI;
 
+  class FadeInAnimationDelegate;
   class FadeOutAnimationDelegate;
   class StartFadeAnimationDelegate;
 
@@ -496,6 +497,9 @@ class ASH_EXPORT ShelfView : public views::AccessiblePaneView,
   //  * In the overflow mode, returns only bubble's bounds.
   gfx::Rect GetBoundsForDragInsertInScreen();
 
+  // Invoked after the fading in animation for item addition is ended.
+  void OnFadeInAnimationEnded();
+
   // Invoked after the fading out animation for item deletion is ended.
   void OnFadeOutAnimationEnded();