MacViews: Handle Space and Return keys correctly for Buttons.

ui/views/controls/button/custom_button.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 "ui/accessibility/ax_node_data.h"
 #include "ui/events/event.h"
 #include "ui/events/event_utils.h"
 #include "ui/events/keycodes/keyboard_codes.h"
 #include "ui/gfx/animation/throb_animation.h"
 #include "ui/gfx/color_palette.h"
 #include "ui/native_theme/native_theme.h"
 #include "ui/views/animation/ink_drop_highlight.h"
 #include "ui/views/animation/ink_drop_impl.h"
 #include "ui/views/controls/button/blue_button.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/style/platform_style.h"
 #include "ui/views/widget/widget.h"
 
 #if defined(USE_AURA)
 #include "ui/aura/client/capture_client.h"
 #include "ui/aura/window.h"
 #endif
 
 namespace views {
 
 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
 
 ////////////////////////////////////////////////////////////////////////////////
 // CustomButton, public:
 
 // static
 const char CustomButton::kViewClassName[] = "CustomButton";
 
 // static
 const CustomButton* CustomButton::AsCustomButton(const views::View* view) {
@@ skipping 184 matching lines @@
 
 void CustomButton::OnMouseMoved(const ui::MouseEvent& event) {
   if (state_ != STATE_DISABLED)
     SetState(HitTestPoint(event.location()) ? STATE_HOVERED : STATE_NORMAL);
 }
 
 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 {
+  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;
+  }
+
+  NOTREACHED();
+  return false;
+}
+
+bool CustomButton::OnKeyReleased(const ui::KeyEvent& event) {
+  const bool click_button =
+      state_ == STATE_PRESSED &&
+      GetButtonActionForKeyEvent(event) == ButtonAction::ACTION_PRESS;
+  if (!click_button)
     return false;
-  }
+
+  SetState(STATE_NORMAL);
+  NotifyClick(event);
   return true;
 }
 
-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;
-}
-
 void CustomButton::OnGestureEvent(ui::GestureEvent* event) {
   if (state_ == STATE_DISABLED) {
     Button::OnGestureEvent(event);
     return;
   }
 
   if (event->type() == ui::ET_GESTURE_TAP && IsTriggerableEvent(*event)) {
     // Set the button state to hot and start the animation fully faded in. The
     // GESTURE_END event issued immediately after will set the state to
     // STATE_NORMAL beginning the fade out animation. See
@@ skipping 21 matching lines @@
   // TODO(beng): remove once NotifyClick takes ui::Event.
   ui::MouseEvent synthetic_event(
       ui::ET_MOUSE_RELEASED, gfx::Point(), gfx::Point(), ui::EventTimeForNow(),
       ui::EF_LEFT_MOUSE_BUTTON, ui::EF_LEFT_MOUSE_BUTTON);
   NotifyClick(synthetic_event);
   return true;
 }
 
 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,
                                    ui::MenuSourceType source_type) {
   if (!context_menu_controller())
     return;
 
   // We're about to show the context menu. Showing the context menu likely means
   // we won't get a mouse exited and reset state. Reset it now to be sure.
   if (state_ != STATE_DISABLED)
@@ skipping 153 matching lines @@
           views::InkDropState::ACTION_PENDING ||
       GetInkDrop()->GetTargetInkDropState() ==
           views::InkDropState::ALTERNATE_ACTION_PENDING) {
     AnimateInkDrop(views::InkDropState::HIDDEN,
                    ui::LocatedEvent::FromIfValid(&event));
   }
   Button::OnClickCanceled(event);
 }
 
 }  // namespace views