[AI Rewriter]: Improve sizing/positioning (#24182)

2024-06-20 11:54:59 +12:00
parent 91f2056589
commit b44bdcc9b4
8 changed files with 357 additions and 25 deletions
@@ -126,6 +126,7 @@ source_set("ui") {
    deps += [
      "//brave/components/ai_rewriter/common",
      "//brave/components/ai_rewriter/resources/page:generated_resources",
+      "//components/web_modal",
    ]
  }

@@ -0,0 +1,24 @@
+# Copyright (c) 2024 The Brave Authors. All rights reserved.
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this file,
+# You can obtain one at https://mozilla.org/MPL/2.0/.
+
+source_set("browsertest") {
+  testonly = true
+  assert(!is_android)
+
+  sources = [ "ai_rewriter_dialog_delegate_browsertest.cc" ]
+
+  defines = [ "HAS_OUT_OF_PROC_TEST_RUNNER" ]
+
+  deps = [
+    "//base/test:test_support",
+    "//brave/components/ai_rewriter/common",
+    "//chrome/browser/ui",
+    "//chrome/test:test_support_ui",
+    "//content/test:test_support",
+    "//net:test_support",
+    "//testing/gmock",
+    "//testing/gtest",
+  ]
+}
@@ -6,6 +6,7 @@
 #include "brave/browser/ui/ai_rewriter/ai_rewriter_dialog_delegate.h"

 #include <memory>
+#include <optional>
 #include <string>
 #include <utility>

@@ -14,12 +15,20 @@
 #include "base/memory/ptr_util.h"
 #include "base/memory/weak_ptr.h"
 #include "base/process/kill.h"
+#include "base/scoped_observation.h"
 #include "brave/browser/ui/webui/ai_rewriter/ai_rewriter_ui.h"
 #include "brave/components/ai_rewriter/common/features.h"
 #include "brave/components/ai_rewriter/common/mojom/ai_rewriter.mojom.h"
 #include "brave/components/constants/webui_url_constants.h"
 #include "chrome/browser/tab_contents/web_contents_collection.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/webui/constrained_web_dialog_ui.h"
+#include "components/constrained_window/constrained_window_views.h"
+#include "components/web_modal/modal_dialog_host.h"
+#include "components/web_modal/web_contents_modal_dialog_host.h"
+#include "components/web_modal/web_contents_modal_dialog_manager.h"
 #include "content/public/browser/focused_node_details.h"
 #include "content/public/browser/host_zoom_map.h"
 #include "content/public/browser/navigation_entry.h"
@@ -34,12 +43,14 @@
 #include "mojo/public/cpp/bindings/remote.h"
 #include "services/service_manager/public/cpp/interface_provider.h"
 #include "ui/base/ui_base_types.h"
+#include "ui/gfx/geometry/point.h"
 #include "ui/gfx/geometry/point_f.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/rect_f.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/widget/widget_delegate.h"
+#include "ui/views/widget/widget_observer.h"
 #include "ui/web_dialogs/web_dialog_delegate.h"

 using content::WebContents;
@@ -86,6 +97,139 @@ class AIRewriterDialogDelegate::DialogContentsObserver
  const raw_ptr<AIRewriterDialogDelegate> dialog_;
 };

