[MD extensions] shortcuts instructions and feedback

This CL provides a line of instructions for creating extensions shortcuts.
It also adds interactive feedback during shortcut entry.

Bug: 769576
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I419cb1f4e3ee63aa47fc56ca784faa5ec9f2fdb6
Reviewed-on: https://chromium-review.googlesource.com/748082
Commit-Queue: Dave Schuyler <dschuyler@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513256}

chrome/app/md_extensions_strings.grdp

@@ -199,6 +199,41 @@
   <message name="IDS_MD_EXTENSIONS_TYPE_A_SHORTCUT" desc="The prompt to the user to enter a keyboard shortcut in order to assign it to an extension.">
     Type a shortcut
   </message>
+<if expr="is_macosx">
+  <message name="IDS_MD_EXTENSIONS_SHORTCUT_INSTRUCTIONS" desc="Instructions explaining that shortcuts must start with either the Control, Alt, or Command key.">
+    Shortcuts must start with Ctrl, Alt, or Command
+  </message>
+  <message name="IDS_MD_EXTENSIONS_INCLUDE_START_MODIFIER" desc="Error message explaining that shortcuts must start with either the Control, Alt, or Command key.">
+    Include Ctrl, Alt, or Command
+  </message>
+</if>
+<if expr="chromeos">
+  <message name="IDS_MD_EXTENSIONS_SHORTCUT_INSTRUCTIONS" desc="Instructions explaining that shortcuts must start with either the Control, Alt, or Search key.">
+    Shortcuts must start with Ctrl, Alt, or Search
+  </message>
+  <message name="IDS_MD_EXTENSIONS_INCLUDE_START_MODIFIER" desc="Error message explaining that shortcuts must start with either the Control, Alt, or Search key.">
+    Include Ctrl, Alt, or Search
+  </message>
+</if>
+<if expr="is_macosx or chromeos">
+  <message name="IDS_MD_EXTENSIONS_TOO_MANY_MODIFIERS" desc="Error message explaining not to use so many modifiers in a shortcut.">
+    Invalid combination
+  </message>
+</if>
+<if expr="not is_macosx and not chromeos">
+  <message name="IDS_MD_EXTENSIONS_SHORTCUT_INSTRUCTIONS" desc="Instructions explaining that shortcuts must start with either the Control key or the Alt key.">
+    Shortcuts must start with either Ctrl or Alt
+  </message>
+  <message name="IDS_MD_EXTENSIONS_INCLUDE_START_MODIFIER" desc="Error message explaining that shortcuts must start with either the Control key or the Alt key.">
+    Include either Ctrl or Alt
+  </message>
+  <message name="IDS_MD_EXTENSIONS_TOO_MANY_MODIFIERS" desc="Error message explaining not to use both Ctrl and Alt in a shortcut.">
+    Either, not both Ctrl and Alt
+  </message>
+</if>
+<message name="IDS_MD_EXTENSIONS_NEED_CHARACTER" desc="Error message explaining that a shortcut needs a character. This is only shown if a valid modifier is already entered.">
+  Need a character
+</message>
 <if expr="chromeos">
   <!-- Extensions Kiosk apps -->
   <message name="IDS_MD_EXTENSIONS_MANAGE_KIOSK_APP" desc="Label of the button to bring up kiosk management overlay on chrome extensions page.">

chrome/browser/resources/md_extensions/keyboard_shortcuts.html

@@ -14,24 +14,34 @@
   <template>
     <style include="md-select cr-shared-style">
       :host {
+        --card-max-width: 928px;
+        --card-min-width: 600px;
         height: 100%;
       }
 
-      #container {
-        height: 100%;
-        overflow: overlay;
-        padding-top: 24px;
+      #instructions,
+      .shortcut-card {
+        margin: 0 auto 16px auto;
+        max-width: var(--card-max-width);
+        min-width: var(--card-min-width);
+        width: 90%;
+      }
+
+      #instructions {
+        @apply --cr-title-text;
       }
 
       .shortcut-card {
-        @apply(--cr-primary-text);
-        @apply(--shadow-elevation-2dp);
+        @apply --cr-primary-text;
+        @apply --shadow-elevation-2dp;
         background-color: white;
-        margin: 0 auto 16px auto;
-        max-width: 928px;
-        min-width: 600px;
         padding-bottom: 8px;
-        width: 90%;
+      }
+
+      #container {
+        height: 100%;
+        overflow: overlay;
+        padding-top: 24px;
       }
 
       .command-entry {
@@ -65,7 +75,7 @@
         display: flex;
         margin-bottom: 9px;
         padding: 16px var(--cr-section-padding);
-        @apply(--cr-title-text);
+        @apply --cr-title-text;
       }
 
       .icon {
@@ -82,6 +92,7 @@
       }
     </style>
     <div id="container">
