snap-groups: Fix re-snap via window layout menu

This CL fixes the issue when a snap group with a partially occluding
window is re-snapped via the layout menu.

The only change is to activate the window *before* committing the snap
event when the layout menu button is pressed, to stack the opposite
snapped window on top and avoid starting partial overview.

Demo: https://b.corp.google.com/issues/348068768#comment9

Test: added, verified it fails w/o fix in patchset 1
Bug: b/348068768
Change-Id: I74190c74bca6164433cf5ede7135fc04c52418ff
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5646911
Commit-Queue: Sophie Wen <sophiewen@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1318150}

ash/frame/caption_buttons/frame_size_button_unittest.cc

@@ -12,9 +12,11 @@
 #include "ash/test/ash_test_base.h"
 #include "ash/test/ash_test_util.h"
 #include "ash/wm/overview/overview_utils.h"
+#include "ash/wm/snap_group/snap_group_controller.h"
 #include "ash/wm/splitview/split_view_constants.h"
 #include "ash/wm/window_positioning_utils.h"
 #include "ash/wm/window_state.h"
+#include "ash/wm/window_util.h"
 #include "ash/wm/wm_event.h"
 #include "ash/wm/workspace/phantom_window_controller.h"
 #include "base/check_op.h"
@@ -511,54 +513,6 @@ TEST_F(FrameSizeButtonTest, SizeButtonPressedWhenSnapButtonHovered) {
   EXPECT_EQ(views::Button::STATE_HOVERED, close_button()->GetState());
 }
 
-class SnapGroupFrameSizeButtonTest : public FrameSizeButtonTest {
- public:
-  SnapGroupFrameSizeButtonTest() : scoped_feature_list_(features::kSnapGroup) {}
-  SnapGroupFrameSizeButtonTest(const SnapGroupFrameSizeButtonTest&) = delete;
-  SnapGroupFrameSizeButtonTest& operator=(const SnapGroupFrameSizeButtonTest&) =
-      delete;
-  ~SnapGroupFrameSizeButtonTest() override = default;
-
- private:
-  base::test::ScopedFeatureList scoped_feature_list_;
-};
-
-// Tests that long press caption button to show snap phantom bounds are updated.
-TEST_F(SnapGroupFrameSizeButtonTest, SnapCaptionButton) {
-  EXPECT_EQ(views::Button::STATE_NORMAL, size_button()->GetState());
-
-  // Create an opposite snapped window with non-default snap ratio.
-  std::unique_ptr<aura::Window> w1(CreateAppWindow());
-  const WindowSnapWMEvent snap_primary(
-      WM_EVENT_SNAP_PRIMARY, chromeos::kTwoThirdSnapRatio,
-      WindowSnapActionSource::kSnapByWindowLayoutMenu);
-  WindowState::Get(w1.get())->OnWMEvent(&snap_primary);
-
-  // Press on the size button and drag toward the close button to show the snap
-  // phantom bounds.
-  wm::ActivateWindow(GetWidget()->GetNativeWindow());
-  ui::test::EventGenerator* generator = GetEventGenerator();
-  generator->MoveMouseTo(CenterPointInScreen(size_button()));
-  generator->PressLeftButton();
-  generator->MoveMouseTo(CenterPointInScreen(close_button()));
-  ASSERT_EQ(views::Button::STATE_PRESSED, size_button()->GetState());
-  ASSERT_TRUE(
-      static_cast<FrameSizeButton*>(size_button())->in_snap_mode_for_testing());
-  auto* snap_controller =
-      static_cast<SnapControllerImpl*>(chromeos::SnapController::Get());
-  ASSERT_TRUE(snap_controller);
-
-  // Test the phantom bounds reflect the opposite snapped `w1`.
-  const gfx::Rect work_area =
-      display::Screen::GetScreen()->GetPrimaryDisplay().work_area();
-  gfx::Rect expected_bounds(work_area);
-  expected_bounds.Subtract(w1->GetBoundsInScreen());
-  EXPECT_TRUE(expected_bounds.ApproximatelyEqual(
-      snap_controller->phantom_window_controller_for_testing()
-          ->GetTargetWindowBounds(),
-      /*tolerance=*/kSplitviewDividerShortSideLength / 2));
-}
-
 class FrameSizeButtonTestRTL : public FrameSizeButtonTest {
  public:
   FrameSizeButtonTestRTL() = default;
@@ -1323,4 +1277,99 @@ TEST_F(MultitaskMenuTest, AdjustedMenuBounds) {
       GetMultitaskMenu()->GetBoundsInScreen()));
 }
 
