MacViews: Fix combobox keyboard shortcuts.

ui/views/controls/combobox/combobox.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/combobox/combobox.h"
 
 #include <stddef.h>
 
 #include <utility>
 
@@ skipping 143 matching lines @@
             size(), GetInkDropCenterBasedOnLastEvent(),
             GetNativeTheme()->GetSystemColor(
                 ui::NativeTheme::kColorId_LabelEnabledColor),
             ink_drop_visible_opacity()));
   }
 
  private:
   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.
 int GetAdjacentIndex(ui::ComboboxModel* model, int increment, int index) {
   DCHECK(increment == -1 || increment == 1);
 
   index += increment;
   while (index >= 0 && index < model->GetItemCount()) {
     if (!model->IsItemSeparatorAt(index) || !model->IsItemEnabledAt(index))
       return index;
     index += increment;
   }
   return kNoSelection;
 }
+#endif
 
 // Returns the image resource ids of an array for the body button.
 //
 // TODO(hajimehoshi): This function should return the images for the 'disabled'
 // status. (crbug/270052)
 const int* GetBodyButtonImageIds(bool focused,
                                  Button::ButtonState state,
                                  size_t* num) {
   DCHECK(num);
   *num = 9;
@@ skipping 424 matching lines @@
   DCHECK_EQ(e.type(), ui::ET_KEY_PRESSED);
 
   DCHECK_GE(selected_index_, 0);
   DCHECK_LT(selected_index_, model()->GetItemCount());
   if (selected_index_ < 0 || selected_index_ > model()->GetItemCount())
     selected_index_ = 0;
 
   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())
         return false;
       show_menu = true;
       break;
 
     // Move to the next item if any, or show the menu on Alt+Down like Windows.
     case ui::VKEY_DOWN:
       if (e.IsAltDown())
         show_menu = true;
       else
         new_index = GetAdjacentIndex(model(), 1, selected_index_);
       break;
 
     // Move to the end of the list.
     case ui::VKEY_END:

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

I think HOME/END should be mapped. Remember we pick "meaningful" keycodes for
these so Cmd+Up is Ctrl+VKEY_HOME (whereas the Home key defaults to
scrollToBeginningOfDocument: which we ignore).

Then.. should we check for Control? I'm inclined not to bother - it's nicer to
do "something" IMO that have it seem broken. PageUp/PageDown seem to work when
the menu is showing, but not to show it so PRIOR/NEXT can be skipped. So I guess
we have

#if defined(OS_MACOSX)
  // Mac Comboboxes and PopupButtons (similar to STYLE_ACTION) show the menu
  // before processing any navigation commands.
  case UP
  case DOWN
  case SPACE
  case HOME
  case END
    show_menu = true;
    break;
#endif

wdyt?

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

Ohk I din't know that Cmd+Up/Down could also be used to show the menu. Done.
Although with this, simple HOME/END would also show the menu, which'll be
different from Cocoa. 
On another note, it doesn't seem right to me to map keycodes on Mac. I would
have preferred the views operating on actual keycodes instead (w/o the mapping
at the BCV layer). Although that may be an involved change at this stage.

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

yeah the mapping is to give things overriding OnKeyEvent something more
cross-platfrom. Adding an edit command field to the key event (ie replace
SetEditCommandForNextKeyEvent) might be nicer

     case ui::VKEY_NEXT:  // Page down.
       new_index = GetAdjacentIndex(model(), -1, model()->GetItemCount());
       break;
 
     // Move to the beginning of the list.
     case ui::VKEY_HOME:
     case ui::VKEY_PRIOR:  // Page up.
       new_index = GetAdjacentIndex(model(), 1, -1);
       break;
 
@@ skipping 11 matching lines @@
         show_menu = true;
       }
       break;
 
     case ui::VKEY_RETURN:
       if (style_ == STYLE_ACTION)
         OnPerformAction();
       else
         show_menu = true;
       break;
-
+#endif  // OS_MACOSX
     default:
       return false;
   }
 
   if (show_menu) {
     ShowDropDownMenu(ui::MENU_SOURCE_KEYBOARD);
   } else if (new_index != selected_index_ && new_index != kNoSelection &&
              style_ != STYLE_ACTION) {
     DCHECK(!model()->IsItemSeparatorAt(new_index));
     selected_index_ = new_index;
     OnPerformAction();
   }
 
   return true;
 }
 
 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;
 }
 
 void Combobox::OnPaint(gfx::Canvas* canvas) {
   switch (style_) {
     case STYLE_NORMAL: {
       OnPaintBackground(canvas);
       PaintText(canvas);
@@ skipping 294 matching lines @@
   const int kMdPaddingWidth = 8;
   int arrow_pad = UseMd() ? kMdPaddingWidth
                           : PlatformStyle::kComboboxNormalArrowPadding;
   int padding = style_ == STYLE_NORMAL
                     ? arrow_pad * 2
                     : kActionLeftPadding + kActionRightPadding;
   return ArrowSize().width() + padding;
 }
 
 }  // namespace views