[Extensions] Let screen reader users know previous shortcut.

This CL replaces the clear button with an edit button, and introduces a
read-only mode for the shortcut input. That way, when the user navigates
over the field in read-only mode, it reads out the current shortcut.

Tbr: cpu@chromium.org
Bug: 1031007
Change-Id: I7881288a6c141b8ca2717bafb9ccb50e12ea7642
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2575346
Commit-Queue: Ghazale Hosseinabadi <ghazale@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: John Lee <johntlee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#839155}

chrome/app/extensions_strings.grdp

@@ -349,6 +349,9 @@
   <message name="IDS_EXTENSIONS_SEARCH" desc="The text displayed in the search box on the chrome://extensions page.">
     Search extensions
   </message>
+  <message name="IDS_EXTENSIONS_EDIT_SHORTCUT" desc="The label for a button to edit the shortcut.">
+    Edit shortcut
+  </message>
   <message name="IDS_EXTENSIONS_SHORTCUT_NOT_SET" desc="The label to indicate that an extension has no shortcut set for a given command.">
     Not set
   </message>

chrome/app/extensions_strings_grdp/IDS_EXTENSIONS_EDIT_SHORTCUT.png.sha1

+1b03298204685a489a8d578057dd3188a2ff5634
\ No newline at end of file

chrome/browser/resources/extensions/shortcut_input.html

@@ -15,23 +15,18 @@
     right: inherit;
   }
 
-  /* Invisible so the element is still in the tab order. */
-  [invisible] {
-    opacity: 0;
-  }
 </style>
 <div id="main">
-  <cr-input id="input" placeholder="$i18n{shortcutTypeAShortcut}"
+    <cr-input id="input" readonly="[[readonly_]]"
+      placeholder="[[computePlaceholder_(readonly_)]]"
       invalid="[[getIsInvalid_(error_)]]"
       error-message="[[getErrorString_(error_,
           '$i18nPolymer{shortcutIncludeStartModifier}',
           '$i18nPolymer{shortcutTooManyModifiers}',
           '$i18nPolymer{shortcutNeedCharacter}')]]"
-      value="[[computeText_(capturing_, shortcut, pendingShortcut_)]]">
-    <cr-icon-button id="clear" aria-label="$i18nPolymer{clear}"
-        slot="suffix" class="icon-cancel no-overlap"
-        invisible$="[[computeClearInvisible_(capturing_, shortcut)]]"
-        hidden$="[[computeClearHidden_(shortcut)]]"
-        on-click="onClearClick_"></cr-icon-button>
+      value="[[computeText_(shortcut)]]">
+    <cr-icon-button id="edit" aria-label="$i18nPolymer{editShortcut}"
+        slot="suffix" class="icon-edit no-overlap"
+        on-click="onEditClick_"></cr-icon-button>
   </cr-input>
 </div>

chrome/browser/resources/extensions/shortcut_input.js