+      <div id="instructions">$i18n{shortcutInstructions}</div>
       <template is="dom-repeat" items="[[calculateShownItems_(items.*)]]">
         <div class="shortcut-card">
           <div class="card-title">

chrome/browser/resources/md_extensions/shortcut_input.html

@@ -25,7 +25,7 @@
           margin-bottom: 0px;
           margin-top: 2px;  /* Offset underline spacing. */
           padding: 0;
-          @apply(--cr-primary-text);
+          @apply --cr-primary-text;
         };
       }
 
@@ -38,8 +38,12 @@
     </style>
     <div id="main">
       <paper-input id="input" placeholder="$i18n{shortcutTypeAShortcut}"
-        value="[[computeText_(capturing_, shortcut, pendingShortcut_)]]"
-        no-label-float>
+          error-message="[[getErrorString_(error_,
+              '$i18nPolymer{shortcutIncludeStartModifier}',
+              '$i18nPolymer{shortcutTooManyModifiers}',
+              '$i18nPolymer{shortcutNeedCharacter}')]]"
+          value="[[computeText_(capturing_, shortcut, pendingShortcut_)]]"
+          no-label-float>
       </paper-input>
       <button id="clear" is="paper-icon-button-light"
           class="icon-clear no-overlap" on-tap="onClearTap_"

chrome/browser/resources/md_extensions/shortcut_input.js

