From a1b61e77ff81ce2e72aa3f3dc925b749d760dee6 Mon Sep 17 00:00:00 2001 From: Ben Klein Date: Wed, 22 Jan 2020 21:21:51 -0500 Subject: [PATCH 1/3] If Text Editor Component uses the scroll event, consume it / prevent bubbling In a normal browser the inner scrollable would scroll to the edge before then scrolling the parent. This change makes it consistent with expected HTML scroll behaviour. Without it, the scroll event continues outside the editor to any other element containing it, even though the scroll event already was acted upon by the child (text editor component). If a package wanted to also use the mouse wheel event when integrating into a text editor (or if the text editor itself is inside something scrollable) this change makes more sense as it prevents multiple responses to a single user input. There doesn't appear to be any side effects to this change. (Tested on linux: Pop_OS 19.10, 1.42.0) --- src/text-editor-component.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/text-editor-component.js b/src/text-editor-component.js index 04537c03843..61379cbfc3f 100644 --- a/src/text-editor-component.js +++ b/src/text-editor-component.js @@ -1736,7 +1736,10 @@ module.exports = class TextEditorComponent { const scrollTopChanged = wheelDeltaY !== 0 && this.setScrollTop(this.getScrollTop() - wheelDeltaY); - if (scrollLeftChanged || scrollTopChanged) this.updateSync(); + if (scrollLeftChanged || scrollTopChanged) { + event.preventDefault(); + this.updateSync(); + } } didResize() { From f9f6f32f8985f8039f1e190af888c90d780c0a52 Mon Sep 17 00:00:00 2001 From: Ben Klein Date: Wed, 22 Jan 2020 22:52:01 -0500 Subject: [PATCH 2/3] tests: Text Editor component Event.preventDefault --- spec/text-editor-component-spec.js | 62 +++++++++++++++++++++++------- 1 file changed, 49 insertions(+), 13 deletions(-) diff --git a/spec/text-editor-component-spec.js b/spec/text-editor-component-spec.js index 611f5b7d08f..44e05cce7f7 100644 --- a/spec/text-editor-component-spec.js +++ b/spec/text-editor-component-spec.js @@ -1680,11 +1680,17 @@ describe('TextEditorComponent', () => { width: 50, scrollSensitivity }); + // stub in place for Event.preventDefault() + const eventPreventDefaultStub = function () {} { const expectedScrollTop = 20 * (scrollSensitivity / 100); const expectedScrollLeft = component.getScrollLeft(); - component.didMouseWheel({ wheelDeltaX: -5, wheelDeltaY: -20 }); + component.didMouseWheel({ + wheelDeltaX: -5, + wheelDeltaY: -20, + preventDefault: eventPreventDefaultStub + }); expect(component.getScrollTop()).toBe(expectedScrollTop); expect(component.getScrollLeft()).toBe(expectedScrollLeft); expect(component.refs.content.style.transform).toBe( @@ -1696,7 +1702,11 @@ describe('TextEditorComponent', () => { const expectedScrollTop = component.getScrollTop() - 10 * (scrollSensitivity / 100); const expectedScrollLeft = component.getScrollLeft(); - component.didMouseWheel({ wheelDeltaX: -5, wheelDeltaY: 10 }); + component.didMouseWheel({ + wheelDeltaX: -5, + wheelDeltaY: 10, + preventDefault: eventPreventDefaultStub + }); expect(component.getScrollTop()).toBe(expectedScrollTop); expect(component.getScrollLeft()).toBe(expectedScrollLeft); expect(component.refs.content.style.transform).toBe( @@ -1707,7 +1717,11 @@ describe('TextEditorComponent', () => { { const expectedScrollTop = component.getScrollTop(); const expectedScrollLeft = 20 * (scrollSensitivity / 100); - component.didMouseWheel({ wheelDeltaX: -20, wheelDeltaY: 10 }); + component.didMouseWheel({ + wheelDeltaX: -20, + wheelDeltaY: 10, + preventDefault: eventPreventDefaultStub + }); expect(component.getScrollTop()).toBe(expectedScrollTop); expect(component.getScrollLeft()).toBe(expectedScrollLeft); expect(component.refs.content.style.transform).toBe( @@ -1719,7 +1733,11 @@ describe('TextEditorComponent', () => { const expectedScrollTop = component.getScrollTop(); const expectedScrollLeft = component.getScrollLeft() - 10 * (scrollSensitivity / 100); - component.didMouseWheel({ wheelDeltaX: 10, wheelDeltaY: -8 }); + component.didMouseWheel({ + wheelDeltaX: 10, + wheelDeltaY: -8, + preventDefault: eventPreventDefaultStub + }); expect(component.getScrollTop()).toBe(expectedScrollTop); expect(component.getScrollLeft()).toBe(expectedScrollLeft); expect(component.refs.content.style.transform).toBe( @@ -1739,7 +1757,11 @@ describe('TextEditorComponent', () => { component.props.platform = 'linux'; { const expectedScrollTop = 20 * (scrollSensitivity / 100); - component.didMouseWheel({ wheelDeltaX: 0, wheelDeltaY: -20 }); + component.didMouseWheel({ + wheelDeltaX: 0, + wheelDeltaY: -20, + preventDefault: eventPreventDefaultStub + }); expect(component.getScrollTop()).toBe(expectedScrollTop); expect(component.refs.content.style.transform).toBe( `translate(0px, -${expectedScrollTop}px)` @@ -1752,7 +1774,8 @@ describe('TextEditorComponent', () => { component.didMouseWheel({ wheelDeltaX: 0, wheelDeltaY: -20, - shiftKey: true + shiftKey: true, + preventDefault: eventPreventDefaultStub }); expect(component.getScrollLeft()).toBe(expectedScrollLeft); expect(component.refs.content.style.transform).toBe( @@ -1766,7 +1789,8 @@ describe('TextEditorComponent', () => { component.didMouseWheel({ wheelDeltaX: -20, wheelDeltaY: 0, - shiftKey: true + shiftKey: true, + preventDefault: eventPreventDefaultStub }); expect(component.getScrollTop()).toBe(expectedScrollTop); expect(component.refs.content.style.transform).toBe( @@ -1778,7 +1802,11 @@ describe('TextEditorComponent', () => { component.props.platform = 'win32'; { const expectedScrollTop = 20 * (scrollSensitivity / 100); - component.didMouseWheel({ wheelDeltaX: 0, wheelDeltaY: -20 }); + component.didMouseWheel({ + wheelDeltaX: 0, + wheelDeltaY: -20, + preventDefault: eventPreventDefaultStub + }); expect(component.getScrollTop()).toBe(expectedScrollTop); expect(component.refs.content.style.transform).toBe( `translate(0px, -${expectedScrollTop}px)` @@ -1791,7 +1819,8 @@ describe('TextEditorComponent', () => { component.didMouseWheel({ wheelDeltaX: 0, wheelDeltaY: -20, - shiftKey: true + shiftKey: true, + preventDefault: eventPreventDefaultStub }); expect(component.getScrollLeft()).toBe(expectedScrollLeft); expect(component.refs.content.style.transform).toBe( @@ -1805,7 +1834,8 @@ describe('TextEditorComponent', () => { component.didMouseWheel({ wheelDeltaX: -20, wheelDeltaY: 0, - shiftKey: true + shiftKey: true, + preventDefault: eventPreventDefaultStub }); expect(component.getScrollTop()).toBe(expectedScrollTop); expect(component.refs.content.style.transform).toBe( @@ -1817,7 +1847,11 @@ describe('TextEditorComponent', () => { component.props.platform = 'darwin'; { const expectedScrollTop = 20 * (scrollSensitivity / 100); - component.didMouseWheel({ wheelDeltaX: 0, wheelDeltaY: -20 }); + component.didMouseWheel({ + wheelDeltaX: 0, + wheelDeltaY: -20, + preventDefault: eventPreventDefaultStub + }); expect(component.getScrollTop()).toBe(expectedScrollTop); expect(component.refs.content.style.transform).toBe( `translate(0px, -${expectedScrollTop}px)` @@ -1830,7 +1864,8 @@ describe('TextEditorComponent', () => { component.didMouseWheel({ wheelDeltaX: 0, wheelDeltaY: -20, - shiftKey: true + shiftKey: true, + preventDefault: eventPreventDefaultStub }); expect(component.getScrollTop()).toBe(expectedScrollTop); expect(component.refs.content.style.transform).toBe( @@ -1844,7 +1879,8 @@ describe('TextEditorComponent', () => { component.didMouseWheel({ wheelDeltaX: -20, wheelDeltaY: 0, - shiftKey: true + shiftKey: true, + preventDefault: eventPreventDefaultStub }); expect(component.getScrollLeft()).toBe(expectedScrollLeft); expect(component.refs.content.style.transform).toBe( From 5b6a4f67b30cc0d25975008773acdb9e11c04cbe Mon Sep 17 00:00:00 2001 From: Ben Klein Date: Wed, 22 Jan 2020 23:10:36 -0500 Subject: [PATCH 3/3] spec/text-editor-component: linter/test var fix --- spec/text-editor-component-spec.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/spec/text-editor-component-spec.js b/spec/text-editor-component-spec.js index 44e05cce7f7..d34f58707d4 100644 --- a/spec/text-editor-component-spec.js +++ b/spec/text-editor-component-spec.js @@ -1681,7 +1681,7 @@ describe('TextEditorComponent', () => { scrollSensitivity }); // stub in place for Event.preventDefault() - const eventPreventDefaultStub = function () {} + const eventPreventDefaultStub = function() {}; { const expectedScrollTop = 20 * (scrollSensitivity / 100); @@ -1753,6 +1753,8 @@ describe('TextEditorComponent', () => { width: 50, scrollSensitivity }); + // stub in place for Event.preventDefault() + const eventPreventDefaultStub = function() {}; component.props.platform = 'linux'; {