+class SnapGroupFrameSizeButtonTest : public MultitaskMenuTest {
+ public:
+  SnapGroupFrameSizeButtonTest() : scoped_feature_list_(features::kSnapGroup) {}
+  SnapGroupFrameSizeButtonTest(const SnapGroupFrameSizeButtonTest&) = delete;
+  SnapGroupFrameSizeButtonTest& operator=(const SnapGroupFrameSizeButtonTest&) =
+      delete;
+  ~SnapGroupFrameSizeButtonTest() override = default;
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+// Tests that long press caption button to show snap phantom bounds are updated.
+TEST_F(SnapGroupFrameSizeButtonTest, SnapCaptionButton) {
+  EXPECT_EQ(views::Button::STATE_NORMAL, size_button()->GetState());
+
+  // Create an opposite snapped window with non-default snap ratio.
+  std::unique_ptr<aura::Window> w1(CreateAppWindow());
+  const WindowSnapWMEvent snap_primary(
+      WM_EVENT_SNAP_PRIMARY, chromeos::kTwoThirdSnapRatio,
+      WindowSnapActionSource::kSnapByWindowLayoutMenu);
+  WindowState::Get(w1.get())->OnWMEvent(&snap_primary);
+
+  // Press on the size button and drag toward the close button to show the snap
+  // phantom bounds.
+  wm::ActivateWindow(GetWidget()->GetNativeWindow());
+  ui::test::EventGenerator* generator = GetEventGenerator();
+  generator->MoveMouseTo(CenterPointInScreen(size_button()));
+  generator->PressLeftButton();
+  generator->MoveMouseTo(CenterPointInScreen(close_button()));
+  ASSERT_EQ(views::Button::STATE_PRESSED, size_button()->GetState());
+  ASSERT_TRUE(
+      static_cast<FrameSizeButton*>(size_button())->in_snap_mode_for_testing());
+  auto* snap_controller =
+      static_cast<SnapControllerImpl*>(chromeos::SnapController::Get());
+  ASSERT_TRUE(snap_controller);
+
+  // Test the phantom bounds reflect the opposite snapped `w1`.
+  const gfx::Rect work_area =
+      display::Screen::GetScreen()->GetPrimaryDisplay().work_area();
+  gfx::Rect expected_bounds(work_area);
+  expected_bounds.Subtract(w1->GetBoundsInScreen());
+  EXPECT_TRUE(expected_bounds.ApproximatelyEqual(
+      snap_controller->phantom_window_controller_for_testing()
+          ->GetTargetWindowBounds(),
+      /*tolerance=*/kSplitviewDividerShortSideLength / 2));
+}
+
+// Tests that when a snap group with a partially occluding window is re-snapped
+// via the layout menu, we do not start partial overview. See http://b/348068768
+// for context.
+TEST_F(SnapGroupFrameSizeButtonTest, ReSnapViaWindowLayoutMenu) {
+  UpdateDisplay("800x600");
+
+  // Create a snap group with `window`, whose frame contains the multitask menu,
+  // and an `opposite` snapped window.
+  aura::Window* window = window_state()->window();
+  std::unique_ptr<aura::Window> opposite(CreateAppWindow());
+  const WindowSnapWMEvent snap_primary(
+      WM_EVENT_SNAP_PRIMARY, chromeos::kDefaultSnapRatio,
+      WindowSnapActionSource::kSnapByWindowLayoutMenu);
+  window_state()->OnWMEvent(&snap_primary);
+
+  const WindowSnapWMEvent snap_secondary(
+      WM_EVENT_SNAP_SECONDARY, chromeos::kDefaultSnapRatio,
+      WindowSnapActionSource::kSnapByWindowLayoutMenu);
+  WindowState::Get(opposite.get())->OnWMEvent(&snap_secondary);
+  auto* snap_group_controller = SnapGroupController::Get();
+  ASSERT_TRUE(
+      snap_group_controller->AreWindowsInSnapGroup(window, opposite.get()));
+
+  // Create a partially occluding window on top of `opposite`.
+  std::unique_ptr<aura::Window> occlude(
+      CreateAppWindow(gfx::Rect(410, 10, 200, 200)));
+  ASSERT_TRUE(
+      opposite->GetBoundsInScreen().Contains(occlude->GetBoundsInScreen()));
+
+  // Hover to show the multitask menu on `window`.
+  ShowMultitaskMenu();
+  MultitaskMenu* multitask_menu = GetMultitaskMenu();
+  views::Button* left_half_button =
+      MultitaskMenuViewTestApi(multitask_menu->multitask_menu_view())
+          .GetHalfButton()
+          ->GetLeftTopButton();
+
+  // Click on the snap button to re-snap `window`. Test we don't start overview
+  // and recall the windows to the front.
+  LeftClickOn(left_half_button);
+  ASSERT_FALSE(IsInOverviewSession());
+  EXPECT_TRUE(SnapGroupController::Get()->AreWindowsInSnapGroup(
+      window, opposite.get()));
+  EXPECT_TRUE(window_util::IsStackedBelow(occlude.get(), window));
+  EXPECT_TRUE(window_util::IsStackedBelow(occlude.get(), opposite.get()));
+}
+
 }  // namespace ash

