From 923d0eca9ed86960af9e735f8a4b2271fa17fb37 Mon Sep 17 00:00:00 2001
From: Tommy Steimel <[email protected]>
Date: Tue, 10 Jun 2025 11:52:42 -0700
Subject: [PATCH] [pip] Tuck picture-in-picture windows when a file dialog is open

Picture-in-picture windows can occlude file dialogs, and initially we
tried to prevent this by closing all pip windows whenever a file
dialog was open. This created its own set of problems, so that feature
was disabled.

This is a replacement of that feature: instead of closing pip windows,
this moves them off to the side of the screen, preventing them from
obscuring the file dialog without having to actually close them.

go/picture-in-picture-tucking-design-doc

Bug: 403792431, 384050903, 415979072
Change-Id: I280333c32a02aee3c345c839e88848de6820ebe2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6449682
Reviewed-by: Evan Liu <[email protected]>
Commit-Queue: Tommy Steimel <[email protected]>
Reviewed-by: Frank Liberato <[email protected]>
Reviewed-by: Fr <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1471975}
---

diff --git a/chrome/browser/file_select_helper.cc b/chrome/browser/file_select_helper.cc
index f03a163..442f428 100644
--- a/chrome/browser/file_select_helper.cc
+++ b/chrome/browser/file_select_helper.cc
@@ -60,6 +60,7 @@
 #else
 #include "chrome/browser/picture_in_picture/picture_in_picture_window_manager.h"
 #include "chrome/browser/picture_in_picture/scoped_disallow_picture_in_picture.h"
+#include "chrome/browser/picture_in_picture/scoped_tuck_picture_in_picture.h"
 #endif  // BUILDFLAG(IS_ANDROID)
 
 using blink::mojom::FileChooserFileInfo;
@@ -614,6 +615,10 @@
           ->ShouldFileDialogBlockPictureInPicture(web_contents_)) {
     scoped_disallow_picture_in_picture_ =
         std::make_unique<ScopedDisallowPictureInPicture>();
+  } else if (PictureInPictureWindowManager::GetInstance()
+                 ->ShouldFileDialogTuckPictureInPicture(web_contents_)) {
+    scoped_tuck_picture_in_picture_ =
+        std::make_unique<ScopedTuckPictureInPicture>();
   }
 #endif  // !BUILDFLAG(IS_ANDROID)
 
@@ -791,6 +796,7 @@
 
 #if !BUILDFLAG(IS_ANDROID)
   scoped_disallow_picture_in_picture_.reset();
+  scoped_tuck_picture_in_picture_.reset();
 #endif  // !BUILDFLAG(IS_ANDROID)
 
   Release();
diff --git a/chrome/browser/file_select_helper.h b/chrome/browser/file_select_helper.h
index e5b8c7d6..2025d39 100644
--- a/chrome/browser/file_select_helper.h
+++ b/chrome/browser/file_select_helper.h
@@ -28,6 +28,7 @@
 
 class Profile;
 class ScopedDisallowPictureInPicture;
