shortcuts: Handle replacing/removing locked accelerators/actions

Previously we allowed replacing locked accelerators and actions. This
is an unintended behavior, as either locked actions or accelerators
cannot be replaced or removed.

Removes an invalid test.

Screenshot: http://shortn/_8DFoMOKY5n

Bug: 1060690
Test: --gtest_filter=ShortcutCustomizationAppBrowserTest*
Change-Id: I0de0dbe3ce36aff5248709cc149dcde9509dc3e8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3180022
Reviewed-by: Zentaro Kavanagh <zentaro@chromium.org>
Commit-Queue: Jimmy Gong <jimmyxgong@chromium.org>
Cr-Commit-Position: refs/heads/main@{#931239}

ash/webui/shortcut_customization_ui/resources/accelerator_lookup_manager.js

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-import {assertNotReached} from 'chrome://resources/js/assert.m.js';
+import {assert, assertNotReached} from 'chrome://resources/js/assert.m.js';
 import {addSingletonGetter} from 'chrome://resources/js/cr.m.js';
 
 import {fakeActionNames} from './fake_data.js';
@@ -205,29 +205,39 @@ export class AcceleratorLookupManager {
   /**
    * @param {!AcceleratorSource} source
    * @param {number} action
-   * @param {AcceleratorKeys} accelerator
+   * @param {AcceleratorKeys} keys
    */
-  removeAccelerator(source, action, accelerator) {
-    const foundIdx = this.getAcceleratorInfoIndex_(source, action, accelerator);
+  removeAccelerator(source, action, keys) {
+    const foundAccel = this.getAcceleratorInfoFromKeys_(source, action, keys);
 
-    if (foundIdx === -1) {
-      // Can only attempt to remove an existing accelerator.
-      assertNotReached();
-    }
-
-    const accelInfos = this.getAccelerators(source, action);
-    const foundAccel = accelInfos[foundIdx];
+    // Can only remove an existing accelerator.
+    assert(foundAccel != null);
 
     if (foundAccel.locked) {
       // Not possible to remove a locked accelerator manually.
       assertNotReached();
     }
 
+    const accelInfos = this.getAccelerators(source, action);
+    const foundIdx = this.getAcceleratorInfoIndex_(source, action, keys);
     // Remove accelerator from main map.
     accelInfos.splice(foundIdx, 1);
 
     // Remove from reverse lookup.
-    this.reverseAcceleratorLookup_.delete(JSON.stringify(accelerator));
+    this.reverseAcceleratorLookup_.delete(JSON.stringify(keys));
+  }
+
+  /**
+   * @param {!AcceleratorSource} source
+   * @param {number} action
+   * @param {!AcceleratorKeys} keys
+   * @return {boolean}
+   */
+  isAcceleratorLocked(source, action, keys) {
+    const accel = this.getAcceleratorInfoFromKeys_(source, action, keys);
+    assert(accel);
+
+    return accel.locked;
   }
 
   /**
@@ -249,11 +259,8 @@ export class AcceleratorLookupManager {
     const accelInfos = this.getAccelerators(source, action);
     const foundIdx = this.getAcceleratorInfoIndex_(source, action, accelKeys);
 
-    // Check if the accelerator is locked, disable if locked.
-    if (accelInfos[foundIdx].locked) {
-      accelInfos[foundIdx].state = AcceleratorState.kDisabledByUser;
-      return;
-    }
+    // Cannot remove a locked accelerator.
+    assert(!accelInfos[foundIdx].locked);
 
     // Otherwise, remove the accelerator.
     accelInfos.splice(foundIdx, 1);
@@ -281,6 +288,23 @@ export class AcceleratorLookupManager {
     return -1;
   }
 
+  /**
+   * @param {!AcceleratorSource} source
+   * @param {number} action
+   * @param {!AcceleratorKeys} keys
+   * @return {?AcceleratorInfo}
+   */
+  getAcceleratorInfoFromKeys_(source, action, keys) {
+    const foundIdx = this.getAcceleratorInfoIndex_(source, action, keys);
+
+    if (foundIdx === -1) {
+      return null;
+    }
+
+    const accelInfos = this.getAccelerators(source, action);
+    return accelInfos[foundIdx];
+  }
+
   reset() {
     this.acceleratorLookup_.clear();
     this.acceleratorNameLookup_.clear();
@@ -290,4 +314,3 @@ export class AcceleratorLookupManager {
 }
 
 addSingletonGetter(AcceleratorLookupManager);
-;

ash/webui/shortcut_customization_ui/resources/accelerator_view.js

@@ -306,11 +306,24 @@ export class AcceleratorViewElement extends PolymerElement {
     if (foundId !== undefined) {
       // TODO(jimmyxgong): Fetch name of accelerator with real implementation.
       const uuidParams = foundId.split('-');
+      const conflictSource =
+          /** @type {!AcceleratorSource} */(parseInt(uuidParams[0], 10));
+      const conflictAction = parseInt(uuidParams[1], 10);
       const conflictAccelName = this.lookupManager_.getAcceleratorName(
-          /**source=*/ parseInt(uuidParams[0], 10),
-          /**action=*/ parseInt(uuidParams[1], 10));
+          conflictSource, conflictAction);
+
+      // Cannot override a locked action.
+      if (!this.shortcutProvider_.isMutable(conflictSource) ||
+          this.lookupManager_.isAcceleratorLocked(
+              conflictSource, conflictAction, pendingKeys)) {
+        // TODO(jimmyxgong): i18n this string.
+        this.statusMessage = 'Shortcut is used by \"' + conflictAccelName +
+            '\". Press a new shortcut to replace.';
+        this.hasError = true;
+        return;
+      }
+
       // TODO(jimmyxgong): i18n this string.
-      // TODO(jimmyxgong): Handle attempting to override a locked action.
       this.statusMessage = 'Shortcut is used by ' + conflictAccelName +
           '. Press a new shortcut or press the same one again to use it for ' +
           'this action instead.';

chrome/test/data/webui/chromeos/shortcut_customization/accelerator_lookup_manager_test.js

@@ -167,48 +167,6 @@ export function acceleratorLookupManagerTest() {
     });
   });
 
-  test('ReplacePreexistingLockedAccelerator', () => {
-    provider.setFakeAcceleratorConfig(fakeAcceleratorConfig);
-    return provider.getAllAcceleratorConfig().then((result) => {
-      assertDeepEquals(fakeAcceleratorConfig, result);
-
-      manager.setAcceleratorLookup(result);
-
-      // Get Snap Window Left accelerator, which has a locked accelerator.
-      const snapWindowLeftAction = 0;
-      const ashMap = fakeAcceleratorConfig.get(AcceleratorSource.kAsh);
-      const snapWindowLeftAccels = ashMap.get(snapWindowLeftAction);
-      // Modifier.Alt + key::219 ('[')
-      const overridenAccel = snapWindowLeftAccels[0].accelerator;
-      // Verify state is kEnabled.
-      assertEquals(AcceleratorState.kEnabled, snapWindowLeftAccels[0].state);
-
-      // Replace New Desk shortcut with Alt+'['.
-      const newDeskAction = 2;
-      const oldNewDeskAccel = ashMap.get(newDeskAction)[0].accelerator;
-
-      replaceAndVerify(
-          AcceleratorSource.kAsh, newDeskAction, oldNewDeskAccel,
-          overridenAccel);
-
-      // Verify that the New Desk shortcut now has the ALT + '[' accelerator.
-      const newDeskLookup =
-          manager.getAccelerators(AcceleratorSource.kAsh, newDeskAction);
-      assertEquals(1, newDeskLookup.length);
-      assertEquals(
-          JSON.stringify(overridenAccel),
-          JSON.stringify(newDeskLookup[0].accelerator));
-
-      // Verify that Snap Window Left's accelerator is not removed (since it is,
-      // locked). But it's state is updated to kDisabledByUser.
-      const snapWindowLeftLookup =
-          manager.getAccelerators(AcceleratorSource.kAsh, snapWindowLeftAction);
-      assertEquals(1, snapWindowLeftLookup.length);
-      assertEquals(
-          AcceleratorState.kDisabledByUser, snapWindowLeftLookup[0].state);
-    });
-  });
-
   test('AddBasicAccelerator', () => {
     provider.setFakeAcceleratorConfig(fakeAcceleratorConfig);
     return provider.getAllAcceleratorConfig().then((result) => {