[User Education] Allow multiple annotations on a menu

Help bubbles attached to menus are considered "annotations" and handled
by a special event-forwarding system to allow closing the help bubble
via mouse when the menu is showing (otherwise, the menu would lose focus
and disappear).

This was implemented assuming only a single help bubble attached to any
given menu, but with tutorials that can attach to submenus, there can
briefly be two simultaneous help bubbles, breaking this assumption.

Now menus may have any number of annotations; events will be sent to
each in turn, and the first one that reports that it handles the event
will take precedence.

This CL mostly changes from using a WeakAutoReset to a
CallbackListSubscription, which is a more natural return value for
adding an annotation callback anyway. A test is added to make sure that
the double-bubble actually works.

Change-Id: I5e49ae283832957f9e969a8266de07bea8096563
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4713887
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Commit-Queue: Dana Fried <dfried@chromium.org>
Reviewed-by: Mickey Burks <mickeyburks@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1174895}

chrome/browser/ui/views/user_education/help_bubble_view_interactive_uitest.cc

@@ -51,7 +51,8 @@ class HelpBubbleViewInteractiveUiTest : public InteractiveBrowserTest {
                    new HelpBubbleView(GetHelpBubbleDelegate(), {anchor},
                                       std::move(params));
                  }),
-        WaitForShow(HelpBubbleView::kHelpBubbleElementIdForTesting),
+        std::move(WaitForShow(HelpBubbleView::kHelpBubbleElementIdForTesting)
+                      .SetTransitionOnlyOnEvent(true)),
         // Prevent direct chaining off the show event.
         FlushEvents());
   }
@@ -61,7 +62,8 @@ class HelpBubbleViewInteractiveUiTest : public InteractiveBrowserTest {
     return Steps(
         WithView(HelpBubbleView::kHelpBubbleElementIdForTesting,
                  [](HelpBubbleView* bubble) { bubble->GetWidget()->Close(); }),
-        WaitForHide(HelpBubbleView::kHelpBubbleElementIdForTesting),
+        std::move(WaitForHide(HelpBubbleView::kHelpBubbleElementIdForTesting)
+                      .SetTransitionOnlyOnEvent(true)),
         // Prevent direct chaining off the hide event.
         FlushEvents());
   }
@@ -219,4 +221,48 @@ IN_PROC_BROWSER_TEST_F(HelpBubbleViewInteractiveUiTest, AnnotateMenu) {
       ClickMouse(), WaitForHide(HelpBubbleView::kHelpBubbleElementIdForTesting),
       EnsurePresent(AppMenuModel::kDownloadsMenuItem));
 }
+
+// Verifies that we can safely show and then close two help bubbles attached to
+// the same menu. This may happen transiently during tutorials.
+IN_PROC_BROWSER_TEST_F(HelpBubbleViewInteractiveUiTest, TwoMenuHelpBubbles) {
+  UNCALLED_MOCK_CALLBACK(base::OnceClosure, button_clicked);
+  constexpr char16_t kButtonText[] = u"button";
+
+  // First bubble has no buttons.
+  auto params1 = GetBubbleParams();
+  params1.arrow = user_education::HelpBubbleArrow::kRightCenter;
+
+  // Second bubble has a default button.
+  auto params2 = GetBubbleParams();
+  params2.arrow = user_education::HelpBubbleArrow::kRightCenter;
+
+  user_education::HelpBubbleButtonParams button;
+  button.text = kButtonText;
+  button.is_default = true;
+  button.callback = button_clicked.Get();
+  params2.buttons.emplace_back(std::move(button));
+
+  EXPECT_CALL(button_clicked, Run).Times(1);
+
+  RunTestSequence(
+      // Show the application menu and attach a bubble to two different menu
+      // items.
+      PressButton(kAppMenuButtonElementId),
+      ShowHelpBubble(AppMenuModel::kDownloadsMenuItem, std::move(params1)),
+      ShowHelpBubble(AppMenuModel::kMoreToolsMenuItem, std::move(params2)),
+
+      // Use the mouse to click the default button on the second bubble and wait
+      // for the bubble to disappear.
+      //
+      // The default button should be targetable because it is at the bottom of
+      // the lower of the two help bubbles.
+      MoveMouseTo(HelpBubbleView::kDefaultButtonIdForTesting), ClickMouse(),
+      WaitForHide(HelpBubbleView::kHelpBubbleElementIdForTesting)
+          .SetTransitionOnlyOnEvent(true),
+      FlushEvents(),
+
+      // Close the remaining help bubble.
+      CloseHelpBubble());
+}
+
 #endif

components/user_education/views/help_bubble_view.cc

@@ -10,6 +10,7 @@
 #include <string>
 #include <utility>
 
+#include "base/callback_list.h"
 #include "base/functional/bind.h"
 #include "base/memory/raw_ptr.h"
 #include "base/metrics/user_metrics.h"