+class ScopedTuckPictureInPicture;
 
 namespace content {
 class FileSelectListener;
@@ -334,6 +335,9 @@
   // When not null, this prevents picture-in-picture windows from opening.
   std::unique_ptr<ScopedDisallowPictureInPicture>
       scoped_disallow_picture_in_picture_;
+
+  // When not null, this tucks picture-in-picture windows out of the way.
+  std::unique_ptr<ScopedTuckPictureInPicture> scoped_tuck_picture_in_picture_;
 #endif  // !BUILDFLAG(IS_ANDROID)
 
 #if BUILDFLAG(IS_CHROMEOS)
diff --git a/chrome/browser/picture_in_picture/BUILD.gn b/chrome/browser/picture_in_picture/BUILD.gn
index 3d018a9..3e2eb8a 100644
--- a/chrome/browser/picture_in_picture/BUILD.gn
+++ b/chrome/browser/picture_in_picture/BUILD.gn
@@ -36,9 +36,11 @@
       "picture_in_picture_occlusion_observer.h",
       "picture_in_picture_occlusion_tracker.h",
       "picture_in_picture_occlusion_tracker_observer.h",
+      "picture_in_picture_window.h",
       "picture_in_picture_window_manager_uma_helper.h",
       "scoped_disallow_picture_in_picture.h",
       "scoped_picture_in_picture_occlusion_observation.h",
+      "scoped_tuck_picture_in_picture.h",
     ]
     public_deps += [
       "//chrome/browser/ui/tabs:tab_strip_model_observer",
@@ -102,6 +104,7 @@
       "picture_in_picture_window_manager_uma_helper.cc",
       "scoped_disallow_picture_in_picture.cc",
       "scoped_picture_in_picture_occlusion_observation.cc",
+      "scoped_tuck_picture_in_picture.cc",
     ]
     deps += [
       "//chrome/app:generated_resources",
diff --git a/chrome/browser/picture_in_picture/picture_in_picture_window.h b/chrome/browser/picture_in_picture/picture_in_picture_window.h
new file mode 100644
index 0000000..f017e54
--- /dev/null
+++ b/chrome/browser/picture_in_picture/picture_in_picture_window.h
@@ -0,0 +1,26 @@
+// Copyright 2025 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_PICTURE_IN_PICTURE_PICTURE_IN_PICTURE_WINDOW_H_
+#define CHROME_BROWSER_PICTURE_IN_PICTURE_PICTURE_IN_PICTURE_WINDOW_H_
+
+// A PictureInPictureWindow is an always-on-top window that displays content to
+// the user. There are two types of PictureInPictureWindows: video
+// picture-in-picture windows (`VideoOverlayWindowViews`) and document
+// picture-in-picture (`PictureInPictureBrowserFrameView`). This class has
+// shared logic for both to be controlled directly by the
+// PictureInPictureWindowManager.
+class PictureInPictureWindow {
+ public:
+  // When `tuck` is true, this forces the PictureInPictureWindow to be tucked
+  // offscreen. When `tuck` is false, it returns the PictureInPictureWindow to
+  // its original position.
+  virtual void SetForcedTucking(bool tuck) = 0;
+
+ protected:
+  PictureInPictureWindow() = default;
+  virtual ~PictureInPictureWindow() = default;
+};
+
+#endif  // CHROME_BROWSER_PICTURE_IN_PICTURE_PICTURE_IN_PICTURE_WINDOW_H_
diff --git a/chrome/browser/picture_in_picture/picture_in_picture_window_manager.cc b/chrome/browser/picture_in_picture/picture_in_picture_window_manager.cc
index 395f622..90346363 100644
--- a/chrome/browser/picture_in_picture/picture_in_picture_window_manager.cc
+++ b/chrome/browser/picture_in_picture/picture_in_picture_window_manager.cc
@@ -28,6 +28,7 @@
 // TODO(crbug.com/421608904): include auto_picture_in_picture_tab_helper for
 // Android.
 #include "chrome/browser/picture_in_picture/auto_picture_in_picture_tab_helper.h"
+#include "chrome/browser/picture_in_picture/picture_in_picture_window.h"
 #include "media/base/media_switches.h"
 #include "net/base/url_util.h"
 #include "third_party/blink/public/common/features.h"
@@ -733,6 +734,61 @@
   number_of_existing_scoped_disallow_picture_in_pictures_--;
 }
 
