MacViews: Handle Space and Return keys correctly for Buttons.

ui/views/controls/button/custom_button_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/views/controls/button/custom_button.h"
 
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
 #include "build/build_config.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/layout.h"
 #include "ui/display/screen.h"
 #include "ui/events/event_utils.h"
 #include "ui/events/test/event_generator.h"
 #include "ui/views/animation/ink_drop_host.h"
 #include "ui/views/animation/ink_drop_impl.h"
 #include "ui/views/animation/test/ink_drop_host_view_test_api.h"
 #include "ui/views/animation/test/test_ink_drop.h"
 #include "ui/views/animation/test/test_ink_drop_host.h"
 #include "ui/views/context_menu_controller.h"
 #include "ui/views/controls/button/checkbox.h"
 #include "ui/views/controls/button/image_button.h"
 #include "ui/views/controls/button/label_button.h"
 #include "ui/views/controls/button/menu_button.h"
 #include "ui/views/controls/button/radio_button.h"
 #include "ui/views/controls/button/toggle_button.h"
 #include "ui/views/controls/link.h"
 #include "ui/views/controls/textfield/textfield.h"
+#include "ui/views/style/platform_style.h"
 #include "ui/views/test/views_test_base.h"
 
 #if defined(USE_AURA)
 #include "ui/aura/test/test_cursor_client.h"
 #include "ui/aura/window.h"
 #include "ui/aura/window_event_dispatcher.h"
 #endif
 
 namespace views {
 
@@ skipping 111 matching lines @@
     widget_->SetContentsView(button_);
   }
 
  protected:
   Widget* widget() { return widget_.get(); }
   TestCustomButton* button() { return button_; }
   void SetDraggedView(View* dragged_view) {
     widget_->dragged_view_ = dragged_view;
   }
 
-  void SimulateKeyEvent(ui::KeyEvent* event) { widget()->OnKeyEvent(event); }
-
  private:
   std::unique_ptr<Widget> widget_;
   TestCustomButton* button_ = nullptr;
 
   DISALLOW_COPY_AND_ASSIGN(CustomButtonTest);
 };
 
 // Tests that hover state changes correctly when visiblity/enableness changes.
 TEST_F(CustomButtonTest, HoverStateOnVisibilityChange) {
   ui::test::EventGenerator generator(widget()->GetNativeWindow());
@@ skipping 462 matching lines @@
   EXPECT_EQ(0, button()->ink_drop_layer_remove_count());
 
   base::RunLoop().RunUntilIdle();  // Complete the Close().
   EXPECT_FALSE(button()->layer());
   EXPECT_EQ(0, button()->ink_drop_layer_add_count());
   EXPECT_EQ(0, button()->ink_drop_layer_remove_count());
 
   delete button();
 }
 
-// Todo(karandeepb): On Mac, a button should get clicked on a Space key press
-// (and not release). Modify this test after fixing crbug.com/607429.
-// Test that Space Key behaves correctly on a focused button.
-TEST_F(CustomButtonTest, ClickOnSpace) {
+// Verify that the Space key clicks the button on key-press on Mac, and
+// key-release on other platforms.
+TEST_F(CustomButtonTest, ActionOnSpace) {
   // Give focus to the button.
   button()->SetFocusForPlatform();
   button()->RequestFocus();
-  EXPECT_EQ(button(), widget()->GetFocusManager()->GetFocusedView());
+  EXPECT_TRUE(button()->HasFocus());
 
   ui::KeyEvent space_press(ui::ET_KEY_PRESSED, ui::VKEY_SPACE, ui::EF_NONE);
-  SimulateKeyEvent(&space_press);
+  EXPECT_TRUE(button()->OnKeyPressed(space_press));

[tapted, 2017/01/04 04:54:31]

why this change? (won't this bypass accelerator handling?)

[karandeepb, 2017/01/04 06:14:45]

This seems more appropriate since we are unit-testing the CustomButton class and
we are interested in the return value of OnKey[Pressed/Released]. It will bypass
accelerator handling but then we haven't set up any custom accelerators. Testing
accelerator handling at the dialog delegate level seems more appropriate.

+
+#if defined(OS_MACOSX)
+  EXPECT_EQ(CustomButton::STATE_NORMAL, button()->state());
+  EXPECT_TRUE(button()->pressed());
+#else
   EXPECT_EQ(CustomButton::STATE_PRESSED, button()->state());
   EXPECT_FALSE(button()->pressed());
+#endif
 
   ui::KeyEvent space_release(ui::ET_KEY_RELEASED, ui::VKEY_SPACE, ui::EF_NONE);
-  SimulateKeyEvent(&space_release);
+
+#if defined(OS_MACOSX)
+  EXPECT_FALSE(button()->OnKeyReleased(space_release));
+#else
+  EXPECT_TRUE(button()->OnKeyReleased(space_release));
+#endif
+
   EXPECT_EQ(CustomButton::STATE_NORMAL, button()->state());
   EXPECT_TRUE(button()->pressed());
 }
 
+// Verify that the Return key clicks the button on key-press on all platforms
+// except Mac. On Mac, the Return key performs the default action associated
+// with a dialog.
+TEST_F(CustomButtonTest, ActionOnReturn) {
+  // Give focus to the button.
+  button()->SetFocusForPlatform();
+  button()->RequestFocus();
+  EXPECT_TRUE(button()->HasFocus());
+
+  ui::KeyEvent return_press(ui::ET_KEY_PRESSED, ui::VKEY_RETURN, ui::EF_NONE);
+
+#if defined(OS_MACOSX)
+  EXPECT_FALSE(button()->OnKeyPressed(return_press));
+  EXPECT_EQ(CustomButton::STATE_NORMAL, button()->state());
+  EXPECT_FALSE(button()->pressed());
+#else
+  EXPECT_TRUE(button()->OnKeyPressed(return_press));
+  EXPECT_EQ(CustomButton::STATE_NORMAL, button()->state());
+  EXPECT_TRUE(button()->pressed());
+#endif
+
+  ui::KeyEvent return_release(ui::ET_KEY_RELEASED, ui::VKEY_RETURN,
+                              ui::EF_NONE);
+  EXPECT_FALSE(button()->OnKeyReleased(return_release));
+}
+
 }  // namespace views