'When' clauses are ignored for all commands in global menu

[monk-time]

• VSCode Version: Code - Insiders 1.17.0-insider (f7962f0, 2017-09-20T05:24:33.599Z)
• OS Version: Windows_NT x64 6.1.7601
• Extensions: none

---

Key bindings for commands that toggle sidebar or panel visibility ignore 'when' clause contexts, making it impossible to limit them to certain contexts.

Use case: some shortcut schemes (e.g. Intellij IDEA) have a shortcut to hide current active sidebar or panel, and a VScode extension that provides such keybindings wants to recreate this functionality through a shortcut that toggles panels and the sidebar only when they are active, essentially turning a toggle into a closer. But since 'when' clause contexts for these actions always evaluate to true, the shortcuts become globally active and act as toggles. And while the last three shortcuts from that snippet are replaceable with a global shortcut for workbench.action.closePanel, there is no corresponding action to close (not toggle) the sidebar.

Steps to reproduce:

1. Pick an unused shortcut, e.g. ctrl+l, and a context that would be inactive during the next steps, e.g. parameterHintsVisible.
2. Add any one of the following settings to keybindings.json:

{ "key": "ctrl+l",                "command": "workbench.action.toggleSidebarVisibility",
                                     "when": "parameterHintsVisible" }

{ "key": "ctrl+l",                "command": "workbench.action.togglePanel",
                                     "when": "parameterHintsVisible" }

{ "key": "ctrl+l",                "command": "workbench.actions.view.problems",
                                     "when": "parameterHintsVisible" }

{ "key": "ctrl+l",                "command": "workbench.debug.action.toggleRepl",
                                     "when": "parameterHintsVisible" }

{ "key": "ctrl+l",                "command": "workbench.action.terminal.toggleTerminal",
                                     "when": "parameterHintsVisible" }

3. Set focus to the current text editor.
4. Press ctrl+l.

Expected result: nothing should happen, the 'when' clause evaluates to false so the shortcut should be disabled.
Actual result: the shortcut works, the relevant panel/sidebar visibility is toggled.

[usernamehw]

@sandy081 What is this issue: bug? as-designed? feature-request? Is there any milestone?

[jrieken]

@monk-time The parameterHintsVisible-clause is set within an editor and its value represents if the signature/parameter help widget is visible or not. That usually happens when typing code, e.g function invocations. So, my question is why you depend on that clause?

[usernamehw]

I am not the issue author, but previous title was more precise: it's not limited to parameterHintsVisible context --- these commands ignore ALL when contexts (at least the ones that I tried).

This works:

{
    "key": "ctrl+shift+8",
    "command": "editor.action.insertSnippet",
    "when": "editorLangId == 'javascript'"
}

This doesn't:

{
    "key": "ctrl+shift+9",
    "command": "workbench.action.togglePanel",
    "when": "editorLangId == 'javascript'"
}

Is this a known issue? All workbench* commands except workbench.action.editor.changeLanguageMode ignore when context.

[monk-time]

@jrieken The poster above is right, and I admit I could've made it clearer: when I was testing this, all clauses that I've tried were ignored. As I wrote in Step 1, parameterHintsVisible is just an example of a clause that is most likely to be false, it could be any other clause.

I'll change the title back if you don't mind.

[usernamehw]

Is this issue related #26318?

[jrieken]

when I was testing this, all clauses that I've tried were ignored

You most likely tested with when-clauses that are only set inside an editor. There can be multiple editors at the same time (side by side, nested, diff etc.) and therefore multiple (nested) containers for context keys exist. The parameterHintsVisible-clause is set multiple times (once per editor) but not "globally" because they only make sense in the scope of an editor. What should the global-value of a editor-local clause be if its true for one and false for another editor? The rule of thumb is that clauses starting with editor are available inside editors only. However we aren't very good with that rule and clauses like parameterHintsVisible and others exist... This list list clauses and the scope in which they are available: https://code.visualstudio.com/docs/getstarted/keybindings#_when-clause-contexts

[monk-time]

@jrieken, I don't have time to understand this undocumented local/global scope distinction, but this issue is about 'when' clauses that don't work as documented. Since it doesn't seem like you have tried to reproduce what I've described in the OP, I took the fresh Insiders build and put a simple action into keybindings.json:

[
    { "key": "ctrl+l",                "command": "editor.action.toggleMinimap",
                                         "when": "inDebugMode" }
]

I create a new file, set the cursor to the main and only editor, debug mode is obviously not active, I hit ctrl+l, and the minimap gets toggled. This means that the clause was completely ignored.

If you are talking about some other issue, please stop changing this one into something that is not related to it and open another issue. Sorry, but I am getting really frustrated here.

[usernamehw]