chromeos/ui/frame/multitask_menu/multitask_menu_view.cc

@@ -419,10 +419,10 @@ void MultitaskMenuView::SetSkipMouseOutDelayForTesting(bool val) {
 }
 
 void MultitaskMenuView::HalfButtonPressed(SnapDirection direction) {
+  wm::GetActivationClient(window_->GetRootWindow())->ActivateWindow(window_);
   SnapController::Get()->CommitSnap(
       window_, direction, kDefaultSnapRatio,
       SnapController::SnapRequestSource::kWindowLayoutMenu);
-  wm::GetActivationClient(window_->GetRootWindow())->ActivateWindow(window_);
   close_callback_.Run();
   base::RecordAction(base::UserMetricsAction(
       direction == SnapDirection::kPrimary ? kHalfSplitPrimaryUserAction
@@ -431,6 +431,7 @@ void MultitaskMenuView::HalfButtonPressed(SnapDirection direction) {
 }
 
 void MultitaskMenuView::PartialButtonPressed(SnapDirection direction) {
+  wm::GetActivationClient(window_->GetRootWindow())->ActivateWindow(window_);
   SnapController::Get()->CommitSnap(
       window_, direction,
       direction == SnapDirection::kPrimary
@@ -439,9 +440,7 @@ void MultitaskMenuView::PartialButtonPressed(SnapDirection direction) {
           : (is_reversed_ ? chromeos::kTwoThirdSnapRatio
                           : chromeos::kOneThirdSnapRatio),
       SnapController::SnapRequestSource::kWindowLayoutMenu);
-  wm::GetActivationClient(window_->GetRootWindow())->ActivateWindow(window_);
   close_callback_.Run();
-
   base::RecordAction(base::UserMetricsAction(
       direction == SnapDirection::kPrimary ? kPartialSplitTwoThirdsUserAction
                                            : kPartialSplitOneThirdUserAction));
@@ -449,10 +448,10 @@ void MultitaskMenuView::PartialButtonPressed(SnapDirection direction) {
 }
 
 void MultitaskMenuView::FullScreenButtonPressed() {
+  wm::GetActivationClient(window_->GetRootWindow())->ActivateWindow(window_);
   auto* widget = views::Widget::GetWidgetForNativeWindow(window_);
   const bool is_fullscreen = widget->IsFullscreen();
   widget->SetFullscreen(!is_fullscreen);
-  wm::GetActivationClient(window_->GetRootWindow())->ActivateWindow(window_);
   close_callback_.Run();
   base::RecordAction(base::UserMetricsAction(
       is_fullscreen ? kExitFullscreenUserAction : kFullscreenUserAction));
@@ -460,6 +459,7 @@ void MultitaskMenuView::FullScreenButtonPressed() {
 }
 
 void MultitaskMenuView::FloatButtonPressed() {
+  wm::GetActivationClient(window_->GetRootWindow())->ActivateWindow(window_);
   if (window_->GetProperty(kWindowStateTypeKey) == WindowStateType::kFloated) {
     base::RecordAction(base::UserMetricsAction(kUnFloatUserAction));
     FloatControllerBase::Get()->UnsetFloat(window_);
@@ -470,7 +470,6 @@ void MultitaskMenuView::FloatButtonPressed() {
                               : FloatStartLocation::kBottomRight);
   }
 
-  wm::GetActivationClient(window_->GetRootWindow())->ActivateWindow(window_);
   close_callback_.Run();
   RecordMultitaskMenuActionType(MultitaskMenuActionType::kFloatButton);
 }