From 34db55b6bafbff86b637397af5001fbe2913dc42 Mon Sep 17 00:00:00 2001
From: Ahmed Fakhry <afakhry@chromium.org>
Date: Wed, 13 Oct 2021 00:08:18 +0000
Subject: [PATCH] capture_mode_settings: Parent the folder selection dialog
 correctly

|SelectFileDialogExtension| used to favor parenting the dialog
window to an existing browser window (if one was available)
ignoring the provided |owning_window| to the SelectFile() API.

This is not the desired behavior, since in capture mode, the
owning window must be respected, otherwise, the window will
be below the capture mode shield (in terms of z-order), making
it dimmed and preventing it from receiving any events.

BUG=1258842
TEST=Manually, added a browser test that fails without the fix.

Change-Id: I1de270c86151847299c6173b1f631c89918f52f0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3218405
Reviewed-by: Alex Danilo <adanilo@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/main@{#930865}
---
 ash/capture_mode/capture_mode_api.cc          |  4 --
 ash/capture_mode/capture_mode_session.h       |  1 +
 ash/capture_mode/capture_mode_test_api.cc     | 28 ++++++++++++
 .../folder_selection_dialog_controller.cc     | 13 +++---
 .../folder_selection_dialog_controller.h      |  6 +++
 .../cpp/capture_mode/capture_mode_api.h       |  3 --
 .../cpp/capture_mode/capture_mode_test_api.h  | 12 +++++
 ash/public/cpp/shell_window_ids.h             | 11 ++---
 ash/wm/window_state.cc                        |  6 +--
 .../capture_mode/capture_mode_browsertest.cc  | 44 +++++++++++++++++++
 .../ui/views/select_file_dialog_extension.cc  | 23 +++++++---
 11 files changed, 122 insertions(+), 29 deletions(-)

diff --git a/ash/capture_mode/capture_mode_api.cc b/ash/capture_mode/capture_mode_api.cc
index 8a4f34f60eebe7..b16ba24a999d62 100644
--- a/ash/capture_mode/capture_mode_api.cc
+++ b/ash/capture_mode/capture_mode_api.cc
@@ -12,8 +12,4 @@ void CaptureScreenshotsOfAllDisplays() {
   CaptureModeController::Get()->CaptureScreenshotsOfAllDisplays();
 }
 
-bool IsCaptureModeSessionActive() {
-  return CaptureModeController::Get()->IsActive();
-}
-
 }  // namespace ash
diff --git a/ash/capture_mode/capture_mode_session.h b/ash/capture_mode/capture_mode_session.h
index 684c23768df71a..0044d0b85a4650 100644
--- a/ash/capture_mode/capture_mode_session.h
+++ b/ash/capture_mode/capture_mode_session.h
@@ -172,6 +172,7 @@ class ASH_EXPORT CaptureModeSession
   friend class CaptureModeAdvancedSettingsTestApi;
   friend class CaptureModeSessionFocusCycler;
   friend class CaptureModeSessionTestApi;
+  friend class CaptureModeTestApi;
   class CursorSetter;
   class ScopedA11yOverrideWindowSetter;
 
diff --git a/ash/capture_mode/capture_mode_test_api.cc b/ash/capture_mode/capture_mode_test_api.cc
index 0e36e77987f6c8..cffe594a976d29 100644
--- a/ash/capture_mode/capture_mode_test_api.cc
+++ b/ash/capture_mode/capture_mode_test_api.cc
@@ -6,10 +6,12 @@
 
 #include "ash/capture_mode/capture_mode_controller.h"
 #include "ash/capture_mode/capture_mode_metrics.h"
+#include "ash/capture_mode/capture_mode_session.h"
 #include "ash/capture_mode/capture_mode_types.h"
 #include "ash/capture_mode/video_recording_watcher.h"
 #include "base/auto_reset.h"
 #include "base/check.h"
