Fix Caps Lock bug

Move the handling of Caps Lock key event to accelerator controller and trigger toggling on key-release. Remove the handling code of Caps Lock for X11 from InputMethodChromeOS::DispatchKeyEvent, because it is now handled only in accelerator controller. This should also work for both x11+ozone environment and x11 environment on desktop.

BUG=700705

Review-Url: https://codereview.chromium.org/2763483002
Cr-Commit-Position: refs/heads/master@{#464499}

ash/accelerators/accelerator_controller.cc

@@ -551,6 +551,8 @@ bool CanHandleToggleCapsLock(const ui::Accelerator& accelerator,
                              const ui::Accelerator& previous_accelerator) {
   chromeos::input_method::InputMethodManager* ime =
       chromeos::input_method::InputMethodManager::Get();
+  if (!ime)
+    return false;
 
   // This shortcust is set to be trigger on release. Either the current
   // accelerator is a Search release or Alt release.
@@ -563,7 +565,7 @@ bool CanHandleToggleCapsLock(const ui::Accelerator& accelerator,
             ui::Accelerator::KeyState::PRESSED &&
         (previous_accelerator.key_code() == ui::VKEY_LWIN ||
          previous_accelerator.key_code() == ui::VKEY_MENU)) {
-      return ime && ime->GetImeKeyboard();
+      return ime->GetImeKeyboard();
     }
   }
 
@@ -577,10 +579,16 @@ bool CanHandleToggleCapsLock(const ui::Accelerator& accelerator,
             ui::Accelerator::KeyState::PRESSED &&
         (previous_accelerator.key_code() == ui::VKEY_LWIN ||
          previous_accelerator.key_code() == ui::VKEY_MENU)) {
-      return ime && ime->GetImeKeyboard();
+      return ime->GetImeKeyboard();
     }
   }
 
+  // Caps Lock release
+  if (accelerator.key_code() == ui::VKEY_CAPITAL &&
+      accelerator.key_state() == ui::Accelerator::KeyState::RELEASED) {
+    return ime->GetImeKeyboard();
+  }
+
   return false;
 }
 

ash/accelerators/accelerator_controller_unittest.cc

@@ -980,7 +980,7 @@ class ToggleCapsLockTest : public AcceleratorControllerTest {
   DISALLOW_COPY_AND_ASSIGN(ToggleCapsLockTest);
 };
 
