Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Prevent menus from stealing focus and enables indent/undent from menus #672

Merged
merged 11 commits into from

3 participants

@tvoliter
  • Menus.js now listens for mousedown on dropdown menus and calls preventDefault() to stop menus from taking focus from the editor. Removed old work around that called EditorManager.focusEditor(). This only worked for the main editor.
  • Select All from the menu now works in inline editors
  • Made indent/unident work from both menus and keyboard commands (tab, Ctrl+] and shift-Tab, Ctrl+[)
  • Modified KeyMap to translate tab, left bracket, and right bracket, to their "common names"
tvoliter added some commits
@tvoliter tvoliter Making indent and unindent work from menu and via both sets of key co…
…mmands
3df6b61
@tvoliter tvoliter Merge remote-tracking branch 'origin/master' into tvoliter/menus
* origin/master:
  Updated CodeMirror SHA--got out of date
  use font stack variable wherever font-family is specified
  fix typo
  Fix font-size on windows. Move @scaleX mixin to brackets_mixins.less.
  refactor bootstrap font stacks to use variables. update @import statements to use url().
3b44f2f
@tvoliter tvoliter Merge remote-tracking branch 'origin/master' into tvoliter/menus
* origin/master:
  Updated CodeMirror SHA--got out of date
  use font stack variable wherever font-family is specified
  fix typo
  Fix font-size on windows. Move @scaleX mixin to brackets_mixins.less.
  refactor bootstrap font stacks to use variables. update @import statements to use url().
e90f798
@tvoliter tvoliter Merge remote-tracking branch 'origin/tvoliter/menus' into tvoliter/menus
* origin/tvoliter/menus:
f5193ef
@tvoliter tvoliter JSLint cleanup 49a89ad
@tvoliter

Hey @peterflynn could you take a look at this? I wanted to verify the focus is handled correctly plus the tab handling got a bit tricky

src/command/KeyMap.js
@@ -204,6 +204,11 @@ define(function (require, exports, module) {
}
}
+ // Translate some keys to their common names
+ if (key === "Ý") { key = "]"; }
+ if (key === "Û") { key = "["; }
@peterflynn Owner

Do we understand why "[" and "]" come up as such weird characters in the keyboard event? Does that only happen if a modifier key is held down? Does it depend on locale? (e.g. would we get a different weird character if the user had a German keyboard layout?)

