MacViews: Fix combobox keyboard shortcuts.

ui/views/controls/combobox/combobox_unittest.cc

Index: ui/views/controls/combobox/combobox_unittest.cc
diff --git a/ui/views/controls/combobox/combobox_unittest.cc b/ui/views/controls/combobox/combobox_unittest.cc
index 18632861446c5db7f93e569e7ca286606721e64f..24691a32e1e77430e9b71d4f0a95ecb36fae79e9 100644
--- a/ui/views/controls/combobox/combobox_unittest.cc
+++ b/ui/views/controls/combobox/combobox_unittest.cc
@@ -20,6 +20,7 @@
 #include "ui/events/keycodes/keyboard_codes.h"
 #include "ui/events/test/event_generator.h"
 #include "ui/views/controls/combobox/combobox_listener.h"
+#include "ui/views/style/platform_style.h"
 #include "ui/views/test/combobox_test_api.h"
 #include "ui/views/test/views_test_base.h"
 #include "ui/views/widget/widget.h"
@@ -275,26 +276,63 @@ class ComboboxTest : public ViewsTestBase {
   DISALLOW_COPY_AND_ASSIGN(ComboboxTest);
 };
 
+#if defined(OS_MACOSX)
+TEST_F(ComboboxTest, KeyTest) {

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

perhaps call this KeyTestMac? Since it's quite different -- flakiness dashboards
and things should keep them separate.

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

nit: comment before describing test

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

Done.

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

Done.

+  InitCombobox(nullptr, Combobox::STYLE_NORMAL);
+  PressKey(ui::VKEY_END);
+  EXPECT_EQ(0, combobox_->selected_index());
+  EXPECT_EQ(0, menu_show_count_);
+
+  PressKey(ui::VKEY_HOME);
+  EXPECT_EQ(0, combobox_->selected_index());
+  EXPECT_EQ(0, menu_show_count_);
+
+  PressKey(ui::VKEY_DOWN);
+  EXPECT_EQ(0, combobox_->selected_index());
+  EXPECT_EQ(1, menu_show_count_);
+
+  PressKey(ui::VKEY_RIGHT);
+  EXPECT_EQ(0, combobox_->selected_index());
+  EXPECT_EQ(1, menu_show_count_);
+
+  PressKey(ui::VKEY_LEFT);
+  EXPECT_EQ(0, combobox_->selected_index());
+  EXPECT_EQ(1, menu_show_count_);
+
+  PressKey(ui::VKEY_UP);
+  EXPECT_EQ(0, combobox_->selected_index());
+  EXPECT_EQ(2, menu_show_count_);
+
+  PressKey(ui::VKEY_PRIOR);
+  EXPECT_EQ(0, combobox_->selected_index());
+  EXPECT_EQ(2, menu_show_count_);
+
+  PressKey(ui::VKEY_NEXT);
+  EXPECT_EQ(0, combobox_->selected_index());
+  EXPECT_EQ(2, menu_show_count_);
+}
+#else

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

nit: // !OS_MACOSX

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

Don't think it's needed now.

 TEST_F(ComboboxTest, KeyTest) {

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

nit: comment before

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

Done.

   InitCombobox(nullptr, Combobox::STYLE_NORMAL);
   PressKey(ui::VKEY_END);
-  EXPECT_EQ(combobox_->selected_index() + 1, model_->GetItemCount());
+  EXPECT_EQ(model_->GetItemCount(), combobox_->selected_index() + 1);
   PressKey(ui::VKEY_HOME);
-  EXPECT_EQ(combobox_->selected_index(), 0);
+  EXPECT_EQ(0, combobox_->selected_index());
   PressKey(ui::VKEY_DOWN);
   PressKey(ui::VKEY_DOWN);
-  EXPECT_EQ(combobox_->selected_index(), 2);
+  EXPECT_EQ(2, combobox_->selected_index());
   PressKey(ui::VKEY_RIGHT);
-  EXPECT_EQ(combobox_->selected_index(), 2);
+  EXPECT_EQ(2, combobox_->selected_index());
   PressKey(ui::VKEY_LEFT);
-  EXPECT_EQ(combobox_->selected_index(), 2);
+  EXPECT_EQ(2, combobox_->selected_index());
   PressKey(ui::VKEY_UP);
-  EXPECT_EQ(combobox_->selected_index(), 1);
+  EXPECT_EQ(1, combobox_->selected_index());
   PressKey(ui::VKEY_PRIOR);
-  EXPECT_EQ(combobox_->selected_index(), 0);
+  EXPECT_EQ(0, combobox_->selected_index());
   PressKey(ui::VKEY_NEXT);
-  EXPECT_EQ(combobox_->selected_index(), model_->GetItemCount() - 1);
+  EXPECT_EQ(model_->GetItemCount() - 1, combobox_->selected_index());
 }
+#endif  // OS_MACOSX
 
 // Check that if a combobox is disabled before it has a native wrapper, then the
 // native wrapper inherits the disabled state when it gets created.
@@ -316,6 +354,10 @@ TEST_F(ComboboxTest, DisabilityTest) {
   EXPECT_FALSE(combobox_->enabled());
 }
 
+// On Mac, key events can't change the currently selected index directly for a
+// combobox.
+#if !defined(OS_MACOSX)
+

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

