From 2a7344091d74de66f77a366ee963eb82bba00cb4 Mon Sep 17 00:00:00 2001 From: Isaac Salier-Hellendag Date: Wed, 23 Mar 2016 19:18:02 -0700 Subject: [PATCH 1/5] Avoid setting hidden input value on textInput Atom currently sets the `value` of the input on every `textInput` event, in an effort to appropriately handle changes made via the OSX diacritic menu (for accents, umlauts, etc). The drawback of this is approach is that updating the value of the input will trigger layout and a subsequent layer tree update. To resolve this, here is my proposal: - Track a flag for `keypress` events. When the diacritic menu is used, there are two `textInput` events, with no `keypress` in between. Therefore, when no `keypress` has occurred just prior to a `textInput`, the editor model can select the previous character to be replaced by the new accented character. - Track a flag for `compositionstart` events. When a user is in IME mode, the diacritic menu cannot be used, so the editor can skip the backward selection. Test Plan: Tested in a plaintext file. - Type Latin characters, verify proper character insertion. - Press and hold a. Diacritic menu appears. Select an option using the keyboard or mouse. Verify that the `a` is replaced by an accented `a`, with no extra characters. - Type test strings in Katakana, 2-Set Korean, Telex (Vietnamese), Simplified Pinyin. Verify that characters are inserted correctly while composing, and after committing strings. --- src/text-editor-component.coffee | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/src/text-editor-component.coffee b/src/text-editor-component.coffee index 9b091100d..50851279b 100644 --- a/src/text-editor-component.coffee +++ b/src/text-editor-component.coffee @@ -244,6 +244,7 @@ class TextEditorComponent listenForDOMEvents: -> @domNode.addEventListener 'mousewheel', @onMouseWheel @domNode.addEventListener 'textInput', @onTextInput + @domNode.addEventListener 'keypress', @onKeyPress @scrollViewNode.addEventListener 'mousedown', @onMouseDown @scrollViewNode.addEventListener 'scroll', @onScrollViewScroll @@ -266,6 +267,7 @@ class TextEditorComponent checkpoint = null @domNode.addEventListener 'compositionstart', => + @imeMode = true checkpoint = @editor.createCheckpoint() @domNode.addEventListener 'compositionupdate', (event) => @editor.insertText(event.data, select: true) @@ -319,26 +321,27 @@ class TextEditorComponent if @mounted @presenter.setFocused(false) + onKeyPress: (event) => + @detectedKeyPress = true + onTextInput: (event) => event.stopPropagation() - - # If we prevent the insertion of a space character, then the browser - # interprets the spacebar keypress as a page-down command. - event.preventDefault() unless event.data is ' ' + event.preventDefault() return unless @isInputEnabled() - inputNode = event.target + # Workaround of the accented character suggestion feature in OS X. + # This will only occur when the user is not composing in IME mode. + # When the user selects a modified character from the OSX menu, `textInput` + # will occur twice, once for the initial character, and once for the + # modified character. However, only a single keypress will have fired. If + # this is the case, select backward to replace the original character. + if not @imeMode and not @detectedKeyPress + @editor.selectLeft() - # Work around of the accented character suggestion feature in OS X. - # Text input fires before a character is inserted, and if the browser is - # replacing the previous un-accented character with an accented variant, it - # will select backward over it. - selectedLength = inputNode.selectionEnd - inputNode.selectionStart - @editor.selectLeft() if selectedLength is 1 - - insertedRange = @editor.insertText(event.data, groupUndo: true) - inputNode.value = event.data if insertedRange + @editor.insertText(event.data, groupUndo: true) + @detectedKeyPress = false + @imeMode = false onVerticalScroll: (scrollTop) => return if @updateRequested or scrollTop is @presenter.getScrollTop() From a99ee14ac0be502d6e18b56b8b9b84a5c1c472e9 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Mon, 4 Apr 2016 17:47:36 -0600 Subject: [PATCH 2/5] Make accented character menu detection work with left/right arrow keys --- spec/text-editor-component-spec.js | 3 ++- src/text-editor-component.coffee | 29 +++++++++++++++++++++-------- 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/spec/text-editor-component-spec.js b/spec/text-editor-component-spec.js index 1d1e4eb9f..30bc9251c 100644 --- a/spec/text-editor-component-spec.js +++ b/spec/text-editor-component-spec.js @@ -3767,11 +3767,12 @@ describe('TextEditorComponent', function () { expect(editor.lineTextForBufferRow(0)).toBe('xyvar quicksort = function () {') }) - it('replaces the last character if the length of the input\'s value does not increase, as occurs with the accented character menu', async function () { + it('replaces the last character if the textInput event is followed by one or more additional keydown events, which occurs when the accented character menu is shown', async function () { componentNode.dispatchEvent(buildTextInputEvent({ data: 'u', target: inputNode })) + componentNode.dispatchEvent(new KeyboardEvent('keydown')) await nextViewUpdatePromise() expect(editor.lineTextForBufferRow(0)).toBe('uvar quicksort = function () {') diff --git a/src/text-editor-component.coffee b/src/text-editor-component.coffee index 50851279b..8e91557c8 100644 --- a/src/text-editor-component.coffee +++ b/src/text-editor-component.coffee @@ -244,13 +244,28 @@ class TextEditorComponent listenForDOMEvents: -> @domNode.addEventListener 'mousewheel', @onMouseWheel @domNode.addEventListener 'textInput', @onTextInput - @domNode.addEventListener 'keypress', @onKeyPress @scrollViewNode.addEventListener 'mousedown', @onMouseDown @scrollViewNode.addEventListener 'scroll', @onScrollViewScroll + @detectAccentedCharacterMenu() @listenForIMEEvents() @trackSelectionClipboard() if process.platform is 'linux' + detectAccentedCharacterMenu: -> + # We need to get clever to detect when the accented character menu is + # opened on OS X. Usually, every keydown event that could cause input is + # paired with a corresponding keypress. However, when pressing and holding + # long enough to open the accented character menu causes additional keydown + # events to fire that aren't followed by their own keypress and textInput + # events. We exploit this by assuming the accented character menu is open + # until a subsequent keypress event proves otherwise. + @domNode.addEventListener 'keydown', (event) => + unless event.keyCode is 229 # Skip keydown events associated with IME compositionupdate events + @openedAccentedCharacterMenu = true + + @domNode.addEventListener 'keypress', => + @openedAccentedCharacterMenu = false + listenForIMEEvents: -> # The IME composition events work like this: # @@ -267,7 +282,9 @@ class TextEditorComponent checkpoint = null @domNode.addEventListener 'compositionstart', => - @imeMode = true + if @openedAccentedCharacterMenu + @editor.selectLeft() + @openedAccentedCharacterMenu = false checkpoint = @editor.createCheckpoint() @domNode.addEventListener 'compositionupdate', (event) => @editor.insertText(event.data, select: true) @@ -321,9 +338,6 @@ class TextEditorComponent if @mounted @presenter.setFocused(false) - onKeyPress: (event) => - @detectedKeyPress = true - onTextInput: (event) => event.stopPropagation() event.preventDefault() @@ -336,12 +350,11 @@ class TextEditorComponent # will occur twice, once for the initial character, and once for the # modified character. However, only a single keypress will have fired. If # this is the case, select backward to replace the original character. - if not @imeMode and not @detectedKeyPress + if @openedAccentedCharacterMenu @editor.selectLeft() + @openedAccentedCharacterMenu = false @editor.insertText(event.data, groupUndo: true) - @detectedKeyPress = false - @imeMode = false onVerticalScroll: (scrollTop) => return if @updateRequested or scrollTop is @presenter.getScrollTop() From f638bcbb6d97a27a90d9d8b780659eb61d87de55 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Mon, 4 Apr 2016 18:56:08 -0600 Subject: [PATCH 3/5] =?UTF-8?q?Don=E2=80=99t=20assume=20the=20accented=20c?= =?UTF-8?q?haracter=20menu=20on=20every=20IME=20event?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/text-editor-component.coffee | 35 +++++++++++++++++++++++++++----- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/src/text-editor-component.coffee b/src/text-editor-component.coffee index 8e91557c8..b76a23ddc 100644 --- a/src/text-editor-component.coffee +++ b/src/text-editor-component.coffee @@ -254,18 +254,43 @@ class TextEditorComponent detectAccentedCharacterMenu: -> # We need to get clever to detect when the accented character menu is # opened on OS X. Usually, every keydown event that could cause input is - # paired with a corresponding keypress. However, when pressing and holding + # followed by a corresponding keypress. However, pressing and holding # long enough to open the accented character menu causes additional keydown # events to fire that aren't followed by their own keypress and textInput - # events. We exploit this by assuming the accented character menu is open - # until a subsequent keypress event proves otherwise. + # events. + # + # Therefore, we assume the accented character menu has been deployed if, + # before observing any keyup event, we observe events in the following + # sequence: + # + # keydown(keyCode: X), keypress, keydown(codeCode: X) + # + # The keyCode X must be the same in the keydown events that bracket the + # keypress, meaning we're *holding* the _same_ key we intially pressed. + # Got that? + lastKeydown = null + lastKeydownBeforeKeypress = null + @domNode.addEventListener 'keydown', (event) => - unless event.keyCode is 229 # Skip keydown events associated with IME compositionupdate events - @openedAccentedCharacterMenu = true + if lastKeydownBeforeKeypress + if lastKeydownBeforeKeypress.keyCode is event.keyCode + @openedAccentedCharacterMenu = true + lastKeydownBeforeKeypress = null + else + lastKeydown = event @domNode.addEventListener 'keypress', => + lastKeydownBeforeKeypress = lastKeydown + lastKeydown = null + + # This cancels the accented character behavior if we type a key normally + # with the menu open. @openedAccentedCharacterMenu = false + @domNode.addEventListener 'keyup', => + lastKeydownBeforeKeypress = null + lastKeydown = null + listenForIMEEvents: -> # The IME composition events work like this: # From 9833e54ec349d9503c1facac44ad8b7e6bc09bc0 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Mon, 4 Apr 2016 19:22:44 -0600 Subject: [PATCH 4/5] Fix typo --- src/text-editor-component.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/text-editor-component.coffee b/src/text-editor-component.coffee index b76a23ddc..12f481be5 100644 --- a/src/text-editor-component.coffee +++ b/src/text-editor-component.coffee @@ -263,7 +263,7 @@ class TextEditorComponent # before observing any keyup event, we observe events in the following # sequence: # - # keydown(keyCode: X), keypress, keydown(codeCode: X) + # keydown(keyCode: X), keypress, keydown(keyCode: X) # # The keyCode X must be the same in the keydown events that bracket the # keypress, meaning we're *holding* the _same_ key we intially pressed. From 402a335eefa8c15b95c917496d29c740a4a1535f Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Mon, 4 Apr 2016 19:50:39 -0600 Subject: [PATCH 5/5] Fix accented character menu spec --- spec/text-editor-component-spec.js | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/spec/text-editor-component-spec.js b/spec/text-editor-component-spec.js index 30bc9251c..b031cba33 100644 --- a/spec/text-editor-component-spec.js +++ b/spec/text-editor-component-spec.js @@ -3745,6 +3745,21 @@ describe('TextEditorComponent', function () { return event } + function buildKeydownEvent ({keyCode, target}) { + let event = new KeyboardEvent('keydown') + Object.defineProperty(event, 'keyCode', { + get: function () { + return keyCode + } + }) + Object.defineProperty(event, 'target', { + get: function () { + return target + } + }) + return event + } + let inputNode beforeEach(function () { @@ -3767,12 +3782,12 @@ describe('TextEditorComponent', function () { expect(editor.lineTextForBufferRow(0)).toBe('xyvar quicksort = function () {') }) - it('replaces the last character if the textInput event is followed by one or more additional keydown events, which occurs when the accented character menu is shown', async function () { - componentNode.dispatchEvent(buildTextInputEvent({ - data: 'u', - target: inputNode - })) - componentNode.dispatchEvent(new KeyboardEvent('keydown')) + it('replaces the last character if a keypress event is bracketed by keydown events with matching keyCodes, which occurs when the accented character menu is shown', async function () { + componentNode.dispatchEvent(buildKeydownEvent({keyCode: 85, target: inputNode})) + componentNode.dispatchEvent(buildTextInputEvent({data: 'u', target: inputNode})) + componentNode.dispatchEvent(new KeyboardEvent('keypress')) + componentNode.dispatchEvent(buildKeydownEvent({keyCode: 85, target: inputNode})) + componentNode.dispatchEvent(new KeyboardEvent('keyup')) await nextViewUpdatePromise() expect(editor.lineTextForBufferRow(0)).toBe('uvar quicksort = function () {')