fix #7836: respect keybinding scope and registration order during eva…

…luation

Also match only first keybinding on keydown, don't traverse over all keybindings.

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

examples/api-tests/src/keybindings.spec.js

@@ -30,7 +30,6 @@ describe('Keybindings', function () {
     const { EditorManager } = require('@theia/editor/lib/browser/editor-manager');
     const Uri = require('@theia/core/lib/common/uri');
     const { WorkspaceService } = require('@theia/workspace/lib/browser/workspace-service');
-    const { MonacoEditor } = require('@theia/monaco/lib/browser/monaco-editor');
 
     /** @type {import('inversify').Container} */
     const container = window['theia'].container;
@@ -62,7 +61,7 @@ describe('Keybindings', function () {
         assert.equal(executedCommand, TerminalCommands.TERMINAL_CLEAR.id);
     });
 
-    it("disabled keybinding should not override enabled", async () => {
+    it('disabled keybinding should not override enabled', async () => {
         const id = '__test:keybindings.left';
         toTearDown.push(commands.registerCommand({ id }, {
             execute: () => { }
@@ -71,7 +70,7 @@ describe('Keybindings', function () {
             command: '__test:keybindings.left',
             keybinding: 'left',
             when: 'false'
-        }, true));
+        }));
 
         const editor = await editorManager.open(new Uri.default(workspaceService.tryGetRoots()[0].uri).resolve('package.json'), {
             mode: 'activate',

examples/api-tests/src/typescript.spec.js

@@ -455,6 +455,48 @@ module.exports = (port, host, argv) => Promise.resolve()
         assert.equal(activeEditor.getControl().getModel().getWordAtPosition({ lineNumber, column }).word, 'Container');
     });
 
+    it('editor.action.triggerSuggest navigate', async function () {
+        const editor = await openEditor(serverUri);
+        // const { [|Container] } = require('inversify');
+        editor.getControl().setPosition({ lineNumber: 5, column: 9 });
+        editor.getControl().setSelection({ startLineNumber: 5, startColumn: 9, endLineNumber: 5, endColumn: 18 });
+        // @ts-ignore
+        assert.equal(editor.getControl().getModel().getWordAtPosition(editor.getControl().getPosition()).word, 'Container');
+
+        const suggest = editor.getControl()._contributions['editor.contrib.suggestController'];
+        const getFocusedLabel = () => {
+            const focusedItem = suggest.widget.getValue().getFocusedItem();
+            return focusedItem && focusedItem.item.completion.label;
+        };
+
+        assert.isUndefined(getFocusedLabel());
+        assert.isFalse(contextKeyService.match('suggestWidgetVisible'));
+
+        await commands.executeCommand('editor.action.triggerSuggest');
+        await waitForAnimation(() => contextKeyService.match('suggestWidgetVisible') && getFocusedLabel() === 'Container');
+
+        assert.equal(getFocusedLabel(), 'Container');
+        assert.isTrue(contextKeyService.match('suggestWidgetVisible'));
+
+        keybindings.dispatchKeyDown('ArrowDown');
+        await waitForAnimation(() => contextKeyService.match('suggestWidgetVisible') && getFocusedLabel() === 'ContainerModule');
+
+        assert.equal(getFocusedLabel(), 'ContainerModule');
+        assert.isTrue(contextKeyService.match('suggestWidgetVisible'));
+
+        keybindings.dispatchKeyDown('ArrowUp');
+        await waitForAnimation(() => contextKeyService.match('suggestWidgetVisible') && getFocusedLabel() === 'Container');
+
+        assert.equal(getFocusedLabel(), 'Container');
+        assert.isTrue(contextKeyService.match('suggestWidgetVisible'));
+
+        keybindings.dispatchKeyDown('Escape');
+        await waitForAnimation(() => !contextKeyService.match('suggestWidgetVisible') && getFocusedLabel() === undefined);
+
+        assert.isUndefined(getFocusedLabel());
+        assert.isFalse(contextKeyService.match('suggestWidgetVisible'));
+    });
+
     it('editor.action.rename', async function () {
         const editor = await openEditor(serverUri);
         // const |container = new Container();

packages/core/src/browser/keybinding.spec.ts

@@ -226,15 +226,15 @@ describe('keybindings', () => {
 
         keybindingRegistry.setKeymap(KeybindingScope.WORKSPACE, keybindingsSpecific);
 
-        let bindings = keybindingRegistry.getKeybindingsForKeySequence([KeyCode.createKeyCode({ first: Key.KEY_A, modifiers: [KeyModifier.CtrlCmd] })]).full;
-        expect(bindings).to.have.lengthOf(1);
+        let match = keybindingRegistry.matchKeybiding([KeyCode.createKeyCode({ first: Key.KEY_A, modifiers: [KeyModifier.CtrlCmd] })]);
+        expect(match && match.kind).to.be.equal('full');
 
-        bindings = keybindingRegistry.getKeybindingsForKeySequence([KeyCode.createKeyCode({ first: Key.KEY_B, modifiers: [KeyModifier.CtrlCmd] })]).full;
-        expect(bindings).to.have.lengthOf(1);
+        match = keybindingRegistry.matchKeybiding([KeyCode.createKeyCode({ first: Key.KEY_B, modifiers: [KeyModifier.CtrlCmd] })]);
+        expect(match && match.kind).to.be.equal('full');
 
-        bindings = keybindingRegistry.getKeybindingsForKeySequence([KeyCode.createKeyCode({ first: Key.KEY_C, modifiers: [KeyModifier.CtrlCmd] })]).full;
-        const keyCode = KeyCode.parse(bindings[0].keybinding);
-        expect(keyCode.key).to.be.equal(validKeyCode.key);
+        match = keybindingRegistry.matchKeybiding([KeyCode.createKeyCode({ first: Key.KEY_C, modifiers: [KeyModifier.CtrlCmd] })]);
+        const keyCode = match && KeyCode.parse(match.binding.keybinding);
+        expect(keyCode?.key).to.be.equal(validKeyCode.key);
     });
 
     it('should return partial keybinding matches', () => {
@@ -249,8 +249,8 @@ describe('keybindings', () => {
         validKeyCodes.push(KeyCode.createKeyCode({ first: Key.KEY_C, modifiers: [KeyModifier.CtrlCmd] }));
         validKeyCodes.push(KeyCode.createKeyCode({ first: Key.KEY_T }));
 
-        const bindings = keybindingRegistry.getKeybindingsForKeySequence(KeySequence.parse('ctrlcmd+x'));
-        expect(bindings.partial.length > 0);
+        const match = keybindingRegistry.matchKeybiding(KeySequence.parse('ctrlcmd+x'));
+        expect(match && match.kind).to.be.equal('partial');
     });
 
     it('should not register a shadowing keybinding', () => {
@@ -276,7 +276,6 @@ describe('keybindings', () => {
 
     it('shadowed bindings should be returned last', () => {
         const keyCode = KeyCode.createKeyCode({ first: Key.KEY_A, modifiers: [KeyModifier.Shift] });
-        let bindings: Keybinding[];
 
         const ignoredDefaultBinding: Keybinding = {
             keybinding: keyCode.toString(),
@@ -303,29 +302,29 @@ describe('keybindings', () => {
         keybindingRegistry.setKeymap(KeybindingScope.WORKSPACE, [workspaceBinding]);
         // now WORKSPACE bindings are overriding the other scopes
 
-        bindings = keybindingRegistry.getKeybindingsForKeySequence([keyCode]).full;
-        expect(bindings).to.have.lengthOf(3);
-        expect(bindings[0].command).to.be.equal(workspaceBinding.command);
+        let match = keybindingRegistry.matchKeybiding([keyCode]);
+        expect(match?.kind).to.be.equal('full');
+        expect(match?.binding?.command).to.be.equal(workspaceBinding.command);
 
         keybindingRegistry.resetKeybindingsForScope(KeybindingScope.WORKSPACE);
         // now it should find USER bindings
 
-        bindings = keybindingRegistry.getKeybindingsForKeySequence([keyCode]).full;
-        expect(bindings).to.have.lengthOf(2);
-        expect(bindings[0].command).to.be.equal(userBinding.command);
+        match = keybindingRegistry.matchKeybiding([keyCode]);
+        expect(match?.kind).to.be.equal('full');
+        expect(match?.binding?.command).to.be.equal(userBinding.command);
 
         keybindingRegistry.resetKeybindingsForScope(KeybindingScope.USER);
         // and finally it should fallback to DEFAULT bindings.
 
-        bindings = keybindingRegistry.getKeybindingsForKeySequence([keyCode]).full;
-        expect(bindings).to.have.lengthOf(1);
-        expect(bindings[0].command).to.be.equal(defaultBinding.command);
+        match = keybindingRegistry.matchKeybiding([keyCode]);
+        expect(match?.kind).to.be.equal('full');
+        expect(match?.binding?.command).to.be.equal(defaultBinding.command);
 
         keybindingRegistry.resetKeybindingsForScope(KeybindingScope.DEFAULT);
         // now the registry should be empty
 
-        bindings = keybindingRegistry.getKeybindingsForKeySequence([keyCode]).full;
-        expect(bindings).to.be.empty;
+        match = keybindingRegistry.matchKeybiding([keyCode]);
+        expect(match).to.be.undefined;
 
     });
 });

packages/core/src/browser/keybinding.ts

@@ -29,7 +29,6 @@ import * as common from '../common/keybinding';
 
 export enum KeybindingScope {
     DEFAULT,
-    DEFAULT_OVERRIDING,
     USER,
     WORKSPACE,
     END
@@ -56,7 +55,7 @@ export interface ResolvedKeybinding extends Keybinding {
 
 export interface ScopedKeybinding extends Keybinding {
     /** Current keybinding scope */
-    scope?: KeybindingScope;
+    scope: KeybindingScope;
 }
 
 export const KeybindingContribution = Symbol('KeybindingContribution');
@@ -100,7 +99,7 @@ export class KeybindingRegistry {
     protected keySequence: KeySequence = [];
 
     protected readonly contexts: { [id: string]: KeybindingContext } = {};
-    protected readonly keymaps: Keybinding[][] = [...Array(KeybindingScope.length)].map(() => []);
+    protected readonly keymaps: ScopedKeybinding[][] = [...Array(KeybindingScope.length)].map(() => []);
 
     @inject(KeyboardLayoutService)
     protected readonly keyboardLayoutService: KeyboardLayoutService;
@@ -167,11 +166,12 @@ export class KeybindingRegistry {
     /**
      * Register a default keybinding to the registry.
      *
+     * Keybindings registered later have higher priority during evaluation.
+     *
      * @param binding
-     * @param override Override existed keybinding
      */
-    registerKeybinding(binding: Keybinding, override?: boolean): Disposable {
-        return this.doRegisterKeybinding(binding, !!override ? KeybindingScope.DEFAULT_OVERRIDING : undefined);
+    registerKeybinding(binding: Keybinding): Disposable {
+        return this.doRegisterKeybinding(binding);
     }
 
     /**
@@ -219,12 +219,14 @@ export class KeybindingRegistry {
     protected doRegisterKeybinding(binding: Keybinding, scope: KeybindingScope = KeybindingScope.DEFAULT): Disposable {
         try {
             this.resolveKeybinding(binding);
-            if (this.containsKeybinding(this.keymaps[scope], binding) && scope !== KeybindingScope.DEFAULT_OVERRIDING) {
+            // TODO: get rid of this check as soon as `context` property is removed
+            if (this.containsKeybinding(this.keymaps[scope], binding)) {
                 throw new Error(`"${binding.keybinding}" is in collision with something else [scope:${scope}]`);
             }
-            this.keymaps[scope].push(binding);
+            const scoped = Object.assign(binding, { scope });
+            this.keymaps[scope].unshift(scoped);
             return Disposable.create(() => {
-                const index = this.keymaps[scope].indexOf(binding);
+                const index = this.keymaps[scope].indexOf(scoped);
                 if (index !== -1) {
                     this.keymaps[scope].splice(index, 1);
                 }
@@ -266,7 +268,7 @@ export class KeybindingRegistry {
      * @param bindings the keybinding reference list
      * @param binding the keybinding to test collisions for
      */
-    containsKeybinding(bindings: Keybinding[], binding: Keybinding): boolean {
+    containsKeybinding(bindings: ScopedKeybinding[], binding: Keybinding): boolean {
         const bindingKeySequence = this.resolveKeybinding(binding);
         const collisions = this.getKeySequenceCollisions(bindings, bindingKeySequence)
             .filter(b => b.context === binding.context && !b.when && !binding.when);
@@ -363,30 +365,13 @@ export class KeybindingRegistry {
         }
     }
 
-    /**
-     * Finds collisions for a binding inside a list of bindings (error-free)
-     *
-     * @param bindings the reference bindings
-     * @param binding the binding to match
-     */
-    protected getKeybindingCollisions(bindings: Keybinding[], binding: Keybinding): KeybindingRegistry.KeybindingsResult {
-        const result = new KeybindingRegistry.KeybindingsResult();
-        try {
-            const bindingKeySequence = this.resolveKeybinding(binding);
-            result.merge(this.getKeySequenceCollisions(bindings, bindingKeySequence));
-        } catch (error) {
-            this.logger.warn(error);
-        }
-        return result;
-    }
-
     /**
      * Finds collisions for a key sequence inside a list of bindings (error-free)
      *
      * @param bindings the reference bindings
      * @param candidate the sequence to match
      */
-    protected getKeySequenceCollisions(bindings: Keybinding[], candidate: KeySequence): KeybindingRegistry.KeybindingsResult {
+    protected getKeySequenceCollisions(bindings: ScopedKeybinding[], candidate: KeySequence): KeybindingRegistry.KeybindingsResult {
         const result = new KeybindingRegistry.KeybindingsResult();
         for (const binding of bindings) {
             try {
@@ -413,28 +398,6 @@ export class KeybindingRegistry {
         return result;
     }
 
-    /**
-     * Get the lists of keybindings matching fully or partially matching a KeySequence.
-     *
-     * @param keySequence The key sequence for which we are looking for keybindings.
-     */
-    getKeybindingsForKeySequence(keySequence: KeySequence): KeybindingRegistry.KeybindingsResult {
-        const result = new KeybindingRegistry.KeybindingsResult();
-
-        for (let scope = KeybindingScope.END; --scope >= KeybindingScope.DEFAULT;) {
-            const matches = this.getKeySequenceCollisions(this.keymaps[scope], keySequence);
-
-            if (scope === KeybindingScope.DEFAULT_OVERRIDING) {
-                matches.full.reverse();
-                matches.partial.reverse();
-            }
-            matches.full.forEach(binding => binding.scope = scope);
-            matches.partial.forEach(binding => binding.scope = scope);
-            result.merge(matches);
-        }
-        return result;
-    }
-
     /**
      * Get the keybindings associated to commandId.
      *
@@ -578,10 +541,9 @@ export class KeybindingRegistry {
 
         this.keyboardLayoutService.validateKeyCode(keyCode);
         this.keySequence.push(keyCode);
-        const bindings = this.getKeybindingsForKeySequence(this.keySequence);
-        const full = bindings.full.find(binding => this.isEnabled(binding, event));
-        const partial = bindings.partial.find(binding => (!full || binding.scope! > full.scope!) && this.isEnabled(binding, event));
-        if (partial) {
+        const match = this.matchKeybiding(this.keySequence, event);
+
+        if (match && match.kind === 'partial') {
             /* Accumulate the keysequence */
             event.preventDefault();
             event.stopPropagation();
@@ -592,14 +554,41 @@ export class KeybindingRegistry {
                 priority: 2
             });
         } else {
-            if (full) {
-                this.executeKeyBinding(full, event);
+            if (match && match.kind === 'full') {
+                this.executeKeyBinding(match.binding, event);
             }
             this.keySequence = [];
             this.statusBar.removeElement('keybinding-status');
         }
     }
 
+    /**
+     * Match first full binding or partial if it belongs to the higher scope in the current context.
+     *
+     * Keybinidngs priority:
+     * - keybining scope
+     * - registraion order within scope
+     */
+    matchKeybiding(keySequence: KeySequence, event?: KeyboardEvent): KeybindingRegistry.Match {
+        let partial: ScopedKeybinding | undefined;
+        for (let scope = KeybindingScope.END; --scope >= KeybindingScope.DEFAULT;) {
+            if (partial) {
+                return { kind: 'partial', binding: partial };
+            }
+            for (const binding of this.keymaps[scope]) {
+                const resolved = this.resolveKeybinding(binding);
+                const compareResult = KeySequence.compare(keySequence, resolved);
+                if (compareResult === KeySequence.CompareResult.FULL && (!event || this.isEnabled(binding, event))) {
+                    return { kind: 'full', binding };
+                }
+                if (!partial && compareResult === KeySequence.CompareResult.PARTIAL && (!event || this.isEnabled(binding, event))) {
+                    partial = binding;
+                }
+            }
+        }
+        return undefined;
+    }
+
     /**
      * Return true of string a pseudo-command id, in other words a command id
      * that has a special meaning and that we won't find in the command
@@ -641,6 +630,10 @@ export class KeybindingRegistry {
 }
 
 export namespace KeybindingRegistry {
+    export type Match = {
+        kind: 'full' | 'partial'
+        binding: ScopedKeybinding
+    } | undefined;
     export class KeybindingsResult {
         full: ScopedKeybinding[] = [];
         partial: ScopedKeybinding[] = [];

packages/monaco/src/browser/monaco-keybinding.ts

@@ -29,10 +29,7 @@ export class MonacoKeybindingContribution implements KeybindingContribution {
 
     registerKeybindings(registry: KeybindingRegistry): void {
         const defaultKeybindings = monaco.keybindings.KeybindingsRegistry.getDefaultKeybindings();
-        // register in reverse order to align with Monaco dispatch logic:
-        // https://github.com/TypeFox/vscode/blob/70b8db24a37fafc77247de7f7cb5bb0195120ed0/src/vs/platform/keybinding/common/keybindingResolver.ts#L302
-        for (let i = defaultKeybindings.length - 1; i >= 0; i--) {
-            const item = defaultKeybindings[i];
+        for (const item of defaultKeybindings) {
             const command = this.commands.validate(item.command);
             if (command) {
                 const when = item.when && item.when.serialize();

packages/monaco/src/typings/monaco/index.d.ts

@@ -75,7 +75,8 @@ declare module monaco.editor {
             'editor.contrib.referencesController': monaco.referenceSearch.ReferencesController
             'editor.contrib.hover': ModesHoverController
             'css.editor.codeLens': CodeLensContribution
-            'editor.contrib.quickFixController': QuickFixController
+            'editor.contrib.quickFixController': QuickFixController,
+            'editor.contrib.suggestController': monaco.suggest.SuggestController
         }
         readonly _modelData: {
             cursor: ICursor
@@ -1187,6 +1188,19 @@ declare module monaco.suggest {
         Top, Inline, Bottom
     }
 
+    // https://github.com/theia-ide/vscode/blob/d24b5f70c69b3e75cd10c6b5247a071265ccdd38/src/vs/editor/contrib/suggest/suggestController.ts#L97
+    export interface SuggestController {
+        readonly widget: {
+            getValue(): SuggestWidget
+        }
+    }
+    export interface SuggestWidget {
+        getFocusedItem(): ISelectedSuggestion | undefined;
+    }
+    export interface ISelectedSuggestion {
+        item: CompletionItem;
+    }
+
     // https://github.com/theia-ide/vscode/blob/standalone/0.19.x/src/vs/editor/contrib/suggest/suggest.ts#L28
     export interface CompletionItem {
         completion: monaco.languages.CompletionItem;