@@ -63,6 +63,14 @@ Polymer({
       value: ShortcutError.NO_ERROR,
     },
 
+
+    /** @private */
+    readonly_: {
+      type: Boolean,
+      value: true,
+      reflectToAttribute: true,
+    },
+
     /** @private */
     pendingShortcut_: {
       type: String,
@@ -82,7 +90,7 @@ Polymer({
 
   /** @private */
   startCapture_() {
-    if (this.capturing_) {
+    if (this.capturing_ || this.readonly_) {
       return;
     }
     this.capturing_ = true;
@@ -99,6 +107,17 @@ Polymer({
     this.$.input.blur();
     this.error_ = ShortcutError.NO_ERROR;
     this.delegate.setShortcutHandlingSuspended(false);
+    this.readonly_ = true;
+  },
+
+  /** @private */
+  clearShortcut_() {
+    this.pendingShortcut_ = '';
+    this.shortcut = '';
+    // We commit the empty shortcut in order to clear the current shortcut
+    // for the extension.
+    this.commitPending_();
+    this.endCapture_();
   },
 
   /**
@@ -106,7 +125,11 @@ Polymer({
    * @private
    */
   onKeyDown_(e) {
-    if (e.target === this.$.clear) {
+    if (this.readonly_) {
+      return;
+    }
+
+    if (e.target === this.$.edit) {
       return;
     }
 
@@ -138,11 +161,15 @@ Polymer({
    * @private
    */
   onKeyUp_(e) {
-    // Ignores pressing 'Space' or 'Enter' on the clear button. In 'Enter's
-    // case, the clear button disappears before key-up, so 'Enter's key-up
-    // target becomes the input field, not the clear button, and needs to
+    // Ignores pressing 'Space' or 'Enter' on the edit button. In 'Enter's
+    // case, the edit button disappears before key-up, so 'Enter's key-up
+    // target becomes the input field, not the edit button, and needs to
     // be caught explicitly.
-    if (e.target === this.$.clear || e.key === 'Enter') {
+    if (this.readonly_) {
+      return;
+    }
+
+    if (e.target === this.$.edit || e.key === 'Enter') {
       return;
     }
 
@@ -224,31 +251,25 @@ Polymer({
   },
 
   /**
-   * @return {string} The text to be displayed in the shortcut field.
+   * @return {string} The placeholder text.
    * @private
    */
-  computeText_() {
-    const shortcutString =
-        this.capturing_ ? this.pendingShortcut_ : this.shortcut;
-    return shortcutString.split('+').join(' + ');
+  computePlaceholder_() {
+    if (this.readonly_) {
+      return this.i18n('shortcutNotSet');
+    }
+    return this.i18n('shortcutTypeAShortcut');
   },
 
-  /**
-   * Invisible when capturing AND we have a shortcut.
-   * @return {boolean} Whether the clear button is invisible.
-   * @private
-   */
-  computeClearInvisible_() {
-    return this.capturing_ && !!this.shortcut;
-  },
 
   /**
-   * Hidden when no shortcut is set.
-   * @return {boolean} Whether the clear button is hidden.
+   * @return {string} The text to be displayed in the shortcut field.
    * @private
    */
-  computeClearHidden_() {
-    return !this.shortcut;
+  computeText_() {
+    const shortcutString =
+        this.capturing_ ? this.pendingShortcut_ : this.shortcut;
+    return shortcutString.split('+').join(' + ');
   },
 
   /**
@@ -260,12 +281,13 @@ Polymer({
   },
 
   /** @private */
-  onClearClick_() {
-    assert(this.shortcut);
-
-    this.pendingShortcut_ = '';
-    this.commitPending_();
-    this.endCapture_();
+  onEditClick_() {
+    // TODO(ghazale): The clearing functionality should be improved.
+    // Instead of clicking the edit button, and then clicking elsewhere to
+    // commit the "empty" shortcut, we want to introduce a separate clear
+    // button.
+    this.clearShortcut_();
+    this.readonly_ = false;
     this.$.input.focus();
   },
 });

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

@@ -228,6 +228,7 @@ content::WebUIDataSource* CreateMdExtensionsSource(Profile* profile,
     {"packDialogKeyFile", IDS_EXTENSIONS_PACK_DIALOG_KEY_FILE_LABEL},
     {"packDialogContent", IDS_EXTENSION_PACK_DIALOG_HEADING},
     {"packDialogConfirm", IDS_EXTENSIONS_PACK_DIALOG_CONFIRM_BUTTON},
+    {"editShortcut", IDS_EXTENSIONS_EDIT_SHORTCUT},
     {"shortcutNotSet", IDS_EXTENSIONS_SHORTCUT_NOT_SET},
     {"shortcutScopeGlobal", IDS_EXTENSIONS_SHORTCUT_SCOPE_GLOBAL},
     {"shortcutScopeLabel", IDS_EXTENSIONS_SHORTCUT_SCOPE_LABEL},

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

@@ -14,8 +14,6 @@ import {assert} from 'chrome://resources/js/assert.m.js';
 import {keyDownOn, keyUpOn} from 'chrome://resources/polymer/v3_0/iron-test-helpers/mock-interactions.js';
 import {flush} from 'chrome://resources/polymer/v3_0/polymer/polymer_bundled.min.js';
 
-import {isChildVisible} from '../test_util.m.js';
-
 import {TestService} from './test_service.js';
 
 window.extension_shortcut_input_tests = {};
@@ -42,18 +40,14 @@ suite(extension_shortcut_input_tests.suiteName, function() {
   test(assert(extension_shortcut_input_tests.TestNames.Basic), function() {
     const field = input.$['input'];
     assertEquals('', field.value);
-    const isClearVisible = isChildVisible.bind(null, input, '#clear', false);
-    expectFalse(isClearVisible());
 
-    // Focus the input. Capture should start.
-    field.focus();
+    // Click the edit button. Capture should start.
+    input.$['edit'].click();
     return input.delegate.whenCalled('setShortcutHandlingSuspended')
         .then((arg) => {
           assertTrue(arg);
           input.delegate.reset();
-
           assertEquals('', field.value);
-          expectFalse(isClearVisible());
 
           // Press character.
           keyDownOn(field, 'A', []);
@@ -65,22 +59,22 @@ suite(extension_shortcut_input_tests.suiteName, function() {
           expectTrue(field.errorMessage.startsWith('Include'));
           // Press ctrl.
           keyDownOn(field, 17, ['ctrl']);
-          assertEquals('Ctrl', field.value);
+          assertEquals('', field.value);
           assertEquals('Type a letter', field.errorMessage);
           // Add shift.
           keyDownOn(field, 16, ['ctrl', 'shift']);
-          assertEquals('Ctrl + Shift', field.value);
+          assertEquals('', field.value);
           assertEquals('Type a letter', field.errorMessage);
           // Remove shift.
           keyUpOn(field, 16, ['ctrl']);
-          assertEquals('Ctrl', field.value);
+          assertEquals('', field.value);
           assertEquals('Type a letter', field.errorMessage);
           // Add alt (ctrl + alt is invalid).
           keyDownOn(field, 18, ['ctrl', 'alt']);
-          assertEquals('Ctrl', field.value);
+          assertEquals('', field.value);
           // Remove alt.
           keyUpOn(field, 18, ['ctrl']);
-          assertEquals('Ctrl', field.value);
+          assertEquals('', field.value);
           assertEquals('Type a letter', field.errorMessage);
 
           // Add 'A'. Once a valid shortcut is typed (like Ctrl + A), it is
@@ -93,10 +87,9 @@ suite(extension_shortcut_input_tests.suiteName, function() {
           expectDeepEquals(['itemid', 'Command', 'Ctrl+A'], arg);
           assertEquals('Ctrl + A', field.value);
           assertEquals('Ctrl+A', input.shortcut);
-          expectTrue(isClearVisible());
 
           // Test clearing the shortcut.
-          input.$['clear'].click();
+          input.$['edit'].click();
           assertEquals(input.$.input, input.shadowRoot.activeElement);
           return input.delegate.whenCalled('updateExtensionCommandKeybinding');
         })
@@ -105,9 +98,8 @@ suite(extension_shortcut_input_tests.suiteName, function() {
           input.delegate.reset();
           expectDeepEquals(['itemid', 'Command', ''], arg);
           assertEquals('', input.shortcut);
-          expectFalse(isClearVisible());
 
-          field.focus();
+          input.$['edit'].click();
           return input.delegate.whenCalled('setShortcutHandlingSuspended');
         })
         .then((arg) => {
@@ -115,6 +107,7 @@ suite(extension_shortcut_input_tests.suiteName, function() {
           expectTrue(arg);
 
           // Test ending capture using the escape key.
+          input.$['edit'].click();
           keyDownOn(field, 27);  // Escape key.
           return input.delegate.whenCalled('setShortcutHandlingSuspended');
         })

ui/webui/resources/cr_elements/cr_input/cr_input_style_css.html

@@ -95,11 +95,11 @@
 
       :host([invalid]) #underline,
       :host([force-underline]) #underline,
-      :host([focused_]:not([readonly])) #underline {
+      :host([focused_]) #underline {
         opacity: 1;
         transition: opacity 120ms ease-in, width 180ms ease-out;
         width: 100%;
       }
-    </style>
+   </style>
   </template>
 </dom-module>