+#include "base/run_loop.h"
 
 namespace ash {
 
@@ -92,6 +94,32 @@ CaptureModeTestApi::GetRecordingOverlayController() {
       .get();
 }
 
+void CaptureModeTestApi::SimulateOpeningFolderSelectionDialog() {
+  DCHECK(controller_->IsActive());
+  auto* session = controller_->capture_mode_session();
+  DCHECK(!session->capture_mode_settings_widget_);
+  session->SetSettingsMenuShown(true);
+  DCHECK(session->capture_mode_settings_widget_);
+  session->OpenFolderSelectionDialog();
+
+  // In browser tests, the dialog creation is asynchronous, so we'll need to
+  // wait for it.
+  if (GetFolderSelectionDialogWindow())
+    return;
+
+  base::RunLoop loop;
+  session->folder_selection_dialog_controller_
+      ->on_dialog_window_added_callback_for_test_ = loop.QuitClosure();
+  loop.Run();
+}
+
+aura::Window* CaptureModeTestApi::GetFolderSelectionDialogWindow() {
+  DCHECK(controller_->IsActive());
+  auto* session = controller_->capture_mode_session();
+  auto* dialog_controller = session->folder_selection_dialog_controller_.get();
+  return dialog_controller ? dialog_controller->dialog_window() : nullptr;
+}
+
 void CaptureModeTestApi::SetType(bool for_video) {
   controller_->SetType(for_video ? CaptureModeType::kVideo
                                  : CaptureModeType::kImage);
diff --git a/ash/capture_mode/folder_selection_dialog_controller.cc b/ash/capture_mode/folder_selection_dialog_controller.cc
index 2a0271641be745..8c345207a848ed 100644
--- a/ash/capture_mode/folder_selection_dialog_controller.cc
+++ b/ash/capture_mode/folder_selection_dialog_controller.cc
@@ -59,13 +59,14 @@ FolderSelectionDialogController::FolderSelectionDialogController(
   DCHECK(root);
   DCHECK(root->IsRootWindow());
 
-  window_observation_.Observe(wm::TransientWindowManager::GetOrCreate(
-      dialog_background_dimmer_.window()));
+  auto* owner = dialog_background_dimmer_.window();
+  owner->SetId(kShellWindowId_CaptureModeFolderSelectionDialogOwner);
+  window_observation_.Observe(wm::TransientWindowManager::GetOrCreate(owner));
 
   dialog_background_dimmer_.SetDimColor(
       AshColorProvider::Get()->GetShieldLayerColor(
           AshColorProvider::ShieldLayerType::kShield40));
-  dialog_background_dimmer_.window()->Show();
+  owner->Show();
 
   select_folder_dialog_->SelectFile(
       ui::SelectFileDialog::SELECT_FOLDER,
@@ -74,7 +75,7 @@ FolderSelectionDialogController::FolderSelectionDialogController(
       /*file_types=*/nullptr,
       /*file_type_index=*/0,
       /*default_extension=*/base::FilePath::StringType(),
-      /*owning_window=*/dialog_background_dimmer_.window(),
+      /*owning_window=*/owner,
       /*params=*/nullptr);
 }
 
@@ -104,7 +105,6 @@ void FolderSelectionDialogController::OnTransientChildAdded(
   DCHECK(!dialog_window_);
 
   dialog_window_ = transient;
-  dialog_window_->SetId(kShellWindowId_CaptureModeFolderSelectionDialog);
 
   // The dialog should never resize, minimize or maximize.
   auto* widget = views::Widget::GetWidgetForNativeWindow(dialog_window_);
@@ -114,6 +114,9 @@ void FolderSelectionDialogController::OnTransientChildAdded(
   widget_delegate->SetCanResize(false);
   widget_delegate->SetCanMinimize(false);
   widget_delegate->SetCanMaximize(false);
+
+  if (on_dialog_window_added_callback_for_test_)
+    std::move(on_dialog_window_added_callback_for_test_).Run();
 }
 
 void FolderSelectionDialogController::OnTransientChildRemoved(
diff --git a/ash/capture_mode/folder_selection_dialog_controller.h b/ash/capture_mode/folder_selection_dialog_controller.h
index 5b7bd836b2bc24..9aef3950240197 100644
--- a/ash/capture_mode/folder_selection_dialog_controller.h
+++ b/ash/capture_mode/folder_selection_dialog_controller.h
@@ -6,6 +6,7 @@
 #define ASH_CAPTURE_MODE_FOLDER_SELECTION_DIALOG_CONTROLLER_H_
 
 #include "ash/wm/window_dimmer.h"
+#include "base/callback_forward.h"
 #include "base/files/file_path.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/scoped_observation.h"
@@ -67,6 +68,8 @@ class FolderSelectionDialogController : public ui::SelectFileDialog::Listener,
                                aura::Window* transient) override;
 
  private:
+  friend class CaptureModeTestApi;
+
   Delegate* const delegate_;
 
   // Dims everything behind the dialog (including the capture bar, the settings
@@ -81,6 +84,9 @@ class FolderSelectionDialogController : public ui::SelectFileDialog::Listener,
   // |select_folder_dialog_| as a transient child of the dimming window.
   aura::Window* dialog_window_ = nullptr;
 
+  // An optional callback that will be invoked when |dialog_window_| gets added.
+  base::OnceClosure on_dialog_window_added_callback_for_test_;
+
   // We observe the transient window manager of the dimming window to know when
   // the dialog window is added or removed.
   base::ScopedObservation<wm::TransientWindowManager,
diff --git a/ash/public/cpp/capture_mode/capture_mode_api.h b/ash/public/cpp/capture_mode/capture_mode_api.h
index 0f41aefb144cba..2d399c887c628e 100644
--- a/ash/public/cpp/capture_mode/capture_mode_api.h
+++ b/ash/public/cpp/capture_mode/capture_mode_api.h
@@ -14,9 +14,6 @@ namespace ash {
 // Note: this won't start a capture mode session.
 void ASH_EXPORT CaptureScreenshotsOfAllDisplays();
 
-// Returns true if a capture mode session is currently active.
-bool ASH_EXPORT IsCaptureModeSessionActive();
-
 }  // namespace ash
 
 #endif  // ASH_PUBLIC_CPP_CAPTURE_MODE_CAPTURE_MODE_API_H_
diff --git a/ash/public/cpp/capture_mode/capture_mode_test_api.h b/ash/public/cpp/capture_mode/capture_mode_test_api.h
index 782dd9670a5671..8a4f603f5c471d 100644
--- a/ash/public/cpp/capture_mode/capture_mode_test_api.h
+++ b/ash/public/cpp/capture_mode/capture_mode_test_api.h
@@ -9,6 +9,10 @@
 #include "base/callback_forward.h"
 #include "base/files/file_path.h"
 
+namespace aura {
+class Window;
+}  // namespace aura
+
 namespace gfx {
 class Rect;
 }  // namespace gfx
@@ -72,6 +76,14 @@ class ASH_EXPORT CaptureModeTestApi {
   // Can only be called while recording is in progress for a Projector session.
   RecordingOverlayController* GetRecordingOverlayController();
 
+  // Simulates the flow taken by users to open the folder selection dialog from
+  // the settings menu, and waits until this dialog gets added.
+  void SimulateOpeningFolderSelectionDialog();
+
+  // Returns a pointer to the folder selection dialog window or nullptr if no
+  // such window exists.
+  aura::Window* GetFolderSelectionDialogWindow();
+
  private:
   // Sets the capture mode type to a video capture if |for_video| is true, or
   // image capture otherwise.
diff --git a/ash/public/cpp/shell_window_ids.h b/ash/public/cpp/shell_window_ids.h
index 99bbe6408407ec..e37a610946cf86 100644
--- a/ash/public/cpp/shell_window_ids.h
+++ b/ash/public/cpp/shell_window_ids.h
@@ -202,11 +202,12 @@ enum NonContainerWindowId {
   // multiple displays connected.
   kShellWindowId_DisplayIdentificationHighlightWindow,
 
-  // The window hosting the folder selection menu for capture mode, which is not
-  // parented to a top-level container, yet it needs to be uniquely identified
-  // so that we can allow a WindowState to be created for it, since it can be
-  // dragged around.
-  kShellWindowId_CaptureModeFolderSelectionDialog,
+  // The window specified as the owner of the folder selection menu for capture
+  // mode, which will be a transient window parent of the about to be created
+  // dialog window. This is needed in order to prevent
+  // |SelectFileDialogExtension| from favoring to parent the dialog to a browser
+  // window (if one exists).
+  kShellWindowId_CaptureModeFolderSelectionDialogOwner,
 };
 
 // A list of system modal container IDs. The order of the list is important that
diff --git a/ash/wm/window_state.cc b/ash/wm/window_state.cc
index 5bc2abf1d59cca..df3cefd3ab23fd 100644
--- a/ash/wm/window_state.cc
+++ b/ash/wm/window_state.cc
@@ -971,12 +971,8 @@ WindowState* WindowState::Get(aura::Window* window) {
   // WindowState is only for windows in top level container, unless they are
   // temporarily hidden when launched by window restore. The will be reparented
   // to a top level container soon, and need a WindowState.
-  // The window of capture mode folder selection dialog is also special. It's
-  // not parented to a top level container, but it needs a window state since it
-  // can be dragged around.
   if (!IsToplevelContainer(window->parent()) &&
-      !IsTemporarilyHiddenForFullrestore(window) &&
-      window->GetId() != kShellWindowId_CaptureModeFolderSelectionDialog) {
+      !IsTemporarilyHiddenForFullrestore(window)) {
     return nullptr;
   }
 
diff --git a/chrome/browser/ui/ash/capture_mode/capture_mode_browsertest.cc b/chrome/browser/ui/ash/capture_mode/capture_mode_browsertest.cc
index baf2dd776ab127..2d503573756307 100644
--- a/chrome/browser/ui/ash/capture_mode/capture_mode_browsertest.cc
+++ b/chrome/browser/ui/ash/capture_mode/capture_mode_browsertest.cc
@@ -2,14 +2,20 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "ash/constants/ash_features.h"
 #include "ash/public/cpp/capture_mode/capture_mode_test_api.h"
+#include "ash/public/cpp/shell_window_ids.h"
 #include "ash/public/cpp/test/shell_test_api.h"
+#include "base/test/scoped_feature_list.h"
+#include "chrome/browser/ash/file_manager/file_manager_test_util.h"
+#include "chrome/browser/extensions/extension_browsertest.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "content/public/test/browser_test.h"
 #include "ui/aura/window.h"
 #include "ui/events/test/event_generator.h"
+#include "ui/wm/core/window_util.h"
 
 // Testing class to test CrOS capture mode, which is a feature to take
 // screenshots and record video.
@@ -31,3 +37,41 @@ IN_PROC_BROWSER_TEST_F(CaptureModeBrowserTest, ContextMenuStaysOpen) {
   ash::CaptureModeTestApi().StartForWindow(/*for_video=*/false);
   EXPECT_TRUE(shell_test_api.IsContextMenuShown());
 }
+
+class AdvancedSettingsCaptureModeBrowserTest
+    : public extensions::ExtensionBrowserTest {
+ public:
+  AdvancedSettingsCaptureModeBrowserTest() {
+    scoped_feature_list_.InitAndEnableFeature(
+        ash::features::kImprovedScreenCaptureSettings);
+  }
+
+  ~AdvancedSettingsCaptureModeBrowserTest() override = default;
+
+  // extensions::ExtensionBrowserTest:
+  void SetUpOnMainThread() override {
+    extensions::ExtensionBrowserTest::SetUpOnMainThread();
+    CHECK(profile());
+    file_manager::test::AddDefaultComponentExtensionsOnMainThread(profile());
+  }
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+// Tests that the capture mode folder selection dialog window gets parented
+// correctly when a browser window is available.
+IN_PROC_BROWSER_TEST_F(AdvancedSettingsCaptureModeBrowserTest,
+                       FolderSelectionDialogParentedCorrectly) {
+  ASSERT_TRUE(browser());
+  ash::CaptureModeTestApi test_api;
+  test_api.StartForFullscreen(/*for_video=*/false);
+  test_api.SimulateOpeningFolderSelectionDialog();
+  auto* dialog_window = test_api.GetFolderSelectionDialogWindow();
+  ASSERT_TRUE(dialog_window);
+  auto* transient_root = wm::GetTransientRoot(dialog_window);
+  ASSERT_TRUE(transient_root);
+  EXPECT_EQ(transient_root->GetId(),
+            ash::kShellWindowId_CaptureModeFolderSelectionDialogOwner);
+  EXPECT_NE(transient_root, browser()->window()->GetNativeWindow());
+}
diff --git a/chrome/browser/ui/views/select_file_dialog_extension.cc b/chrome/browser/ui/views/select_file_dialog_extension.cc
index bdfa06266b38ad..576d28f67844e4 100644
--- a/chrome/browser/ui/views/select_file_dialog_extension.cc
+++ b/chrome/browser/ui/views/select_file_dialog_extension.cc
@@ -10,7 +10,7 @@
 #include <utility>
 
 #include "ash/constants/ash_features.h"
-#include "ash/public/cpp/capture_mode/capture_mode_api.h"
+#include "ash/public/cpp/shell_window_ids.h"
 #include "ash/public/cpp/tablet_mode.h"
 #include "base/bind.h"
 #include "base/callback.h"
@@ -48,6 +48,7 @@
 #include "extensions/browser/app_window/app_window.h"
 #include "extensions/browser/app_window/native_app_window.h"
 #include "extensions/browser/extension_system.h"
+#include "ui/aura/window.h"
 #include "ui/base/base_window.h"
 #include "ui/gfx/color_palette.h"
 #include "ui/shell_dialogs/select_file_policy.h"
@@ -442,8 +443,21 @@ void SelectFileDialogExtension::SelectFileWithFileManagerParams(
   // The web contents to associate the dialog with.
   content::WebContents* web_contents = nullptr;
 
+  // The folder selection dialog created for capture mode should never be
+  // parented to a browser window (if one exists). https://crbug.com/1258842.
+  const bool is_for_capture_mode =
+      owner.window &&
+      owner.window->GetId() ==
+          ash::kShellWindowId_CaptureModeFolderSelectionDialogOwner;
+
+  const bool skip_finding_browser = is_for_capture_mode ||
+                                    owner.android_task_id.has_value() ||
+                                    owner.lacros_window_id.has_value();
+
+  can_resize_ = !ash::TabletMode::IsInTabletMode() && !is_for_capture_mode;
+
   // Obtain BaseWindow and WebContents if the owner window is browser.
-  if (!owner.android_task_id.has_value() && !owner.lacros_window_id.has_value())
+  if (!skip_finding_browser)
     FindRuntimeContext(owner.window, &base_window, &web_contents);
 
   if (web_contents)
@@ -475,11 +489,6 @@ void SelectFileDialogExtension::SelectFileWithFileManagerParams(
   gfx::NativeWindow parent_window =
       base_window ? base_window->GetNativeWindow() : owner.window;
 
-#if BUILDFLAG(IS_CHROMEOS_ASH)
-  can_resize_ =
-      !ash::TabletMode::IsInTabletMode() && !ash::IsCaptureModeSessionActive();
-#endif
-
   if (ash::features::IsFileManagerSwaEnabled()) {
     // SystemFilesAppDialogDelegate is a self-deleting class that calls the
     // delete operator once the dialog for which it was created has been closed.