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

@@ -12,8 +12,4 @@ void CaptureScreenshotsOfAllDisplays() {
   CaptureModeController::Get()->CaptureScreenshotsOfAllDisplays();
 }
 
-bool IsCaptureModeSessionActive() {
-  return CaptureModeController::Get()->IsActive();
-}
-
 }  // namespace ash

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;
 

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);

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(

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,

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_

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.

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

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;
   }
 

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());
+}

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.