MacViews: Fix combobox keyboard shortcuts.

ui/views/controls/combobox/combobox.cc

Index: ui/views/controls/combobox/combobox.cc
diff --git a/ui/views/controls/combobox/combobox.cc b/ui/views/controls/combobox/combobox.cc
index 25304929c47b18fba106eae70dfc98664fd1ade5..8a89419b0a07d9ee4456218f0b5e7f792368ecaf 100644
--- a/ui/views/controls/combobox/combobox.cc
+++ b/ui/views/controls/combobox/combobox.cc
@@ -161,6 +161,7 @@ class TransparentButton : public CustomButton {
   DISALLOW_COPY_AND_ASSIGN(TransparentButton);
 };
 
+#if !defined(OS_MACOSX)
 // Returns the next or previous valid index (depending on |increment|'s value).
 // Skips separator or disabled indices. Returns -1 if there is no valid adjacent
 // index.
@@ -175,6 +176,7 @@ int GetAdjacentIndex(ui::ComboboxModel* model, int increment, int index) {
   }
   return kNoSelection;
 }
+#endif
 
 // Returns the image resource ids of an array for the body button.
 //
@@ -619,6 +621,18 @@ bool Combobox::OnKeyPressed(const ui::KeyEvent& e) {
   bool show_menu = false;
   int new_index = kNoSelection;
   switch (e.key_code()) {
+#if defined(OS_MACOSX)
+    case ui::VKEY_DOWN:
+    case ui::VKEY_UP:
+      show_menu = true;
+      break;
+    case ui::VKEY_SPACE:
+      if (style_ == STYLE_ACTION)

[karandeepb, 2017/01/13 08:28:57]

Does STYLE_ACTION have something analogous on Cocoa? Calling OnPerformAction on
SPACE seemed reasonable.

[tapted, 2017/01/13 19:06:39]

STYLE_ACTION on mac is effectively NSPopupButton. But there's no Mac equivalent
for a control where one side "clicks" and the other side shows a menu (you
always need to click twice: once on the button and once on the menu) -- and
clicking anywhere on an NSPopupButton shows the menu. So I think SPACE should
behave the same regardless of |style_|.

[karandeepb, 2017/01/16 04:26:15]

Done. Although, it seems to me that the intention of STYLE_ACTION was to be a
button with a rarely used dropdown. Though I think STYLE_ACTION is pretty weird.
Also, it is only used once in the whole codebase.

[tapted, 2017/01/17 21:48:32]

yup - and even that is likely going away -- https://go/uvfmt

[karandeepb, 2017/01/18 03:09:56]

Oh, that's good, should result in some code cleanup here.

+        OnPerformAction();
+      else
+        show_menu = true;
+      break;
+#else
     // Show the menu on F4 without modifiers.
     case ui::VKEY_F4:
       if (e.IsAltDown() || e.IsAltGrDown() || e.IsControlDown())
@@ -667,7 +681,7 @@ bool Combobox::OnKeyPressed(const ui::KeyEvent& e) {
       else
         show_menu = true;
       break;
-
+#endif  // OS_MACOSX
     default:
       return false;
   }
@@ -688,7 +702,8 @@ bool Combobox::OnKeyReleased(const ui::KeyEvent& e) {
   if (style_ != STYLE_ACTION)
     return false;  // crbug.com/127520
 
-  if (e.key_code() == ui::VKEY_SPACE && style_ == STYLE_ACTION)
+  if (e.key_code() == ui::VKEY_SPACE && style_ == STYLE_ACTION &&
+      text_button_->state() == Button::STATE_PRESSED)

[tapted, 2017/01/13 19:06:39]

(does showing the menu instead manage to swallow the release? - we might not
need this)

[karandeepb, 2017/01/16 04:26:16]

So the key release is swallowed when the menu is shown on pressing Space.
However on pressing Space with an active [NS]menu, we do get a key release event
here. This happens through BCV::keyUp. I would have expected the NSMenu to
consume both the key press and key up in this case. 

     OnPerformAction();
 
   return false;