From c94040f46e641e57c533c430f299dc1acbbf032c Mon Sep 17 00:00:00 2001 From: abose Date: Fri, 17 Jan 2025 11:27:25 +0530 Subject: [PATCH 1/4] fix: option-shift-left and right not selecting words in mac We assigned reserved mac keyboard shortcut for word selection nav to code folding. release that shortcut to be unused in all platforms. Expand and collapse current rewired to `ctrl/cmd-shift-[` and `ctrl/cmd-shift-]` in all platforms as that shortcut was released from redundant recent navigation flows. Also, it aligns with `ctrl-[`and `ctrl-]` for indent changing. Expand and collpase all shortcut was never used in usage stats, so removed it. --- src/extensions/default/CodeFolding/main.js | 16 ++++------------ .../NavigationAndHistory/keyboard.json | 8 -------- 2 files changed, 4 insertions(+), 20 deletions(-) diff --git a/src/extensions/default/CodeFolding/main.js b/src/extensions/default/CodeFolding/main.js index c791352b1d..77e1349d3e 100644 --- a/src/extensions/default/CodeFolding/main.js +++ b/src/extensions/default/CodeFolding/main.js @@ -49,12 +49,8 @@ define(function (require, exports, module) { GUTTER_NAME = "CodeMirror-foldgutter", CODE_FOLDING_GUTTER_PRIORITY = Editor.CODE_FOLDING_GUTTER_PRIORITY, codeFoldingMenuDivider = "codefolding.divider", - collapseKey = "Alt-Shift-Left", - collapseKeyDisplay = "Alt-Shift-←", - expandKey = "Alt-Shift-Right", - expandKeyDisplay = "Alt-Shift-→", - collapseAllKey = "Ctrl-Alt-[", - expandAllKey = "Ctrl-Alt-]"; + collapseKey = "Ctrl-Shift-[", + expandKey = "Ctrl-Shift-]"; ExtensionUtils.loadStyleSheet(module, "main.less"); @@ -360,8 +356,6 @@ define(function (require, exports, module) { KeyBindingManager.removeBinding(collapseKey); KeyBindingManager.removeBinding(expandKey); - KeyBindingManager.removeBinding(collapseAllKey); - KeyBindingManager.removeBinding(expandAllKey); //remove menus Menus.getMenu(Menus.AppMenuBar.VIEW_MENU).removeMenuDivider(codeFoldingMenuDivider.id); @@ -419,10 +413,8 @@ define(function (require, exports, module) { Menus.getMenu(Menus.AppMenuBar.VIEW_MENU).addMenuItem(EXPAND); //register keybindings - KeyBindingManager.addBinding(COLLAPSE_ALL, [ {key: collapseAllKey}]); - KeyBindingManager.addBinding(EXPAND_ALL, [ {key: expandAllKey}]); - KeyBindingManager.addBinding(COLLAPSE, [{key: collapseKey, displayKey: collapseKeyDisplay}]); - KeyBindingManager.addBinding(EXPAND, [{key:expandKey, displayKey: expandKeyDisplay}]); + KeyBindingManager.addBinding(COLLAPSE, [{key: collapseKey}]); + KeyBindingManager.addBinding(EXPAND, [{key:expandKey}]); // Add gutters & restore saved expand/collapse state in all currently open editors diff --git a/src/extensionsIntegrated/NavigationAndHistory/keyboard.json b/src/extensionsIntegrated/NavigationAndHistory/keyboard.json index 0c696514cc..b60cff33df 100644 --- a/src/extensionsIntegrated/NavigationAndHistory/keyboard.json +++ b/src/extensionsIntegrated/NavigationAndHistory/keyboard.json @@ -6,10 +6,6 @@ { "key": "Ctrl-Tab", "platform": "mac" - }, - { - "key": "Cmd-Shift-]", - "platform": "mac" } ], "recent-files.prev": [ @@ -19,10 +15,6 @@ { "key": "Ctrl-Shift-Tab", "platform": "mac" - }, - { - "key": "Cmd-Shift-[", - "platform": "mac" } ], "navigation.jump.back": [ From 43f686af7d39e41dd83fcee4ca2ea6373cd2bbfc Mon Sep 17 00:00:00 2001 From: abose Date: Fri, 17 Jan 2025 11:57:31 +0530 Subject: [PATCH 2/4] fix: mac option-left and right not navigation words as per apple spec We earlier used to associate this with navigate back and forward, now changing navigation shortcut in mac to Cmd-alt-Left and right which is unused and stantard mac browser fwd and back nav shortcut --- .../NavigationAndHistory/keyboard.json | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/extensionsIntegrated/NavigationAndHistory/keyboard.json b/src/extensionsIntegrated/NavigationAndHistory/keyboard.json index b60cff33df..6b4629c63f 100644 --- a/src/extensionsIntegrated/NavigationAndHistory/keyboard.json +++ b/src/extensionsIntegrated/NavigationAndHistory/keyboard.json @@ -20,13 +20,25 @@ "navigation.jump.back": [ { "key": "Alt-Left", - "displayKey": "Alt-←" + "displayKey": "Alt-←", + "platform": "win" + }, + { + "key": "Cmd-Alt-Left", + "displayKey": "Cmd-Alt-←", + "platform": "mac" } ], "navigation.jump.fwd": [ { "key": "Alt-Right", - "displayKey": "Alt-→" + "displayKey": "Alt-→", + "platform": "win" + }, + { + "key": "Cmd-Alt-Right", + "displayKey": "Cmd-Alt-→", + "platform": "mac" } ] } From bc8f3eafe2c0c8589bb3adf964c51e572b564233 Mon Sep 17 00:00:00 2001 From: abose Date: Fri, 17 Jan 2025 12:32:59 +0530 Subject: [PATCH 3/4] fix: in mac browsers, "Cmd-Alt-Right" and left is reserved for browser navigation and not assignable So added framework option nativeOnly and browserOnly to selectiveley apply shortcut. in browser, mac, nav back and forward will be assigned to `Ctrl-Alt-Right` and left instead. --- src/command/KeyBindingManager.js | 15 ++++++++++++--- .../NavigationAndHistory/keyboard.json | 18 ++++++++++++++++-- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/src/command/KeyBindingManager.js b/src/command/KeyBindingManager.js index 272fe3ea89..aa47fcea7e 100644 --- a/src/command/KeyBindingManager.js +++ b/src/command/KeyBindingManager.js @@ -719,11 +719,19 @@ define(function (require, exports, module) { normalized, normalizedDisplay, explicitPlatform = keyBinding.platform || platform, + explicitBrowserOnly = keyBinding.browserOnly, + explicitNativeOnly = keyBinding.nativeOnly, targetPlatform, command, bindingsToDelete = [], existing; + if(Phoenix.isNativeApp && explicitBrowserOnly) { + return null; + } + if(!Phoenix.isNativeApp && explicitNativeOnly) { + return null; + } // For platform: "all", use explicit current platform if (explicitPlatform && explicitPlatform !== "all") { targetPlatform = explicitPlatform; @@ -983,12 +991,13 @@ define(function (require, exports, module) { * Returns record(s) for valid key binding(s). * * @param {!string | Command} command - A command ID or command object - * @param {{key: string, displayKey:string, platform: string}} keyBindings + * @param {{key: string, displayKey:string, platform: string, browserOnly: boolean, nativeOnly:boolean}} keyBindings * A single key binding or an array of keybindings. * In an array of keybinding `platform` property is also available. Example: * "Shift-Cmd-F". Mac and Win key equivalents are automatically * mapped to each other. Use displayKey property to display a different - * string (e.g. "CMD+" instead of "CMD="). + * string (e.g. "CMD+" instead of "CMD="). if browserOnly is true, then the shortcut will only apply in browser + * if nativeOnly is set, the shortcut will only apply in native apps * @param {?string} platform The target OS of the keyBindings either * "mac", "win" or "linux". If undefined, all platforms not explicitly * defined will use the key binding. @@ -1022,7 +1031,7 @@ define(function (require, exports, module) { // process platform-specific bindings first keyBindings.sort(_sortByPlatform); - keyBindings.forEach(function addSingleBinding(keyBindingRequest) { + keyBindings.forEach(function (keyBindingRequest) { // attempt to add keybinding keyBinding = _addBinding(commandID, keyBindingRequest, { platform: keyBindingRequest.platform, diff --git a/src/extensionsIntegrated/NavigationAndHistory/keyboard.json b/src/extensionsIntegrated/NavigationAndHistory/keyboard.json index 6b4629c63f..c7d452bd80 100644 --- a/src/extensionsIntegrated/NavigationAndHistory/keyboard.json +++ b/src/extensionsIntegrated/NavigationAndHistory/keyboard.json @@ -26,7 +26,14 @@ { "key": "Cmd-Alt-Left", "displayKey": "Cmd-Alt-←", - "platform": "mac" + "platform": "mac", + "nativeOnly": true + }, + { + "key": "Ctrl-Alt-Left", + "displayKey": "Ctrl-Alt-←", + "platform": "mac", + "browserOnly": true } ], "navigation.jump.fwd": [ @@ -38,7 +45,14 @@ { "key": "Cmd-Alt-Right", "displayKey": "Cmd-Alt-→", - "platform": "mac" + "platform": "mac", + "nativeOnly": true + }, + { + "key": "Ctrl-Alt-Right", + "displayKey": "Ctrl-Alt-→", + "platform": "mac", + "browserOnly": true } ] } From 1aaecf0a02bd638c3105dff25fb41cc9ede6d97d Mon Sep 17 00:00:00 2001 From: abose Date: Fri, 17 Jan 2025 13:02:49 +0530 Subject: [PATCH 4/4] test: key binding manager tests for browserOnly and nativeOnly key bindings --- test/spec/KeyBindingManager-test.js | 52 +++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/test/spec/KeyBindingManager-test.js b/test/spec/KeyBindingManager-test.js index 3719a56b9c..a5bec81a19 100644 --- a/test/spec/KeyBindingManager-test.js +++ b/test/spec/KeyBindingManager-test.js @@ -368,6 +368,58 @@ define(function (require, exports, module) { expect(KeyBindingManager.getKeymap()).toEqual(expected); }); + + it("should handle browserOnly and nativeOnly bindings correctly", function () { + // Test browser-only bindings + let result = KeyBindingManager.addBinding("test.browser", { + key: "F6", + browserOnly: true + }); + let keymap = KeyBindingManager.getKeymap(); + if(Phoenix.isNativeApp) { + expect(result).toBeNull(); + expect(keymap["F6"]).not.toBeDefined(); + } else { + expect(result.key).toBe("F6"); + expect(keymap["F6"].key).toBe("F6"); + } + // Test native-only bindings + result = KeyBindingManager.addBinding("test.native", { + key: "F7", + nativeOnly: true + }); + keymap = KeyBindingManager.getKeymap(); + if(!Phoenix.isNativeApp) { + expect(result).toBeNull(); + expect(keymap["F7"]).not.toBeDefined(); + } else { + expect(result.key).toBe("F7"); + expect(keymap["F7"].key).toBe("F7"); + } + }); + + it("should handle browserOnly and nativeOnly in multiple bindings", function () { + // Test browser-only bindings + let result = KeyBindingManager.addBinding("test.allPlats", [ + {key: "F6", browserOnly: true}, + {key: "F7", nativeOnly: true} + ]); + let keymap = KeyBindingManager.getKeymap(); + expect(result.length).toBe(1); + if (Phoenix.isNativeApp) { + // Native app environment + expect(result[0].key).toBe("F7"); // Only native binding should be added + expect(keymap["F7"]).toBeDefined(); + expect(keymap["F7"].key).toBe("F7"); + expect(keymap["F6"]).not.toBeDefined(); // Browser-only should not be defined + } else { + // Browser environment + expect(result[0].key).toBe("F6"); // Only browser binding should be added + expect(keymap["F6"]).toBeDefined(); + expect(keymap["F6"].key).toBe("F6"); + expect(keymap["F7"]).not.toBeDefined(); // Native-only should not be defined + } + }); }); describe("removeBinding", function () {