MacViews: Handle Space and Return keys correctly for Buttons.

ui/views/controls/button/custom_button.cc

Index: ui/views/controls/button/custom_button.cc
diff --git a/ui/views/controls/button/custom_button.cc b/ui/views/controls/button/custom_button.cc
index 115dcb4b486be897aa89d77250af8ee40d0c7e31..db8ca5d4fb4b42f9f20143cb0256836d90beab5f 100644
--- a/ui/views/controls/button/custom_button.cc
+++ b/ui/views/controls/button/custom_button.cc
@@ -20,6 +20,7 @@
 #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/style/platform_style.h"
 #include "ui/views/widget/widget.h"
 
 #if defined(USE_AURA)
@@ -34,6 +35,16 @@ namespace {
 // How long the hover animation takes if uninterrupted.
 const int kHoverFadeDurationMs = 150;
 
+CustomButton::ButtonAction GetButtonActionForKeyEvent(
+    const ui::KeyEvent& event) {
+  if (event.key_code() == ui::VKEY_SPACE)
+    return PlatformStyle::kButtonActionOnSpace;
+  else if (event.key_code() == ui::VKEY_RETURN &&

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

nit: "no else after return" -
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

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

Done.

+           PlatformStyle::kClickControlOnReturn)
+    return CustomButton::ACTION_CLICK;
+  return CustomButton::ACTION_NONE;
+}
+
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -238,31 +249,39 @@ bool CustomButton::OnKeyPressed(const ui::KeyEvent& event) {
   if (state_ == STATE_DISABLED)
     return false;
 
-  // Space sets button state to pushed. Enter clicks the button. This matches
-  // the Windows native behavior of buttons, where Space clicks the button on
-  // KeyRelease and Enter clicks the button on KeyPressed.
-  if (event.key_code() == ui::VKEY_SPACE) {
-    SetState(STATE_PRESSED);
-    if (GetInkDrop()->GetTargetInkDropState() !=
-        views::InkDropState::ACTION_PENDING) {
-      AnimateInkDrop(views::InkDropState::ACTION_PENDING, nullptr /* event */);
-    }
-  } else if (event.key_code() == ui::VKEY_RETURN) {
-    SetState(STATE_NORMAL);
-    NotifyClick(event);
-  } else {
-    return false;
+  switch (GetButtonActionForKeyEvent(event)) {
+    case ButtonAction::ACTION_PRESS:
+      SetState(STATE_PRESSED);
+      if (GetInkDrop()->GetTargetInkDropState() !=
+          views::InkDropState::ACTION_PENDING) {

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

views:: not needed, same below

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

Done.

+        AnimateInkDrop(views::InkDropState::ACTION_PENDING,
+                       nullptr /* event */);
+      }
+      return true;
+      break;

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

nit: no break

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

Done.

+    case ButtonAction::ACTION_CLICK:
+      SetState(STATE_NORMAL);
+      NotifyClick(event);
+      return true;
+      break;

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

nit: no break

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

Done.

+    case ButtonAction::ACTION_NONE:
+      return false;
   }
-  return true;
+
+  NOTREACHED();
+  return false;
 }
 
 bool CustomButton::OnKeyReleased(const ui::KeyEvent& event) {
-  if ((state_ == STATE_PRESSED) && (event.key_code() == ui::VKEY_SPACE)) {
-    SetState(STATE_NORMAL);
-    NotifyClick(event);
-    return true;
-  }
-  return false;
+  const bool click_button =
+      state_ == STATE_PRESSED &&
+      GetButtonActionForKeyEvent(event) == ButtonAction::ACTION_PRESS;
+  if (!click_button)
+    return false;
+
+  SetState(STATE_NORMAL);
+  NotifyClick(event);
+  return true;
 }
 
 void CustomButton::OnGestureEvent(ui::GestureEvent* event) {
@@ -306,9 +325,9 @@ bool CustomButton::AcceleratorPressed(const ui::Accelerator& accelerator) {
 
 bool CustomButton::SkipDefaultKeyEventProcessing(const ui::KeyEvent& event) {
   // If this button is focused and the user presses space or enter, don't let
-  // that be treated as an accelerator.
-  return (event.key_code() == ui::VKEY_SPACE) ||
-         (event.key_code() == ui::VKEY_RETURN);

[karandeepb, 2017/01/04 03:55:40]

I am not entirely sure about the reason behind this piece of code. This was
added in https://codereview.chromium.org/1645013004. If this button is focused,
it would have already consumed OnKeyEvent for space and enter, hence we wouldn't
reach here. The only use-case I think this handles is when
OnKey[Pressed/Released] are overriden by subclasses to not handle Space/Enter.
On Mac, we want to handle Return key as an accelerator even if a button has
focus.

Also the comments for View::SkipDefaultKeyEventProcessing says: In that
case(when true is returned), OnKeyPressed will subsequently be invoked for that
event. This seems wrong to me. I think OnKeyPressed is invoked before, then
SkipDefaultKeyEventProcessing is used by FocusManager::OnKeyEvent.

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

I think it would reach here if CustomButton::OnKeyPressed() returned false. I
guess that's rare. perhaps a test was failing in that CL.
On windows both space and return do something - I think we need something
different here.
It says "Invoked [before handling] by the focus manager for tab traversal,
accelerators and other focus related actions." - I think that's still correct.
That is, yes: View::OnKeyPressed is invoked before, but
FocusManager::OnKeyPressed [not an override] is invoked after.

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

Yeah that was my guess as well. This I guess can happen when OnKeyPressed is
overriden by a subclass.
But GetButtonActionForKeyEvent(event) would not return ACTION_NONE on windows
for space and enter.
You mean FocusManager::OnKeyEvent? There's no FocusManager::OnKeyPressed and the
comment in view.h refers to OnKeyPressed, which was confusing to me.

[tapted, 2017/01/05 00:34:41]

Ah. Yup.
Ah, right. yeah that last sentence can probably be ignored. Perhaps fire off a
CL to sky@ that removes it?

(and yes: I meant FocusManager::OnKeyEvent)

[karandeepb, 2017/01/05 10:50:08]

Will do!

+  // that be treated as an accelerator if there is a button action corresponding
+  // to it.
+  return GetButtonActionForKeyEvent(event) != ButtonAction::ACTION_NONE;
 }
 
 void CustomButton::ShowContextMenu(const gfx::Point& p,