[go: up one dir, main page]

ime: Revert MODECHANGE key behaviour back to original.

In a previous patch, we changed the MODECHANGE key behaviour so that
pressing it once will only show the current input method. This was to
follow the UX specs.

However, upon consideration, we're going to change it back to the
original behaviour of switching to next input method on first key press.

The original code used a event rewriter to convert the MODECHANGE key
into Ctrl-Shift-Space (which switches to the next key press). We then
changed the code to be in ash accelerators because of the extra logic
involved. Now that we don't need this extra logic, we could revert back
to event rewriter, but we think it's better to keep it in accelerators
because we still have some extra logic around histogram logging.

TBR=holte@chromium.org

(cherry picked from commit fe901037fd6ae87e8eee4efa17016ea6ca0c503b)

Bug: 953901
Change-Id: I2cf4dc41300b62ee6f57085408e199a2a55062b6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1642487
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#666233}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1647272
Reviewed-by: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/branch-heads/3809@{#201}
Cr-Branched-From: d82dec1a818f378c464ba307ddd9c92133eac355-refs/heads/master@{#665002}
diff --git a/ash/accelerators/accelerator_controller_impl.cc b/ash/accelerators/accelerator_controller_impl.cc
index 46d4994..de03850 100644
--- a/ash/accelerators/accelerator_controller_impl.cc
+++ b/ash/accelerators/accelerator_controller_impl.cc
@@ -223,6 +223,11 @@
                             ImeSwitchType::kAccelerator);
 }
 
+void RecordImeSwitchByModeChangeKey() {
+  UMA_HISTOGRAM_ENUMERATION("InputMethod.ImeSwitch",
+                            ImeSwitchType::kModeChangeKey);
+}
+
 void HandleCycleBackwardMRU(const ui::Accelerator& accelerator) {
   if (accelerator.key_code() == ui::VKEY_TAB)
     base::RecordAction(base::UserMetricsAction("Accel_PrevWindow_Tab"));
@@ -346,9 +351,12 @@
   return !keyboard::KeyboardController::Get()->IsKeyboardVisible();
 }
 
-void HandleSwitchToNextIme() {
+void HandleSwitchToNextIme(const ui::Accelerator& accelerator) {
   base::RecordAction(UserMetricsAction("Accel_Next_Ime"));
-  RecordImeSwitchByAccelerator();
+  if (accelerator.key_code() == ui::VKEY_MODECHANGE)
+    RecordImeSwitchByModeChangeKey();
+  else
+    RecordImeSwitchByAccelerator();
   Shell::Get()->ime_controller()->SwitchToNextIme();
 }
 
@@ -612,11 +620,6 @@
   }
 }
 
-void HandleShowOrSwitchIme() {
-  base::RecordAction(UserMetricsAction("Accel_Show_Or_Switch_Ime"));
-  Shell::Get()->ime_controller()->ShowOrSwitchIme();
-}
-
 void HandleCrosh() {
   base::RecordAction(UserMetricsAction("Accel_Open_Crosh"));
 
@@ -1476,7 +1479,6 @@
     case RESTORE_TAB:
     case ROTATE_WINDOW:
     case SHOW_IME_MENU_BUBBLE:
-    case SHOW_OR_SWITCH_IME:
     case SHOW_SHORTCUT_VIEWER:
     case SHOW_TASK_MANAGER:
     case SUSPEND:
@@ -1712,9 +1714,6 @@
     case SHOW_IME_MENU_BUBBLE:
       HandleShowImeMenuBubble();
       break;
-    case SHOW_OR_SWITCH_IME:
-      HandleShowOrSwitchIme();
-      break;
     case SHOW_SHORTCUT_VIEWER:
       HandleShowKeyboardShortcutViewer();
       break;
@@ -1740,7 +1739,7 @@
       HandleSwitchToLastUsedIme(accelerator);
       break;
     case SWITCH_TO_NEXT_IME:
-      HandleSwitchToNextIme();
+      HandleSwitchToNextIme(accelerator);
       break;
     case SWITCH_TO_NEXT_USER:
       HandleCycleUser(CycleUserDirection::NEXT);
diff --git a/ash/accelerators/accelerator_controller_unittest.cc b/ash/accelerators/accelerator_controller_unittest.cc
index 7258a99..cf66b10 100644
--- a/ash/accelerators/accelerator_controller_unittest.cc
+++ b/ash/accelerators/accelerator_controller_unittest.cc
@@ -2304,30 +2304,14 @@
 }
 
 // Tests the IME mode change key.