+void PictureInPictureWindowManager::OnPictureInPictureWindowShown(
+    PictureInPictureWindow* window) {
+  picture_in_picture_window_ = window;
+  if (IsPictureInPictureForceTucked()) {
+    picture_in_picture_window_->SetForcedTucking(true);
+    RecordPictureInPictureTucked(PictureInPictureTuckedType::kNewWindowTucked);
+  }
+}
+
+void PictureInPictureWindowManager::OnPictureInPictureWindowHidden(
+    PictureInPictureWindow* window) {
+  if (picture_in_picture_window_ == window) {
+    picture_in_picture_window_ = nullptr;
+  }
+}
+
+bool PictureInPictureWindowManager::ShouldFileDialogTuckPictureInPicture(
+    content::WebContents* owner_web_contents) {
+  if (!base::FeatureList::IsEnabled(media::kFileDialogsTuckPictureInPicture)) {
+    return false;
+  }
+
+  // File dialogs opened inside document picture-in-picture windows should not
+  // tuck picture-in-picture.
+  if (pip_window_controller_ &&
+      pip_window_controller_->GetChildWebContents() == owner_web_contents) {
+    return false;
+  }
+
+  return true;
+}
+
+void PictureInPictureWindowManager::OnScopedTuckPictureInPictureCreated(
+    base::PassKey<ScopedTuckPictureInPicture>) {
+  number_of_existing_scoped_tuck_picture_in_pictures_++;
+  if (picture_in_picture_window_) {
+    picture_in_picture_window_->SetForcedTucking(true);
+    RecordPictureInPictureTucked(
+        PictureInPictureTuckedType::kExistingWindowTucked);
+  }
+}
+
+void PictureInPictureWindowManager::OnScopedTuckPictureInPictureDestroyed(
+    base::PassKey<ScopedTuckPictureInPicture>) {
+  CHECK_NE(number_of_existing_scoped_tuck_picture_in_pictures_, 0u);
+  number_of_existing_scoped_tuck_picture_in_pictures_--;
+  if (picture_in_picture_window_ && !IsPictureInPictureForceTucked()) {
+    picture_in_picture_window_->SetForcedTucking(false);
+  }

diff --git a/chrome/browser/picture_in_picture/picture_in_picture_window_manager_unittest.cc b/chrome/browser/picture_in_picture/picture_in_picture_window_manager_unittest.cc
index ef8e05f..aaa616e8 100644
--- a/chrome/browser/picture_in_picture/picture_in_picture_window_manager_unittest.cc
+++ b/chrome/browser/picture_in_picture/picture_in_picture_window_manager_unittest.cc
@@ -20,8 +20,10 @@
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/simple_test_tick_clock.h"
 #include "chrome/browser/picture_in_picture/auto_picture_in_picture_tab_helper.h"
+#include "chrome/browser/picture_in_picture/picture_in_picture_window.h"
 #include "chrome/browser/picture_in_picture/picture_in_picture_window_manager_uma_helper.h"
 #include "chrome/browser/picture_in_picture/scoped_disallow_picture_in_picture.h"
+#include "chrome/browser/picture_in_picture/scoped_tuck_picture_in_picture.h"
 #include "media/base/media_switches.h"
 #include "ui/views/bubble/bubble_dialog_delegate_view.h"
 #include "ui/views/view.h"
@@ -65,6 +67,25 @@
   MOCK_METHOD(std::optional<url::Origin>, GetOrigin, (), (override));
 };
 
+#if !BUILDFLAG(IS_ANDROID)
+class MockPictureInPictureWindow : public PictureInPictureWindow {
+ public:
+  MockPictureInPictureWindow() = default;
+  MockPictureInPictureWindow(const MockPictureInPictureWindow&) = delete;
+  MockPictureInPictureWindow& operator=(const MockPictureInPictureWindow&) =
+      delete;
+  ~MockPictureInPictureWindow() override = default;
+
+  bool is_tucking() const { return is_tucking_; }
+
+  // PictureInPictureWindow:
+  void SetForcedTucking(bool tuck) override { is_tucking_ = tuck; }
+
+ private:
+  bool is_tucking_ = false;
+};
+#endif  // !BUILDFLAG(IS_ANDROID)
+
 class PictureInPictureWindowManagerTest
     : public ChromeRenderViewHostTestHarness {
  public:
@@ -475,6 +496,103 @@
   }
 }
 
