Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Don’t handle or replay keystrokes during IME composition #178

Merged
merged 1 commit into from
Nov 15, 2016
Merged

Don’t handle or replay keystrokes during IME composition #178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 41 additions & 1 deletion spec/keymap-manager-spec.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ describe "KeymapManager", ->
event = buildKeydownEvent(key: 'q')
keymapManager.handleKeyboardEvent(event)
assert(not event.defaultPrevented)

describe "when the keystroke matches one binding on any particular element", ->
[events, elementA, elementB] = []

Expand Down Expand Up @@ -261,17 +262,23 @@ describe "KeymapManager", ->
describe "when subsequent keystrokes yield no matches", ->
it "disables the bindings with the longest keystroke sequences and replays the queued keystrokes", ->
keymapManager.handleKeyboardEvent(vEvent = buildKeydownEvent(key: 'v', target: editor))
keymapManager.handleKeyboardEvent(buildKeyupEvent(key: 'v', target: editor))
keymapManager.handleKeyboardEvent(iEvent = buildKeydownEvent(key: 'i', target: editor))
keymapManager.handleKeyboardEvent(buildKeyupEvent(key: 'i', target: editor))
keymapManager.handleKeyboardEvent(wEvent = buildKeydownEvent(key: 'w', target: editor))
keymapManager.handleKeyboardEvent(buildKeyupEvent(key: 'w', target: editor))
assert.equal(vEvent.defaultPrevented, true)
assert.equal(iEvent.defaultPrevented, true)
assert.equal(wEvent.defaultPrevented, true)
assert.deepEqual(events, ['enter-visual-mode', 'select-inside-word'])

events = []
keymapManager.handleKeyboardEvent(vEvent = buildKeydownEvent(key: 'v', target: editor))
keymapManager.handleKeyboardEvent(buildKeyupEvent(key: 'v', target: editor))
keymapManager.handleKeyboardEvent(iEvent = buildKeydownEvent(key: 'i', target: editor))
keymapManager.handleKeyboardEvent(buildKeyupEvent(key: 'i', target: editor))
keymapManager.handleKeyboardEvent(kEvent = buildKeydownEvent(key: 'k', target: editor))
keymapManager.handleKeyboardEvent(buildKeyupEvent(key: 'k', target: editor))
assert.equal(vEvent.defaultPrevented, true)
assert.equal(iEvent.defaultPrevented, true)
assert.equal(kEvent.defaultPrevented, false)
Expand All @@ -281,16 +288,22 @@ describe "KeymapManager", ->

it "dispatches a text-input event for any replayed keyboard events that would have inserted characters", ->
keymapManager.handleKeyboardEvent(buildKeydownEvent(key: 'd', target: editor))
keymapManager.handleKeyboardEvent(buildKeyupEvent(key: 'd', target: editor))
keymapManager.handleKeyboardEvent(buildKeydownEvent(key: 'o', target: editor))
keymapManager.handleKeyboardEvent(buildKeyupEvent(key: 'o', target: editor))
keymapManager.handleKeyboardEvent(lastEvent = buildKeydownEvent(key: 'q', target: editor))
keymapManager.handleKeyboardEvent(buildKeyupEvent(key: 'q', target: editor))

assert.deepEqual(events, ['input:d', 'input:o'])
assert(not lastEvent.defaultPrevented)

events = []
keymapManager.handleKeyboardEvent(buildKeydownEvent(key: 'Shift', target: editor))
keymapManager.handleKeyboardEvent(buildKeydownEvent(key: 'S', target: editor, shiftKey: true))
keymapManager.handleKeyboardEvent(buildKeyupEvent(key: 'S', target: editor, shiftKey: true))
keymapManager.handleKeyboardEvent(buildKeyupEvent(key: 'Shift', target: editor))
keymapManager.handleKeyboardEvent(buildKeydownEvent(key: 'y', target: editor))
keymapManager.handleKeyboardEvent(buildKeyupEvent(key: 'y', target: editor))
assert.deepEqual(events, ['input:S'])

describe "when the currently queued keystrokes exactly match at least one binding", ->
Expand Down Expand Up @@ -354,15 +367,19 @@ describe "KeymapManager", ->
assert.deepEqual(events, ['control-dog'])

describe "when focused element changed in the middle of replaying keystroke", ->
it "replay keystroke against newly focused element", ->
it "replays keystrokes against newly focused elements", ->
keymapManager.handleKeyboardEvent(buildKeydownEvent(key: 'm', target: editor))
keymapManager.handleKeyboardEvent(buildKeyupEvent(key: 'm', target: editor))
keymapManager.handleKeyboardEvent(buildKeydownEvent(key: 'j', target: editor))
keymapManager.handleKeyboardEvent(buildKeyupEvent(key: 'j', target: editor))
assert.deepEqual(events, ['editor-m-j'])

events = []
assert.deepEqual(document.activeElement, inputElement)
keymapManager.handleKeyboardEvent(buildKeydownEvent(key: 'm', target: editor))
keymapManager.handleKeyboardEvent(buildKeyupEvent(key: 'm', target: editor))
keymapManager.handleKeyboardEvent(buildKeydownEvent(key: 'a', target: editor))
keymapManager.handleKeyboardEvent(buildKeyupEvent(key: 'a', target: editor))
assert.deepEqual(events, ['focus-input2', 'editor2-a'])
assert.deepEqual(document.activeElement, input2Element)

