From 6d86d50a2c19937cba6949c28dd2951b6ccc665a Mon Sep 17 00:00:00 2001
From: Yaw Frempong <yawfrempong@google.com>
Date: Mon, 10 Aug 2026 21:44:42 -0700
Subject: [PATCH] [Download Bubble] Disable subpage dialog buttons when occluded

DownloadBubbleSecurityView wires its primary/secondary subpage buttons
onto the bubble's DialogClientView OK/Cancel buttons. The earlier change
that added picture-in-picture occlusion tracking to the download bubble
only disabled the row-view controls; the dialog buttons used by the
subpage were left enabled while the bubble is occluded.

Mirror the row-view pattern: make DownloadBubbleSecurityView a
PictureInPictureOcclusionObserver, observe the bubble widget in
AddedToWidget(), and disable the dialog OK/Cancel buttons via
DialogDelegate::SetButtonEnabled() while occluded. Also consult the
occlusion state in UpdateButton() so re-populating the buttons does not
re-enable them.

Reviewed in https://crrev.com/i/9596093

Bug: 514009699
Change-Id: I7b01408059e9de9c8cc26bb0fa2f1e1a6c8e91f3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8170820
Reviewed-by: Brian Lefler <bcl@google.com>
Reviewed-by: Lily Chen <chlily@chromium.org>
Commit-Queue: Yaw Frempong <yawfrempong@google.com>
Cr-Commit-Position: refs/heads/main@{#1676911}
---

diff --git a/chrome/browser/ui/views/download/bubble/download_bubble_security_view.cc b/chrome/browser/ui/views/download/bubble/download_bubble_security_view.cc
index 6270c8d..0853bdda 100644
--- a/chrome/browser/ui/views/download/bubble/download_bubble_security_view.cc
+++ b/chrome/browser/ui/views/download/bubble/download_bubble_security_view.cc
@@ -537,9 +537,10 @@
       &HandleButtonClickWithDefaultClose, weak_factory_.GetWeakPtr(),
       button_info.command, is_secondary_button));
 
