Fix accelerator handling for in-menu buttons in the app menu.

BUG=557558

Review URL: https://codereview.chromium.org/1544803004

Cr-Commit-Position: refs/heads/master@{#374787}

ui/views/controls/button/custom_button.cc

@@ -267,13 +267,6 @@ void CustomButton::OnGestureEvent(ui::GestureEvent* event) {
 }
 
 bool CustomButton::AcceleratorPressed(const ui::Accelerator& accelerator) {
-  // Should only handle accelerators when active. However, only top level
-  // widgets can be active, so for child widgets check if they are focused
-  // instead.
-  if ((IsChildWidget() && !FocusInChildWidget()) ||
-      (!IsChildWidget() && !GetWidget()->IsActive())) {
-    return false;
-  }
   SetState(STATE_NORMAL);
   // TODO(beng): remove once NotifyClick takes ui::Event.
   ui::MouseEvent synthetic_event(
@@ -460,14 +453,4 @@ void CustomButton::OnClickCanceled(const ui::Event& event) {
   Button::OnClickCanceled(event);
 }
 
-bool CustomButton::IsChildWidget() const {
-  return GetWidget() && GetWidget()->GetTopLevelWidget() != GetWidget();
-}
-
-bool CustomButton::FocusInChildWidget() const {
-  return GetWidget() &&
-         GetWidget()->GetRootView()->Contains(
-             GetFocusManager()->GetFocusedView());
-}
-
 }  // namespace views

ui/views/controls/button/custom_button.h

@@ -161,11 +161,6 @@ class VIEWS_EXPORT CustomButton : public Button,
   }
 
  private:
-  // Returns true if this is not a top level widget. Virtual for tests.
-  virtual bool IsChildWidget() const;
-  // Returns true if the focus is not in a top level widget. Virtual for tests.
-  virtual bool FocusInChildWidget() const;
-
   ButtonState state_;
 
   gfx::ThrobAnimation hover_animation_;

ui/views/controls/button/custom_button_unittest.cc

@@ -56,37 +56,13 @@ class TestCustomButton : public CustomButton, public ButtonListener {
     canceled_ = false;
   }
 
-  // CustomButton methods:
-  bool IsChildWidget() const override { return is_child_widget_; }
-  bool FocusInChildWidget() const override { return focus_in_child_widget_; }
-
-  void set_child_widget(bool b) { is_child_widget_ = b; }
-  void set_focus_in_child_widget(bool b) { focus_in_child_widget_ = b; }
-
  private:
   bool pressed_ = false;
   bool canceled_ = false;
-  bool is_child_widget_ = false;
-  bool focus_in_child_widget_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(TestCustomButton);
 };
 