Expand All @@ -380,10 +397,15 @@ describe "KeymapManager", ->
describe "when a subsequent keystroke begins a new match of an already pending binding", ->
it "recognizes the match", ->
keymapManager.handleKeyboardEvent(buildKeydownEvent(key: 'd', target: editor))
keymapManager.handleKeyboardEvent(buildKeyupEvent(key: 'd', target: editor))
keymapManager.handleKeyboardEvent(buildKeydownEvent(key: 'o', target: editor))
keymapManager.handleKeyboardEvent(buildKeyupEvent(key: 'o', target: editor))
keymapManager.handleKeyboardEvent(buildKeydownEvent(key: 'd', target: editor))
keymapManager.handleKeyboardEvent(buildKeyupEvent(key: 'd', target: editor))
keymapManager.handleKeyboardEvent(buildKeydownEvent(key: 'o', target: editor))
keymapManager.handleKeyboardEvent(buildKeyupEvent(key: 'o', target: editor))
keymapManager.handleKeyboardEvent(buildKeydownEvent(key: 'g', target: editor))
keymapManager.handleKeyboardEvent(buildKeyupEvent(key: 'g', target: editor))

assert.deepEqual(events, ['input:d', 'input:o', 'dog'])

Expand Down Expand Up @@ -442,6 +464,8 @@ describe "KeymapManager", ->
keymapManager.handleKeyboardEvent(buildKeyupEvent(key: 'y', ctrlKey: true, target: elementA))
assert.deepEqual(events, ['y-keydown'])
keymapManager.handleKeyboardEvent(buildKeydownEvent(key: 'y', ctrlKey: true, target: elementA))
assert.deepEqual(events, ['y-keydown'])
keymapManager.handleKeyboardEvent(buildKeyupEvent(key: 'y', ctrlKey: true, target: elementA))
assert.deepEqual(events, ['y-keydown', 'y-keydown'])
getFakeClock().tick(keymapManager.getPartialMatchTimeout())
assert.deepEqual(events, ['y-keydown', 'y-keydown'])
Expand Down Expand Up @@ -566,6 +590,22 @@ describe "KeymapManager", ->

assert.deepEqual(events, ['c', 'b1'])

it "does not attempt to handle keyboard events if composition is in progress", ->
eventCount = 0
elementA = document.createElement('div')
elementA.addEventListener 'x-command', -> eventCount++
keymapManager.add 'test', {'div': {'x': 'x-command'}}

keymapManager.handleKeyboardEvent(buildKeydownEvent({key: 'x', target: elementA}))
keymapManager.handleCompositionStart()
keymapManager.handleKeyboardEvent(buildKeyupEvent({key: 'x', target: elementA}))
keymapManager.handleKeyboardEvent(buildKeydownEvent({key: 'x', target: elementA}))
keymapManager.handleKeyboardEvent(buildKeyupEvent({key: 'x', target: elementA}))
keymapManager.handleCompositionEnd()
keymapManager.handleKeyboardEvent(buildKeydownEvent({key: 'x', target: elementA}))

assert.equal(eventCount, 2)

describe "::add(source, bindings)", ->
it "normalizes keystrokes containing capitalized alphabetic characters", ->
keymapManager.add 'test', '*': 'ctrl-shift-l': 'a'
Expand Down
32 changes: 23 additions & 9 deletions src/keymap-manager.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ class KeymapManager
@[key] = value for key, value of options
@watchSubscriptions = {}
@customKeystrokeResolvers = []
@compositionInProgress = false
@clear()

# Public: Clear all registered key bindings and enqueued keystrokes. For use
Expand Down Expand Up @@ -491,6 +492,12 @@ class KeymapManager
# keystroke is the atom keybind syntax, e.g. 'ctrl-a'
keystroke = @keystrokeForKeyboardEvent(event)

# If an input method editor (IME) is visible, allow the operating system to
# fully handle all keyboard events to prevent interference with menu
# interactions.
if @compositionInProgress
return

# We dont care about bare modifier keys in the bindings. e.g. `ctrl y` isnt going to work.
if event.type is 'keydown' and @queuedKeystrokes.length > 0 and isBareModifier(keystroke)
event.preventDefault()
Expand Down Expand Up @@ -622,10 +629,16 @@ class KeymapManager
# event. This means the current event has removed any hope that the queued
# key events will ever match any binding. So we will clear the state and
# start over after replaying the events in `terminatePendingState`.
@terminatePendingState()
@terminatePendingState() if event.type is 'keyup'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reasoning behind this change? I did some testing with my recent changes and it seems to work fine but I couldn't figure out why you added this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because compositionstart happens after keydown, we need to wait until keyup to know whether or not replay should be suppressed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, duh. Connecting that now with what you said in the PR description. Cool.

else
@clearQueuedKeystrokes()

handleCompositionStart: ->
@compositionInProgress = true

handleCompositionEnd: ->
@compositionInProgress = false

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where do these actually get called? The only call sites I see are in specs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They'll have to get called in Atom with some new event handlers.

# Public: Translate a keydown event to a keystroke string.
#
# * `event` A `KeyboardEvent` of type 'keydown'
Expand Down Expand Up @@ -762,16 +775,17 @@ class KeymapManager
@cancelPendingState()
@clearQueuedKeystrokes()

keyEventOptions =
replay: true
disabledBindings: bindingsToDisable
unless @compositionInProgress
keyEventOptions =
replay: true
disabledBindings: bindingsToDisable

for event in eventsToReplay
keyEventOptions.disabledBindings = bindingsToDisable
@handleKeyboardEvent(event, keyEventOptions)
for event in eventsToReplay
keyEventOptions.disabledBindings = bindingsToDisable
@handleKeyboardEvent(event, keyEventOptions)

# We can safely re-enable the bindings when we no longer have any partial matches
bindingsToDisable = null if bindingsToDisable? and not @pendingPartialMatches?
# We can safely re-enable the bindings when we no longer have any partial matches
bindingsToDisable = null if bindingsToDisable? and not @pendingPartialMatches?

if fromTimeout and @pendingPartialMatches?
@terminatePendingState(true)
Expand Down