+TEST_F(PictureInPictureWindowManagerTest, CanForceTuckPictureInPicture) {
+  {
+    // Force-tucking before opening a picture-in-picture window should tuck it.
+    auto tuck = std::make_unique<ScopedTuckPictureInPicture>();
+    MockPictureInPictureWindow pip_window;
+
+    PictureInPictureWindowManager::GetInstance()->OnPictureInPictureWindowShown(
+        &pip_window);
+    EXPECT_TRUE(pip_window.is_tucking());
+
+    tuck.reset();
+    EXPECT_FALSE(pip_window.is_tucking());
+
+    PictureInPictureWindowManager::GetInstance()
+        ->OnPictureInPictureWindowHidden(&pip_window);
+  }
+
+  {
+    // Force-tucking after opening a picture-in-picture window should tuck it.
+    MockPictureInPictureWindow pip_window;
+    PictureInPictureWindowManager::GetInstance()->OnPictureInPictureWindowShown(
+        &pip_window);
+
+    EXPECT_FALSE(pip_window.is_tucking());
+    auto tuck = std::make_unique<ScopedTuckPictureInPicture>();
+    EXPECT_TRUE(pip_window.is_tucking());
+
+    tuck.reset();
+    EXPECT_FALSE(pip_window.is_tucking());
+
+    PictureInPictureWindowManager::GetInstance()
+        ->OnPictureInPictureWindowHidden(&pip_window);
+  }
+
+  {
+    MockPictureInPictureWindow pip_window;
+    {
+      ScopedTuckPictureInPicture tuck1;
+
+      {
+        // Multiple ScopedTuckPictureInPicture should still tuck
+        // picture-in-picture windows.
+        ScopedTuckPictureInPicture tuck2;
+
+        PictureInPictureWindowManager::GetInstance()
+            ->OnPictureInPictureWindowShown(&pip_window);
+        EXPECT_TRUE(pip_window.is_tucking());
+      }
+
+      // When one of them is destroyed but the other remains, it should still
+      // remain tucked.
+      EXPECT_TRUE(pip_window.is_tucking());
+    }
+
+    // Once both have been destroyed, picture-in-picture windows should be
+    // untucked.
+    EXPECT_FALSE(pip_window.is_tucking());
+
+    PictureInPictureWindowManager::GetInstance()
+        ->OnPictureInPictureWindowHidden(&pip_window);
+  }
+}
+
+TEST_F(PictureInPictureWindowManagerTest,
+       ShouldFileDialogTuckPictureInPicture) {
+  PictureInPictureWindowManager::GetInstance()->EnterDocumentPictureInPicture(
+      web_contents(), child_web_contents());
+
+  {
+    base::test::ScopedFeatureList feature_list;
+    feature_list.InitAndEnableFeature(media::kFileDialogsTuckPictureInPicture);
+
+    // With the feature enabled, file dialogs that aren't on a document
+    // picture-in-picture window should tuck picture-in-picture windows.
+    EXPECT_TRUE(PictureInPictureWindowManager::GetInstance()
+                    ->ShouldFileDialogTuckPictureInPicture(web_contents()));
+    EXPECT_FALSE(
+        PictureInPictureWindowManager::GetInstance()
+            ->ShouldFileDialogTuckPictureInPicture(child_web_contents()));
+  }
+
+  {
+    base::test::ScopedFeatureList feature_list;
+    feature_list.InitAndDisableFeature(media::kFileDialogsTuckPictureInPicture);
+
+    // With the feature disabled, no file dialogs should tuck
+    // picture-in-picture windows.
+    EXPECT_FALSE(PictureInPictureWindowManager::GetInstance()
+                     ->ShouldFileDialogTuckPictureInPicture(web_contents()));
+    EXPECT_FALSE(
+        PictureInPictureWindowManager::GetInstance()
+            ->ShouldFileDialogTuckPictureInPicture(child_web_contents()));
+  }
+
+  PictureInPictureWindowManager::GetInstance()->ExitPictureInPicture();
+}
+
 TEST_F(PictureInPictureWindowManagerTest,
        EnterAndCloseDocumentPip_NormalCloseDoesCommit) {
   base::SimpleTestTickClock test_clock;
diff --git a/chrome/browser/ui/views/overlay/video_overlay_window_views_unittest.cc b/chrome/browser/ui/views/overlay/video_overlay_window_views_unittest.cc
index 8b015e8..50e5fc34 100644
--- a/chrome/browser/ui/views/overlay/video_overlay_window_views_unittest.cc
+++ b/chrome/browser/ui/views/overlay/video_overlay_window_views_unittest.cc
@@ -15,6 +15,7 @@
 #include "chrome/browser/picture_in_picture/auto_pip_setting_overlay_view.h"
 #include "chrome/browser/picture_in_picture/picture_in_picture_occlusion_tracker.h"
 #include "chrome/browser/picture_in_picture/picture_in_picture_window_manager.h"
+#include "chrome/browser/picture_in_picture/scoped_tuck_picture_in_picture.h"
 #include "chrome/browser/ui/views/chrome_layout_provider.h"
 #include "chrome/browser/ui/views/overlay/back_to_tab_button.h"
 #include "chrome/browser/ui/views/overlay/close_image_button.h"
@@ -795,6 +796,45 @@
   ASSERT_EQ(nullptr, live_status);
 }
 
