Fix logic for changing keybindings in shortcut editor (#16214)

* Fix keybinding logic

* Fix shortcut/keybinding being set at wrong level

The data for use by commands were set on level of individual keys
rather than on the level of individual keybindings which let to context
menu working on a different item than one would expect (based on key
number, not keybinding number).

packages/shortcuts-extension/src/components/ShortcutItem.tsx

@@ -219,12 +219,7 @@ export class ShortcutItem extends React.Component<
 
   getShortCutForDisplayOnly(binding: IKeybinding): JSX.Element[] {
     return binding.keys.map((keyboardKey: string, index: number) => (
-      <div
-        className="jp-Shortcuts-ShortcutKeysContainer"
-        key={index}
-        data-keybinding={index}
-        data-shortcut={this.props.shortcut.id}
-      >
+      <div className="jp-Shortcuts-ShortcutKeysContainer" key={index}>
         <div className="jp-Shortcuts-ShortcutKeys">
           {this.toSymbols(keyboardKey)}
         </div>
@@ -248,6 +243,8 @@ export class ShortcutItem extends React.Component<
       <div
         className="jp-Shortcuts-ShortcutContainer"
         key={this.props.shortcut.id + '_' + index}
+        data-keybinding={index}
+        data-shortcut={this.props.shortcut.id}
         onClick={() => this.toggleInputReplaceMethod(index)}
       >
         {this.isLocationBeingEdited(index)

packages/shortcuts-extension/src/components/ShortcutUI.tsx

@@ -358,16 +358,24 @@ export class ShortcutUI
     const userShortcuts = settings.user.shortcuts ?? [];
     const newUserShortcuts = [];
     let found = false;
+    // Copy over existing user keybindings
     for (let shortcut of userShortcuts) {
+      // If this is the query keybinding, update it with new `keys`
       if (
         shortcut.command === target.command &&
         shortcut.selector === target.selector &&
-        JSONExt.deepEqual(shortcut.args ?? {}, target.args ?? {})
+        JSONExt.deepEqual(shortcut.args ?? {}, target.args ?? {}) &&
+        keybinding &&
+        JSONExt.deepEqual(keybinding.keys, shortcut.keys)
       ) {
         const matchesDefault =
           keybinding &&
           keybinding.isDefault &&
           JSONExt.deepEqual(keybinding.keys, keys);
+
+        // If the new `keys` are empty, do not copy this one over.
+        // Also, if the keybinding is a default keybinding and the desired
+        // new `keys` are the same as default, it does not need to be added.
         if (keys.length !== 0 && !matchesDefault) {
           newUserShortcuts.push({
             command: shortcut.command,

packages/shortcuts-extension/test/components/ShortcutUI.spec.ts

@@ -182,6 +182,47 @@ describe('@jupyterlab/shortcut-extension', () => {
           selector: 'body'
         });
       });
+
+      it('should replace the default keybinding in presence of non-default keybinding', async () => {
+        const userKeybinding = {
+          keys: ['Ctrl A'],
+          isDefault: false
+        };
+        const defaultKeybinding = {
+          keys: ['Ctrl B'],
+          isDefault: true
+        };
+        const target = {
+          id: 'test-id',
+          command: 'test:command',
+          keybindings: [userKeybinding, defaultKeybinding],
+          args: {},
+          selector: 'body',
+          category: 'test'
+        };
+        registerKeybinding(target, userKeybinding);
+        registerKeybinding(target, defaultKeybinding);
+        await shortcutUI.replaceKeybinding(target, defaultKeybinding, [
+          'Ctrl X'
+        ]);
+        expect(data.user.shortcuts).toHaveLength(3);
+        expect(data.user.shortcuts[0]).toEqual({
+          command: 'test:command',
+          keys: ['Ctrl A'],
+          selector: 'body'
+        });
+        expect(data.user.shortcuts[1]).toEqual({
+          command: 'test:command',
+          keys: ['Ctrl B'],
+          selector: 'body',
+          disabled: true
+        });
+        expect(data.user.shortcuts[2]).toEqual({
+          command: 'test:command',
+          keys: ['Ctrl X'],
+          selector: 'body'
+        });
+      });
     });
 
     describe('#deleteKeybinding()', () => {