cros: teach event rewriter about external Chrome OS-branded keyboards

This teaches event-rewriter about existence of external Chrome OS
branded keyboards. Because current Chrome OS keyboards are
indistinguishable from generic external USB/BT keyboards from HID
perspective, we rely on UDEV to tag relevant devices with
CROS_KEYBOARD_TOP_ROW_LAYOUT property.

This allows us to make Search->CapsLock and similar mappings work not
only for internal keyboards, but external Chrome OS keyboards as well.

OS keyboards.

Bug: b/144367328
Test: unit_tests, check that Caps lock remapping affects external Chrome
Change-Id: I0b3f932e108b43f7aafe51e155923c4f0b6b5975
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2051211
Commit-Queue: Dmitry Torokhov <dtor@chromium.org>
Reviewed-by: Kevin Schoedel <kpschoedel@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#741122}

chrome/browser/ui/webui/settings/chromeos/device_keyboard_handler.cc

@@ -20,25 +20,33 @@ namespace {
 
 struct KeyboardsStateResult {
   bool has_internal_keyboard = false;
-  bool has_external_non_apple_keyboard = false;
-  bool has_apple_keyboard = false;
+  bool has_external_apple_keyboard = false;
+  bool has_external_chromeos_keyboard = false;
+  bool has_external_generic_keyboard = false;
 };
 
 KeyboardsStateResult GetKeyboardsState() {
   KeyboardsStateResult result;
   for (const ui::InputDevice& keyboard :
        ui::DeviceDataManager::GetInstance()->GetKeyboardDevices()) {
-    result.has_internal_keyboard |=
-        (keyboard.type == ui::INPUT_DEVICE_INTERNAL);
-
-    const ui::EventRewriterChromeOS::DeviceType type =
-        ui::EventRewriterChromeOS::GetDeviceType(keyboard);
-    if (type == ui::EventRewriterChromeOS::kDeviceAppleKeyboard) {
-      result.has_apple_keyboard = true;
-    } else if (type ==
-                   ui::EventRewriterChromeOS::kDeviceExternalNonAppleKeyboard ||
-               type == ui::EventRewriterChromeOS::kDeviceExternalUnknown) {
-      result.has_external_non_apple_keyboard = true;
+    switch (ui::EventRewriterChromeOS::GetDeviceType(keyboard)) {
+      case ui::EventRewriterChromeOS::kDeviceInternalKeyboard:
+        result.has_internal_keyboard = true;
+        break;
+      case ui::EventRewriterChromeOS::kDeviceExternalAppleKeyboard:
+        result.has_external_apple_keyboard = true;
+        break;
+      case ui::EventRewriterChromeOS::kDeviceExternalChromeOsKeyboard:
+        result.has_external_chromeos_keyboard = true;
+        break;
+      case ui::EventRewriterChromeOS::kDeviceExternalGenericKeyboard:
+      case ui::EventRewriterChromeOS::kDeviceExternalUnknown:
+        result.has_external_generic_keyboard = true;
+        break;
+      case ui::EventRewriterChromeOS::kDeviceHotrodRemote:
+      case ui::EventRewriterChromeOS::kDeviceVirtualCoreKeyboard:
+      case ui::EventRewriterChromeOS::kDeviceUnknown:
+        break;
     }
   }
 
@@ -130,8 +138,8 @@ void KeyboardHandler::UpdateShowKeys() {
   // kHasChromeOSKeyboard will be unset on Chromebooks that have standalone Caps
   // Lock keys.
   const KeyboardsStateResult keyboards_state = GetKeyboardsState();
-  const bool has_caps_lock = keyboards_state.has_apple_keyboard ||
-                             keyboards_state.has_external_non_apple_keyboard ||
+  const bool has_caps_lock = keyboards_state.has_external_apple_keyboard ||
+                             keyboards_state.has_external_generic_keyboard ||
                              !base::CommandLine::ForCurrentProcess()->HasSwitch(
                                  chromeos::switches::kHasChromeOSKeyboard);
 
@@ -139,9 +147,10 @@ void KeyboardHandler::UpdateShowKeys() {
   keyboard_params.SetKey("showCapsLock", base::Value(has_caps_lock));
   keyboard_params.SetKey(
       "showExternalMetaKey",
-      base::Value(keyboards_state.has_external_non_apple_keyboard));
-  keyboard_params.SetKey("showAppleCommandKey",
-                         base::Value(keyboards_state.has_apple_keyboard));
+      base::Value(keyboards_state.has_external_generic_keyboard));
+  keyboard_params.SetKey(
+      "showAppleCommandKey",
+      base::Value(keyboards_state.has_external_apple_keyboard));
   keyboard_params.SetKey("hasInternalKeyboard",
                          base::Value(keyboards_state.has_internal_keyboard));
 

chrome/browser/ui/webui/settings/chromeos/device_keyboard_handler_unittest.cc

@@ -219,6 +219,15 @@ TEST_F(KeyboardHandlerTest, ExternalKeyboard) {
       0x05ac, 0x026c, 0x0111);
   fake_udev->AddFakeDevice(external_apple_kbd.name,
                            external_apple_kbd.sys_path.value(), {}, {});
+  // Chrome OS external USB keyboard.
+  const ui::InputDevice external_chromeos_kbd(
+      4, ui::INPUT_DEVICE_USB, "LG USB Keyboard", "",
+      base::FilePath("/devices/pci0000:00/0000:00:14.0/usb1/1-1/1-1:1.0/"
+                     "0003:04CA:0082.000B/input/input4"),
+      0x04ca, 0x0082, 0x0111);
+  fake_udev->AddFakeDevice(external_chromeos_kbd.name,
+                           external_chromeos_kbd.sys_path.value(), {},
+                           {{"CROS_KEYBOARD_TOP_ROW_LAYOUT", "1"}});
 
   // An internal keyboard shouldn't change the defaults.
   base::CommandLine::ForCurrentProcess()->AppendSwitch(
@@ -241,6 +250,16 @@ TEST_F(KeyboardHandlerTest, ExternalKeyboard) {
   EXPECT_FALSE(HasAppleCommandKey());
   EXPECT_FALSE(HasAssistantKey());
 
+  // However when connecting external ChromeOS-branded keyboard, we should not
+  // see neither CapsLock not meta keys.
+  device_data_manager_test_api_.SetKeyboardDevices(
+      std::vector<ui::InputDevice>{internal_kbd, external_chromeos_kbd});
+  EXPECT_TRUE(HasInternalSearchKey());
+  EXPECT_FALSE(HasCapsLock());
+  EXPECT_FALSE(HasExternalMetaKey());
+  EXPECT_FALSE(HasAppleCommandKey());
+  EXPECT_FALSE(HasAssistantKey());
+
   // Simulate an external Apple keyboard being connected. Now users can remap
   // the command key.
   device_data_manager_test_api_.SetKeyboardDevices(
@@ -266,7 +285,7 @@ TEST_F(KeyboardHandlerTest, ExternalKeyboard) {
   // should show the capslock and external meta remapping.
   // https://crbug.com/834594.
   device_data_manager_test_api_.SetKeyboardDevices(std::vector<ui::InputDevice>{
-      {4, ui::INPUT_DEVICE_USB, "Topre Corporation Realforce 87", "",
+      {5, ui::INPUT_DEVICE_USB, "Topre Corporation Realforce 87", "",
        external_generic_kbd.sys_path, 0x046d, 0xc31c, 0x0111}});
   EXPECT_FALSE(HasInternalSearchKey());
   EXPECT_TRUE(HasCapsLock());

ui/chromeos/events/event_rewriter_chromeos.h

@@ -44,8 +44,10 @@ class EventRewriterChromeOS : public ui::EventRewriter {
  public:
   enum DeviceType {
     kDeviceUnknown = 0,
-    kDeviceAppleKeyboard,
-    kDeviceExternalNonAppleKeyboard,
+    kDeviceInternalKeyboard,
+    kDeviceExternalAppleKeyboard,
+    kDeviceExternalChromeOsKeyboard,
+    kDeviceExternalGenericKeyboard,
     kDeviceExternalUnknown,
     kDeviceHotrodRemote,
     kDeviceVirtualCoreKeyboard,  // X-server generated events.
@@ -122,14 +124,16 @@ class EventRewriterChromeOS : public ui::EventRewriter {
                         ui::EventRewriter* sticky_keys_controller);
   ~EventRewriterChromeOS() override;
 
-  static DeviceType GetDeviceType(const ui::InputDevice& keyboard_device);
-
   // Calls KeyboardDeviceAddedInternal.
   void KeyboardDeviceAddedForTesting(
       int device_id,
       const std::string& device_name,
-      KeyboardTopRowLayout layout = kKbdTopRowLayoutDefault,
-      InputDeviceType device_type = INPUT_DEVICE_UNKNOWN);
+      const std::string& layout_string = std::string(),
+      InputDeviceType device_type = INPUT_DEVICE_INTERNAL);
+
+  // Reset the internal rewriter state so that next set of tests can be ran on
+  // the same rewriter, if needed.
+  void ResetStateForTesting();
 
   // Calls RewriteMouseEvent().
   void RewriteMouseButtonEventForTesting(
@@ -162,17 +166,19 @@ class EventRewriterChromeOS : public ui::EventRewriter {
       const MutableKeyState& state,
       std::unique_ptr<ui::Event>* rewritten_event);
 
-  // Given the file path of a keyboard device, returns true if we get back
-  // the layout type of the top row keys without getting an error. Type
-  // value is stored in |out_layout|.
-  static bool GetKeyboardTopRowLayout(const base::FilePath& device_path,
-                                      KeyboardTopRowLayout* out_layout)
-      WARN_UNUSED_RESULT;
-
-  // Given the file path of a keyboard device, returns true if we get back
-  // the Assistant key property without getting an error. Property value
-  // is stored in |has_assistant_key|.
-  static bool HasAssistantKeyOnKeyboard(const base::FilePath& device_path,
+  // Given a keyboard device, returns its type.
+  static DeviceType GetDeviceType(const ui::InputDevice& keyboard_device);
+
+  // Given a keyboard device, returns its top row layout. Will return default
+  // kKbdTopRowLayoutDefault if the device is not tagged with a specific
+  // layout, or when failing to retrieve device layout from udev.
+  static KeyboardTopRowLayout GetKeyboardTopRowLayout(
+      const ui::InputDevice& keyboard_device);
+
+  // Given a keyboard device, returns true if we get back the Assistant key
+  // property without getting an error. Property value is stored in
+  // |has_assistant_key|.
+  static bool HasAssistantKeyOnKeyboard(const ui::InputDevice& keyboard_device,
                                         bool* has_assistant_key);
 
   // Part of rewrite phases below. This method is public only so that
@@ -194,8 +200,8 @@ class EventRewriterChromeOS : public ui::EventRewriter {
   bool ForceTopRowAsFunctionKeys() const;
 
   // Adds a device to |device_id_to_info_| only if no failure occurs in
-  // retrieving the top row layout from udev, and returns the device type of
-  // this keyboard even if it wasn't stored in |device_id_to_info_|.
+  // identifying the keyboard, and returns the device type of this keyboard
+  // even if it wasn't stored in |device_id_to_info_|.
   DeviceType KeyboardDeviceAdded(int device_id);
 
   // Inserts a new entry to |device_id_to_info_|.

ui/chromeos/events/keyboard_layout_util.cc

@@ -12,10 +12,8 @@ namespace ui {
 bool DeviceUsesKeyboardLayout2() {
   for (const InputDevice& keyboard :
        DeviceDataManager::GetInstance()->GetKeyboardDevices()) {
-    EventRewriterChromeOS::KeyboardTopRowLayout layout;
-    if (EventRewriterChromeOS::GetKeyboardTopRowLayout(keyboard.sys_path,
-                                                       &layout) &&
-        layout == EventRewriterChromeOS::kKbdTopRowLayout2) {
+    if (EventRewriterChromeOS::GetKeyboardTopRowLayout(keyboard) ==
+        EventRewriterChromeOS::kKbdTopRowLayout2) {
       return true;
     }
   }
@@ -27,7 +25,7 @@ bool DeviceKeyboardHasAssistantKey() {
   for (const InputDevice& keyboard :
        DeviceDataManager::GetInstance()->GetKeyboardDevices()) {
     bool has_assistant_key = false;
-    if (EventRewriterChromeOS::HasAssistantKeyOnKeyboard(keyboard.sys_path,
+    if (EventRewriterChromeOS::HasAssistantKeyOnKeyboard(keyboard,
                                                          &has_assistant_key) &&
         has_assistant_key) {
       return true;