Can you find even 1 when context for which workbench.action.terminal.toggleTerminal won't ignore it? I tried:
[findWidgetVisible, inQuickOpen, explorerViewletVisible, terminalFocus, searchViewletVisible, config.editor.minimap.enabled, replaceActive, globalMessageVisible, parameterHintsVisible, renameInputVisible, openEditorsFocus, explorerViewletFocus, inDebugMode]
and some negations.

It ignores: "when": "thing" & "when": "!thing"

[jrieken]

nothing should happen, the 'when' clause evaluates to false so the shortcut should be disabled.

@monk-time I am sorry, I mis read that and though about the opposite.

I believe the problem is that those command appear in the global menu (with their assigned shortcut) and that therefore they are bypassing our context key checks... It's actually not us handling the keybinding at that point... Needs some thinking

[jrieken]

Yep, command is being called by global menu, not the keybindings service.... @bpasero Is this something new?

[bpasero]

Blocked by dynamic menus still not being enabled on macOS and Linux.

[sbatten]

As discussed this will be unblocked debt week in october

[sbatten]

resolved with #59936 and #59855

[chrmarti]

Still works:

	{
		"key": "ctrl+l",
		"command": "workbench.action.toggleSidebarVisibility",
		"when": "false"
	}

VS Code version: Code - Insiders 1.29.0-insider (d7ac6e8, 2018-11-01T06:11:43.390Z)
OS version: Darwin x64 17.7.0

[sbatten]

sorry, my fixes actually bring back the behavior described here. the behavior of keybindings working out of context needs to be fixed in the keybindingservice because currently lookupKeybinding returns the latest keybinding but ignores context. this should be changed imo to use an optionally supplied context.

thoughts @alexandrudima

[alexdima]

@sbatten As we are all aware from multiple discussions on this topic, contexts are different depending on where the focus sits. I can implement a new method in IKeybindingService to look up keybindings based on context, but then won't you have 100% the same problem as the one described in #55453 , which cannot be solved due to performance concerns -- specifically #55453 (comment)

[sbatten]

@alexandrudima this iteration, I have made some improvements that greatly reduce the amount of updates the menu based on context. My current view is that the global application menu reflects the global context so enablement/disablement/keybindings must be based on context keys in the global scope. Thoughts?

[alexdima]

@sbatten

My current view is that the global application menu reflects the global context so enablement/disablement/keybindings must be based on context keys in the global scope. Thoughts?

That is also incorrect, but in a different, more subtle way, and arguably would make things a lot worse. Before jumping to implement something worse, let's look at the problem.

Take, for example, the Selection menu:

Now let's look at a default keybinding from there:

{ "key": "shift+alt+up",          "command": "editor.action.copyLinesUpAction",
                                     "when": "editorTextFocus && !editorReadonly" },

editorTextFocus is undefined in the global context, so the when clause would not hold. So, the action would get no keybinding rendered next to it if we were to lookup keybindings using the global context.

---

Also, I think this bug affects only the native menu, because in the native menu case the menu itself (Electron via C++ code) listens to those keybindings and executes the menu item directly, outside of our logic. For custom menus, this is no longer a problem.

---

I think there are two possible correct fixes:

1. lookup keybindings based on focused context and constantly update the menus.
2. enhance Electron to provide information if a native menu item is invoked because the user clicked on it, or used arrow keys + enter, or invoked the keyboard shortcut. If we were to differentiate between these cases, then we could interpret ourselves the keyboard shortcut.

[jrieken]

enhance Electron to provide information if a native menu item is invoked because the user clicked on it, or used arrow keys + enter, or invoked the keyboard shortcut.

We might be able to know that. I have tested mac and clicking vs keyboard shortcuts can be differentiated by the event - it's only partial but when pressing cmd+z the metaKey modifier is set, when clicking all are false.

[sbatten]

@alexandrudima

My current view is that the global application menu reflects the global context so enablement/disablement/keybindings must be based on context keys in the global scope.

This was more of a statement of how I think things should be not necessarily how they are, but I agree this should be fleshed out. With this view actually in place, it limits the menu updates to relevant updates only for enablement/disablement/keybindings thanks to the support in @jrieken's menu model.

For keybindings, we could fix this issue by trying to update the menu on every focus change, but that seems excessive. We also would still run into this issue with enablement/disablement, however. Keybindings get resolved for the menu each time it is updated. Enablement/Disablement does not. The context is set once when the menus are created in the model.

lookup keybindings based on focused context and constantly update the menus.

This makes me ask the question why the context service doesn't support evaluating against the current context based on focus if that's what matters instead of being required to track focus around. If I could set a flag on the context service to do this before handing it to the menu model and the keybinding service could use that when evaluating keybinds, it could work.

Yes, I am coupling two issues but they are both related to resolving commands against a focused context.