let's move TEST_F(ComboboxTest, KeyTest) in here too since that comment applies
to that as well

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

Done.

 // Verifies that we don't select a separator line in combobox when navigating
 // through keyboard.
 TEST_F(ComboboxTest, SkipSeparatorSimple) {
@@ -436,6 +478,7 @@ TEST_F(ComboboxTest, SkipMultipleSeparatorsAtEnd) {
   PressKey(ui::VKEY_END);
   EXPECT_EQ(6, combobox_->selected_index());
 }
+#endif  // OS_MACOSX
 
 TEST_F(ComboboxTest, GetTextForRowTest) {
   std::set<int> separators;
@@ -546,7 +589,8 @@ TEST_F(ComboboxTest, NotifyOnClickWithReturnKey) {
 
   // With STYLE_NORMAL, the click event is ignored. Instead the menu is shown.
   PressKey(ui::VKEY_RETURN);
-  EXPECT_EQ(1, menu_show_count_);
+  EXPECT_EQ(PlatformStyle::kReturnClicksFocusedControl ? 1 : 0,
+            menu_show_count_);
   EXPECT_FALSE(listener.on_perform_action_called());
 }
 
@@ -559,8 +603,14 @@ TEST_F(ComboboxTest, NotifyOnClickWithReturnKeyActionStyle) {
   // With STYLE_ACTION, the click event is notified and the menu is not shown.
   PressKey(ui::VKEY_RETURN);
   EXPECT_EQ(0, menu_show_count_);
-  EXPECT_TRUE(listener.on_perform_action_called());
-  EXPECT_EQ(0, listener.perform_action_index());
+
+  if (PlatformStyle::kReturnClicksFocusedControl) {
+    EXPECT_TRUE(listener.on_perform_action_called());
+    EXPECT_EQ(0, listener.perform_action_index());
+  } else {
+    EXPECT_FALSE(listener.on_perform_action_called());
+    EXPECT_EQ(-1, listener.perform_action_index());
+  }
 }
 
 TEST_F(ComboboxTest, NotifyOnClickWithSpaceKey) {
@@ -586,10 +636,16 @@ TEST_F(ComboboxTest, NotifyOnClickWithSpaceKeyActionStyle) {
   combobox_->set_listener(&listener);
 
   // With STYLE_ACTION, the click event is notified after releasing and the menu
-  // is not shown.
+  // is not shown. On Mac the click event is notified on key press.
   PressKey(ui::VKEY_SPACE);
   EXPECT_EQ(0, menu_show_count_);
+#if defined(OS_MACOSX)
+  EXPECT_TRUE(listener.on_perform_action_called());
+  EXPECT_EQ(0, listener.perform_action_index());
+#else
   EXPECT_FALSE(listener.on_perform_action_called());
+  EXPECT_EQ(-1, listener.perform_action_index());
+#endif
 
   ReleaseKey(ui::VKEY_SPACE);
   EXPECT_EQ(0, menu_show_count_);
@@ -638,11 +694,17 @@ TEST_F(ComboboxTest, ConsumingPressKeyEvents) {
   InitCombobox(nullptr, Combobox::STYLE_NORMAL);
 
   EXPECT_TRUE(combobox_->OnKeyPressed(
-      ui::KeyEvent(ui::ET_KEY_PRESSED, ui::VKEY_RETURN, ui::EF_NONE)));
-  EXPECT_EQ(1, menu_show_count_);
-  EXPECT_TRUE(combobox_->OnKeyPressed(
       ui::KeyEvent(ui::ET_KEY_PRESSED, ui::VKEY_SPACE, ui::EF_NONE)));
-  EXPECT_EQ(2, menu_show_count_);
+  EXPECT_EQ(1, menu_show_count_);
+
+  ui::KeyEvent return_press(ui::ET_KEY_PRESSED, ui::VKEY_RETURN, ui::EF_NONE);
+  if (PlatformStyle::kReturnClicksFocusedControl) {
+    EXPECT_TRUE(combobox_->OnKeyPressed(return_press));
+    EXPECT_EQ(2, menu_show_count_);
+  } else {
+    EXPECT_FALSE(combobox_->OnKeyPressed(return_press));
+    EXPECT_EQ(1, menu_show_count_);
+  }
 }
 
 TEST_F(ComboboxTest, ConsumingKeyPressEventsActionStyle) {
@@ -650,8 +712,9 @@ TEST_F(ComboboxTest, ConsumingKeyPressEventsActionStyle) {
   // or an enter key will be consumed and the menu is not shown.
   InitCombobox(nullptr, Combobox::STYLE_ACTION);
 
-  EXPECT_TRUE(combobox_->OnKeyPressed(
-      ui::KeyEvent(ui::ET_KEY_PRESSED, ui::VKEY_RETURN, ui::EF_NONE)));
+  EXPECT_EQ(PlatformStyle::kReturnClicksFocusedControl,
+            combobox_->OnKeyPressed(ui::KeyEvent(
+                ui::ET_KEY_PRESSED, ui::VKEY_RETURN, ui::EF_NONE)));
   EXPECT_EQ(0, menu_show_count_);
   EXPECT_TRUE(combobox_->OnKeyPressed(
       ui::KeyEvent(ui::ET_KEY_PRESSED, ui::VKEY_SPACE, ui::EF_NONE)));