@@ -5,6 +5,14 @@
 cr.define('extensions', function() {
   'use strict';
 
+  /** @enum {number} */
+  const ShortcutError = {
+    NO_ERROR: 0,
+    INCLUDE_START_MODIFIER: 1,
+    TOO_MANY_MODIFIERS: 2,
+    NEED_CHARACTER: 3,
+  };
+
   // The UI to display and manage keyboard shortcuts set for extension commands.
   const ShortcutInput = Polymer({
     is: 'extensions-shortcut-input',
@@ -36,6 +44,12 @@ cr.define('extensions', function() {
         value: false,
       },
 
+      /** @private {!ShortcutError} */
+      error_: {
+        type: Number,
+        value: 0,
+      },
+
       /** @private */
       pendingShortcut_: {
         type: String,
@@ -67,7 +81,9 @@ cr.define('extensions', function() {
         return;
       this.pendingShortcut_ = '';
       this.capturing_ = false;
-      this.$['input'].blur();
+      const input = this.$.input;
+      input.blur();
+      input.invalid = false;
       this.delegate.setShortcutHandlingSuspended(false);
     },
 
@@ -109,6 +125,23 @@ cr.define('extensions', function() {
       this.handleKey_(e);
     },
 
+    /**
+     * @param {!ShortcutError} error
+     * @param {string} includeStartModifier
+     * @param {string} tooManyModifiers
+     * @param {string} needCharacter
+     * @return {string} UI string.
+     * @private
+     */
+    getErrorString_: function(
+        error, includeStartModifier, tooManyModifiers, needCharacter) {
+      if (error == ShortcutError.TOO_MANY_MODIFIERS)
+        return tooManyModifiers;
+      if (error == ShortcutError.NEED_CHARACTER)
+        return needCharacter;
+      return includeStartModifier;
+    },
+
     /**
      * @param {!KeyboardEvent} e
      * @private
@@ -123,17 +156,27 @@ cr.define('extensions', function() {
       // We don't allow both Ctrl and Alt in the same keybinding.
       // TODO(devlin): This really should go in extensions.hasValidModifiers,
       // but that requires updating the existing page as well.
-      if ((e.ctrlKey && e.altKey) || !extensions.hasValidModifiers(e)) {
-        this.pendingShortcut_ = 'invalid';
+      if (e.ctrlKey && e.altKey) {
+        this.error_ = ShortcutError.TOO_MANY_MODIFIERS;
+        this.$.input.invalid = true;
+        return;
+      }
+      if (!extensions.hasValidModifiers(e)) {
+        this.pendingShortcut_ = '';
+        this.error_ = ShortcutError.INCLUDE_START_MODIFIER;
+        this.$.input.invalid = true;
         return;
       }
-
       this.pendingShortcut_ = extensions.keystrokeToString(e);
-
-      if (extensions.isValidKeyCode(e.keyCode)) {
-        this.commitPending_();
-        this.endCapture_();
+      if (!extensions.isValidKeyCode(e.keyCode)) {
+        this.error_ = ShortcutError.NEED_CHARACTER;
+        this.$.input.invalid = true;
+        return;
       }
+      this.$.input.invalid = false;
+
+      this.commitPending_();
+      this.endCapture_();
     },
 
     /** @private */

chrome/browser/ui/webui/extensions/extensions_ui.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/ui/webui/extensions/extensions_ui.h"
 
 #include <memory>
+#include <utility>
 
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
@@ -257,6 +258,14 @@ content::WebUIDataSource* CreateMdExtensionsSource() {
                              IDS_MD_EXTENSIONS_SHORTCUT_SCOPE_IN_CHROME);
   source->AddLocalizedString("shortcutTypeAShortcut",
                              IDS_MD_EXTENSIONS_TYPE_A_SHORTCUT);
+  source->AddLocalizedString("shortcutInstructions",
+                             IDS_MD_EXTENSIONS_SHORTCUT_INSTRUCTIONS);
+  source->AddLocalizedString("shortcutIncludeStartModifier",
+                             IDS_MD_EXTENSIONS_INCLUDE_START_MODIFIER);
+  source->AddLocalizedString("shortcutTooManyModifiers",
+                             IDS_MD_EXTENSIONS_TOO_MANY_MODIFIERS);
+  source->AddLocalizedString("shortcutNeedCharacter",
+                             IDS_MD_EXTENSIONS_NEED_CHARACTER);
   source->AddString(
       "suspiciousInstallHelpUrl",
       base::ASCIIToUTF16(google_util::AppendGoogleLocaleParam(

chrome/test/data/webui/extensions/extension_shortcut_input_test.js

@@ -26,7 +26,7 @@ cr.define('extension_shortcut_input_tests', function() {
   }
 
   /** @enum {string} */
-  var TestNames = {
+  const TestNames = {
     Basic: 'basic',
   };
 
@@ -44,12 +44,9 @@ cr.define('extension_shortcut_input_tests', function() {
     });
 
     test(assert(TestNames.Basic), function() {
-      var field = input.$['input'];
-      var fieldText = function() {
-        return field.value;
-      };
-      expectEquals('', fieldText());
-      var isClearVisible =
+      const field = input.$['input'];
+      expectEquals('', field.value);
+      const isClearVisible =
           extension_test_util.isVisible.bind(null, input, '#clear', false);
       expectFalse(isClearVisible());
 
@@ -60,24 +57,36 @@ cr.define('extension_shortcut_input_tests', function() {
             assertTrue(arg);
             input.delegate.reset();
 
-            expectEquals('', fieldText());
+            expectEquals('', field.value);
             expectFalse(isClearVisible());
 
+            // Press character.
+            MockInteractions.keyDownOn(field, 'A', []);
+            expectEquals('', field.value);
+            expectTrue(field.errorMessage.startsWith('Include'));
+            // Add shift to character.
+            MockInteractions.keyDownOn(field, 'A', ['shift']);
+            expectEquals('', field.value);
+            expectTrue(field.errorMessage.startsWith('Include'));
             // Press ctrl.
             MockInteractions.keyDownOn(field, 17, ['ctrl']);
-            expectEquals('Ctrl', fieldText());
+            expectEquals('Ctrl', field.value);
+            expectEquals('Need a character', field.errorMessage);
             // Add shift.
             MockInteractions.keyDownOn(field, 16, ['ctrl', 'shift']);
-            expectEquals('Ctrl + Shift', fieldText());
+            expectEquals('Ctrl + Shift', field.value);
+            expectEquals('Need a character', field.errorMessage);
             // Remove shift.
             MockInteractions.keyUpOn(field, 16, ['ctrl']);
-            expectEquals('Ctrl', fieldText());
+            expectEquals('Ctrl', field.value);
+            expectEquals('Need a character', field.errorMessage);
             // Add alt (ctrl + alt is invalid).
             MockInteractions.keyDownOn(field, 18, ['ctrl', 'alt']);
-            expectEquals('invalid', fieldText());
+            expectEquals('Ctrl', field.value);
             // Remove alt.
             MockInteractions.keyUpOn(field, 18, ['ctrl']);
-            expectEquals('Ctrl', fieldText());
+            expectEquals('Ctrl', field.value);
+            expectEquals('Need a character', field.errorMessage);
 
             // Add 'A'. Once a valid shortcut is typed (like Ctrl + A), it is
             // committed.
@@ -87,7 +96,7 @@ cr.define('extension_shortcut_input_tests', function() {
           .then((arg) => {
             input.delegate.reset();
             expectDeepEquals(['itemid', 'Command', 'Ctrl+A'], arg);
-            expectEquals('Ctrl + A', fieldText());
+            expectEquals('Ctrl + A', field.value);
             expectEquals('Ctrl+A', input.shortcut);
             expectTrue(isClearVisible());