(Reland) Hide option to remap internal search when no internal KB is connected

To avoid confusion when there is no internal keyboard connected:
- We should not show the option to remap the internal "Search"
key since that key doesn't exist.
- We should also avoid labeling the options to remap the external
meta keys as "External Meta" or "External Command", and just say
"Meta" or "Command" as "External" here is redundant.

TBR=stevenjb@chromium.org
BUG=890522, 890950
TEST=Expanded existing tests.

Reviewed-on: https://chromium-review.googlesource.com/c/1253022Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#599409}
Change-Id: I3136efe54d2544423e4dd9b23fe8ffabffe3caf0
Reviewed-on: https://chromium-review.googlesource.com/c/1292890Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601403}

chrome/app/settings_strings.grdp

@@ -3916,9 +3916,15 @@
     <message name="IDS_SETTINGS_KEYBOARD_KEY_EXTERNAL_COMMAND" desc="In Device Settings, the dropdown list item for the external Command key on Apple keyboards.">
       External Command
     </message>
+    <message name="IDS_SETTINGS_KEYBOARD_KEY_COMMAND" desc="In Device Settings, the dropdown list item for the external Command key on Apple keyboards when there's no internal keyboard on the device.">
+      Command
+    </message>
     <message name="IDS_SETTINGS_KEYBOARD_KEY_EXTERNAL_META" desc="In Device Settings, the dropdown list item for the external Meta key (Search key on ChromeOS keyboards, and Windows key on Windows keyboards).">
       External Meta
     </message>
+    <message name="IDS_SETTINGS_KEYBOARD_KEY_META" desc="In Device Settings, the dropdown list item for the external Meta key (Search key on ChromeOS keyboards, and Windows key on Windows keyboards) when there's no internal keyboard on the device.">
+      Meta
+    </message>
     <message name="IDS_SETTINGS_KEYBOARD_SEND_FUNCTION_KEYS" desc="In Device Settings, the checkbox label for interpreting the top-row keys as function keys instead.">
       Treat top-row keys as function keys
     </message>

chrome/browser/resources/settings/device_page/keyboard.html

@@ -13,13 +13,15 @@
 <dom-module id="settings-keyboard">
   <template>
     <style include="settings-shared"></style>
-    <div class="settings-box first">
-      <div class="start">$i18n{keyboardKeySearch}</div>
-      <settings-dropdown-menu label="$i18n{keyboardKeySearch}"
-          pref="{{prefs.settings.language.xkb_remap_search_key_to}}"
-          menu-options="[[keyMapTargets_]]">
-      </settings-dropdown-menu>
-    </div>
+    <template is="dom-if" if="[[hasInternalKeyboard_]]">
+      <div class="settings-box first" id="internalSearchKey">
+        <div class="start">$i18n{keyboardKeySearch}</div>
+        <settings-dropdown-menu label="$i18n{keyboardKeySearch}"
+            pref="{{prefs.settings.language.xkb_remap_search_key_to}}"
+            menu-options="[[keyMapTargets_]]">
+        </settings-dropdown-menu>
+      </div>
+    </template>
     <div class="settings-box">
       <div class="start">$i18n{keyboardKeyCtrl}</div>
       <settings-dropdown-menu label="$i18n{keyboardKeyCtrl}"
@@ -68,8 +70,11 @@
     </template>
     <template is="dom-if" if="[[showExternalMetaKey_]]">
       <div class="settings-box" id="externalMetaKey">
-        <div class="start">$i18n{keyboardKeyExternalMeta}</div>
-        <settings-dropdown-menu label="$i18n{keyboardKeyExternalMeta}"
+        <div class="start">
+          [[getExternalMetaKeyLabel_(hasInternalKeyboard_)]]
+        </div>
+        <settings-dropdown-menu
+            label="[[getExternalMetaKeyLabel_(hasInternalKeyboard_)]]"
             pref="{{prefs.settings.language.remap_external_meta_key_to}}"
             menu-options="[[keyMapTargets_]]">
         </settings-dropdown-menu>
