CrOS Shelf: Fix shelf centering on non-primary displays

The issue was that we were calculating centering coordinates according
to the whole screen rather than each individual display.

Add a test for this.

This change also fixes a bug in ash/screen_util where the shelf bounds
were not returned in terms of the whole screen's coordinates. A few
unit tests in ash/wm also needed fixing in that sense.

Bug: 997768
Change-Id: I91ab6ef2f85601edc115743dd0ab0ae83076be94
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1773891
Commit-Queue: Manu Cornet <manucornet@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#692973}

ash/display/window_tree_host_manager_unittest.cc

@@ -1418,6 +1418,7 @@ class RootWindowTestObserver : public aura::WindowObserver {
     shelf_display_bounds_ = screen_util::GetDisplayBoundsWithShelf(window);
   }
 
+  // Returns the shelf display bounds, in screen coordinates.
   const gfx::Rect& shelf_display_bounds() const {
     return shelf_display_bounds_;
   }
@@ -1460,7 +1461,8 @@ TEST_F(WindowTreeHostManagerTest, ReplacePrimary) {
 
   display_info_list.push_back(new_first_display_info);
   display_manager()->OnNativeDisplaysChanged(display_info_list);
-  EXPECT_EQ("0,0 500x500", test_observer.shelf_display_bounds().ToString());
+  // The shelf is now on the second display.
+  EXPECT_EQ("400,0 500x500", test_observer.shelf_display_bounds().ToString());
   primary_root->RemoveObserver(&test_observer);
 }
 

ash/screen_util.cc