@@ -319,7 +320,7 @@ class MenuEventMonitor {
  public:
   MenuEventMonitor(HelpBubbleView* help_bubble, views::MenuItemView* menu_item)
       : help_bubble_(help_bubble),
-        callback_handle_(menu_item->GetMenuController()->SetAnnotationCallback(
+        callback_handle_(menu_item->GetMenuController()->AddAnnotationCallback(
             base::BindRepeating(&MenuEventMonitor::OnEvent,
                                 base::Unretained(this)))) {}
 
@@ -454,7 +455,7 @@ class MenuEventMonitor {
   const raw_ptr<HelpBubbleView> help_bubble_;
   raw_ptr<views::Button> hovered_button_ = nullptr;
   // std::unique_ptr<views::EventMonitor> event_monitor_;
-  views::MenuController::AnnotationCallbackHandle callback_handle_;
+  base::CallbackListSubscription callback_handle_;
 };
 
 }  // namespace internal

ui/views/controls/menu/menu_controller.cc

@@ -8,6 +8,7 @@
 #include <set>
 #include <utility>
 
+#include "base/callback_list.h"
 #include "base/containers/flat_set.h"
 #include "base/functional/bind.h"
 #include "base/i18n/case_conversion.h"
@@ -3504,28 +3505,30 @@ void MenuController::SetAnchorParametersForItem(MenuItemView* item,
   }
 }
 
-MenuController::AnnotationCallbackHandle MenuController::SetAnnotationCallback(
+base::CallbackListSubscription MenuController::AddAnnotationCallback(
     AnnotationCallback callback) {
-  // TODO(dfried): change this so multiple annotations can be supported.
-  // This check avoids a potential race/UAF situation.
-  CHECK(!annotation_callback_)
-      << "Only one annotation callback allowed at a time.";
-  return AnnotationCallbackHandle(
-      AsWeakPtr(), &MenuController::annotation_callback_, callback);
+  return annotation_callbacks_.Add(base::BindRepeating(
+      [](AnnotationCallback callback, bool& result,
+         const ui::LocatedEvent& event) {
+        if (result) {
+          // A different annotation has already handled this event.
+          return;
+        }
+        result = callback.Run(event);
+      },
+      std::move(callback)));
 }
 
 bool MenuController::MaybeForwardToAnnotation(SubmenuView* source,
                                               const ui::LocatedEvent& event) {
-  if (!annotation_callback_) {
-    return false;
-  }
-
   const std::unique_ptr<ui::Event> cloned = event.Clone();
   auto* located = static_cast<ui::LocatedEvent*>(cloned.get());
   const gfx::Point screen_loc = View::ConvertPointToScreen(
       source->GetScrollViewContainer(), event.location());
   located->set_root_location(screen_loc);
-  return annotation_callback_.Run(*located);
+  bool result = false;
+  annotation_callbacks_.Notify(result, *located);
+  return result;
 }
 
 bool MenuController::CanProcessInputEvents() const {

ui/views/controls/menu/menu_controller.h

@@ -13,10 +13,10 @@
 #include <utility>
 #include <vector>
 
+#include "base/callback_list.h"
 #include "base/containers/flat_set.h"
 #include "base/functional/callback_forward.h"
 #include "base/memory/raw_ptr.h"
-#include "base/memory/weak_auto_reset.h"
 #include "base/memory/weak_ptr.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
@@ -123,8 +123,6 @@ class VIEWS_EXPORT MenuController
   // processed by the menu (except for purposes of e.g. hot-tracking).
   using AnnotationCallback =
       base::RepeatingCallback<bool(const ui::LocatedEvent& event)>;
-  using AnnotationCallbackHandle =
-      base::WeakAutoReset<MenuController, AnnotationCallback>;
 
   // If a menu is currently active, this returns the controller for it.
   static MenuController* GetActiveInstance();
@@ -289,10 +287,11 @@ class VIEWS_EXPORT MenuController
   // Sets the customized rounded corners of the context menu.
   void SetMenuRoundedCorners(absl::optional<gfx::RoundedCornersF> corners);
 
-  // Sets the annotation event handler. The handle should be discarded when the
-  // calling code no longer wants to intercept events for the annotation. It is
-  // safe to discard the handle after the menu controller has been destroyed.
-  AnnotationCallbackHandle SetAnnotationCallback(AnnotationCallback callback);
+  // Adds an annotation event handler. The subscription should be discarded when
+  // the calling code no longer wants to intercept events for the annotation. It
+  // is safe to discard the handle after the menu controller has been destroyed.
+  base::CallbackListSubscription AddAnnotationCallback(
+      AnnotationCallback callback);
 
  private:
   friend class internal::MenuRunnerImpl;
@@ -819,9 +818,11 @@ class VIEWS_EXPORT MenuController
   // The rounded corners of the context menu.
   absl::optional<gfx::RoundedCornersF> rounded_corners_ = absl::nullopt;
 
-  // The current annotation callback. Set if there is a menu annotation; see
-  // `AnnotationCallback` for more information.
-  AnnotationCallback annotation_callback_;
+  // The current annotation callbacks. Callbacks will be wrapped in such a way
+  // that a callback list can be used, with the return value as an out
+  // parameter. See `AnnotationCallback` for more information.
+  base::RepeatingCallbackList<void(bool&, const ui::LocatedEvent& event)>
+      annotation_callbacks_;
 };
 
 }  // namespace views