From c7bfbc181c727ac026c75e5c2477014e7f97dbab Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Mon, 7 Aug 2017 09:50:10 -0400 Subject: [PATCH 1/4] :bug: Fix atom/tabs#461 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On Linux, when the user performs a middle-button mouse click, Chromium fires both a mouse-down event *and* a paste event. This commit teaches the TextEditorComponent to ignore the paste event. When the user performs a middle-mouse click on a tab, we get close the tab and attempt to prevent Chromium's default processing for the event. [1] This prevents Chromium's default processing for the *mouse down* event, but then Chromium also fires a *paste* event, and that event pastes the clipboard's current content into the newly-focused text editor. :scream_cat: Since Atom already has its own logic for handling pasting, we shouldn't (🤞) need to handle browser paste events. By ignoring the browser paste events on Linux, we fix atom/tabs#461. [1] https://github.com/atom/tabs/blob/ce1d92e0abb669155caa178bb71166b9f16f329a/lib/tab-bar-view.coffee#L416-L418 --- spec/text-editor-component-spec.js | 32 ++++++++++++++++++++++++++---- src/text-editor-component.js | 14 +++++++++++-- 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/spec/text-editor-component-spec.js b/spec/text-editor-component-spec.js index ad631b855..34d09865e 100644 --- a/spec/text-editor-component-spec.js +++ b/spec/text-editor-component-spec.js @@ -2687,18 +2687,42 @@ describe('TextEditorComponent', () => { expect(component.getScrollLeft()).toBe(maxScrollLeft) }) - it('positions the cursor on clicking the middle mouse button on Linux', async () => { - // The browser synthesizes the paste as a textInput event on mouseup - // so it is not possible to test it here. + it('pastes the previously selected text when clicking the middle mouse button on Linux', async () => { + spyOn(electron.ipcRenderer, 'send').andCallFake(function (eventName, selectedText) { + if (eventName === 'write-text-to-selection-clipboard') { + clipboard.writeText(selectedText, 'selection') + } + }) + const {component, editor} = buildComponent({platform: 'linux'}) + // Middle mouse pasting. editor.setSelectedBufferRange([[1, 6], [1, 10]]) + await conditionPromise(() => TextEditor.clipboard.read() === 'sort') component.didMouseDownOnContent({ button: 1, clientX: clientLeftForCharacter(component, 10, 0), clientY: clientTopForLine(component, 10) }) - expect(editor.getSelectedBufferRange()).toEqual([[10, 0], [10, 0]]) + expect(TextEditor.clipboard.read()).toBe('sort') + expect(editor.lineTextForBufferRow(10)).toBe('sort') + editor.undo() + + // Ensure left clicks don't interfere. + editor.setSelectedBufferRange([[1, 2], [1, 5]]) + await conditionPromise(() => TextEditor.clipboard.read() === 'var') + component.didMouseDownOnContent({ + button: 0, + detail: 1, + clientX: clientLeftForCharacter(component, 10, 0), + clientY: clientTopForLine(component, 10) + }) + component.didMouseDownOnContent({ + button: 1, + clientX: clientLeftForCharacter(component, 10, 0), + clientY: clientTopForLine(component, 10) + }) + expect(editor.lineTextForBufferRow(10)).toBe('var') }) }) diff --git a/src/text-editor-component.js b/src/text-editor-component.js index e403a1e8f..a910e1828 100644 --- a/src/text-editor-component.js +++ b/src/text-editor-component.js @@ -5,6 +5,7 @@ const {Point, Range} = require('text-buffer') const LineTopIndex = require('line-top-index') const TextEditor = require('./text-editor') const {isPairedCharacter} = require('./text-utils') +const clipboard = require('./safe-clipboard') const electron = require('electron') const $ = etch.dom @@ -641,6 +642,7 @@ class TextEditorComponent { didBlurHiddenInput: this.didBlurHiddenInput, didFocusHiddenInput: this.didFocusHiddenInput, didTextInput: this.didTextInput, + didPaste: this.didPaste, didKeydown: this.didKeydown, didKeyup: this.didKeyup, didKeypress: this.didKeypress, @@ -1549,6 +1551,11 @@ class TextEditorComponent { } } + didPaste (event) { + // TODO Explain the motivation for this logic + if ((this.props.platform || process.platform) === 'linux') event.preventDefault() + } + didTextInput (event) { if (!this.isInputEnabled()) return @@ -1654,8 +1661,10 @@ class TextEditorComponent { // textInput event with the contents of the selection clipboard will be // dispatched by the browser automatically on mouseup. if (platform === 'linux' && button === 1) { + const selection = clipboard.readText('selection') const screenPosition = this.screenPositionForMouseEvent(event) model.setCursorScreenPosition(screenPosition, {autoscroll: false}) + model.insertText(selection) return } @@ -3356,8 +3365,8 @@ class CursorsAndInputComponent { renderHiddenInput () { const { lineHeight, hiddenInputPosition, didBlurHiddenInput, didFocusHiddenInput, - didTextInput, didKeydown, didKeyup, didKeypress, didCompositionStart, - didCompositionUpdate, didCompositionEnd + didPaste, didTextInput, didKeydown, didKeyup, didKeypress, + didCompositionStart, didCompositionUpdate, didCompositionEnd } = this.props let top, left @@ -3376,6 +3385,7 @@ class CursorsAndInputComponent { on: { blur: didBlurHiddenInput, focus: didFocusHiddenInput, + paste: didPaste, textInput: didTextInput, keydown: didKeydown, keyup: didKeyup, From 47420761f524429b649b67e12ab6708308554393 Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Mon, 7 Aug 2017 11:32:57 -0400 Subject: [PATCH 2/4] =?UTF-8?q?=E2=9C=85=20Add=20basic=20test=20for=20Text?= =?UTF-8?q?EditorComponent::didPaste(event)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- spec/text-editor-component-spec.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/spec/text-editor-component-spec.js b/spec/text-editor-component-spec.js index 34d09865e..3b2f2072a 100644 --- a/spec/text-editor-component-spec.js +++ b/spec/text-editor-component-spec.js @@ -3016,6 +3016,17 @@ describe('TextEditorComponent', () => { }) }) + describe('paste event', () => { + it("prevents the browser's default processing for the event on Linux", () => { + const {component} = buildComponent({platform: 'linux'}) + const event = { preventDefault: () => {} } + spyOn(event, 'preventDefault') + + component.didPaste(event) + expect(event.preventDefault).toHaveBeenCalled() + }) + }) + describe('keyboard input', () => { it('handles inserted accented characters via the press-and-hold menu on macOS correctly', () => { const {editor, component, element} = buildComponent({text: ''}) From b9ace6a5b66ae7f2311f56ec6468e24035d9789b Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Mon, 7 Aug 2017 12:31:04 -0400 Subject: [PATCH 3/4] :art: --- src/text-editor-component.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/text-editor-component.js b/src/text-editor-component.js index a910e1828..3417fc929 100644 --- a/src/text-editor-component.js +++ b/src/text-editor-component.js @@ -70,6 +70,7 @@ class TextEditorComponent { this.updateSync = this.updateSync.bind(this) this.didBlurHiddenInput = this.didBlurHiddenInput.bind(this) this.didFocusHiddenInput = this.didFocusHiddenInput.bind(this) + this.didPaste = this.didPaste.bind(this) this.didTextInput = this.didTextInput.bind(this) this.didKeydown = this.didKeydown.bind(this) this.didKeyup = this.didKeyup.bind(this) @@ -1553,7 +1554,7 @@ class TextEditorComponent { didPaste (event) { // TODO Explain the motivation for this logic - if ((this.props.platform || process.platform) === 'linux') event.preventDefault() + if (this.getPlatform() === 'linux') event.preventDefault() } didTextInput (event) { From 3dc2e6199098c7a2defc53da7a3138c43862f154 Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Mon, 7 Aug 2017 12:43:13 -0400 Subject: [PATCH 4/4] Add comment explaining motivation for didPaste implemenation --- src/text-editor-component.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/text-editor-component.js b/src/text-editor-component.js index 3417fc929..881aaad81 100644 --- a/src/text-editor-component.js +++ b/src/text-editor-component.js @@ -1553,7 +1553,12 @@ class TextEditorComponent { } didPaste (event) { - // TODO Explain the motivation for this logic + // On Linux, Chromium translates a middle-button mouse click into a + // mousedown event *and* a paste event. Since Atom supports the middle mouse + // click as a way of closing a tab, we only want the mousedown event, not + // the paste event. And since we don't use the `paste` event for any + // behavior in Atom, we can no-op the event to eliminate this issue. + // See https://github.com/atom/atom/pull/15183#issue-248432413. if (this.getPlatform() === 'linux') event.preventDefault() }