@@ -79,8 +79,11 @@ gfx::Rect GetDisplayWorkAreaBoundsInScreenForActiveDeskContainer(
 }
 
 gfx::Rect GetDisplayBoundsWithShelf(aura::Window* window) {
-  if (!Shell::Get()->display_manager()->IsInUnifiedMode())
-    return window->GetRootWindow()->bounds();
+  if (!Shell::Get()->display_manager()->IsInUnifiedMode()) {
+    return display::Screen::GetScreen()
+        ->GetDisplayNearestWindow(window)
+        .bounds();
+  }
 
   // In Unified Mode, the display that should contain the shelf depends on the
   // current shelf alignment.

ash/shelf/shelf_constants.h

@@ -6,6 +6,7 @@
 #define ASH_SHELF_SHELF_CONSTANTS_H_
 
 #include "ash/ash_export.h"
+#include "base/macros.h"
 #include "chromeos/constants/chromeos_switches.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/gfx/color_palette.h"

ash/shelf/shelf_layout_manager.cc

@@ -972,8 +972,6 @@ void ShelfLayoutManager::UpdateBoundsAndOpacity(
       status_animation_setter.AddObserver(observer);
 
     gfx::Rect shelf_bounds = target_bounds.shelf_bounds;
-    ::wm::ConvertRectToScreen(shelf_widget_->GetNativeWindow()->parent(),
-                              &shelf_bounds);
     shelf_widget_->SetBounds(shelf_bounds);
 
     GetLayer(nav_widget)->SetOpacity(target_bounds.opacity);
@@ -993,23 +991,17 @@ void ShelfLayoutManager::UpdateBoundsAndOpacity(
 
     gfx::Rect status_bounds = target_bounds.status_bounds_in_shelf;
     status_bounds.Offset(target_bounds.shelf_bounds.OffsetFromOrigin());
-    ::wm::ConvertRectToScreen(status_widget->GetNativeWindow()->parent(),
-                              &status_bounds);
     status_widget->SetBounds(status_bounds);
 
     gfx::Vector2d nav_offset = target_bounds.shelf_bounds.OffsetFromOrigin();
     gfx::Rect nav_bounds = target_bounds.nav_bounds_in_shelf;
     nav_bounds.Offset(nav_offset);
-    ::wm::ConvertRectToScreen(nav_widget->GetNativeWindow()->parent(),
-                              &nav_bounds);
     nav_widget->SetBounds(nav_bounds);
 
     gfx::Vector2d hotseat_offset =
         target_bounds.shelf_bounds.OffsetFromOrigin();
     gfx::Rect hotseat_bounds = target_bounds.hotseat_bounds_in_shelf;
     hotseat_bounds.Offset(hotseat_offset);
-    ::wm::ConvertRectToScreen(hotseat_widget->GetNativeWindow()->parent(),
-                              &hotseat_bounds);
     hotseat_widget->SetBounds(hotseat_bounds);
 
     // Do not update the work area when the alignment changes to BOTTOM_LOCKED

ash/shelf/shelf_layout_manager_unittest.cc

@@ -3266,7 +3266,7 @@ TEST_F(ShelfLayoutManagerTest, NoShelfUpdateDuringOverviewAnimation) {
   EXPECT_EQ(0, observer.metrics_change_count());
 }
 
-// Tests that shelf bounds is updated properly after overview animation.
+// Tests that shelf bounds are updated properly after overview animation.
 TEST_F(ShelfLayoutManagerTest, ShelfBoundsUpdateAfterOverviewAnimation) {
   // Run overview animations.
   ui::ScopedAnimationDurationScaleMode regular_animations(
@@ -3306,4 +3306,50 @@ TEST_F(ShelfLayoutManagerTest, ShelfBoundsUpdateAfterOverviewAnimation) {
   EXPECT_EQ(bottom_shelf_bounds, GetShelfWidget()->GetWindowBoundsInScreen());
 }
 
+// Tests that the shelf on a second display is properly centered.
+TEST_F(ShelfLayoutManagerTest, ShelfRemainsCenteredOnSecondDisplay) {
+  // Create two displays.
+  UpdateDisplay("600x400,1000x700");
+  aura::Window::Windows root_windows = Shell::GetAllRootWindows();
+  EXPECT_EQ(2U, root_windows.size());
+
+  Shelf* shelf_1 = Shelf::ForWindow(root_windows[0]);
+  Shelf* shelf_2 = Shelf::ForWindow(root_windows[1]);
+  EXPECT_NE(shelf_1, shelf_2);
+  EXPECT_NE(shelf_1->GetWindow()->GetRootWindow(),
+            shelf_2->GetWindow()->GetRootWindow());
+
+  ShelfView* shelf_view_1 = shelf_1->GetShelfViewForTesting();
+  ShelfView* shelf_view_2 = shelf_2->GetShelfViewForTesting();
+
+  const display::Display display_1 =
+      display::Screen::GetScreen()->GetPrimaryDisplay();
+  const display::Display display_2 =
+      display::Screen::GetScreen()->GetDisplayNearestWindow(root_windows[1]);
+  EXPECT_NE(display_1, display_2);
+
+  // Add an app shortcut.
+  ShelfController* controller = Shell::Get()->shelf_controller();
+  const std::string app_id("app_id");
+  ShelfItem item;
+  item.type = TYPE_PINNED_APP;
+  item.id = ShelfID(app_id);
+  controller->model()->Add(item);
+  gfx::Point app_center_1 = shelf_1->GetShelfViewForTesting()
+                                ->first_visible_button_for_testing()
+                                ->bounds()
+                                .CenterPoint();
+  views::View::ConvertPointToScreen(shelf_view_1, &app_center_1);
+
+  gfx::Point app_center_2 = shelf_2->GetShelfViewForTesting()
+                                ->first_visible_button_for_testing()
+                                ->bounds()
+                                .CenterPoint();
+  views::View::ConvertPointToScreen(shelf_view_2, &app_center_2);
+
+  // The app icon should be at the horizontal center of each display.
+  EXPECT_EQ(display_1.bounds().CenterPoint().x(), app_center_1.x());
+  EXPECT_EQ(display_2.bounds().CenterPoint().x(), app_center_2.x());
+}
+
 }  // namespace ash

ash/shelf/shelf_view.cc

@@ -946,11 +946,10 @@ void ShelfView::CalculateIdealBounds() {
   // ScrollableShelfView.
   if (!is_overflow_mode() && !chromeos::switches::ShouldShowScrollableShelf()) {
     // Now add the necessary padding to center app icons.
-    const gfx::Size screen_size =
-        screen_util::GetDisplayBoundsWithShelf(GetWidget()->GetNativeWindow())
-            .size();
-    const int screen_size_primary =
-        shelf()->PrimaryAxisValue(screen_size.width(), screen_size.height());
+    const gfx::Rect display_bounds =
+        screen_util::GetDisplayBoundsWithShelf(GetWidget()->GetNativeWindow());
+    const int display_size_primary = shelf()->PrimaryAxisValue(
+        display_bounds.size().width(), display_bounds.size().height());
 
     const int available_size_for_app_icons = GetAvailableSpaceForAppIcons();
     const int icons_size = GetSizeOfAppIcons(number_of_visible_apps(),
@@ -959,14 +958,15 @@ void ShelfView::CalculateIdealBounds() {
 
     if (app_centering_strategy.center_on_screen) {
       // This is how far the first icon needs to be from the screen edge.
-      padding_for_centering = (screen_size_primary - icons_size) / 2;
+      padding_for_centering = (display_size_primary - icons_size) / 2;
 
-      // Let's see how far this view is from the edge of the screen to
+      // Let's see how far this view is from the edge of this display to
       // compute how much extra padding is needed.
       gfx::Point origin = gfx::Point(0, 0);
       views::View::ConvertPointToScreen(this, &origin);
-
-      padding_for_centering -= origin.x();
+      padding_for_centering -= shelf_->IsHorizontalAlignment()
+                                   ? (origin.x() - display_bounds.x())
+                                   : (origin.y() - display_bounds.y());
     } else {
       padding_for_centering =
           (available_size_for_app_icons - icons_size) / 2;

ash/wm/lock_action_handler_layout_manager_unittest.cc

@@ -459,7 +459,7 @@ TEST_F(LockActionHandlerLayoutManagerTest, MultipleMonitors) {
   window_state->SetRestoreBoundsInScreen(gfx::Rect(400, 0, 30, 40));
   window_state->Maximize();
 
-  // Maximize the window with as the restore bounds is inside 2nd display but
+  // Maximize the window width as the restore bounds is inside 2nd display but
   // lock container windows are always on primary display.
   EXPECT_EQ(root_windows[0], window->GetRootWindow());
   target_bounds = gfx::Rect(300, 400);

ash/wm/lock_layout_manager_unittest.cc

@@ -373,7 +373,7 @@ TEST_F(LockLayoutManagerTest, AccessibilityPanelWithMultipleMonitors) {
   // for the primary shelf, so it should not influence the screen bounds.
   window->SetBoundsInScreen(gfx::Rect(0, 0, 30, 40), GetSecondaryDisplay());
 
-  target_bounds = gfx::Rect(300, 0, 400, 500);
+  target_bounds = gfx::Rect(600, 0, 400, 500);
   EXPECT_EQ(root_windows[1], window->GetRootWindow());
   EXPECT_EQ(target_bounds, window->GetBoundsInScreen());
 }

ash/wm/pip/pip_window_resizer_unittest.cc

@@ -526,7 +526,7 @@ TEST_P(PipWindowResizerTest, PipWindowFlungAvoidsFloatingKeyboard) {
 
 TEST_P(PipWindowResizerTest, PipWindowDoesNotChangeDisplayOnDrag) {
   PreparePipWindow(gfx::Rect(200, 200, 100, 100));
-  display::Display display = WindowState::Get(window())->GetDisplay();
+  const display::Display display = WindowState::Get(window())->GetDisplay();
   gfx::Rect rect_in_screen = window()->bounds();
   ::wm::ConvertRectToScreen(window()->parent(), &rect_in_screen);
   EXPECT_TRUE(display.bounds().Contains(rect_in_screen));

ash/wm/work_area_insets.cc

@@ -26,11 +26,10 @@ gfx::Insets CalculateWorkAreaInsets(const gfx::Insets accessibility_insets,
   // The virtual keyboard always hides the shelf (in any orientation).
   // Therefore, if the keyboard is shown, there is no need to reduce the work
   // area by the size of the shelf.
-  if (!keyboard_bounds.IsEmpty()) {
-    work_area_insets += gfx::Insets(0, 0, keyboard_bounds.height(), 0);
-  } else {
+  if (keyboard_bounds.IsEmpty())
     work_area_insets += shelf_insets;
-  }
+  else
+    work_area_insets += gfx::Insets(0, 0, keyboard_bounds.height(), 0);
   return work_area_insets;
 }
 
@@ -40,10 +39,13 @@ gfx::Rect CalculateWorkAreaBounds(const gfx::Insets accessibility_insets,
                                   const gfx::Rect shelf_bounds,
                                   const gfx::Rect keyboard_bounds_in_screen,
                                   aura::Window* window) {
+  // The shelf bounds are not in screen coordinates.
+  gfx::Rect shelf_bounds_in_screen(shelf_bounds);
+  ::wm::ConvertRectToScreen(window, &shelf_bounds_in_screen);
+
   gfx::Rect work_area_bounds = screen_util::GetDisplayBoundsWithShelf(window);
   work_area_bounds.Inset(accessibility_insets);
-  work_area_bounds.Subtract(shelf_bounds);
-  ::wm::ConvertRectToScreen(window, &work_area_bounds);
+  work_area_bounds.Subtract(shelf_bounds_in_screen);
   work_area_bounds.Subtract(keyboard_bounds_in_screen);
   return work_area_bounds;
 }