-TEST_F(AcceleratorControllerTest,
-       ChangeIMEMode_ModeIndicatorHiddenShowsIndicator) {
+TEST_F(AcceleratorControllerTest, ChangeIMEMode_SwitchesInputMethod) {
+  AddTestImes();
+
   ImeController* controller = Shell::Get()->ime_controller();
 
   TestImeControllerClient client;
   controller->SetClient(client.CreateInterfacePtr());
 
-  EXPECT_FALSE(controller->mode_indicator_observer()->active_widget());
-  EXPECT_EQ(0, client.show_mode_indicator_count_);
-
-  ProcessInController(ui::Accelerator(ui::VKEY_MODECHANGE, ui::EF_NONE));
-  controller->FlushMojoForTesting();
-
-  EXPECT_EQ(1, client.show_mode_indicator_count_);
-}
-
-TEST_F(AcceleratorControllerTest,
-       ChangeIMEMode_ModeIndicatorVisibleSwitchesInputMethod) {
-  ImeController* controller = Shell::Get()->ime_controller();
-
-  TestImeControllerClient client;
-  controller->SetClient(client.CreateInterfacePtr());
-
-  controller->ShowModeIndicator(gfx::Rect(10, 10), base::ASCIIToUTF16("test"));
   EXPECT_EQ(0, client.next_ime_count_);
 
   ProcessInController(ui::Accelerator(ui::VKEY_MODECHANGE, ui::EF_NONE));
diff --git a/ash/accelerators/accelerator_table_unittest.cc b/ash/accelerators/accelerator_table_unittest.cc
index 95cf786..6a80805 100644
--- a/ash/accelerators/accelerator_table_unittest.cc
+++ b/ash/accelerators/accelerator_table_unittest.cc
@@ -20,7 +20,7 @@
 constexpr int kNonSearchAcceleratorsNum = 92;
 // The hash of non-Search-based accelerators. See HashAcceleratorData().
 constexpr char kNonSearchAcceleratorsHash[] =
-    "2fc4ce8b0d4f3629b7a4c9bdf1fa0deb";
+    "aebfcdff71290bb9af4060871b72e4f7";
 
 struct Cmp {
   bool operator()(const AcceleratorData& lhs,
diff --git a/ash/ime/ime_controller.cc b/ash/ime/ime_controller.cc
index 3e531c04..bff6107 100644
--- a/ash/ime/ime_controller.cc
+++ b/ash/ime/ime_controller.cc
@@ -238,22 +238,6 @@
   client_.FlushForTesting();
 }
 
-void ImeController::ShowOrSwitchIme() {
-  if (mode_indicator_observer_->active_widget()) {
-    SwitchToNextIme();
-
-    UMA_HISTOGRAM_ENUMERATION("InputMethod.ModeChangeKeyAction",
-                              ModeChangeKeyAction::kSwitchIme);
-    UMA_HISTOGRAM_ENUMERATION("InputMethod.ImeSwitch",
-                              ImeSwitchType::kModeChangeKey);
-  } else {
-    client_->ShowModeIndicator();
-
-    UMA_HISTOGRAM_ENUMERATION("InputMethod.ModeChangeKeyAction",
-                              ModeChangeKeyAction::kShowIndicator);
-  }
-}
-
 bool ImeController::IsCapsLockEnabled() const {
   return is_caps_lock_enabled_;
 }
diff --git a/ash/ime/ime_controller.h b/ash/ime/ime_controller.h
index 25228625..5bbe32b 100644
--- a/ash/ime/ime_controller.h
+++ b/ash/ime/ime_controller.h
@@ -130,10 +130,6 @@
     return mode_indicator_observer_.get();
   }
 
-  // Asynchronously show the current IME mode indicator. If it's already shown,
-  // switch to the next available IME.
-  void ShowOrSwitchIme();
-
  private:
   // Returns the IDs of the subset of input methods which are active and are
   // associated with |accelerator|. For example, two Japanese IMEs can be
diff --git a/ash/public/cpp/accelerators.cc b/ash/public/cpp/accelerators.cc
index 828b4915..fedff86 100644
--- a/ash/public/cpp/accelerators.cc
+++ b/ash/public/cpp/accelerators.cc
@@ -184,7 +184,7 @@
     {true, ui::VKEY_ASSISTANT, ui::EF_NONE, START_VOICE_INTERACTION},
 
     // IME mode change key.
-    {true, ui::VKEY_MODECHANGE, ui::EF_NONE, SHOW_OR_SWITCH_IME},
+    {true, ui::VKEY_MODECHANGE, ui::EF_NONE, SWITCH_TO_NEXT_IME},
 
     // Debugging shortcuts that need to be available to end-users in
     // release builds.
diff --git a/ash/public/cpp/accelerators.h b/ash/public/cpp/accelerators.h
index 0b47828..517af75 100644
--- a/ash/public/cpp/accelerators.h
+++ b/ash/public/cpp/accelerators.h
@@ -70,7 +70,6 @@
   SCALE_UI_RESET,
   SCALE_UI_UP,
   SHOW_IME_MENU_BUBBLE,
-  SHOW_OR_SWITCH_IME,
   SHOW_SHORTCUT_VIEWER,
   SHOW_STYLUS_TOOLS,
   SHOW_TASK_MANAGER,
diff --git a/chrome/browser/ui/ash/keyboard_shortcut_viewer_metadata_unittest.cc b/chrome/browser/ui/ash/keyboard_shortcut_viewer_metadata_unittest.cc
index 7a25f536..9ef7968 100644
--- a/chrome/browser/ui/ash/keyboard_shortcut_viewer_metadata_unittest.cc
+++ b/chrome/browser/ui/ash/keyboard_shortcut_viewer_metadata_unittest.cc
@@ -22,7 +22,7 @@
 // The total number of Ash accelerators.
 constexpr int kAshAcceleratorsTotalNum = 104;
 // The hash of Ash accelerators.
-constexpr char kAshAcceleratorsHash[] = "1e3e31c4f37997da2a04d438bf150cd6";
+constexpr char kAshAcceleratorsHash[] = "abc138d68a319b5ba56430ff56c54a6e";
 #if defined(GOOGLE_CHROME_BUILD)
 // Internal builds add an extra accelerator for the Feedback app.
 // The total number of Chrome accelerators (available on Chrome OS).
diff --git a/tools/metrics/actions/actions.xml b/tools/metrics/actions/actions.xml
index 83f665d..b2637a9 100644
--- a/tools/metrics/actions/actions.xml
+++ b/tools/metrics/actions/actions.xml
@@ -964,11 +964,6 @@
   <description>Please enter the description of this user action.</description>
 </action>
 
-<action name="Accel_Show_Or_Switch_Ime">
-  <owner>essential-inputs-team@google.com</owner>
-  <description>Emitted when the user presses the MODECHANGE key.</description>
-</action>
-
 <action name="Accel_Show_Stylus_Tools">
   <owner>jdufault@chromium.org</owner>
   <description>