Skip to content

Commit

Permalink
fix(editor): Make keyboard shortcuts more strict; don't accept extra …
Browse files Browse the repository at this point in the history 
…Ctrl/Alt/Shift keys (#8024)

## Summary
Keyboard shortcuts such as Shift+S get triggered even when other keys
(Ctrl/Alt) are pressed, creating conflicts with browser or OS shortcuts.

This PR makes keyboard shortcuts more strict: the exact combination
needs to be pressed, no extra keys allowed.
--> Ctrl+Shift+S will no longer trigger the Shift+S shortcut to add a
sticky note

## Related tickets and issues
https://n8nio.slack.com/archives/C035KBDA917/p1702545933647959
  • Loading branch information
@elsmr
elsmr authored Dec 20, 2023
1 parent 81994ce commit 8df49e1
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 25 deletions.
9 changes: 9 additions & 0 deletions cypress/e2e/25-stickies.cy.ts
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { META_KEY } from '../constants';
import { WorkflowPage as WorkflowPageClass } from '../pages/workflow';
import { getPopper } from '../utils';
import { Interception } from 'cypress/types/net-stubbing';
Expand Down Expand Up @@ -35,6 +36,14 @@ describe('Canvas Actions', () => {
workflowPage.actions.addStickyFromContextMenu();
workflowPage.actions.hitAddStickyShortcut();

workflowPage.getters.stickies().should('have.length', 3);

// Should not add a sticky for ctrl+shift+s
cy.get('body')
.type(META_KEY, { delay: 500, release: false })
.type('{shift}', { release: false })
.type('s');

workflowPage.getters.stickies().should('have.length', 3);
workflowPage.getters
.stickies()
Expand Down
51 changes: 26 additions & 25 deletions packages/editor-ui/src/views/NodeView.vue
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1168,7 +1168,13 @@ export default defineComponent({
async keyDown(e: KeyboardEvent) {
this.contextMenu.close();
if (e.key === 's' && this.isCtrlKeyPressed(e)) {
const ctrlModifier = this.isCtrlKeyPressed(e) && !e.shiftKey && !e.altKey;
const shiftModifier = e.shiftKey && !e.altKey && !this.isCtrlKeyPressed(e);
const ctrlAltModifier = this.isCtrlKeyPressed(e) && e.altKey && !e.shiftKey;
const noModifierKeys = !this.isCtrlKeyPressed(e) && !e.shiftKey && !e.altKey;
const readOnly = this.isReadOnlyRoute || this.readOnlyEnv;
if (e.key === 's' && ctrlModifier && !readOnly) {
e.stopPropagation();
e.preventDefault();
Expand Down Expand Up @@ -1201,7 +1207,7 @@ export default defineComponent({
return;
}
if (e.key === 'Escape') {
if (e.key === 'Escape' && noModifierKeys) {
this.createNodeActive = false;
if (this.activeNode) {
void this.externalHooks.run('dataDisplay.nodeEditingFinished');
Expand All @@ -1220,42 +1226,37 @@ export default defineComponent({
.map((node) => node && this.workflowsStore.getNodeByName(node.name))
.filter((node) => !!node) as INode[];
if (e.key === 'd' && !this.isCtrlKeyPressed(e)) {
if (e.key === 'd' && noModifierKeys && !readOnly) {
void this.callDebounced('toggleActivationNodes', { debounceTime: 350 }, selectedNodes);
} else if (e.key === 'd' && this.isCtrlKeyPressed(e)) {
} else if (e.key === 'd' && ctrlModifier && !readOnly) {
if (selectedNodes.length > 0) {
e.preventDefault();
void this.duplicateNodes(selectedNodes);
}
} else if (e.key === 'p' && !this.isCtrlKeyPressed(e)) {
} else if (e.key === 'p' && noModifierKeys && !readOnly) {
if (selectedNodes.length > 0) {
e.preventDefault();
this.togglePinNodes(selectedNodes, 'keyboard-shortcut');
}
} else if (e.key === 'Delete' || e.key === 'Backspace') {
} else if ((e.key === 'Delete' || e.key === 'Backspace') && noModifierKeys && !readOnly) {
e.stopPropagation();
e.preventDefault();
void this.callDebounced('deleteNodes', { debounceTime: 500 }, selectedNodes);
} else if (e.key === 'Tab') {
} else if (e.key === 'Tab' && noModifierKeys && !readOnly) {
this.onToggleNodeCreator({
source: NODE_CREATOR_OPEN_SOURCES.TAB,
createNodeActive: !this.createNodeActive && !this.isReadOnlyRoute && !this.readOnlyEnv,
});
} else if (
e.key === 'Enter' &&
this.isCtrlKeyPressed(e) &&
!this.isReadOnlyRoute &&
!this.readOnlyEnv
) {
} else if (e.key === 'Enter' && ctrlModifier && !readOnly) {
void this.onRunWorkflow();
} else if (e.key === 'S' && e.shiftKey && !this.isReadOnlyRoute && !this.readOnlyEnv) {
} else if (e.key === 'S' && shiftModifier && !readOnly) {
void this.onAddNodes({ nodes: [{ type: STICKY_NODE_TYPE }], connections: [] });
} else if (e.key === this.controlKeyCode) {
this.ctrlKeyPressed = true;
} else if (e.key === ' ') {
this.moveCanvasKeyPressed = true;
} else if (e.key === 'F2' && !this.isReadOnlyRoute && !this.readOnlyEnv) {
} else if (e.key === 'F2' && noModifierKeys && !readOnly) {
const lastSelectedNode = this.lastSelectedNode;
if (lastSelectedNode !== null && lastSelectedNode.type !== STICKY_NODE_TYPE) {
void this.callDebounced(
Expand All @@ -1264,21 +1265,21 @@ export default defineComponent({
lastSelectedNode.name,
);
}
} else if (e.key === 'a' && this.isCtrlKeyPressed(e)) {
} else if (e.key === 'a' && ctrlModifier) {
// Select all nodes
e.stopPropagation();
e.preventDefault();
void this.callDebounced('selectAllNodes', { debounceTime: 1000 });
} else if (e.key === 'c' && this.isCtrlKeyPressed(e)) {
} else if (e.key === 'c' && ctrlModifier) {
void this.callDebounced('copyNodes', { debounceTime: 1000 }, selectedNodes);
} else if (e.key === 'x' && this.isCtrlKeyPressed(e)) {
} else if (e.key === 'x' && ctrlModifier && !readOnly) {
// Cut nodes
e.stopPropagation();
e.preventDefault();
void this.callDebounced('cutNodes', { debounceTime: 1000 }, selectedNodes);
} else if (e.key === 'n' && this.isCtrlKeyPressed(e) && e.altKey) {
} else if (e.key === 'n' && ctrlAltModifier) {
// Create a new workflow
e.stopPropagation();
e.preventDefault();
Expand All @@ -1296,7 +1297,7 @@ export default defineComponent({
title: this.$locale.baseText('nodeView.showMessage.keyDown.title'),
type: 'success',
});
} else if (e.key === 'Enter') {
} else if (e.key === 'Enter' && noModifierKeys) {
// Activate the last selected node
const lastSelectedNode = this.lastSelectedNode;
Expand All @@ -1309,13 +1310,13 @@ export default defineComponent({
}
this.ndvStore.activeNodeName = lastSelectedNode.name;
}
} else if (e.key === 'ArrowRight' && e.shiftKey) {
} else if (e.key === 'ArrowRight' && shiftModifier) {
// Select all downstream nodes
e.stopPropagation();
e.preventDefault();
void this.callDebounced('selectDownstreamNodes', { debounceTime: 1000 });
} else if (e.key === 'ArrowRight') {
} else if (e.key === 'ArrowRight' && noModifierKeys) {
// Set child node active
const lastSelectedNode = this.lastSelectedNode;
if (lastSelectedNode === null) {
Expand All @@ -1337,13 +1338,13 @@ export default defineComponent({
false,
true,
);
} else if (e.key === 'ArrowLeft' && e.shiftKey) {
} else if (e.key === 'ArrowLeft' && shiftModifier) {
// Select all downstream nodes
e.stopPropagation();
e.preventDefault();
void this.callDebounced('selectUpstreamNodes', { debounceTime: 1000 });
} else if (e.key === 'ArrowLeft') {
} else if (e.key === 'ArrowLeft' && noModifierKeys) {
// Set parent node active
const lastSelectedNode = this.lastSelectedNode;
if (lastSelectedNode === null) {
Expand All @@ -1369,7 +1370,7 @@ export default defineComponent({
false,
true,
);
} else if (['ArrowUp', 'ArrowDown'].includes(e.key)) {
} else if (['ArrowUp', 'ArrowDown'].includes(e.key) && noModifierKeys) {
// Set sibling node as active
// Check first if it has a parent node
Expand Down

0 comments on commit 8df49e1

Please sign in to comment.