I didn't get to the bottom of why this happens. I did see that even with no modifier keys down this is the character that comes in through the event.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/command/Menus.js
((10 lines not shown))
}
}
}
+ // Prevent the menus from taking focus away from the editor
+ $("#main-toolbar .dropdown").mousedown(function (e) {
@peterflynn Owner

Might want to add a note here that the only reason we don't also have to do this for top-level menu bar items is (I think) because Bootstrap already takes care of it in bootstrap-dropdown.js.

I'll add a note. Interestingly I don't see code that does this in bootstrap-dropdown.js that does this directly. The $.fn.dropdown returns false... I am not sure what that does or if it is related to focus.

@peterflynn Owner

It's the "return false" in that click handler that does it -- that's an old-school equivalent to preventDefault().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/editor/Editor.js
((20 lines not shown))
*/
- function _handleTabKey(instance) {
+ function _handleIndent() {
@peterflynn Owner

I think we need to distinguish between the indent and plain Tab key. At least in the editors I tried (Sublime, TextMate), Tab's behavior varies more depending on context. If you put the cursor in the middle of a line and hit Tab, you insert a tab/spaces there in the middle; if you hit Ctrl+] instead, you insert a tab/spaces at the start of the line, indenting the entire line from the beginning.

I think a real Indent/Unindent command was not part of the acceptance criteria, though, so if you want we could just remove those menu items and tackle this as a separate (mini) enhancement.

Good call. I removed the key handling for Tab and Shift-Tab in bracket.js and restored the handler in Editor.js. I left the "\t" to "Tab" conversion in KeyMap.js since I think we will want that in the future.

I think i will just remove the menu items. I don't have a good way to display tab and Shift tab for indent because right now for the shortcut to appear the command has to be registered, but we want code mirror to handle these internally. I had a chat with NJ and I think we will need do make some architectural changes later so we can own all the keyboard handling and decide when to proxy to code mirror.

@peterflynn Owner

I'm confused: why don't we want to leave the Indent/Unindent menu items in place, but registered only to Ctrl+]/[ ?

If we are removing the menu items, then you should remove _handleUnindent() in Editor too, since it's now unused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/brackets.js
@@ -183,8 +183,13 @@ define(function (require, exports, module) {
{"Shift-F3": Commands.EDIT_FIND_PREVIOUS, "platform": "win"},
{"Ctrl-Alt-F": Commands.EDIT_REPLACE, "platform": "mac"},
{"Ctrl-H": Commands.EDIT_REPLACE, "platform": "win"},
- {"Ctrl-Tab": Commands.EDIT_INDENT},
- {"Ctrl-Shift-Tab": Commands.EDIT_UNINDENT},
+
+ // There are two shortcut keys for indenting/unindenting. The first ones
+ // listed will be the ones displayed in the UI.
+ {"Ctrl-]": Commands.EDIT_INDENT},
+ {"Ctrl-[": Commands.EDIT_UNINDENT},
+ {"Tab": Commands.EDIT_INDENT},
+ {"Shift-Tab": Commands.EDIT_UNINDENT},
@peterflynn Owner

Mapping Tab and Shift+Tab to a command seems to break usage of the Tab key outside of editor UI. We don't have any real multi-text-field forms in our UI yet, which is where this would be really evident. But you can repro more minor issues, e.g. Tab used to terminate the File > New text editor but it no longer does.

Especially since the Tab and Shift+Tab shortcuts won't be shown on the menu item, maybe we should just avoid making those into global shortcuts, and leave them scoped just to Editor as we had before? (As a bonus, I think this would make it easier to address my comment below about Tab vs. Indent needing to have slightly different behavior).

Good call. I removed the key handling for Tab and Shift-Tab in bracket.js and restored the handler in Editor.js. I left the "\t" to "Tab" conversion in KeyMap.js since I think we will want that in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@peterflynn
Owner

Done with initial review.

tvoliter added some commits
@tvoliter tvoliter Make CodeMirror handle tab and shift tab internally so tab is overloa…
…ded outside the editor
75d5d55
@tvoliter tvoliter Removed menus for Ctrl-[ and Ctrl-] since we really don't handling th…
…at type of indent/unindent yet. Also refixed issue #461 by connecting the select all to _selectAllVisible()
7915881
@tvoliter tvoliter remove unused comment bc29d87
@tvoliter tvoliter Merge remote-tracking branch 'origin/master' into tvoliter/menus
* origin/master: (24 commits)
  Better fix for #649.
  Change code padding to 14px.
  Set z-index: auto for JSLint and Search Results toolbars.
  - show hide sidebar function was in two locations. Removed duplicate - added safeguard when registering commands to detect bad command strings - added more obvious comments about keeping Command string names in sync with native code
  Fix Command string rename VIEW_REFRESH_WINDOW > DEBUG_REFRESH_WINDOW
  Increase editor padding, decrease line number size.
  Fix project title font size and color.
  Fixing stupid comment
  Fix dirty bit appearance in inline editor
  Make project background gradient work on IE.
  Remove project background image.
  Use heavier font for folders and selected files. Replace project background png with gradient.
  Update dirty indicator.
  Experimental feature: option to use tab characters instead of spaces. Use Debug menu item to toggle; affects all running Editor instances. Setting is NOT persisted between launches.
  Remove font changes.
  More cleanup.
  LESS cleanup.
  Usability tweaks: - Tooltip on JSLint star - Tooltip on Go Live icon (changes based on status) - Show error message instead of silently no-op if Go Live clicked when no   doc or the wrong type of doc is open
  More project panel styling.
  First pass at Northstar styles for JSTree.
  ...
2b3a283
@redmunds redmunds commented on the diff
src/command/Menus.js
@@ -106,11 +103,22 @@ define(function (require, exports, module) {
shortcut = keyCmd.replace(/-/g, "+");
}
- $("#" + menuID).append("<span class='menu-shortcut'>" + shortcut + "</span>");
+ var $menu = $("#" + menuID);
+ // Some commands have multiple key commands. Only add the first one.
+ if ($menu.find(".menu-shortcut").length === 0) {
+ $menu.append("<span class='menu-shortcut'>" + shortcut + "</span>");
@redmunds Collaborator

Seems like the code can exit the loop after the first shortcut string is added. Or at least, only build the "shortcut" string once you've determined that it's the first one.

@peterflynn Owner

I think that's the idea -- if there's more than one shortcut bound to the same menu item, we don't want to list all the shortcuts in the menu UI... just the first one we come across. There's no break or anything, so it won't actually exit the loop; it just won't add another shortcut label to the UI if one is already present.

@peterflynn Owner

Although, Ty, this does raise one issue: if there's more than one shortcut, the one that wins is completely arbitrary, not the first one listed in the code in brackets.js (because we're iterating a map here, not an ordered list).

Maybe because of this, for now we should print a warning to the console whenever a 2nd shortcut for the same menu is encountered? I think with the removal of the Tab bindings above, we no longer have any commands that would hit this caseat the moment...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/editor/Editor.js
((20 lines not shown))
*/
- function _handleTabKey(instance) {
+ function _handleTabKey() {
@peterflynn Owner

Could we back out the changes to this function's signature & body now that it's not being changed into a global command handler?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tvoliter added some commits
@tvoliter tvoliter revert _handleTabKey() to early version since only code mirror key ha…
…ndling is connected to it right now.
ae5cdd6
@tvoliter tvoliter Merge remote-tracking branch 'origin/master' into tvoliter/menus
* origin/master:
  Back to semibold.
  More project panel font tweaks.
  Updated font weights for project panel.
2e3d80e
@peterflynn
Owner

Looks good! Merging now...

@peterflynn peterflynn merged commit 69e71a6 into master
@peterflynn peterflynn referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 17, 2012
  1. @tvoliter
  2. @tvoliter

    Merge remote-tracking branch 'origin/master' into tvoliter/menus

    tvoliter authored
    * origin/master:
      Updated CodeMirror SHA--got out of date
      use font stack variable wherever font-family is specified
      fix typo
      Fix font-size on windows. Move @scaleX mixin to brackets_mixins.less.
      refactor bootstrap font stacks to use variables. update @import statements to use url().
  3. @tvoliter

    Merge remote-tracking branch 'origin/master' into tvoliter/menus

    tvoliter authored
    * origin/master:
      Updated CodeMirror SHA--got out of date
      use font stack variable wherever font-family is specified
      fix typo
      Fix font-size on windows. Move @scaleX mixin to brackets_mixins.less.
      refactor bootstrap font stacks to use variables. update @import statements to use url().
  4. @tvoliter
  5. @tvoliter

    JSLint cleanup

    tvoliter authored
Commits on Apr 19, 2012
  1. @tvoliter
  2. @tvoliter

    Removed menus for Ctrl-[ and Ctrl-] since we really don't handling th…

    tvoliter authored
    …at type of indent/unindent yet. Also refixed issue #461 by connecting the select all to _selectAllVisible()
  3. @tvoliter

    remove unused comment

    tvoliter authored
  4. @tvoliter

    Merge remote-tracking branch 'origin/master' into tvoliter/menus

    tvoliter authored
    * origin/master: (24 commits)
      Better fix for #649.
      Change code padding to 14px.
      Set z-index: auto for JSLint and Search Results toolbars.
      - show hide sidebar function was in two locations. Removed duplicate - added safeguard when registering commands to detect bad command strings - added more obvious comments about keeping Command string names in sync with native code
      Fix Command string rename VIEW_REFRESH_WINDOW > DEBUG_REFRESH_WINDOW
      Increase editor padding, decrease line number size.
      Fix project title font size and color.
      Fixing stupid comment
      Fix dirty bit appearance in inline editor
      Make project background gradient work on IE.
      Remove project background image.
      Use heavier font for folders and selected files. Replace project background png with gradient.
      Update dirty indicator.
      Experimental feature: option to use tab characters instead of spaces. Use Debug menu item to toggle; affects all running Editor instances. Setting is NOT persisted between launches.
      Remove font changes.
      More cleanup.
      LESS cleanup.
      Usability tweaks: - Tooltip on JSLint star - Tooltip on Go Live icon (changes based on status) - Show error message instead of silently no-op if Go Live clicked when no   doc or the wrong type of doc is open
      More project panel styling.
      First pass at Northstar styles for JSTree.
      ...
  5. @tvoliter

    revert _handleTabKey() to early version since only code mirror key ha…

    tvoliter authored
    …ndling is connected to it right now.
  6. @tvoliter

    Merge remote-tracking branch 'origin/master' into tvoliter/menus

    tvoliter authored
    * origin/master:
      Back to semibold.
      More project panel font tweaks.
      Updated font weights for project panel.
This page is out of date. Refresh to see the latest.
View
2  src/brackets.js
@@ -172,8 +172,6 @@ define(function (require, exports, module) {
{"Shift-F3": Commands.EDIT_FIND_PREVIOUS, "platform": "win"},
{"Ctrl-Alt-F": Commands.EDIT_REPLACE, "platform": "mac"},
{"Ctrl-H": Commands.EDIT_REPLACE, "platform": "win"},
- {"Ctrl-Tab": Commands.EDIT_INDENT},
- {"Ctrl-Shift-Tab": Commands.EDIT_UNINDENT},
// VIEW
{"Ctrl-Shift-H": Commands.VIEW_HIDE_SIDEBAR},
View
3  src/command/KeyMap.js
@@ -204,6 +204,9 @@ define(function (require, exports, module) {
}
}
+ // Translate some keys to their common names }
+ if (key === "\t") { key = "Tab"; }
+
return _buildKeyDescriptor(hasCtrl, hasAlt, hasShift, key);
}
View
18 src/command/Menus.js
@@ -41,7 +41,6 @@ define(function (require, exports, module) {
"menu-edit-find-next": Commands.EDIT_FIND_NEXT,
"menu-edit-find-previous": Commands.EDIT_FIND_PREVIOUS,
"menu-edit-replace": Commands.EDIT_REPLACE,
-
// View
"menu-view-hide-sidebar": Commands.VIEW_HIDE_SIDEBAR,
@@ -69,10 +68,8 @@ define(function (require, exports, module) {
function createExecFunc(commandStr) {
return function () {
- // TODO TY: should flash menu here
+ // TODO TY: should flash menu here on Mac
//console.log(commandStr);
-
- EditorManager.focusEditor();
CommandManager.execute(commandStr);
};
}
@@ -106,11 +103,22 @@ define(function (require, exports, module) {
shortcut = keyCmd.replace(/-/g, "+");
}
- $("#" + menuID).append("<span class='menu-shortcut'>" + shortcut + "</span>");
+ var $menu = $("#" + menuID);
+ // Some commands have multiple key commands. Only add the first one.
+ if ($menu.find(".menu-shortcut").length === 0) {
+ $menu.append("<span class='menu-shortcut'>" + shortcut + "</span>");
@redmunds Collaborator

Seems like the code can exit the loop after the first shortcut string is added. Or at least, only build the "shortcut" string once you've determined that it's the first one.

@peterflynn Owner

I think that's the idea -- if there's more than one shortcut bound to the same menu item, we don't want to list all the shortcuts in the menu UI... just the first one we come across. There's no break or anything, so it won't actually exit the loop; it just won't add another shortcut label to the UI if one is already present.

@peterflynn Owner

Although, Ty, this does raise one issue: if there's more than one shortcut, the one that wins is completely arbitrary, not the first one listed in the code in brackets.js (because we're iterating a map here, not an ordered list).

Maybe because of this, for now we should print a warning to the console whenever a 2nd shortcut for the same menu is encountered? I think with the removal of the Tab bindings above, we no longer have any commands that would hit this caseat the moment...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ }
}
}
}
+ // Prevent clicks on the top-level menu bar from taking focus
+ // Note, bootstrap handles this already for the menu drop downs
+ $("#main-toolbar .dropdown").mousedown(function (e) {
+ e.preventDefault();
+ });
+
+
// Other debug menu items
// $("#menu-debug-wordwrap").click(function() {
View
9 src/editor/Editor.js
@@ -47,8 +47,6 @@ define(function (require, exports, module) {
TextRange = require("document/TextRange").TextRange,
ViewUtils = require("utils/ViewUtils");
-
-
/**
* @private
@@ -191,7 +189,7 @@ define(function (require, exports, module) {
function _handleSelectAll() {
var editor = EditorManager.getFocusedEditor();
if (editor) {
- editor._codeMirror.execCommand("selectAll");
+ editor._selectAllVisible();
}
}
@@ -294,6 +292,7 @@ define(function (require, exports, module) {
// Editor supplies some standard keyboard behavior extensions of its own
var codeMirrorKeyMap = {
"Tab" : _handleTabKey,
+
"Left" : function (instance) {
if (!_handleSoftTabNavigation(instance, -1, "moveH")) {
CodeMirror.commands.goCharLeft(instance);
@@ -971,10 +970,6 @@ define(function (require, exports, module) {
CommandManager.register(Commands.EDIT_FIND_PREVIOUS, _findPrevious);
CommandManager.register(Commands.EDIT_SELECT_ALL, _handleSelectAll);
- // TODO: code mirror handles
- // CommandManager.register(Commands.EDIT_INDENT, _handleTabKey);
- // CommandManager.register(Commands.EDIT_UNINDENT, _handleTabKey);
-
// Define public API
exports.Editor = Editor;
});
View
3  src/index.html
@@ -98,9 +98,6 @@
<li><a href="#" id="menu-edit-find-previous">Find Previous</a></li>
<li><hr class="divider"></li>
<li><a href="#" id="menu-edit-replace">Replace</a></li>
- <li><hr class="divider"></li>
- <li><a href="#" id="menu-edit-indent">Indent</a></li>
- <li><a href="#" id="menu-edit-unindent">Unindent</a></li>
</ul>
</li>
Something went wrong with that request. Please try again.