+TEST_F(VideoOverlayWindowViewsTest, CanBeTuckedToTheSideOfTheScreen) {
+  // Place the window on the left side of the screen.
+  SetDisplayWorkArea({0, 0, 2000, 2000});
+  overlay_window().SetBounds({{400, 400}, {500, 500}});
+
+  // If we tell it to force tucking, it should tuck to the left side of the
+  // screen.
+  overlay_window().SetForcedTucking(true);
+  EXPECT_LT(overlay_window().GetWindowBoundsInScreen().x(), 0);
+
+  // If we tell it to stop tucking, it should be put back in its original
+  // position.
+  overlay_window().SetForcedTucking(false);
+  EXPECT_EQ(overlay_window().GetWindowBoundsInScreen().x(), 400);
+}
+
+TEST_F(VideoOverlayWindowViewsTest, UntucksWhenReshownIfNecessary) {
+  // Place the window on the left side of the screen.
+  SetDisplayWorkArea({0, 0, 2000, 2000});
+  overlay_window().SetBounds({{400, 400}, {500, 500}});
+  overlay_window().ShowInactive();
+
+  // Start tucking via a ScopedTuckPictureInPicture.
+  auto scoped_tuck = std::make_unique<ScopedTuckPictureInPicture>();
+  EXPECT_LT(overlay_window().GetWindowBoundsInScreen().x(), 0);
+
+  // Hide ourselves. This will mean if tucking ends then the
+  // PictureInPictureWindowManager won't actually notify us.
+  overlay_window().Hide();
+
+  // End tucking.
+  scoped_tuck.reset();
+
+  // Show ourselves. We should check with the PictureInPictureWindowManager and
+  // realize we should no longer tuck.
+  overlay_window().ShowInactive();
+  EXPECT_EQ(overlay_window().GetWindowBoundsInScreen().x(), 400);
+}
+
 TEST_F(VideoOverlayWindowViewsTest,
        ReplayAndForward10SecondsNotDrawnWhen2024UIIsDisabled) {
   overlay_window().ForceControlsVisibleForTesting(true);
diff --git a/chrome/browser/ui/views/picture_in_picture/picture_in_picture_tucker_unittest.cc b/chrome/browser/ui/views/picture_in_picture/picture_in_picture_tucker_unittest.cc
new file mode 100644
index 0000000..372d502a
--- /dev/null
+++ b/chrome/browser/ui/views/picture_in_picture/picture_in_picture_tucker_unittest.cc
@@ -0,0 +1,144 @@
+// Copyright 2025 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/views/picture_in_picture/picture_in_picture_tucker.h"
+
+#include "ui/display/display.h"
+#include "ui/display/screen.h"
+#include "ui/display/test/test_screen.h"
+#include "ui/views/test/views_test_base.h"
+#include "ui/views/widget/widget.h"
+
+class PictureInPictureTuckerTest : public views::ViewsTestBase {
+ public:
+  void SetUp() override {
+    display::Screen::SetScreenInstance(&test_screen_);
+    views::ViewsTestBase::SetUp();
+
+    widget_ = CreateTestWidget(views::Widget::InitParams::CLIENT_OWNS_WIDGET);
+    tucker_ = std::make_unique<PictureInPictureTucker>(*widget_.get());
+  }
+
+  void TearDown() override {
+    tucker_.reset();
+    widget_.reset();
+    views::ViewsTestBase::TearDown();
+    display::Screen::SetScreenInstance(nullptr);
+  }
+
+ protected:
+  display::test::TestScreen& test_screen() { return test_screen_; }
+  views::Widget* widget() { return widget_.get(); }
+  PictureInPictureTucker* tucker() { return tucker_.get(); }
+
+ private:
+  display::test::TestScreen test_screen_;
+  std::unique_ptr<views::Widget> widget_;
+  std::unique_ptr<PictureInPictureTucker> tucker_;
+};
+
+TEST_F(PictureInPictureTuckerTest, TucksTowardCloserEdge) {
+  const gfx::Rect work_area =
+      display::Screen::GetScreen()
+          ->GetDisplayNearestWindow(widget()->GetNativeWindow())
+          .work_area();
+
+  // Place the widget on the left side of the screen.
+  const gfx::Rect initial_leftside_bounds({work_area.x() + 20, 50}, {200, 200});
+  widget()->SetBounds(initial_leftside_bounds);
+  EXPECT_EQ(initial_leftside_bounds, widget()->GetWindowBoundsInScreen());
+
+  // Tuck the widget. This should tuck towards the left side.
+  tucker()->Tuck();
+  EXPECT_LT(widget()->GetWindowBoundsInScreen().x(),
+            initial_leftside_bounds.x());
+
+  // Untucking should place the widget back where it started.
+  tucker()->Untuck();
+  EXPECT_EQ(initial_leftside_bounds, widget()->GetWindowBoundsInScreen());
+
+  // Place the widget on the right side of the screen.
+  const gfx::Rect initial_rightside_bounds({work_area.right() - 220, 50},
+                                           {200, 200});
+  widget()->SetBounds(initial_rightside_bounds);
+  EXPECT_EQ(initial_rightside_bounds, widget()->GetWindowBoundsInScreen());
+
+  // Tuck the widget. This should tuck towards the right side.
+  tucker()->Tuck();
+  EXPECT_GT(widget()->GetWindowBoundsInScreen().x(),
+            initial_rightside_bounds.x());
+
+  // Untucking should place the widget back where it started.
+  tucker()->Untuck();
+  EXPECT_EQ(initial_rightside_bounds, widget()->GetWindowBoundsInScreen());
+}
+
+TEST_F(PictureInPictureTuckerTest,
+       RetuckingForResizeRemembersOriginalPosition) {
+  const gfx::Rect initial_bounds({20, 50}, {200, 200});
+  widget()->SetBounds(initial_bounds);
+  EXPECT_EQ(initial_bounds, widget()->GetWindowBoundsInScreen());
+
+  // Tuck the widget.
+  tucker()->Tuck();
+  ASSERT_NE(initial_bounds, widget()->GetWindowBoundsInScreen());
+
+  // Resize the widget and retuck it.
+  gfx::Size new_size = {300, 300};
+  widget()->SetSize(new_size);
+  tucker()->Tuck();
+
... (truncated)