MacViews: Handle Space and Return keys correctly for Buttons.

ui/views/controls/button/custom_button_unittest.cc

Index: ui/views/controls/button/custom_button_unittest.cc
diff --git a/ui/views/controls/button/custom_button_unittest.cc b/ui/views/controls/button/custom_button_unittest.cc
index 1425641559b085b38a43dddfcd383d4d2180de41..d8d470f8b0aceb062265852f3f5dd6cf40b903e2 100644
--- a/ui/views/controls/button/custom_button_unittest.cc
+++ b/ui/views/controls/button/custom_button_unittest.cc
@@ -27,6 +27,7 @@
 #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)
@@ -158,8 +159,6 @@ class CustomButtonTest : public ViewsTestBase {
     widget_->dragged_view_ = dragged_view;
   }
 
-  void SimulateKeyEvent(ui::KeyEvent* event) { widget()->OnKeyEvent(event); }
-
  private:
   std::unique_ptr<Widget> widget_;
   TestCustomButton* button_ = nullptr;
@@ -642,24 +641,61 @@ TEST_F(CustomButtonTest, NoLayerAddedForWidgetVisibilityChanges) {
   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