From 63716080a676f0520d732c631e0e80eff59bf6c4 Mon Sep 17 00:00:00 2001
From: Lily Chen <chlily@chromium.org>
Date: Mon, 15 Dec 2025 15:22:24 -0800
Subject: [PATCH] [Download Bubble] Use BubbleCloser to close bubble from inactive

The download bubble normally relies on close-on-deactivate behavior from
BubbleDialogDelegate to allow the user to close it by pressing Esc,
clicking elsewhere, etc. However, it is sometimes created with
ShowInactive(), to avoid stealing focus which may be disruptive to the
user. In an already inactive state, close-on-deactivate cannot occur, so
there would have been no way to close the bubble.

We had two separate workarounds for this issue. First, the bubble
subscribes to BrowserList activation events and activates itself when
the browser it is attached to becomes active, relying on
close-on-deactivate to work as usual once the bubble is active. Later,
we added a BubbleCloser utility which uses EventMonitor to directly
close the bubble in response to input events.

This CL removes the former (self-activation) workaround in favor of just
using BubbleCloser. The self-activation workaround was causing issues
with unexpected input to the download bubble, when it was active but
occluded by other elements.

Bug: 440892551, 363930141, 421877606, 421348748, 398173038, 464313652
Change-Id: I2f195b1568391960cd67c84c536c5c6885faa665
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7254096
Commit-Queue: Lily Chen <chlily@chromium.org>
Reviewed-by: Daniel Rubery <drubery@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1559032}
---

diff --git a/chrome/browser/ui/views/download/bubble/download_bubble_interactive_uitest.cc b/chrome/browser/ui/views/download/bubble/download_bubble_interactive_uitest.cc
index 1378bcc3..7d4c86a 100644
--- a/chrome/browser/ui/views/download/bubble/download_bubble_interactive_uitest.cc
+++ b/chrome/browser/ui/views/download/bubble/download_bubble_interactive_uitest.cc
@@ -300,6 +300,9 @@
 #endif  // BUILDFLAG(IS_MAC)
 
   bool IsPartialViewEnabled() {
+    // TODO(chlily): This is now solely a function of Profile prefs. Add
+    // explicit coverage for the pref being enabled/disabled, and simplify the
+    // rest of the tests by assuming the pref's default value.
     return download::IsDownloadBubblePartialViewEnabled(browser()->profile());
   }
 
@@ -587,4 +590,28 @@
       WaitForState(kDownloadsButtonVisible, false));
 }
 
+IN_PROC_BROWSER_TEST_F(DownloadBubbleInteractiveUiTest,
+                       ClosePartialBubbleOnEscKeypress) {
+  RunTestSequence(
+      // Download a test file so that the partial view shows up.
+      Do(DownloadTestFile()),
+      ObserveState(kDownloadsButtonVisible, GetContainerView()),
+      WaitForState(kDownloadsButtonVisible, true),
+      Check(DownloadBubbleIsShowingDetails(IsPartialViewEnabled()),
+            "Partial view shows after download, if enabled."),
+      If([&] { return IsPartialViewEnabled(); },
+         // The bubble, if enabled, should be shown as inactive to avoid
+         // stealing focus from the page.
+         Then(Check(DownloadBubbleIsActive(false),
+                    "Partial view, if enabled, is inactive."))),
+      SendKeyPress(kBrowserViewElementId, ui::VKEY_ESCAPE),
+      EnsureNotPresent(kToolbarDownloadBubbleElementId),
+      Check(DownloadBubbleIsShowingDetails(false),
+            "Inactive bubble was closed"),
+      // Hide the bubble so it's not showing while tearing down the
+      // test browser (which causes a crash on Mac).
+      Do(ChangeBubbleVisibility(false)), Do(ChangeButtonVisibility(false)),
+      WaitForState(kDownloadsButtonVisible, false));
+}
+
 }  // namespace
diff --git a/chrome/browser/ui/views/download/bubble/download_toolbar_ui_controller.cc b/chrome/browser/ui/views/download/bubble/download_toolbar_ui_controller.cc
index b84bff1..8435b37 100644
--- a/chrome/browser/ui/views/download/bubble/download_toolbar_ui_controller.cc
+++ b/chrome/browser/ui/views/download/bubble/download_toolbar_ui_controller.cc
@@ -780,20 +780,7 @@
   return weak_factory_.GetWeakPtr();
 }
 
