views: activate menus when they are opened by keyboard

This change causes menu widgets to be become the active widget if the
menu was shown by keyboard. This ensures that the menu widget is
announced by screenreaders. There is one special case: editable
combobox menus, even if opened by keyboard, never activate since that
would pull keyboard focus away from the editable combobox itself.

To support that, this change also:
* Adds Widget::Predicate, a type for predicates that accept a Widget;
* Adds support to MenuCocoaWatcherMac for ignoring certain widget
  activations;
* Adds an IsNotMenuWidget predicate to MenuController for use with
  MenuCocoaWatcherMac

Bug: 1097769
Change-Id: I4f5289d66428f97e75c38e4e887b8ce9061dada8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2520141
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Leonard Grey <lgrey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#824630}

ui/views/controls/menu/menu_cocoa_watcher_mac.h

@@ -10,6 +10,7 @@
 #include "base/callback.h"
 #include "base/macros.h"
 #include "ui/views/views_export.h"
+#include "ui/views/widget/widget.h"
 
 namespace views {
 
@@ -20,12 +21,17 @@ namespace views {
 // correct behavior.
 class VIEWS_EXPORT MenuCocoaWatcherMac {
  public:
-  explicit MenuCocoaWatcherMac(base::OnceClosure callback);
+  // For window activation changes, the callback is only invoked if
+  // |activation_is_interesting| returns true on the involved Widget (which may
+  // be nullptr).
+  explicit MenuCocoaWatcherMac(Widget::Predicate activation_is_interesting,
+                               base::OnceClosure callback);
   ~MenuCocoaWatcherMac();
 
  private:
   void ExecuteCallback();
 
+  Widget::Predicate activation_is_interesting_;
   // The closure to call when the notification comes in.
   base::OnceClosure callback_;
 

ui/views/controls/menu/menu_cocoa_watcher_mac.mm

@@ -11,8 +11,11 @@
 
 namespace views {
 
-MenuCocoaWatcherMac::MenuCocoaWatcherMac(base::OnceClosure callback)
-    : callback_(std::move(callback)) {
+MenuCocoaWatcherMac::MenuCocoaWatcherMac(
+    Widget::Predicate activation_is_interesting,
+    base::OnceClosure callback)
+    : activation_is_interesting_(activation_is_interesting),
+      callback_(std::move(callback)) {
   observer_token_other_menu_ = [[NSNotificationCenter defaultCenter]
       addObserverForName:NSMenuDidBeginTrackingNotification
                   object:nil
@@ -25,7 +28,11 @@ MenuCocoaWatcherMac::MenuCocoaWatcherMac(base::OnceClosure callback)
                   object:nil
                    queue:nil
               usingBlock:^(NSNotification* notification) {
-                ExecuteCallback();
+                Widget* widget =
+                    Widget::GetWidgetForNativeWindow([NSApp keyWindow]);
+                if (activation_is_interesting_.Run(widget)) {
+                  ExecuteCallback();
+                }
               }];
   observer_token_app_change_ =
       [[[NSWorkspace sharedWorkspace] notificationCenter]

ui/views/controls/menu/menu_controller.cc

@@ -74,7 +74,7 @@ namespace views {
 
 namespace {
 
-#if defined(OS_APPLE)
+#if defined(OS_MAC)
 bool AcceleratorShouldCancelMenu(const ui::Accelerator& accelerator) {
   // Since AcceleratorShouldCancelMenu() is called quite early in key
   // event handling, it is actually invoked for modifier keys themselves
@@ -339,6 +339,31 @@ static void RepostEventImpl(const ui::LocatedEvent* event,
 }
 #endif  // defined(OS_WIN)
 
+#if defined(OS_MAC)
+// Note: this just checks whether |widget| is *a* menu widget, not whether
+// it belongs to |weak_controller|. The only reason |weak_controller| is passed
+// in is so that we can guarantee that window activations are handled if they
+// come after the controller has been torn down.
+bool IsMenuWidget(base::WeakPtr<MenuController> weak_controller,
+                  Widget* widget) {
+  MenuController* controller = weak_controller.get();
+  if (!controller || !widget)
+    return false;
+  // TODO(ellyjones): It's surprising that a MenuController has no notion of
+  // which Widgets correspond to the menus it is currently showing. Perhaps
+  // refactor it so that it does?
+  if (strcmp(widget->GetRootView()->GetClassName(), "MenuHostRootView"))
+    return false;
+
+  return true;
+}
+
+bool IsNotMenuWidget(base::WeakPtr<MenuController> weak_controller,
+                     Widget* widget) {
+  return !IsMenuWidget(weak_controller, widget);
+}
+#endif
+
 }  // namespace
 
 // MenuScrollTask --------------------------------------------------------------
@@ -516,9 +541,11 @@ void MenuController::Run(Widget* parent,
     menu_pre_target_handler_ = MenuPreTargetHandler::Create(this, owner_);
   }
 
-#if defined(OS_APPLE)
-  menu_cocoa_watcher_ = std::make_unique<MenuCocoaWatcherMac>(base::BindOnce(
-      &MenuController::Cancel, this->AsWeakPtr(), ExitType::kAll));
+#if defined(OS_MAC)
+  menu_cocoa_watcher_ = std::make_unique<MenuCocoaWatcherMac>(
+      base::BindRepeating(&IsNotMenuWidget, this->AsWeakPtr()),
+      base::BindOnce(&MenuController::Cancel, this->AsWeakPtr(),
+                     ExitType::kAll));
 #endif
 
   // Reset current state.
@@ -548,7 +575,7 @@ void MenuController::Run(Widget* parent,
 }
 
 void MenuController::Cancel(ExitType type) {
-#if defined(OS_APPLE)
+#if defined(OS_MAC)
   menu_closure_animation_.reset();
 #endif
 
@@ -884,12 +911,12 @@ bool MenuController::OnMouseWheel(SubmenuView* source,
 void MenuController::OnGestureEvent(SubmenuView* source,
                                     ui::GestureEvent* event) {
   if (owner_ && send_gesture_events_to_owner()) {
-#if defined(OS_APPLE)
+#if defined(OS_MAC)
     NOTIMPLEMENTED();
-#else   // !defined(OS_APPLE)
+#else   // !defined(OS_MAC)
     event->ConvertLocationToTarget(source->GetWidget()->GetNativeWindow(),
                                    owner()->GetNativeWindow());
-#endif  // defined(OS_APPLE)
+#endif  // defined(OS_MAC)
     owner()->OnGestureEvent(event);
     // Reset |send_gesture_events_to_owner_| when the first gesture ends.
     if (event->type() == ui::ET_GESTURE_END)
@@ -1194,7 +1221,7 @@ ui::PostDispatchAction MenuController::OnWillDispatchKeyEvent(
   base::WeakPtr<MenuController> this_ref = AsWeakPtr();
   if (event->type() == ui::ET_KEY_PRESSED) {
     bool key_handled = false;
-#if defined(OS_APPLE)
+#if defined(OS_MAC)
     // Special handling for Option-Up and Option-Down, which should behave like
     // Home and End respectively in menus.
     if ((event->flags() & ui::EF_ALT_DOWN)) {
@@ -1241,7 +1268,7 @@ ui::PostDispatchAction MenuController::OnWillDispatchKeyEvent(
 
   ui::Accelerator accelerator(*event);
 
-#if defined(OS_APPLE)
+#if defined(OS_MAC)
   if (AcceleratorShouldCancelMenu(accelerator)) {
     Cancel(ExitType::kAll);
     return ui::POST_DISPATCH_PERFORM_DEFAULT;
@@ -1307,7 +1334,7 @@ void MenuController::TurnOffMenuSelectionHoldForTest() {
 }
 
 void MenuController::OnMenuItemDestroying(MenuItemView* menu_item) {
-#if defined(OS_APPLE)
+#if defined(OS_MAC)
   if (menu_closure_animation_ && menu_closure_animation_->item() == menu_item)
     menu_closure_animation_.reset();
 #endif
@@ -1551,7 +1578,7 @@ bool MenuController::OnKeyPressed(const ui::KeyEvent& event) {
       break;
 
 // On Mac, treat space the same as return.
-#if !defined(OS_APPLE)
+#if !defined(OS_MAC)
     case ui::VKEY_SPACE:
       SendAcceleratorToHotTrackedView(event.flags());
       break;
@@ -1563,7 +1590,7 @@ bool MenuController::OnKeyPressed(const ui::KeyEvent& event) {
       // Fallthrough to accept or dismiss combobox menus on F4, like windows.
       FALLTHROUGH;
     case ui::VKEY_RETURN:
-#if defined(OS_APPLE)
+#if defined(OS_MAC)
     case ui::VKEY_SPACE:
 #endif
       // An odd special case: if a prefix selection is in flight, space should
@@ -1607,7 +1634,7 @@ bool MenuController::OnKeyPressed(const ui::KeyEvent& event) {
       CloseSubmenu();
       break;
 
-#if !defined(OS_APPLE)
+#if !defined(OS_MAC)
     case ui::VKEY_APPS: {
       Button* hot_view = GetFirstHotTrackedView(pending_state_.item);
       if (hot_view) {
@@ -1715,7 +1742,7 @@ void MenuController::UpdateInitialLocation(const gfx::Rect& bounds,
 }
 
 void MenuController::Accept(MenuItemView* item, int event_flags) {
-#if defined(OS_APPLE)
+#if defined(OS_MAC)
   menu_closure_animation_ = std::make_unique<MenuClosureAnimationMac>(
       item, item->GetParentMenuItem()->GetSubmenu(),
       base::BindOnce(&MenuController::ReallyAccept, base::Unretained(this),
@@ -1729,7 +1756,7 @@ void MenuController::Accept(MenuItemView* item, int event_flags) {
 void MenuController::ReallyAccept(MenuItemView* item, int event_flags) {
   DCHECK(!for_drop_);
   result_ = item;
-#if defined(OS_APPLE)
+#if defined(OS_MAC)
   // Reset the closure animation since it's now finished - this also unblocks
   // input events for the menu.
   menu_closure_animation_.reset();
@@ -2865,7 +2892,7 @@ void MenuController::RepostEventAndCancel(SubmenuView* source,
     if (last_part.type != MenuPart::NONE)
       exit_type = ExitType::kOutermost;
   }
-#if defined(OS_APPLE)
+#if defined(OS_MAC)
   // When doing a menu closure animation, target the deepest submenu - that way
   // MenuClosureAnimationMac will fade out all the menus in sync, rather than
   // the shallowest menu only.
@@ -3198,7 +3225,7 @@ void MenuController::UnregisterAlertedItem(MenuItemView* item) {
 }
 
 bool MenuController::CanProcessInputEvents() const {
-#if defined(OS_APPLE)
+#if defined(OS_MAC)
   return !menu_closure_animation_;
 #else
   return true;

ui/views/controls/menu/menu_controller.h

@@ -121,6 +121,13 @@ class VIEWS_EXPORT MenuController
     send_gesture_events_to_owner_ = send_gesture_events_to_owner;
   }
 
+  bool should_take_keyboard_focus() const {
+    return should_take_keyboard_focus_;
+  }
+  void set_should_take_keyboard_focus(bool should_take_keyboard_focus) {
+    should_take_keyboard_focus_ = should_take_keyboard_focus;
+  }
+
   // Returns the owner of child windows.
   // WARNING: this may be NULL.
   Widget* owner() { return owner_; }
@@ -759,6 +766,9 @@ class VIEWS_EXPORT MenuController
   // Whether to use the touchable layout.
   bool use_touchable_layout_ = false;
 
+  // Whether to take keyboard focus.
+  bool should_take_keyboard_focus_ = false;
+
   // During mouse event handling, this is the RootView to forward mouse events
   // to. We need this, because if we forward one event to it (e.g., mouse
   // pressed), subsequent events (like dragging) should also go to it, even if

ui/views/controls/menu/menu_host.cc

@@ -128,7 +128,9 @@ void MenuHost::InitMenuHost(Widget* parent,
   // If MenuHost has no parent widget, it needs to be marked
   // Activatable, so that calling Show in ShowMenuHost will
   // get keyboard focus.
-  if (parent == nullptr)
+  const bool take_focus =
+      menu_controller && menu_controller->should_take_keyboard_focus();
+  if (parent == nullptr || take_focus)
     params.activatable = Widget::InitParams::ACTIVATABLE_YES;
 #if defined(OS_WIN)
   // On Windows use the software compositor to ensure that we don't block
@@ -161,10 +163,13 @@ void MenuHost::ShowMenuHost(bool do_capture) {
   // Doing a capture may make us get capture lost. Ignore it while we're in the
   // process of showing.
   base::AutoReset<bool> reseter(&ignore_capture_lost_, true);
-  ShowInactive();
+  MenuController* menu_controller =
+      submenu_->GetMenuItem()->GetMenuController();
+  if (menu_controller && menu_controller->should_take_keyboard_focus())
+    Show();
+  else
+    ShowInactive();
   if (do_capture) {
-    MenuController* menu_controller =
-        submenu_->GetMenuItem()->GetMenuController();
     if (menu_controller && menu_controller->send_gesture_events_to_owner()) {
       // TransferGesture when owner needs gesture events so that the incoming
       // touch events after MenuHost is created are properly translated into

ui/views/controls/menu/menu_runner.cc

@@ -6,6 +6,7 @@
 
 #include <utility>
 
+#include "build/build_config.h"
 #include "ui/views/controls/menu/menu_runner_handler.h"
 #include "ui/views/controls/menu/menu_runner_impl.h"
 #include "ui/views/views_delegate.h"
@@ -13,6 +14,30 @@
 
 namespace views {
 
+namespace {
+
+bool ShouldTakeKeyboardFocus(ui::MenuSourceType source_type, int run_types) {
+#if defined(OS_MAC)
+  // An awkward hack: if the menu comes from an editable combobox, we can't take
+  // keyboard focus in the menu even if the menu was opened by the keyboard,
+  // because if we did, the editable combobox would become impossible to type
+  // in. This is inconsistent with opening other menus with the keyboard, and
+  // produces a weird accessibility experience in which editable combobox menus
+  // aren't announced when they appear but other menus are, but there's no real
+  // way around it.
+  return source_type == ui::MENU_SOURCE_KEYBOARD &&
+         !(run_types & MenuRunner::EDITABLE_COMBOBOX);
+#else
+  // A second awkward hack: on Aura platforms, for whatever reason, activating
+  // the menu widget causes there to be *no* active aura::Window, which confuses
+  // MenuPreTargetHandlerAura. Never do that.
+  // TODO(ellyjones): Why does that happen? How can there be no active window?
+  return false;
+#endif
+}
+
+}  // namespace
+
 MenuRunner::MenuRunner(ui::MenuModel* menu_model,
                        int32_t run_types,
                        base::RepeatingClosure on_menu_closed_callback)
@@ -69,7 +94,11 @@ void MenuRunner::RunMenuAt(Widget* parent,
     }
   }
 
-  impl_->RunMenuAt(parent, button_controller, bounds, anchor, run_types_);
+  int32_t types = run_types_;
+  if (ShouldTakeKeyboardFocus(source_type, run_types_))
+    types |= TAKE_KEYBOARD_FOCUS;
+
+  impl_->RunMenuAt(parent, button_controller, bounds, anchor, types);
 }
 
 bool MenuRunner::IsRunning() const {

ui/views/controls/menu/menu_runner.h

@@ -108,6 +108,9 @@ class VIEWS_EXPORT MenuRunner {
 
     // Indicates that the menu should show mnemonics.
     SHOULD_SHOW_MNEMONICS = 1 << 10,
+
+    // Whether the menu should take keyboard focus immediately when shown.
+    TAKE_KEYBOARD_FOCUS = 1 << 11,
   };
 
   // Creates a new MenuRunner, which may use a native menu if available.

ui/views/controls/menu/menu_runner_impl.cc

@@ -188,6 +188,8 @@ void MenuRunnerImpl::RunMenuAt(Widget* parent,
       (run_types & MenuRunner::SEND_GESTURE_EVENTS_TO_OWNER) != 0);
   controller->set_use_touchable_layout(
       (run_types & MenuRunner::USE_TOUCHABLE_LAYOUT) != 0);
+  controller->set_should_take_keyboard_focus(
+      (run_types & MenuRunner::TAKE_KEYBOARD_FOCUS) != 0);
   controller_ = controller->AsWeakPtr();
   menu_->set_controller(controller_.get());
   menu_->PrepareForRun(owns_controller_, has_mnemonics,

ui/views/widget/widget.h

@@ -98,6 +98,7 @@ class VIEWS_EXPORT Widget : public internal::NativeWidgetDelegate,
   using Widgets = std::set<Widget*>;
   using ShapeRects = std::vector<gfx::Rect>;
   using PaintAsActiveCallbackList = base::RepeatingClosureList;
+  using Predicate = base::RepeatingCallback<bool(Widget*)>;
 
   enum class FrameType {
     kDefault,      // Use whatever the default would be.