-// Tests the four combinations of the TOGGLE_CAPS_LOCK accelerator.
+// Tests the five combinations of the TOGGLE_CAPS_LOCK accelerator.
 TEST_F(ToggleCapsLockTest, ToggleCapsLockAccelerators) {
   chromeos::input_method::InputMethodManager* input_method_manager =
       chromeos::input_method::InputMethodManager::Get();
@@ -1020,6 +1020,16 @@ TEST_F(ToggleCapsLockTest, ToggleCapsLockAccelerators) {
   EXPECT_FALSE(ProcessInController(press_search_then_alt));
   EXPECT_TRUE(ProcessInController(release_alt_before_search));
   EXPECT_TRUE(input_method_manager->GetImeKeyboard()->CapsLockIsEnabled());
+  input_method_manager->GetImeKeyboard()->SetCapsLockEnabled(false);
+
+  // 5. Press Caps Lock, Release Caps Lock.
+  const ui::Accelerator press_caps_lock(ui::VKEY_CAPITAL, ui::EF_NONE);
+  EXPECT_FALSE(ProcessInController(press_caps_lock));
+  EXPECT_FALSE(input_method_manager->GetImeKeyboard()->CapsLockIsEnabled());
+  const ui::Accelerator release_caps_lock(
+      CreateReleaseAccelerator(ui::VKEY_CAPITAL, ui::EF_NONE));
+  EXPECT_TRUE(ProcessInController(release_caps_lock));
+  EXPECT_TRUE(input_method_manager->GetImeKeyboard()->CapsLockIsEnabled());
 }
 
 class PreferredReservedAcceleratorsTest : public test::AshTestBase {

ash/accelerators/accelerator_table.cc

@@ -91,6 +91,7 @@ const AcceleratorData kAcceleratorData[] = {
     // down. The key_code here is MENU (for Alt) and Search is a modifier
     // (EF_COMMAND_DOWN is used for Search as a modifier).
     {false, ui::VKEY_MENU, ui::EF_COMMAND_DOWN, TOGGLE_CAPS_LOCK},
+    {false, ui::VKEY_CAPITAL, ui::EF_NONE, TOGGLE_CAPS_LOCK},
     {true, ui::VKEY_VOLUME_MUTE, ui::EF_NONE, VOLUME_MUTE},
     {true, ui::VKEY_VOLUME_DOWN, ui::EF_NONE, VOLUME_DOWN},
     {true, ui::VKEY_VOLUME_UP, ui::EF_NONE, VOLUME_UP},

ash/accelerators/accelerator_table_unittest.cc

@@ -15,12 +15,12 @@ namespace ash {
 
 namespace {
 
-// The number of non-Search-based accelerators as of 2017-04-06.
-constexpr int kNonSearchAcceleratorsNum = 92;
-// The hash of non-Search-based accelerators as of 2017-04-06.
+// The number of non-Search-based accelerators as of 2017-04-11.
+constexpr int kNonSearchAcceleratorsNum = 93;
+// The hash of non-Search-based accelerators as of 2017-04-11.
 // See HashAcceleratorData().
 constexpr char kNonSearchAcceleratorsHash[] =
-    "6c6695ca5f4d7298504e4d1e8a148902";
+    "c24f35143a69b4378834207913b5a708";
 
 struct Cmp {
   bool operator()(const AcceleratorData& lhs, const AcceleratorData& rhs) {

chrome/browser/chromeos/events/event_rewriter_unittest.cc

@@ -979,10 +979,7 @@ TEST_F(EventRewriterTest, TestRewriteModifiersRemapToCapsLock) {
   search.Init(prefs::kLanguageRemapSearchKeyTo, prefs());
   search.SetValue(chromeos::input_method::kCapsLockKey);
 
-  chromeos::input_method::FakeImeKeyboard ime_keyboard;
   rewriter_->KeyboardDeviceAddedForTesting(kKeyboardDeviceId, "PC Keyboard");
-  rewriter_->set_ime_keyboard_for_testing(&ime_keyboard);
-  EXPECT_FALSE(ime_keyboard.caps_lock_is_enabled_);
 
   // Press Search.
   EXPECT_EQ(GetExpectedResultAsString(
@@ -991,8 +988,6 @@ TEST_F(EventRewriterTest, TestRewriteModifiersRemapToCapsLock) {
             GetRewrittenEventAsString(rewriter_, ui::ET_KEY_PRESSED,
                                       ui::VKEY_LWIN, ui::DomCode::META_LEFT,
                                       ui::EF_COMMAND_DOWN, ui::DomKey::META));
-  // Confirm that the Caps Lock status is changed.
-  EXPECT_TRUE(ime_keyboard.caps_lock_is_enabled_);
 
   // Release Search.
   EXPECT_EQ(GetExpectedResultAsString(ui::ET_KEY_RELEASED, ui::VKEY_CAPITAL,
@@ -1001,8 +996,6 @@ TEST_F(EventRewriterTest, TestRewriteModifiersRemapToCapsLock) {
             GetRewrittenEventAsString(rewriter_, ui::ET_KEY_RELEASED,
                                       ui::VKEY_LWIN, ui::DomCode::META_LEFT,
                                       ui::EF_NONE, ui::DomKey::META));
-  // Confirm that the Caps Lock status is not changed.
-  EXPECT_TRUE(ime_keyboard.caps_lock_is_enabled_);
 
   // Press Search.
   EXPECT_EQ(GetExpectedResultAsString(
@@ -1012,8 +1005,6 @@ TEST_F(EventRewriterTest, TestRewriteModifiersRemapToCapsLock) {
                                       ui::VKEY_LWIN, ui::DomCode::META_LEFT,
                                       ui::EF_COMMAND_DOWN | ui::EF_CAPS_LOCK_ON,
                                       ui::DomKey::META));
-  // Confirm that the Caps Lock status is changed.
-  EXPECT_FALSE(ime_keyboard.caps_lock_is_enabled_);
 
   // Release Search.
   EXPECT_EQ(GetExpectedResultAsString(ui::ET_KEY_RELEASED, ui::VKEY_CAPITAL,
@@ -1022,8 +1013,6 @@ TEST_F(EventRewriterTest, TestRewriteModifiersRemapToCapsLock) {
             GetRewrittenEventAsString(rewriter_, ui::ET_KEY_RELEASED,
                                       ui::VKEY_LWIN, ui::DomCode::META_LEFT,
                                       ui::EF_NONE, ui::DomKey::META));
-  // Confirm that the Caps Lock status is not changed.
-  EXPECT_FALSE(ime_keyboard.caps_lock_is_enabled_);
 
   // Press Caps Lock (on an external keyboard).
   EXPECT_EQ(GetExpectedResultAsString(
@@ -1034,30 +1023,13 @@ TEST_F(EventRewriterTest, TestRewriteModifiersRemapToCapsLock) {
                                       ui::EF_CAPS_LOCK_ON | ui::EF_MOD3_DOWN,
                                       ui::DomKey::CAPS_LOCK));
 
-#if defined(USE_X11)
-  // Confirm that calling RewriteForTesting() does not change the state of
-  // |ime_keyboard|. In this case, X Window system itself should change the
-  // Caps Lock state, not ash::EventRewriter.
-  EXPECT_FALSE(ime_keyboard.caps_lock_is_enabled_);
-#elif defined(USE_OZONE)
-  // Under Ozone the rewriter is responsible for changing the caps lock
-  // state when the final key is Caps Lock, regardless of whether the
-  // initial key is Caps Lock.
-  EXPECT_TRUE(ime_keyboard.caps_lock_is_enabled_);
-#endif
-
   // Release Caps Lock (on an external keyboard).
   EXPECT_EQ(GetExpectedResultAsString(ui::ET_KEY_RELEASED, ui::VKEY_CAPITAL,
                                       ui::DomCode::CAPS_LOCK, ui::EF_NONE,
                                       ui::DomKey::CAPS_LOCK),
             GetRewrittenEventAsString(rewriter_, ui::ET_KEY_RELEASED,
                                       ui::VKEY_CAPITAL, ui::DomCode::CAPS_LOCK,
                                       ui::EF_NONE, ui::DomKey::CAPS_LOCK));
-#if defined(USE_X11)
-  EXPECT_FALSE(ime_keyboard.caps_lock_is_enabled_);
-#elif defined(USE_OZONE)
-  EXPECT_TRUE(ime_keyboard.caps_lock_is_enabled_);
-#endif
 }
 
 TEST_F(EventRewriterTest, TestRewriteCapsLock) {
@@ -1075,7 +1047,6 @@ TEST_F(EventRewriterTest, TestRewriteCapsLock) {
             GetRewrittenEventAsString(rewriter_, ui::ET_KEY_PRESSED,
                                       ui::VKEY_F16, ui::DomCode::F16,
                                       ui::EF_MOD3_DOWN, ui::DomKey::F16));
-  EXPECT_TRUE(ime_keyboard.caps_lock_is_enabled_);
 }
 
 TEST_F(EventRewriterTest, TestRewriteDiamondKey) {

ui/base/ime/input_method_chromeos.cc

@@ -63,20 +63,9 @@ void InputMethodChromeOS::DispatchKeyEvent(
   DCHECK(event->IsKeyEvent());
   DCHECK(!(event->flags() & ui::EF_IS_SYNTHESIZED));
 
-  // For linux_chromeos, the ime keyboard cannot track the caps lock state by
-  // itself, so need to call SetCapsLockEnabled() method to reflect the caps
-  // lock state by the key event.
-  if (!chromeos::IsRunningAsSystemCompositor()) {
-    chromeos::input_method::InputMethodManager* manager =
-        chromeos::input_method::InputMethodManager::Get();
-    if (manager) {
-      chromeos::input_method::ImeKeyboard* keyboard = manager->GetImeKeyboard();
-      if (keyboard && event->type() == ui::ET_KEY_PRESSED) {
-        keyboard->SetCapsLockEnabled((event->key_code() == ui::VKEY_CAPITAL) ?
-            !keyboard->CapsLockIsEnabled() : event->IsCapsLockOn());
-      }
-    }
-  }
+  // The Caps Lock toggling has been removed from here, because now it is
+  // handled in accelerator controller.
+  // (see https://bugs.chromium.org/p/chromium/issues/detail?id=700705).
 
   // If |context_| is not usable, then we can only dispatch the key event as is.
   // We only dispatch the key event to input method when the |context_| is an

ui/chromeos/events/event_rewriter_chromeos.cc

@@ -804,24 +804,9 @@ bool EventRewriterChromeOS::RewriteModifierKeys(const ui::KeyEvent& key_event,
       used_modifier_latches_ |= pressed_modifier_latches_;
       latched_modifier_latches_ = ui::EF_NONE;
     }
-    // Toggle Caps Lock if the remapped key is ui::VKEY_CAPITAL.
-    if (state->key_code == ui::VKEY_CAPITAL
-        // ... except on linux Chrome OS, where InputMethodChromeOS handles it.
-        && (base::SysInfo::IsRunningOnChromeOS() || ime_keyboard_for_testing_)
-#if defined(USE_X11)
-        // ... but for X11, do nothing if the original key is ui::VKEY_CAPITAL
-        // (i.e. a Caps Lock key on an external keyboard is pressed) since X
-        // handles that itself.
-        && incoming.key_code != ui::VKEY_CAPITAL
-#endif
-        ) {
-      ::chromeos::input_method::ImeKeyboard* ime_keyboard =
-          ime_keyboard_for_testing_
-              ? ime_keyboard_for_testing_
-              : ::chromeos::input_method::InputMethodManager::Get()
-                    ->GetImeKeyboard();
-      ime_keyboard->SetCapsLockEnabled(!ime_keyboard->CapsLockIsEnabled());
-    }
+    // The handling of Caps Lock toggling is now moved to accelerator
+    // controller.
+    // (see https://bugs.chromium.org/p/chromium/issues/detail?id=700705).
   }
   return exact_event;
 }