MacViews: Fix combobox keyboard shortcuts.

ui/views/controls/combobox/combobox_unittest.cc

 // Copyright 2013 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 <set>
 
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "ui/base/ime/input_method.h"
 #include "ui/base/ime/text_input_client.h"
 #include "ui/base/models/combobox_model.h"
 #include "ui/base/models/menu_model.h"
 #include "ui/events/event.h"
 #include "ui/events/event_constants.h"
 #include "ui/events/event_utils.h"
 #include "ui/events/keycodes/dom/dom_code.h"
 #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"
 
 using base::ASCIIToUTF16;
 
 namespace views {
 
 using test::ComboboxTestApi;
 
@@ skipping 235 matching lines @@
 
   // The current menu show count.
   int menu_show_count_ = 0;
 
   std::unique_ptr<ui::test::EventGenerator> event_generator_;
 
  private:
   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(combobox_->selected_index() + 1, model_->GetItemCount());
+  EXPECT_EQ(0, combobox_->selected_index());
+  EXPECT_EQ(0, menu_show_count_);
+
   PressKey(ui::VKEY_HOME);
-  EXPECT_EQ(combobox_->selected_index(), 0);
+  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(model_->GetItemCount(), combobox_->selected_index() + 1);
+  PressKey(ui::VKEY_HOME);
+  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.
 TEST_F(ComboboxTest, DisabilityTest) {
   model_.reset(new TestComboboxModel());
 
   ASSERT_FALSE(combobox_);
   combobox_ = new TestCombobox(model_.get(), Combobox::STYLE_NORMAL);
   combobox_->SetEnabled(false);
 
   widget_ = new Widget;
   Widget::InitParams params =
       CreateParams(Widget::InitParams::TYPE_WINDOW_FRAMELESS);
   params.bounds = gfx::Rect(100, 100, 100, 100);
   widget_->Init(params);
   View* container = new View();
   widget_->SetContentsView(container);
   container->AddChildView(combobox_);
   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) {
   std::set<int> separators;
   separators.insert(2);
   InitCombobox(&separators, Combobox::STYLE_NORMAL);
   EXPECT_EQ(0, combobox_->selected_index());
   PressKey(ui::VKEY_DOWN);
   EXPECT_EQ(1, combobox_->selected_index());
   PressKey(ui::VKEY_DOWN);
@@ skipping 100 matching lines @@
   EXPECT_EQ(5, combobox_->selected_index());
   PressKey(ui::VKEY_HOME);
   EXPECT_EQ(0, combobox_->selected_index());
   PressKey(ui::VKEY_NEXT);
   EXPECT_EQ(6, combobox_->selected_index());
   PressKey(ui::VKEY_PRIOR);
   EXPECT_EQ(0, combobox_->selected_index());
   PressKey(ui::VKEY_END);
   EXPECT_EQ(6, combobox_->selected_index());
 }
+#endif  // OS_MACOSX
 
 TEST_F(ComboboxTest, GetTextForRowTest) {
   std::set<int> separators;
   separators.insert(0);
   separators.insert(1);
   separators.insert(9);
   InitCombobox(&separators, Combobox::STYLE_NORMAL);
   for (int i = 0; i < combobox_->GetRowCount(); ++i) {
     if (separators.count(i) != 0) {
       EXPECT_TRUE(combobox_->GetTextForRow(i).empty()) << i;
@@ skipping 90 matching lines @@
 }
 
 TEST_F(ComboboxTest, NotifyOnClickWithReturnKey) {
   InitCombobox(nullptr, Combobox::STYLE_NORMAL);
 
   TestComboboxListener listener;
   combobox_->set_listener(&listener);
 
   // 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());
 }
 
 TEST_F(ComboboxTest, NotifyOnClickWithReturnKeyActionStyle) {
   InitCombobox(nullptr, Combobox::STYLE_ACTION);
 
   TestComboboxListener listener;
   combobox_->set_listener(&listener);
 
   // 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) {
   InitCombobox(nullptr, Combobox::STYLE_NORMAL);
 
   TestComboboxListener listener;
   combobox_->set_listener(&listener);
 
   // With STYLE_NORMAL, the click event is ignored. Instead the menu is shwon.
   PressKey(ui::VKEY_SPACE);
   EXPECT_EQ(1, menu_show_count_);
   EXPECT_FALSE(listener.on_perform_action_called());
 
   ReleaseKey(ui::VKEY_SPACE);
   EXPECT_EQ(1, menu_show_count_);
   EXPECT_FALSE(listener.on_perform_action_called());
 }
 
 TEST_F(ComboboxTest, NotifyOnClickWithSpaceKeyActionStyle) {
   InitCombobox(nullptr, Combobox::STYLE_ACTION);
 
   TestComboboxListener listener;
   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_);
   EXPECT_TRUE(listener.on_perform_action_called());
   EXPECT_EQ(0, listener.perform_action_index());
 }
 
 TEST_F(ComboboxTest, NotifyOnClickWithMouse) {
   InitCombobox(nullptr, Combobox::STYLE_ACTION);
 
@@ skipping 28 matching lines @@
   PerformMouseRelease(left_point);
 
   EXPECT_EQ(1, menu_show_count_);  // Unchanged.
   EXPECT_EQ(0, listener.perform_action_index());
 }
 
 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)));
+      ui::KeyEvent(ui::ET_KEY_PRESSED, ui::VKEY_SPACE, 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_);
+
+  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) {
   // When the combobox's style is STYLE_ACTION, pressing events of a space key
   // 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)));
   EXPECT_EQ(0, menu_show_count_);
 }
 
 TEST_F(ComboboxTest, ContentWidth) {
   std::vector<std::string> values;
   VectorComboboxModel model(&values);
   TestCombobox combobox(&model, Combobox::STYLE_NORMAL);
@@ skipping 164 matching lines @@
   EXPECT_EQ(ui::MenuModel::TYPE_COMMAND, menu_model->GetTypeAt(0));
   EXPECT_EQ(ui::MenuModel::TYPE_COMMAND, menu_model->GetTypeAt(1));
 
   EXPECT_EQ(ASCIIToUTF16("PEANUT BUTTER"), menu_model->GetLabelAt(0));
   EXPECT_EQ(ASCIIToUTF16("JELLY"), menu_model->GetLabelAt(1));
   EXPECT_FALSE(menu_model->IsVisibleAt(0));
   EXPECT_TRUE(menu_model->IsVisibleAt(1));
 }
 
 }  // namespace views