+class AIRewriterDialogDelegate::DialogPositioner
+    : public views::WidgetObserver,
+      public web_modal::ModalDialogHostObserver {
+ public:
+  DialogPositioner(content::WebContents* target_contents,
+                   web_modal::WebContentsModalDialogHost* host,
+                   views::Widget* dialog_widget)
+      : top_(host->GetDialogPosition({}).y()),
+        target_contents_(target_contents->GetWeakPtr()),
+        dialog_widget_(dialog_widget->GetWeakPtr()) {
+    // TODO(fallaciousreasoning): In a follow up PR we should handle reparenting
+    // |target_contents_| into another browser. For now, we just assume it
+    // remains in the same widget (which it won't necessarily).
+
+    auto* browser = chrome::FindBrowserWithTab(target_contents);
+    CHECK(browser);
+
+    auto* host_widget = views::Widget::GetWidgetForNativeWindow(
+        browser->window()->GetNativeWindow());
+    CHECK(host_widget);
+
+    host_widget_ = host_widget->GetWeakPtr();
+
+    host_widget_observation_.Observe(host_widget);
+    dialog_widget_observation_.Observe(dialog_widget);
+    host_observation_.Observe(host);
+
+    UpdatePosition(target_contents->GetFocusedFrame());
+  }
+
+  DialogPositioner(const DialogPositioner&) = delete;
+  DialogPositioner& operator=(const DialogPositioner&) = delete;
+  ~DialogPositioner() override = default;
+
+  // web_modal::ModalDialogHostObserver:
+  void OnPositionRequiresUpdate() override {
+    if (!target_contents_) {
+      return;
+    }
+
+    UpdatePosition(target_contents_->GetFocusedFrame());
+    UpdateFocusedBounds();
+  }
+
+  void OnHostDestroying() override {
+    host_widget_observation_.Reset();
+    host_observation_.Reset();
+    dialog_widget_observation_.Reset();
+  }
+
+  // views::WidgetObserver
+  void OnWidgetBoundsChanged(views::Widget* widget,
+                             const gfx::Rect& new_bounds) override {
+    OnPositionRequiresUpdate();
+  }
+
+ private:
+  void UpdateFocusedBounds() {
+    if (!target_contents_) {
+      return;
+    }
+    auto* browser = chrome::FindBrowserWithTab(target_contents_.get());
+    if (!browser) {
+      return;
+    }
+
+    mojo::Remote<mojom::AIRewriterAgent> agent;
+    auto* frame = target_contents_->GetFocusedFrame();
+    frame->GetRemoteInterfaces()->GetInterface(
+        agent.BindNewPipeAndPassReceiver());
+    auto* raw_agent = agent.get();
+    raw_agent->GetFocusBounds(base::BindOnce(
+        [](mojo::Remote<mojom::AIRewriterAgent> agent,
+           base::WeakPtr<AIRewriterDialogDelegate::DialogPositioner> positioner,
+           content::WeakDocumentPtr document_pointer,
+           const gfx::RectF& focus_rect) {
+          if (!positioner) {
+            return;
+          }
+          positioner->last_bounds_ = focus_rect;
+          positioner->UpdatePosition(
+              document_pointer.AsRenderFrameHostIfValid());
+        },
+        std::move(agent), weak_ptr_factory_.GetWeakPtr(),
+        frame->GetWeakDocumentPtr()));
+  }
+
+  void UpdatePosition(content::RenderFrameHost* rfh) {
+    if (!rfh || !last_bounds_ || !dialog_widget_ || !host_widget_) {
+      return;
+    }
+
+    auto transformed = TransformFrameRectToView(rfh, last_bounds_.value());
+
+    auto dialog_bounds = dialog_widget_->GetWindowBoundsInScreen();
+    dialog_bounds.set_x(transformed.CenterPoint().x() -
+                        dialog_bounds.width() / 2);
+    dialog_bounds.set_y(transformed.bottom() + top_);
+
+    auto host_bounds = host_widget_->GetWindowBoundsInScreen();
+    host_bounds.set_origin(gfx::Point());
+
+    if (constrained_window::PlatformClipsChildrenToViewport() &&
+        !host_bounds.Contains(dialog_bounds)) {
+      dialog_bounds.AdjustToFit(host_bounds);
+    }
+
+    dialog_widget_->SetBounds(dialog_bounds);
+  }
+
+  // The offset for the top Chrome - this shouldn't change while the dialog is
+  // open.
+  int top_ = 0;
+
+  // The last bounds we received from the |target_contents_| for the location of
+  // the focused element.
+  std::optional<gfx::RectF> last_bounds_;
+
+  base::WeakPtr<content::WebContents> target_contents_;
+  base::WeakPtr<views::Widget> dialog_widget_;
+  base::WeakPtr<views::Widget> host_widget_;
+
+  base::ScopedObservation<views::Widget, views::WidgetObserver>
+      host_widget_observation_{this};
+  base::ScopedObservation<views::Widget, views::WidgetObserver>
+      dialog_widget_observation_{this};
+  base::ScopedObservation<web_modal::ModalDialogHost,
+                          web_modal::ModalDialogHostObserver>
+      host_observation_{this};
+  base::WeakPtrFactory<AIRewriterDialogDelegate::DialogPositioner>
+      weak_ptr_factory_{this};
+};
+
 // static
 AIRewriterDialogDelegate* AIRewriterDialogDelegate::Show(
    WebContents* contents,
@@ -129,9 +273,12 @@ void AIRewriterDialogDelegate::OnFocusChangedInPage(
  CloseDialog();
 }
 void AIRewriterDialogDelegate::ShowDialog() {
+  const gfx::Size min_size(600, 550);
+  const gfx::Size max_size(600, 2000);
  ConstrainedWebDialogDelegate* dialog_delegate =
-      ShowConstrainedWebDialog(target_contents_->GetBrowserContext(),
-                               base::WrapUnique(this), &*target_contents_);
+      ShowConstrainedWebDialogWithAutoResize(
+          target_contents_->GetBrowserContext(), base::WrapUnique(this),
+          &*target_contents_, min_size, max_size);

  DCHECK(dialog_delegate);
  content::WebContents* dialog_contents = dialog_delegate->GetWebContents();
@@ -142,30 +289,15 @@ void AIRewriterDialogDelegate::ShowDialog() {
  auto* widget = views::Widget::GetWidgetForNativeWindow(
      dialog_delegate->GetNativeDialog());

-  mojo::Remote<mojom::AIRewriterAgent> agent;
-  auto* frame = target_contents_->GetFocusedFrame();
-  frame->GetRemoteInterfaces()->GetInterface(
-      agent.BindNewPipeAndPassReceiver());
-  auto* raw_agent = agent.get();
-  raw_agent->GetFocusBounds(base::BindOnce(
-      [](mojo::Remote<mojom::AIRewriterAgent> agent,
-         base::WeakPtr<views::Widget> widget,
-         content::WeakDocumentPtr document_pointer,
-         const gfx::RectF& focus_rect) {
-        auto* host = document_pointer.AsRenderFrameHostIfValid();
-        if (!widget || !host) {
-          return;
-        }
+  auto* manager = web_modal::WebContentsModalDialogManager::FromWebContents(
+      target_contents_.get());
+  CHECK(manager);
+  auto* dialog_host = manager->delegate()->GetWebContentsModalDialogHost();
+  CHECK(dialog_host);
+  positioner_ = std::make_unique<DialogPositioner>(target_contents_.get(),
+                                                   dialog_host, widget);

-        auto transformed = TransformFrameRectToView(host, focus_rect);
-        auto widget_bounds = widget->GetWindowBoundsInScreen();
-
-        widget_bounds.set_x(transformed.CenterPoint().x() -
-                            widget_bounds.width() / 2);
-        widget_bounds.set_y(transformed.bottom());
-        widget->SetBounds(widget_bounds);
-      },
-      std::move(agent), widget->GetWeakPtr(), frame->GetWeakDocumentPtr()));
+  widget_for_testing_ = widget;
 }

 void AIRewriterDialogDelegate::CloseDialog() {
@@ -182,6 +314,10 @@ void AIRewriterDialogDelegate::ResetDialogObserver() {
  dialog_observer_.reset();
 }

+AIRewriterUI* AIRewriterDialogDelegate::GetRewriterUIForTesting() {
+  return GetRewriterUI();
+}
+
 AIRewriterUI* AIRewriterDialogDelegate::GetRewriterUI() {
  auto* dialog_contents = GetDialogWebContents();
  if (!dialog_contents) {
@@ -9,6 +9,7 @@
 #include <memory>
 #include <string>

+#include "base/memory/raw_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "ui/web_dialogs/web_dialog_delegate.h"
@@ -19,6 +20,10 @@ class NavigationHandle;
 struct FocusedNodeDetails;
 }  // namespace content

+namespace views {
+class Widget;
+}
+
 namespace ai_rewriter {

 class AIRewriterUI;
@@ -36,12 +41,18 @@ class AIRewriterDialogDelegate : public ui::WebDialogDelegate,
  void CloseDialog();
  content::WebContents* GetDialogWebContents();

+  void UpdateBounds();
+
  // content::WebContentsObserver:
  void DidFinishNavigation(content::NavigationHandle* handle) override;
  void OnFocusChangedInPage(content::FocusedNodeDetails* details) override;

+  views::Widget* widget_for_testing() { return widget_for_testing_; }
+  AIRewriterUI* GetRewriterUIForTesting();
+
 private:
  class DialogContentsObserver;
+  class DialogPositioner;

  explicit AIRewriterDialogDelegate(content::WebContents* contents);

@@ -52,6 +63,9 @@ class AIRewriterDialogDelegate : public ui::WebDialogDelegate,

  base::WeakPtr<content::WebContents> target_contents_;
  std::unique_ptr<DialogContentsObserver> dialog_observer_;
+  std::unique_ptr<DialogPositioner> positioner_;
+
+  raw_ptr<views::Widget> widget_for_testing_;
 };

 }  // namespace ai_rewriter
@@ -0,0 +1,142 @@
+// Copyright (c) 2024 The Brave Authors. All rights reserved.
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this file,
+// You can obtain one at https://mozilla.org/MPL/2.0/.
+
+#include "brave/browser/ui/ai_rewriter/ai_rewriter_dialog_delegate.h"
+
+#include "base/path_service.h"
+#include "base/run_loop.h"
+#include "base/test/scoped_feature_list.h"
+#include "brave/browser/ui/webui/ai_rewriter/ai_rewriter_ui.h"
+#include "brave/components/ai_rewriter/common/features.h"
+#include "brave/components/constants/brave_paths.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_window.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/test/browser_test.h"
+#include "content/public/test/browser_test_utils.h"
+#include "net/dns/mock_host_resolver.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/gfx/geometry/vector2d.h"
+#include "ui/views/widget/widget.h"
+#include "url/gurl.h"
+
+class AIRewriterDialogDelegateBrowserTest : public InProcessBrowserTest {
+ public:
+  AIRewriterDialogDelegateBrowserTest() {
+    features_.InitAndEnableFeature(ai_rewriter::features::kAIRewriter);
+  }
+
+  AIRewriterDialogDelegateBrowserTest(
+      const AIRewriterDialogDelegateBrowserTest&) = delete;
+  AIRewriterDialogDelegateBrowserTest& operator=(
+      const AIRewriterDialogDelegateBrowserTest&) = delete;
+  ~AIRewriterDialogDelegateBrowserTest() override = default;
+
+  void SetUpOnMainThread() override {
+    InProcessBrowserTest::SetUpOnMainThread();
+
+    host_resolver()->AddRule("*", "127.0.0.1");
+    content::SetupCrossSiteRedirector(embedded_test_server());
+
+    auto test_data_dir = base::PathService::CheckedGet(brave::DIR_TEST_DATA);
+
+    embedded_test_server()->ServeFilesFromDirectory(test_data_dir);
+
+    ASSERT_TRUE(embedded_test_server()->Start());
+
+    html_with_input_ =
+        embedded_test_server()->GetURL("example.com", "/rewriter-example.html");
+  }
+
+  content::WebContents* contents() {
+    return browser()->tab_strip_model()->GetActiveWebContents();
+  }
+
+  views::Widget* browser_widget() {
+    return views::Widget::GetWidgetForNativeWindow(
+        browser()->window()->GetNativeWindow());
+  }
+
+  void MoveWindow(gfx::Vector2d by) {
+    auto bounds = browser_widget()->GetWindowBoundsInScreen();
+    bounds.Offset(by);
+    browser_widget()->SetBounds(bounds);
+  }
+
+  void OpenPageWithInput() {
+    EXPECT_TRUE(ui_test_utils::NavigateToURL(browser(), html_with_input_));
+    content::WaitForLoadStop(contents());
+
+    EXPECT_TRUE(content::ExecJs(contents()->GetFocusedFrame(),
+                                "document.querySelector('textarea').select()"));
+  }
+
+ private:
+  base::test::ScopedFeatureList features_;
+  GURL html_with_input_;
+};
+
+IN_PROC_BROWSER_TEST_F(AIRewriterDialogDelegateBrowserTest, CanShowADialog) {
+  OpenPageWithInput();
+
+  ASSERT_TRUE(ai_rewriter::AIRewriterDialogDelegate::Show(
+      contents(), "It was a dark and stormy night..."));
+}
+
+// For now, Dialog positioning doesn't work on MacOS.
+#if !BUILDFLAG(IS_MAC)
+#define MAYBE_DialogMovesWithParentWindow DialogMovesWithParentWindow
+#else
+#define MAYBE_DialogMovesWithParentWindow DISABLED_DialogMovesWithParentWindow
+#endif
+IN_PROC_BROWSER_TEST_F(AIRewriterDialogDelegateBrowserTest,
+                       MAYBE_DialogMovesWithParentWindow) {
+  OpenPageWithInput();
+
+  auto* dialog = ai_rewriter::AIRewriterDialogDelegate::Show(
+      contents(), "It was a dark and stormy night...");
+
+  auto* dialog_widget = dialog->widget_for_testing();
+  EXPECT_TRUE(dialog_widget);
+
+  auto position = dialog_widget->GetWindowBoundsInScreen().origin();
+
+  gfx::Vector2d offset(100, 100);
+  MoveWindow(offset);
+
+  auto next_position = dialog_widget->GetWindowBoundsInScreen().origin();
+  EXPECT_EQ(offset, next_position - position);
+}
+
+IN_PROC_BROWSER_TEST_F(AIRewriterDialogDelegateBrowserTest,
+                       ChangingFocusClosesDialog) {
+  OpenPageWithInput();
+
+  auto* dialog = ai_rewriter::AIRewriterDialogDelegate::Show(
+      contents(), "It was a dark and stormy night...");
+
+  auto dialog_widget = dialog->widget_for_testing()->GetWeakPtr();
+  EXPECT_TRUE(content::ExecJs(contents()->GetFocusedFrame(),
+                              "document.querySelector('textarea').blur()"));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(dialog_widget);
+}
+
+IN_PROC_BROWSER_TEST_F(AIRewriterDialogDelegateBrowserTest, CanInsertText) {
+  OpenPageWithInput();
+
+  auto* dialog = ai_rewriter::AIRewriterDialogDelegate::Show(
+      contents(), "It was a dark and stormy night...");
+
+  std::string mock_generated("It was a bright and sunny day...");
+  dialog->GetRewriterUIForTesting()->InsertTextAndClose(mock_generated,
+                                                        base::DoNothing());
+
+  EXPECT_EQ(mock_generated,
+            content::EvalJs(contents()->GetFocusedFrame(),
+                            R"(document.querySelector('textarea').value)"));
+}
@@ -19,6 +19,10 @@ const FiltersContainer = styled.div`
  gap: ${spacing['2Xl']};

  padding: 0 ${spacing['2Xl']} ${spacing['2Xl']} ${spacing['2Xl']};
+
+  & > leo-buttonmenu {
+    --leo-menu-max-height: 280px !important;
+  }
 `

 export default function BeginGeneration() {
@@ -1128,6 +1128,7 @@ test("brave_browser_tests") {
      "//brave/app/theme:brave_theme_resources_grit",
      "//brave/app/theme:brave_unscaled_resources_grit",
      "//brave/browser/sharing_hub:browser_tests",
+      "//brave/browser/ui/ai_rewriter:browsertest",
      "//brave/browser/ui/geolocation:browser_tests",
      "//brave/browser/ui/whats_new:browser_test",
      "//brave/components/brave_wallet/browser:test_support",
@@ -0,0 +1,10 @@
+<html>
+
+<head>
+</head>
+
+<body>
+    <textarea>It was a dark and stormy night...</textarea>
+</body>
+
+</html>