-class TestWidget : public Widget {
- public:
-  TestWidget() : Widget() {}
-
-  // Widget method:
-  bool IsActive() const override { return active_; }
-
-  void set_active(bool active) { active_ = active; }
-
- private:
-  bool active_ = false;
-
-  DISALLOW_COPY_AND_ASSIGN(TestWidget);
-};
-
 // An InkDropDelegate that keeps track of ink drop visibility.
 class TestInkDropDelegate : public InkDropDelegate {
  public:
@@ -165,7 +141,7 @@ class CustomButtonTest : public ViewsTestBase {
 
     // Create a widget so that the CustomButton can query the hover state
     // correctly.
-    widget_.reset(new TestWidget);
+    widget_.reset(new Widget);
     Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_POPUP);
     params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
     params.bounds = gfx::Rect(0, 0, 650, 650);
@@ -190,13 +166,13 @@ class CustomButtonTest : public ViewsTestBase {
   }
 
  protected:
-  TestWidget* widget() { return widget_.get(); }
+  Widget* widget() { return widget_.get(); }
   TestCustomButton* button() { return button_; }
   bool ink_shown() const { return ink_shown_; }
   bool ink_hidden() const { return ink_hidden_; }
 
  private:
-  scoped_ptr<TestWidget> widget_;
+  scoped_ptr<Widget> widget_;
   TestCustomButton* button_;
   bool ink_shown_ = false;
   bool ink_hidden_ = false;
@@ -316,33 +292,6 @@ TEST_F(CustomButtonTest, NotifyAction) {
   EXPECT_FALSE(button()->pressed());
 }
 
-TEST_F(CustomButtonTest, HandleAccelerator) {
-  // Child widgets shouldn't handle accelerators when they are not focused.
-  EXPECT_FALSE(button()->IsChildWidget());
-  EXPECT_FALSE(button()->FocusInChildWidget());
-  EXPECT_FALSE(widget()->IsActive());
-  button()->AcceleratorPressed(ui::Accelerator(ui::VKEY_RETURN, ui::EF_NONE));
-  EXPECT_FALSE(button()->pressed());
-  // Child without focus.
-  button()->set_child_widget(true);
-  button()->set_focus_in_child_widget(false);
-  button()->AcceleratorPressed(ui::Accelerator(ui::VKEY_RETURN, ui::EF_NONE));
-  EXPECT_FALSE(button()->pressed());
-  button()->Reset();
-  // Child with focus.
-  button()->set_child_widget(true);
-  button()->set_focus_in_child_widget(true);
-  button()->AcceleratorPressed(ui::Accelerator(ui::VKEY_RETURN, ui::EF_NONE));
-  EXPECT_TRUE(button()->pressed());
-  button()->Reset();
-  // Not a child, but active.
-  button()->set_child_widget(false);
-  button()->set_focus_in_child_widget(true);
-  widget()->set_active(true);
-  button()->AcceleratorPressed(ui::Accelerator(ui::VKEY_RETURN, ui::EF_NONE));
-  EXPECT_TRUE(button()->pressed());
-}
-
 // Tests that OnClickCanceled gets called when NotifyClick is not expected
 // anymore.
 TEST_F(CustomButtonTest, NotifyActionNoClick) {

ui/views/view.cc

@@ -1145,7 +1145,25 @@ bool View::AcceleratorPressed(const ui::Accelerator& accelerator) {
 }
 
 bool View::CanHandleAccelerators() const {
-  return enabled() && IsDrawn() && GetWidget() && GetWidget()->IsVisible();
+  const Widget* widget = GetWidget();
+  if (!enabled() || !IsDrawn() || !widget || !widget->IsVisible())
+    return false;
+#if defined(USE_AURA) && !defined(OS_CHROMEOS)
+  // Non-ChromeOS aura windows have an associated FocusManagerEventHandler which
+  // adds currently focused view as an event PreTarget (see
+  // DesktopNativeWidgetAura::InitNativeWidget). However, the focused view isn't
+  // always the right view to handle accelerators: It should only handle them
+  // when active. Only top level widgets can be active, so for child widgets
+  // check if they are focused instead. ChromeOS also behaves different than
+  // Linux when an extension popup is about to handle the accelerator.
+  bool child = widget && widget->GetTopLevelWidget() != widget;
+  bool focus_in_child =
+      widget &&
+      widget->GetRootView()->Contains(GetFocusManager()->GetFocusedView());
+  if ((child && !focus_in_child) || (!child && !widget->IsActive()))
+    return false;
+#endif
+  return true;
 }
 
 // Focus -----------------------------------------------------------------------

ui/views/view_unittest.cc

@@ -2101,6 +2101,80 @@ bool TestView::AcceleratorPressed(const ui::Accelerator& accelerator) {
   return true;
 }
 
+// On non-ChromeOS aura there is extra logic to determine whether a view should
+// handle accelerators or not (see View::CanHandleAccelerators for details).
+// This test targets that extra logic, but should also work on other platforms.
+TEST_F(ViewTest, HandleAccelerator) {
+  ui::Accelerator return_accelerator(ui::VKEY_RETURN, ui::EF_NONE);
+  TestView* view = new TestView();
+  view->Reset();
+  view->AddAccelerator(return_accelerator);
+  EXPECT_EQ(view->accelerator_count_map_[return_accelerator], 0);
+
+  // Create a window and add the view as its child.
+  scoped_ptr<Widget> widget(new Widget);
+  Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_POPUP);
+  params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
+  params.bounds = gfx::Rect(0, 0, 100, 100);
+  widget->Init(params);
+  View* root = widget->GetRootView();
+  root->AddChildView(view);
+  widget->Show();
+
+  FocusManager* focus_manager = widget->GetFocusManager();
+  ASSERT_TRUE(focus_manager);
+
+#if defined(USE_AURA) && !defined(OS_CHROMEOS)
+  // When a non-child view is not active, it shouldn't handle accelerators.
+  EXPECT_FALSE(widget->IsActive());
+  EXPECT_FALSE(focus_manager->ProcessAccelerator(return_accelerator));
+  EXPECT_EQ(0, view->accelerator_count_map_[return_accelerator]);
+#endif
+
+  // When a non-child view is active, it should handle accelerators.
+  view->accelerator_count_map_[return_accelerator] = 0;
+  widget->Activate();
+  EXPECT_TRUE(widget->IsActive());
+  EXPECT_TRUE(focus_manager->ProcessAccelerator(return_accelerator));
+  EXPECT_EQ(1, view->accelerator_count_map_[return_accelerator]);
+
+  // Add a child view associated with a child widget.
+  TestView* child_view = new TestView();
+  child_view->Reset();
+  child_view->AddAccelerator(return_accelerator);
+  EXPECT_EQ(child_view->accelerator_count_map_[return_accelerator], 0);
+  view->AddChildView(child_view);
+  Widget* child_widget = new Widget;
+  Widget::InitParams child_params =
+      CreateParams(Widget::InitParams::TYPE_CONTROL);
+  child_params.parent = widget->GetNativeView();
+  child_widget->Init(child_params);
+  child_widget->SetContentsView(child_view);
+
+  FocusManager* child_focus_manager = child_widget->GetFocusManager();
+  ASSERT_TRUE(child_focus_manager);
+
+  // When a child view is in focus, it should handle accelerators.
+  child_view->accelerator_count_map_[return_accelerator] = 0;
+  view->accelerator_count_map_[return_accelerator] = 0;
+  child_focus_manager->SetFocusedView(child_view);
+  EXPECT_FALSE(child_view->GetWidget()->IsActive());
+  EXPECT_TRUE(child_focus_manager->ProcessAccelerator(return_accelerator));
+  EXPECT_EQ(1, child_view->accelerator_count_map_[return_accelerator]);
+  EXPECT_EQ(0, view->accelerator_count_map_[return_accelerator]);
+
+#if defined(USE_AURA) && !defined(OS_CHROMEOS)
+  // When a child view is not in focus, its parent should handle accelerators.
+  child_view->accelerator_count_map_[return_accelerator] = 0;
+  view->accelerator_count_map_[return_accelerator] = 0;
+  child_focus_manager->ClearFocus();
+  EXPECT_FALSE(child_view->GetWidget()->IsActive());
+  EXPECT_TRUE(child_focus_manager->ProcessAccelerator(return_accelerator));
+  EXPECT_EQ(0, child_view->accelerator_count_map_[return_accelerator]);
+  EXPECT_EQ(1, view->accelerator_count_map_[return_accelerator]);
+#endif
+}
+
 // TODO: these tests were initially commented out when getting aura to
 // run. Figure out if still valuable and either nuke or fix.
 #if 0