+  bubble_delegate_->SetButtonEnabled(button_type, !occluded_);
+
   if (button_type == ui::mojom::DialogButton::kCancel) {
     bubble_delegate_->SetCancelCallbackWithClose(callback);
-    bubble_delegate_->SetButtonEnabled(button_type, true);
     views::LabelButton* button = bubble_delegate_->GetCancelButton();
     if (button_info.text_color) {
       button->SetEnabledTextColors(*button_info.text_color);
@@ -688,6 +689,25 @@
 
 DownloadBubbleSecurityView::~DownloadBubbleSecurityView() = default;
 
+void DownloadBubbleSecurityView::AddedToWidget() {
+  views::Widget* widget = GetWidget();
+  pip_occlusion_observation_.Observe(widget);
+}
+
+void DownloadBubbleSecurityView::OnOcclusionStateChanged(bool occluded) {
+  if (occluded_ == occluded) {
+    return;
+  }
+
+  // If transitioning from occluded to un-occluded, restart the input protection
+  // timer to prevent clickjacking/unintended clicks.
+  if (occluded_ && !occluded && bubble_delegate_) {
+    bubble_delegate_->TriggerInputProtection();
+  }
+  occluded_ = occluded;
+  UpdateButtons();
+}
+
 int DownloadBubbleSecurityView::GetMinimumBubbleWidth() const {
   return ChromeLayoutProvider::Get()->GetSnappedDialogWidth(
       bubble_delegate_->GetDialogClientView()->GetMinimumSize().width());
diff --git a/chrome/browser/ui/views/download/bubble/download_bubble_security_view.h b/chrome/browser/ui/views/download/bubble/download_bubble_security_view.h
index 65b4902..62ffc349 100644
--- a/chrome/browser/ui/views/download/bubble/download_bubble_security_view.h
+++ b/chrome/browser/ui/views/download/bubble/download_bubble_security_view.h
@@ -14,6 +14,8 @@
 #include "base/types/optional_ref.h"
 #include "chrome/browser/download/download_item_warning_data.h"
 #include "chrome/browser/download/download_ui_model.h"
+#include "chrome/browser/picture_in_picture/picture_in_picture_occlusion_observer.h"
+#include "chrome/browser/picture_in_picture/scoped_picture_in_picture_occlusion_observation.h"
 #include "chrome/browser/ui/download/download_bubble_security_view_info.h"
 #include "components/download/public/common/download_danger_type.h"
 #include "components/offline_items_collection/core/offline_item.h"
@@ -34,6 +36,7 @@
 
 class DownloadBubbleSecurityView
     : public views::View,
+      public PictureInPictureOcclusionObserver,
       public DownloadBubbleSecurityViewInfoObserver {
   METADATA_HEADER(DownloadBubbleSecurityView, views::View)
 
@@ -90,6 +93,12 @@
       delete;
   ~DownloadBubbleSecurityView() override;
 
+  // views::View:
+  void AddedToWidget() override;
+
+  // PictureInPictureOcclusionObserver:
+  void OnOcclusionStateChanged(bool occluded) override;
+
   // Whether this view is properly associated with a download. The rest of the
   // public method calls on this view do not make sense if not initialized.
   bool IsInitialized() const;
@@ -184,6 +193,12 @@
   // double-logging.
   bool did_log_action_ = false;
 
+  // Whether the bubble widget is currently occluded by a picture-in-picture
+  // window. The dialog buttons are disabled while occluded.
+  bool occluded_ = false;
+
+  ScopedPictureInPictureOcclusionObservation pip_occlusion_observation_{this};
+
   base::WeakPtrFactory<DownloadBubbleSecurityView> weak_factory_{this};
 };
 
diff --git a/chrome/browser/ui/views/download/bubble/download_bubble_security_view_unittest.cc b/chrome/browser/ui/views/download/bubble/download_bubble_security_view_unittest.cc
index c1ecf56..6553966 100644
--- a/chrome/browser/ui/views/download/bubble/download_bubble_security_view_unittest.cc
+++ b/chrome/browser/ui/views/download/bubble/download_bubble_security_view_unittest.cc
@@ -14,6 +14,8 @@
 #include "chrome/browser/download/download_ui_model.h"
 #include "chrome/browser/download/mock_download_core_service.h"
 #include "chrome/browser/download/offline_item_utils.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/ui/download/download_bubble_security_view_info.h"
 #include "chrome/browser/ui/views/chrome_layout_provider.h"
 #include "chrome/browser/ui/views/download/bubble/download_bubble_contents_view.h"
@@ -31,6 +33,7 @@
 #include "ui/base/mojom/dialog_button.mojom.h"
 #include "ui/color/color_id.h"
 #include "ui/views/bubble/bubble_dialog_delegate_view.h"
+#include "ui/views/controls/button/md_text_button.h"
 #include "ui/views/view.h"
 #include "ui/views/window/dialog_client_view.h"
 
@@ -166,15 +169,17 @@
     bubble_delegate_ = bubble_delegate.get();
     bubble_navigator_ = std::make_unique<MockDownloadBubbleNavigationHandler>(
         *security_view_info_);
-    views::BubbleDialogDelegate::CreateBubbleDeprecated(
-        std::move(bubble_delegate),
-        views::Widget::InitParams::NATIVE_WIDGET_OWNS_WIDGET);
-    bubble_delegate_->GetWidget()->Show();
+
     security_view_ = bubble_delegate_->SetContentsView(
         std::make_unique<DownloadBubbleSecurityView>(
             security_view_delegate_.get(), *security_view_info_,
             bubble_navigator_->GetWeakPtr(), bubble_delegate_));
 
+    views::BubbleDialogDelegate::CreateBubbleDeprecated(
+        std::move(bubble_delegate),
+        views::Widget::InitParams::NATIVE_WIDGET_OWNS_WIDGET);
+    bubble_delegate_->GetWidget()->Show();
+
     DownloadCoreServiceFactory::GetInstance()->SetTestingFactory(
         profile_, base::BindRepeating(&BuildMockDownloadCoreService));
     MockDownloadCoreService* mock_dcs = static_cast<MockDownloadCoreService*>(
@@ -315,6 +320,46 @@
             static_cast<int>(ui::mojom::DialogButton::kNone));
 }
 
+TEST_F(DownloadBubbleSecurityViewTest,
+       DialogButtonsDisabledWhenOccludedByPictureInPicture) {
+  security_view_info_->InitializeForDownload(*row1_model_);
+  security_view_info_->SetSubpageButtonsForTesting(
+      {SubpageButton(DownloadCommands::Command::DISCARD, std::u16string(),
+                     /*is_prominent=*/true),
+       SubpageButton(DownloadCommands::Command::KEEP, std::u16string(),
+                     /*is_prominent=*/false, ui::kColorAlertHighSeverity)});
+
+  ASSERT_NE(nullptr, bubble_delegate_->GetOkButton());
+  ASSERT_NE(nullptr, bubble_delegate_->GetCancelButton());
+  EXPECT_TRUE(bubble_delegate_->GetOkButton()->GetEnabled());
+  EXPECT_TRUE(bubble_delegate_->GetCancelButton()->GetEnabled());
+
+  PictureInPictureOcclusionTracker* tracker =
+      PictureInPictureWindowManager::GetInstance()->GetOcclusionTracker();
+  ASSERT_NE(nullptr, tracker);
+  tracker->SetWidgetOcclusionStateForTesting(bubble_delegate_->GetWidget(),
+                                             /*occluded=*/true);
+
+  EXPECT_FALSE(bubble_delegate_->GetOkButton()->GetEnabled());
+  EXPECT_FALSE(bubble_delegate_->GetCancelButton()->GetEnabled());
+
+  // Re-initializing the view while occluded must not re-enable the buttons.
+  security_view_info_->InitializeForDownload(*row1_model_);
+  security_view_info_->SetSubpageButtonsForTesting(
+      {SubpageButton(DownloadCommands::Command::DISCARD, std::u16string(),
+                     /*is_prominent=*/true),
+       SubpageButton(DownloadCommands::Command::KEEP, std::u16string(),
+                     /*is_prominent=*/false, ui::kColorAlertHighSeverity)});
+  EXPECT_FALSE(bubble_delegate_->GetOkButton()->GetEnabled());
+  EXPECT_FALSE(bubble_delegate_->GetCancelButton()->GetEnabled());
+
+  tracker->SetWidgetOcclusionStateForTesting(bubble_delegate_->GetWidget(),
+                                             /*occluded=*/false);

diff --git a/chrome/browser/ui/views/download/bubble/download_bubble_security_view_unittest.cc b/chrome/browser/ui/views/download/bubble/download_bubble_security_view_unittest.cc
index c1ecf56..6553966 100644
--- a/chrome/browser/ui/views/download/bubble/download_bubble_security_view_unittest.cc
+++ b/chrome/browser/ui/views/download/bubble/download_bubble_security_view_unittest.cc
@@ -14,6 +14,8 @@
 #include "chrome/browser/download/download_ui_model.h"
 #include "chrome/browser/download/mock_download_core_service.h"
 #include "chrome/browser/download/offline_item_utils.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/ui/download/download_bubble_security_view_info.h"
 #include "chrome/browser/ui/views/chrome_layout_provider.h"
 #include "chrome/browser/ui/views/download/bubble/download_bubble_contents_view.h"
@@ -31,6 +33,7 @@
 #include "ui/base/mojom/dialog_button.mojom.h"
 #include "ui/color/color_id.h"
 #include "ui/views/bubble/bubble_dialog_delegate_view.h"
+#include "ui/views/controls/button/md_text_button.h"
 #include "ui/views/view.h"
 #include "ui/views/window/dialog_client_view.h"
 
@@ -166,15 +169,17 @@
     bubble_delegate_ = bubble_delegate.get();
     bubble_navigator_ = std::make_unique<MockDownloadBubbleNavigationHandler>(
         *security_view_info_);
-    views::BubbleDialogDelegate::CreateBubbleDeprecated(
-        std::move(bubble_delegate),
-        views::Widget::InitParams::NATIVE_WIDGET_OWNS_WIDGET);
-    bubble_delegate_->GetWidget()->Show();
+
     security_view_ = bubble_delegate_->SetContentsView(
         std::make_unique<DownloadBubbleSecurityView>(
             security_view_delegate_.get(), *security_view_info_,
             bubble_navigator_->GetWeakPtr(), bubble_delegate_));
 
+    views::BubbleDialogDelegate::CreateBubbleDeprecated(
+        std::move(bubble_delegate),
+        views::Widget::InitParams::NATIVE_WIDGET_OWNS_WIDGET);
+    bubble_delegate_->GetWidget()->Show();
+
     DownloadCoreServiceFactory::GetInstance()->SetTestingFactory(
         profile_, base::BindRepeating(&BuildMockDownloadCoreService));
     MockDownloadCoreService* mock_dcs = static_cast<MockDownloadCoreService*>(
@@ -315,6 +320,46 @@
             static_cast<int>(ui::mojom::DialogButton::kNone));
 }
 
+TEST_F(DownloadBubbleSecurityViewTest,
+       DialogButtonsDisabledWhenOccludedByPictureInPicture) {
+  security_view_info_->InitializeForDownload(*row1_model_);
+  security_view_info_->SetSubpageButtonsForTesting(
+      {SubpageButton(DownloadCommands::Command::DISCARD, std::u16string(),
+                     /*is_prominent=*/true),
+       SubpageButton(DownloadCommands::Command::KEEP, std::u16string(),
+                     /*is_prominent=*/false, ui::kColorAlertHighSeverity)});
+
+  ASSERT_NE(nullptr, bubble_delegate_->GetOkButton());
+  ASSERT_NE(nullptr, bubble_delegate_->GetCancelButton());
+  EXPECT_TRUE(bubble_delegate_->GetOkButton()->GetEnabled());
+  EXPECT_TRUE(bubble_delegate_->GetCancelButton()->GetEnabled());
+
+  PictureInPictureOcclusionTracker* tracker =
+      PictureInPictureWindowManager::GetInstance()->GetOcclusionTracker();
+  ASSERT_NE(nullptr, tracker);
+  tracker->SetWidgetOcclusionStateForTesting(bubble_delegate_->GetWidget(),
+                                             /*occluded=*/true);
+
+  EXPECT_FALSE(bubble_delegate_->GetOkButton()->GetEnabled());
+  EXPECT_FALSE(bubble_delegate_->GetCancelButton()->GetEnabled());
+
+  // Re-initializing the view while occluded must not re-enable the buttons.
+  security_view_info_->InitializeForDownload(*row1_model_);
+  security_view_info_->SetSubpageButtonsForTesting(
+      {SubpageButton(DownloadCommands::Command::DISCARD, std::u16string(),
+                     /*is_prominent=*/true),
+       SubpageButton(DownloadCommands::Command::KEEP, std::u16string(),
+                     /*is_prominent=*/false, ui::kColorAlertHighSeverity)});
+  EXPECT_FALSE(bubble_delegate_->GetOkButton()->GetEnabled());
+  EXPECT_FALSE(bubble_delegate_->GetCancelButton()->GetEnabled());
+
+  tracker->SetWidgetOcclusionStateForTesting(bubble_delegate_->GetWidget(),
+                                             /*occluded=*/false);
+
+  EXPECT_TRUE(bubble_delegate_->GetOkButton()->GetEnabled());
+  EXPECT_TRUE(bubble_delegate_->GetCancelButton()->GetEnabled());
+}
+
 TEST_F(DownloadBubbleSecurityViewTest, VerifyLogWarningActions) {
   DownloadItemWarningData::AddWarningActionEvent(
       &download_item1_, WarningSurface::BUBBLE_MAINPAGE, WarningAction::SHOWN);
@@ -501,7 +546,8 @@
   security_view_info_->InitializeForDownload(*row1_model_);
   security_view_info_->SetSubpageButtonsForTesting({SubpageButton(
       DownloadCommands::Command::DISCARD,
-      std::u16string(u"really really really really really really long "
+      std::u16string(u"really really really really really really really "
+                     u"really really really really really really long "
                      u"button text"),
       /*is_prominent=*/true)});
   UpdateView();