From 1cab51cefa3b54b09413d64b60d76a4e0bb6700b Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Fri, 8 Feb 2013 15:56:55 -0700 Subject: [PATCH] RootView can no longer be focused. Allowing root view to be focused was stealing focus away from the editor whenever a click event made it to the root view. This unnecessary switching of focus was interfering with the ability to drag tabs. But if RootView can't be focused, focus ends up being returned to the document body when there are no focusable elements. This would be fine, except for the fact that we frequently bind global events on root view, and so they aren't triggered when events are triggered on the body. We could just bind all global events on the body, but this would require us to always attach elements to the DOM during specs, which is a serious performance killer in specs. The workaround is in the keymap. When the keymap handles a key event that was triggered on the body, it triggers the corresponding semantic event on the root view anyway, so from the event perspective, it's as if the root view actually had focus. The only place this might fall down is if someone wants to capture raw key events. But that's the keymap's job anyway, and we maybe add a hook on the keymap if such a need ever arises. --- spec/app/keymap-spec.coffee | 18 ++++++++++++++++-- spec/app/root-view-spec.coffee | 4 ++-- spec/spec-helper.coffee | 5 ++--- src/app/keymap.coffee | 1 + src/app/root-view.coffee | 2 +- .../fuzzy-finder/spec/fuzzy-finder-spec.coffee | 4 ++-- src/packages/tree-view/keymaps/tree-view.cson | 2 +- 7 files changed, 25 insertions(+), 11 deletions(-) diff --git a/spec/app/keymap-spec.coffee b/spec/app/keymap-spec.coffee index 01ef31019..22c56c337 100644 --- a/spec/app/keymap-spec.coffee +++ b/spec/app/keymap-spec.coffee @@ -1,5 +1,6 @@ Keymap = require 'keymap' $ = require 'jquery' +RootView = require 'root-view' describe "Keymap", -> fragment = null @@ -23,8 +24,8 @@ describe "Keymap", -> keymap.bindKeys '.command-mode', 'x': 'deleteChar' keymap.bindKeys '.insert-mode', 'x': 'insertChar' - deleteCharHandler = jasmine.createSpy 'deleteCharHandler' - insertCharHandler = jasmine.createSpy 'insertCharHandler' + deleteCharHandler = jasmine.createSpy('deleteCharHandler') + insertCharHandler = jasmine.createSpy('insertCharHandler') fragment.on 'deleteChar', deleteCharHandler fragment.on 'insertChar', insertCharHandler @@ -149,6 +150,19 @@ describe "Keymap", -> keymap.handleKeyEvent(keydownEvent('y', target: target)) expect(bazHandler).toHaveBeenCalled() + describe "when the event's target is the document body", -> + it "triggers the mapped event on the rootView", -> + rootView = new RootView + keymap.bindKeys 'body', 'x': 'foo' + fooHandler = jasmine.createSpy("fooHandler") + rootView.on 'foo', fooHandler + + result = keymap.handleKeyEvent(keydownEvent('x', target: document.body)) + expect(result).toBe(false) + expect(fooHandler).toHaveBeenCalled() + expect(deleteCharHandler).not.toHaveBeenCalled() + expect(insertCharHandler).not.toHaveBeenCalled() + describe "when at least one binding partially matches the event's keystroke", -> [quitHandler, closeOtherWindowsHandler] = [] diff --git a/spec/app/root-view-spec.coffee b/spec/app/root-view-spec.coffee index b046bef30..1e2dc9786 100644 --- a/spec/app/root-view-spec.coffee +++ b/spec/app/root-view-spec.coffee @@ -201,11 +201,11 @@ describe "RootView", -> expect(rootView.find('#two')).not.toMatchSelector(':focus') describe "when there are no visible focusable elements", -> - it "retains focus itself", -> + it "surrenders focus to the body", -> rootView.remove() rootView = new RootView(require.resolve 'fixtures') rootView.attachToDom() - expect(rootView).toMatchSelector(':focus') + expect(document.activeElement).toBe $('body')[0] describe "panes", -> [pane1, newPaneContent] = [] diff --git a/spec/spec-helper.coffee b/spec/spec-helper.coffee index c46be84fe..d00160158 100644 --- a/spec/spec-helper.coffee +++ b/spec/spec-helper.coffee @@ -93,9 +93,8 @@ window.keyIdentifierForKey = (key) -> "U+00" + charCode.toString(16) window.keydownEvent = (key, properties={}) -> - event = $.Event "keydown", _.extend({originalEvent: { keyIdentifier: keyIdentifierForKey(key) }}, properties) - # event.keystroke = (new Keymap).keystrokeStringForEvent(event) - event + properties = $.extend({originalEvent: { keyIdentifier: keyIdentifierForKey(key) }}, properties) + $.Event("keydown", properties) window.mouseEvent = (type, properties) -> if properties.point diff --git a/src/app/keymap.coffee b/src/app/keymap.coffee index 97cf4303b..d7e2ae6e9 100644 --- a/src/app/keymap.coffee +++ b/src/app/keymap.coffee @@ -99,6 +99,7 @@ class Keymap b.specificity - a.specificity triggerCommandEvent: (keyEvent, commandName) -> + keyEvent.target = rootView[0] if keyEvent.target == document.body and window.rootView commandEvent = $.Event(commandName) commandEvent.keyEvent = keyEvent aborted = false diff --git a/src/app/root-view.coffee b/src/app/root-view.coffee index 4063ee863..2b86d95dc 100644 --- a/src/app/root-view.coffee +++ b/src/app/root-view.coffee @@ -18,7 +18,7 @@ class RootView extends View disabledPackages: [] @content: -> - @div id: 'root-view', tabindex: 0, => + @div id: 'root-view', => @div id: 'horizontal', outlet: 'horizontal', => @div id: 'vertical', outlet: 'vertical', => @div id: 'panes', outlet: 'panes' diff --git a/src/packages/fuzzy-finder/spec/fuzzy-finder-spec.coffee b/src/packages/fuzzy-finder/spec/fuzzy-finder-spec.coffee index 76f8bd476..9d90f26d3 100644 --- a/src/packages/fuzzy-finder/spec/fuzzy-finder-spec.coffee +++ b/src/packages/fuzzy-finder/spec/fuzzy-finder-spec.coffee @@ -205,7 +205,7 @@ describe 'FuzzyFinder', -> expect(finder.miniEditor.isFocused).toBeFalsy() describe "when no editors are open", -> - it "detaches the finder and focuses the previously focused element", -> + it "detaches the finder and surrenders focus to the body", -> rootView.attachToDom() rootView.getActiveEditor().destroyActiveEditSession() @@ -217,7 +217,7 @@ describe 'FuzzyFinder', -> finder.cancel() expect(finder.hasParent()).toBeFalsy() - expect($(document.activeElement).view()).toBe rootView + expect(document.activeElement).toBe $('body')[0] expect(finder.miniEditor.isFocused).toBeFalsy() describe "cached file paths", -> diff --git a/src/packages/tree-view/keymaps/tree-view.cson b/src/packages/tree-view/keymaps/tree-view.cson index 1e198d26d..0bdf1b863 100644 --- a/src/packages/tree-view/keymaps/tree-view.cson +++ b/src/packages/tree-view/keymaps/tree-view.cson @@ -1,4 +1,4 @@ -'#root-view': +'body': 'meta-\\': 'tree-view:toggle' 'meta-|': 'tree-view:reveal-active-file'