-// If the browser was inactive when the bubble was shown, then the bubble would
-// be inactive. This would prevent close-on-deactivate, making the bubble
-// unclosable. To work around this, we activate the bubble when the current
-// browser becomes active, so that clicking outside the bubble will deactivate
-// and close it.
 void DownloadToolbarUIController::OnBrowserSetLastActive(Browser* browser) {
-  if (browser_view_ && browser == browser_view_->browser() &&
-      bubble_delegate_ && !bubble_delegate_->GetWidget()->IsClosed()) {
-    // We need to defer activating the download bubble when the browser window
-    // is being activated, otherwise this is ineffective on macOS.
-    content::GetUIThreadTaskRunner()->PostTask(
-        FROM_HERE, base::BindOnce(&views::Widget::Activate,
-                                  bubble_delegate_->GetWidget()->GetWeakPtr()));
-  }
   UpdateIconDormant();
 }
 
@@ -865,9 +852,12 @@
 
 DownloadToolbarUIController::BubbleCloser::BubbleCloser(
     views::Button* toolbar_button,
+    views::Widget* bubble_widget,
     base::WeakPtr<DownloadDisplay> download_display)
     : download_display_(download_display) {
   CHECK(toolbar_button);
+  CHECK(bubble_widget);
+  bubble_widget_observation_.Observe(bubble_widget);
   if (toolbar_button->GetWidget() &&
       toolbar_button->GetWidget()->GetTopLevelWidget()->GetNativeWindow()) {
     event_monitor_ = views::EventMonitor::CreateWindowMonitor(
@@ -882,6 +872,14 @@
 
 void DownloadToolbarUIController::BubbleCloser::OnEvent(
     const ui::Event& event) {
+  // If the bubble widget has become active in the meantime (since starting as
+  // inactive), we should do nothing and defer to the close-on-deactivate
+  // behavior from BubbleDialogDelegate which is in effect when the bubble is
+  // active.
+  if (bubble_widget_observation_.IsObserving() &&
+      bubble_widget_observation_.GetSource()->IsActive()) {
+    return;
+  }
   CHECK(event_monitor_);
   if (event.IsKeyEvent() && event.AsKeyEvent()->key_code() != ui::VKEY_ESCAPE) {
     return;
@@ -893,6 +891,13 @@
   // `this` will be deleted.
 }
 
+void DownloadToolbarUIController::BubbleCloser::OnWidgetDestroyed(
+    views::Widget* widget) {
+  if (bubble_widget_observation_.IsObservingSource(widget)) {
+    bubble_widget_observation_.Reset();
+  }
+}
+
 void DownloadToolbarUIController::CreateBubbleDialogDelegate() {
   std::vector<DownloadUIModel::DownloadUIModelPtr> primary_view_models =
       GetPrimaryViewModels();
@@ -945,14 +950,15 @@
   bubble_delegate->set_margins(GetPrimaryViewMargin());
   bubble_delegate->SetEnableArrowKeyTraversal(true);
   bubble_delegate_ = bubble_delegate.get();
-  views::BubbleDialogDelegate::CreateBubble(std::move(bubble_delegate));
+  views::Widget* bubble_widget =
+      views::BubbleDialogDelegate::CreateBubble(std::move(bubble_delegate));
+  CHECK(bubble_widget);
 
   if (!is_primary_partial_view_ && !button_click_time_.is_null()) {
     // If the main view was shown after clicking on the toolbar button,
     // record the time from click to shown. (The main view can be shown without
     // clicking the toolbar button, e.g. from clicking on a notification.)
-    bubble_delegate_->GetWidget()
-        ->GetCompositor()
+    bubble_widget->GetCompositor()
         ->RequestSuccessfulPresentationTimeForNextFrame(base::BindOnce(
             [](base::TimeTicks click_time,
                const viz::FrameTimingDetails& frame_timing_details) {
@@ -969,18 +975,14 @@
 
   CloseAutofillPopup();
   if (ShouldShowBubbleAsInactive()) {
-    if (button) {
-      bubble_delegate_->GetWidget()->ShowInactive();
-      bubble_closer_ =
-          std::make_unique<BubbleCloser>(button, weak_factory_.GetWeakPtr());
-      bubble_delegate_->GetWidget()
-          ->GetRootView()
-          ->GetViewAccessibility()
-          .AnnounceText(
-              l10n_util::GetStringUTF16(IDS_SHOW_BUBBLE_INACTIVE_DESCRIPTION));
-    }
+    CHECK(button);
+    bubble_widget->ShowInactive();
+    bubble_closer_ = std::make_unique<BubbleCloser>(button, bubble_widget,
+                                                    weak_factory_.GetWeakPtr());
+    bubble_widget->GetRootView()->GetViewAccessibility().AnnounceText(
+        l10n_util::GetStringUTF16(IDS_SHOW_BUBBLE_INACTIVE_DESCRIPTION));
   } else {
-    bubble_delegate_->GetWidget()->Show();
+    bubble_widget->Show();
   }
 
   action_item_->SetIsShowingBubble(true);
diff --git a/chrome/browser/ui/views/download/bubble/download_toolbar_ui_controller.h b/chrome/browser/ui/views/download/bubble/download_toolbar_ui_controller.h
index 2cdb711..a10586e 100644
--- a/chrome/browser/ui/views/download/bubble/download_toolbar_ui_controller.h
+++ b/chrome/browser/ui/views/download/bubble/download_toolbar_ui_controller.h
@@ -21,6 +21,7 @@
 #include "ui/base/metadata/metadata_header_macros.h"
 #include "ui/events/event_observer.h"
 #include "ui/views/bubble/bubble_dialog_delegate_view.h"
+#include "ui/views/widget/widget_observer.h"
 
 namespace offline_items_collection {
 struct ContentId;
@@ -28,9 +29,9 @@

diff --git a/chrome/browser/ui/views/download/bubble/download_bubble_interactive_uitest.cc b/chrome/browser/ui/views/download/bubble/download_bubble_interactive_uitest.cc
index 1378bcc3..7d4c86a 100644
--- a/chrome/browser/ui/views/download/bubble/download_bubble_interactive_uitest.cc
+++ b/chrome/browser/ui/views/download/bubble/download_bubble_interactive_uitest.cc
@@ -300,6 +300,9 @@
 #endif  // BUILDFLAG(IS_MAC)
 
   bool IsPartialViewEnabled() {
+    // TODO(chlily): This is now solely a function of Profile prefs. Add
+    // explicit coverage for the pref being enabled/disabled, and simplify the
+    // rest of the tests by assuming the pref's default value.
     return download::IsDownloadBubblePartialViewEnabled(browser()->profile());
   }
 
@@ -587,4 +590,28 @@
       WaitForState(kDownloadsButtonVisible, false));
 }
 
+IN_PROC_BROWSER_TEST_F(DownloadBubbleInteractiveUiTest,
+                       ClosePartialBubbleOnEscKeypress) {
+  RunTestSequence(
+      // Download a test file so that the partial view shows up.
+      Do(DownloadTestFile()),
+      ObserveState(kDownloadsButtonVisible, GetContainerView()),
+      WaitForState(kDownloadsButtonVisible, true),
+      Check(DownloadBubbleIsShowingDetails(IsPartialViewEnabled()),
+            "Partial view shows after download, if enabled."),
+      If([&] { return IsPartialViewEnabled(); },
+         // The bubble, if enabled, should be shown as inactive to avoid
+         // stealing focus from the page.
+         Then(Check(DownloadBubbleIsActive(false),
+                    "Partial view, if enabled, is inactive."))),
+      SendKeyPress(kBrowserViewElementId, ui::VKEY_ESCAPE),
+      EnsureNotPresent(kToolbarDownloadBubbleElementId),
+      Check(DownloadBubbleIsShowingDetails(false),
+            "Inactive bubble was closed"),
+      // Hide the bubble so it's not showing while tearing down the
+      // test browser (which causes a crash on Mac).
+      Do(ChangeBubbleVisibility(false)), Do(ChangeButtonVisibility(false)),
+      WaitForState(kDownloadsButtonVisible, false));
+}
+
 }  // namespace