@@ -77,9 +82,11 @@
     </template>
     <template is="dom-if" if="[[showAppleCommandKey_]]">
       <div class="settings-box" id="externalCommandKey">
-        <div class="start">$i18n{keyboardKeyExternalCommand}</div>
+        <div class="start">
+          [[getExternalCommandKeyLabel_(hasInternalKeyboard_)]]
+        </div>
         <settings-dropdown-menu
-            label="$i18n{keyboardKeyExternalCommand}"
+            label="[[getExternalCommandKeyLabel_(hasInternalKeyboard_)]]"
             pref="{{prefs.settings.language.remap_external_command_key_to}}"
             menu-options="[[keyMapTargets_]]">
         </settings-dropdown-menu>

chrome/browser/resources/settings/device_page/keyboard.js

@@ -39,6 +39,9 @@ Polymer({
     /** @private Whether to show diamond key options. */
     showDiamondKey_: Boolean,
 
+    /** @private Whether this device has an internal keyboard. */
+    hasInternalKeyboard_: Boolean,
+
     /**
      * Whether to show a remapping option for external keyboard's Meta key
      * (Search/Windows keys). This is true only when there's an external
@@ -133,6 +136,7 @@ Polymer({
    * @private
    */
   onShowKeysChange_: function(keyboardParams) {
+    this.hasInternalKeyboard_ = keyboardParams['hasInternalKeyboard'];
     this.showCapsLock_ = keyboardParams['showCapsLock'];
     this.showDiamondKey_ = keyboardParams['showDiamondKey'];
     this.showExternalMetaKey_ = keyboardParams['showExternalMetaKey'];
@@ -147,4 +151,15 @@ Polymer({
   onShowLanguageInputTap_: function() {
     settings.navigateTo(settings.routes.LANGUAGES);
   },
+
+  getExternalMetaKeyLabel_: function(hasInternalKeyboard) {
+    return loadTimeData.getString(
+        hasInternalKeyboard ? 'keyboardKeyExternalMeta' : 'keyboardKeyMeta');
+  },
+
+  getExternalCommandKeyLabel_: function(hasInternalKeyboard) {
+    return loadTimeData.getString(
+        hasInternalKeyboard ? 'keyboardKeyExternalCommand' :
+                              'keyboardKeyCommand');
+  },
 });

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

@@ -20,6 +20,7 @@
 namespace {
 
 struct KeyboardsStateResult {
+  bool has_internal_keyboard = false;
   bool has_external_non_apple_keyboard = false;
   bool has_apple_keyboard = false;
 };
@@ -28,6 +29,9 @@ KeyboardsStateResult GetKeyboardsState() {
   KeyboardsStateResult result;
   for (const ui::InputDevice& keyboard :
        ui::InputDeviceManager::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) {
@@ -146,6 +150,8 @@ void KeyboardHandler::UpdateShowKeys() {
       base::Value(keyboards_state.has_external_non_apple_keyboard));
   keyboard_params.SetKey("showAppleCommandKey",
                          base::Value(keyboards_state.has_apple_keyboard));
+  keyboard_params.SetKey("hasInternalKeyboard",
+                         base::Value(keyboards_state.has_internal_keyboard));
 
   FireWebUIListener(kShowKeysChangedName, keyboard_params);
 }

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

@@ -47,7 +47,8 @@ class KeyboardHandlerTest : public testing::Test {
   bool GetLastShowKeysChangedMessage(bool* has_caps_lock_out,
                                      bool* has_diamond_key_out,
                                      bool* has_external_meta_key_out,
-                                     bool* has_apple_command_key_out)
+                                     bool* has_apple_command_key_out,
+                                     bool* has_internal_search_out)
       WARN_UNUSED_RESULT {
     for (auto it = web_ui_.call_data().rbegin();
          it != web_ui_.call_data().rend(); ++it) {
@@ -70,6 +71,7 @@ class KeyboardHandlerTest : public testing::Test {
           {"showDiamondKey", has_diamond_key_out},
           {"showExternalMetaKey", has_external_meta_key_out},
           {"showAppleCommandKey", has_apple_command_key_out},
+          {"hasInternalKeyboard", has_internal_search_out},
       };
 
       for (const auto& pair : path_to_out_param) {
@@ -92,7 +94,7 @@ class KeyboardHandlerTest : public testing::Test {
     bool has_caps_lock = false;
     bool ignored = false;
     if (!GetLastShowKeysChangedMessage(&has_caps_lock, &ignored, &ignored,
-                                       &ignored)) {
+                                       &ignored, &ignored)) {
       ADD_FAILURE() << "Didn't get " << KeyboardHandler::kShowKeysChangedName;
       return false;
     }
@@ -106,7 +108,7 @@ class KeyboardHandlerTest : public testing::Test {
     bool has_diamond_key = false;
     bool ignored = false;
     if (!GetLastShowKeysChangedMessage(&ignored, &has_diamond_key, &ignored,
-                                       &ignored)) {
+                                       &ignored, &ignored)) {
       ADD_FAILURE() << "Didn't get " << KeyboardHandler::kShowKeysChangedName;
       return false;
     }
@@ -120,7 +122,7 @@ class KeyboardHandlerTest : public testing::Test {
     bool has_external_meta = false;
     bool ignored = false;
     if (!GetLastShowKeysChangedMessage(&ignored, &ignored, &has_external_meta,
-                                       &ignored)) {
+                                       &ignored, &ignored)) {
       ADD_FAILURE() << "Didn't get " << KeyboardHandler::kShowKeysChangedName;
       return false;
     }
@@ -134,13 +136,27 @@ class KeyboardHandlerTest : public testing::Test {
     bool has_apple_command_key = false;
     bool ignored = false;
     if (!GetLastShowKeysChangedMessage(&ignored, &ignored, &ignored,
-                                       &has_apple_command_key)) {
+                                       &has_apple_command_key, &ignored)) {
       ADD_FAILURE() << "Didn't get " << KeyboardHandler::kShowKeysChangedName;
       return false;
     }
     return has_apple_command_key;
   }
 
+  // Returns true if the last keys-changed message reported that the device has
+  // an internal keyboard and hence an internal Search key remap option.
+  // A failure is added if a message wasn't found.
+  bool HasInternalSearchKey() {
+    bool has_internal_search_key = false;
+    bool ignored = false;
+    if (!GetLastShowKeysChangedMessage(&ignored, &ignored, &ignored, &ignored,
+                                       &has_internal_search_key)) {
+      ADD_FAILURE() << "Didn't get " << KeyboardHandler::kShowKeysChangedName;
+      return false;
+    }
+    return has_internal_search_key;
+  }
+
   ws::InputDeviceClientTestApi input_device_client_test_api_;
   content::TestWebUI web_ui_;
   TestKeyboardHandler handler_;
@@ -154,6 +170,7 @@ TEST_F(KeyboardHandlerTest, DefaultKeys) {
   base::CommandLine::ForCurrentProcess()->AppendSwitch(
       chromeos::switches::kHasChromeOSKeyboard);
   handler_test_api_.Initialize();
+  EXPECT_FALSE(HasInternalSearchKey());
   EXPECT_FALSE(HasCapsLock());
   EXPECT_FALSE(HasDiamondKey());
   EXPECT_FALSE(HasExternalMetaKey());
@@ -164,6 +181,7 @@ TEST_F(KeyboardHandlerTest, NonChromeOSKeyboard) {
   // If kHasChromeOSKeyboard isn't passed, we should assume there's a Caps Lock
   // key.
   handler_test_api_.Initialize();
+  EXPECT_FALSE(HasInternalSearchKey());
   EXPECT_TRUE(HasCapsLock());
   EXPECT_FALSE(HasDiamondKey());
   EXPECT_FALSE(HasExternalMetaKey());
@@ -177,6 +195,7 @@ TEST_F(KeyboardHandlerTest, ExternalKeyboard) {
   input_device_client_test_api_.SetKeyboardDevices(std::vector<ui::InputDevice>{
       {1, ui::INPUT_DEVICE_INTERNAL, "internal keyboard"}});
   handler_test_api_.Initialize();
+  EXPECT_TRUE(HasInternalSearchKey());
   EXPECT_FALSE(HasCapsLock());
   EXPECT_FALSE(HasDiamondKey());
   EXPECT_FALSE(HasExternalMetaKey());
@@ -185,7 +204,9 @@ TEST_F(KeyboardHandlerTest, ExternalKeyboard) {
   // Simulate an external keyboard being connected. We should assume there's a
   // Caps Lock and Meta keys now.
   input_device_client_test_api_.SetKeyboardDevices(std::vector<ui::InputDevice>{
+      {1, ui::INPUT_DEVICE_INTERNAL, "internal keyboard"},
       {2, ui::INPUT_DEVICE_USB, "external keyboard"}});
+  EXPECT_TRUE(HasInternalSearchKey());
   EXPECT_TRUE(HasCapsLock());
   EXPECT_FALSE(HasDiamondKey());
   EXPECT_TRUE(HasExternalMetaKey());
@@ -194,7 +215,9 @@ TEST_F(KeyboardHandlerTest, ExternalKeyboard) {
   // Simulate an external Apple keyboard being connected. Now users can remap
   // the command key.
   input_device_client_test_api_.SetKeyboardDevices(std::vector<ui::InputDevice>{
+      {1, ui::INPUT_DEVICE_INTERNAL, "internal keyboard"},
       {3, ui::INPUT_DEVICE_USB, "Apple Inc. Apple Keyboard"}});
+  EXPECT_TRUE(HasInternalSearchKey());
   EXPECT_TRUE(HasCapsLock());
   EXPECT_FALSE(HasDiamondKey());
   EXPECT_FALSE(HasExternalMetaKey());
@@ -205,6 +228,7 @@ TEST_F(KeyboardHandlerTest, ExternalKeyboard) {
   input_device_client_test_api_.SetKeyboardDevices(std::vector<ui::InputDevice>{
       {2, ui::INPUT_DEVICE_USB, "external keyboard"},
       {3, ui::INPUT_DEVICE_USB, "Apple Inc. Apple Keyboard"}});
+  EXPECT_FALSE(HasInternalSearchKey());
   EXPECT_TRUE(HasCapsLock());
   EXPECT_FALSE(HasDiamondKey());
   EXPECT_TRUE(HasExternalMetaKey());
@@ -216,6 +240,7 @@ TEST_F(KeyboardHandlerTest, ExternalKeyboard) {
   // https://crbug.com/834594.
   input_device_client_test_api_.SetKeyboardDevices(std::vector<ui::InputDevice>{
       {4, ui::INPUT_DEVICE_USB, "Topre Corporation Realforce 87"}});
+  EXPECT_FALSE(HasInternalSearchKey());
   EXPECT_TRUE(HasCapsLock());
   EXPECT_FALSE(HasDiamondKey());
   EXPECT_TRUE(HasExternalMetaKey());
@@ -223,6 +248,7 @@ TEST_F(KeyboardHandlerTest, ExternalKeyboard) {
 
   // Disconnect the external keyboard and check that the key goes away.
   input_device_client_test_api_.SetKeyboardDevices({});
+  EXPECT_FALSE(HasInternalSearchKey());
   EXPECT_FALSE(HasCapsLock());
   EXPECT_FALSE(HasDiamondKey());
   EXPECT_FALSE(HasExternalMetaKey());

chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc

@@ -634,6 +634,7 @@ void AddDeviceStrings(content::WebUIDataSource* html_source) {
       {"keyboardKeyCtrl", IDS_SETTINGS_KEYBOARD_KEY_LEFT_CTRL},
       {"keyboardKeyAlt", IDS_SETTINGS_KEYBOARD_KEY_LEFT_ALT},
       {"keyboardKeyCapsLock", IDS_SETTINGS_KEYBOARD_KEY_CAPS_LOCK},
+      {"keyboardKeyCommand", IDS_SETTINGS_KEYBOARD_KEY_COMMAND},
       {"keyboardKeyDiamond", IDS_SETTINGS_KEYBOARD_KEY_DIAMOND},
       {"keyboardKeyEscape", IDS_SETTINGS_KEYBOARD_KEY_ESCAPE},
       {"keyboardKeyBackspace", IDS_SETTINGS_KEYBOARD_KEY_BACKSPACE},
@@ -641,6 +642,7 @@ void AddDeviceStrings(content::WebUIDataSource* html_source) {
       {"keyboardKeyExternalCommand",
        IDS_SETTINGS_KEYBOARD_KEY_EXTERNAL_COMMAND},
       {"keyboardKeyExternalMeta", IDS_SETTINGS_KEYBOARD_KEY_EXTERNAL_META},
+      {"keyboardKeyMeta", IDS_SETTINGS_KEYBOARD_KEY_META},
       {"keyboardSendFunctionKeys", IDS_SETTINGS_KEYBOARD_SEND_FUNCTION_KEYS},
       {"keyboardSendFunctionKeysDescription",
        ui::DeviceUsesKeyboardLayout2()

chrome/test/data/webui/settings/device_page_tests.js

@@ -610,15 +610,17 @@ cr.define('device_page_tests', function() {
             expectFalse(!!keyboardPage.$$('#capsLockKey'));
             expectFalse(!!keyboardPage.$$('#diamondKey'));
 
-            // Pretend the diamond key is available.
+            // Pretend the diamond key is available, and no internal keyboard.
             let keyboardParams = {
               'showCapsLock': false,
               'showDiamondKey': true,
               'showExternalMetaKey': false,
               'showAppleCommandKey': false,
+              'hasInternalKeyboard': false,
             };
             cr.webUIListenerCallback('show-keys-changed', keyboardParams);
             Polymer.dom.flush();
+            expectFalse(!!keyboardPage.$$('#internalSearchKey'));
             expectFalse(!!keyboardPage.$$('#capsLockKey'));
             expectTrue(!!keyboardPage.$$('#diamondKey'));
             expectFalse(!!keyboardPage.$$('#externalMetaKey'));
@@ -628,6 +630,7 @@ cr.define('device_page_tests', function() {
             keyboardParams['showCapsLock'] = true;
             cr.webUIListenerCallback('show-keys-changed', keyboardParams);
             Polymer.dom.flush();
+            expectFalse(!!keyboardPage.$$('#internalSearchKey'));
             expectTrue(!!keyboardPage.$$('#capsLockKey'));
             expectTrue(!!keyboardPage.$$('#diamondKey'));
             expectFalse(!!keyboardPage.$$('#externalMetaKey'));
@@ -637,6 +640,7 @@ cr.define('device_page_tests', function() {
             keyboardParams['showExternalMetaKey'] = true;
             cr.webUIListenerCallback('show-keys-changed', keyboardParams);
             Polymer.dom.flush();
+            expectFalse(!!keyboardPage.$$('#internalSearchKey'));
             expectTrue(!!keyboardPage.$$('#capsLockKey'));
             expectTrue(!!keyboardPage.$$('#diamondKey'));
             expectTrue(!!keyboardPage.$$('#externalMetaKey'));
@@ -646,6 +650,17 @@ cr.define('device_page_tests', function() {
             keyboardParams['showAppleCommandKey'] = true;
             cr.webUIListenerCallback('show-keys-changed', keyboardParams);
             Polymer.dom.flush();
+            expectFalse(!!keyboardPage.$$('#internalSearchKey'));
+            expectTrue(!!keyboardPage.$$('#capsLockKey'));
+            expectTrue(!!keyboardPage.$$('#diamondKey'));
+            expectTrue(!!keyboardPage.$$('#externalMetaKey'));
+            expectTrue(!!keyboardPage.$$('#externalCommandKey'));
+
+            // Add an internal keyboard.
+            keyboardParams['hasInternalKeyboard'] = true;
+            cr.webUIListenerCallback('show-keys-changed', keyboardParams);
+            Polymer.dom.flush();
+            expectTrue(!!keyboardPage.$$('#internalSearchKey'));
             expectTrue(!!keyboardPage.$$('#capsLockKey'));
             expectTrue(!!keyboardPage.$$('#diamondKey'));
             expectTrue(!!keyboardPage.$$('#externalMetaKey'));