From e17cd2578c99b6fbb05c252b0acfc0db11ba97ae Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 19 May 2017 14:54:26 -0400 Subject: [PATCH 1/9] Use a capturing event handler on document to process contextmenu events --- lib/context-menu-interceptor.js | 41 +++++++ test/context-menu-interceptor.test.js | 148 ++++++++++++++++++++++++++ 2 files changed, 189 insertions(+) create mode 100644 lib/context-menu-interceptor.js create mode 100644 test/context-menu-interceptor.test.js diff --git a/lib/context-menu-interceptor.js b/lib/context-menu-interceptor.js new file mode 100644 index 0000000000..78ba4a846a --- /dev/null +++ b/lib/context-menu-interceptor.js @@ -0,0 +1,41 @@ +import React from 'react'; +import PropTypes from 'prop-types'; + +export default class ContextMenuInterceptor extends React.Component { + static propTypes = { + onWillShowContextMenu: PropTypes.func.isRequired, + children: PropTypes.element.isRequired, + } + + static registration = new Map() + + static handle(event) { + for (const [element, callback] of ContextMenuInterceptor.registration) { + if (element.contains(event.target)) { + callback(event); + } + } + } + + static dispose() { + document.removeEventListener('contextmenu', contextMenuHandler, {capture: true}); + } + + componentDidMount() { + // Helpfully, addEventListener dedupes listeners for us. + document.addEventListener('contextmenu', contextMenuHandler, {capture: true}); + ContextMenuInterceptor.registration.set(this.element, this.props.onWillShowContextMenu); + } + + render() { + return
{ this.element = e; }}>{this.props.children}
; + } + + componentWillUnmount() { + ContextMenuInterceptor.registration.remove(this.element); + } +} + +function contextMenuHandler(event) { + ContextMenuInterceptor.handle(event); +} diff --git a/test/context-menu-interceptor.test.js b/test/context-menu-interceptor.test.js new file mode 100644 index 0000000000..974eff6fa9 --- /dev/null +++ b/test/context-menu-interceptor.test.js @@ -0,0 +1,148 @@ +import React from 'react'; +import {mount} from 'enzyme'; + +import ContextMenuInterceptor from '../lib/context-menu-interceptor'; + +class SampleComponent extends React.Component { + render() { + return ( +
+
+ This element has content. +
+
+ ); + } +} + +describe('ContextMenuInterceptor', function() { + let rootElement, rootHandler, rootHandlerCalled; + + beforeEach(function() { + rootHandlerCalled = false; + rootHandler = event => { + rootHandlerCalled = true; + event.preventDefault(); + return false; + }; + document.addEventListener('contextmenu', rootHandler); + + rootElement = document.createElement('div'); + document.body.appendChild(rootElement); + }); + + afterEach(function() { + document.removeEventListener('contextmenu', rootHandler); + ContextMenuInterceptor.dispose(); + rootElement.remove(); + }); + + it('responds to native contextmenu events before they reach the document root', function() { + let interceptorHandlerCalled = false; + let rootHandlerCalledFirst = false; + const handler = () => { + interceptorHandlerCalled = true; + rootHandlerCalledFirst = rootHandlerCalled; + }; + + const wrapper = mount( + + + , + {attachTo: rootElement}, + ); + + const targetDOMNode = wrapper.find('.child').getDOMNode(); + const event = new MouseEvent('contextmenu', { + bubbles: true, + cancelable: true, + }); + targetDOMNode.dispatchEvent(event); + + assert.isTrue(interceptorHandlerCalled); + assert.isFalse(rootHandlerCalledFirst); + assert.isTrue(rootHandlerCalled); + }); + + it('can prevent event propagation', function() { + let interceptorHandlerCalled = false; + const handler = () => { + interceptorHandlerCalled = true; + event.stopPropagation(); + }; + + const wrapper = mount( + + + , + {attachTo: rootElement}, + ); + + const targetDOMNode = wrapper.find('.child').getDOMNode(); + const event = new MouseEvent('contextmenu', { + bubbles: true, + cancelable: true, + }); + targetDOMNode.dispatchEvent(event); + + assert.isTrue(interceptorHandlerCalled); + assert.isFalse(rootHandlerCalled); + }); + + it('ignores contextmenu events from other children', function() { + let interceptorHandlerCalled = false; + const handler = () => { + interceptorHandlerCalled = true; + }; + + const wrapper = mount( +
+ + + +
+
+ This is another div. +
+
+
, + {attachTo: rootElement}, + ); + + const targetDOMNode = wrapper.find('.otherNode').getDOMNode(); + const event = new MouseEvent('contextmenu', { + bubbles: true, + cancelable: true, + }); + targetDOMNode.dispatchEvent(event); + + assert.isFalse(interceptorHandlerCalled); + assert.isTrue(rootHandlerCalled); + }); + + it('cleans itself up on .dispose()', function() { + let interceptorHandlerCalled = false; + const handler = () => { + interceptorHandlerCalled = true; + }; + + const wrapper = mount( + + + , + {attachTo: rootElement}, + ); + + ContextMenuInterceptor.dispose(); + + const targetDOMNode = wrapper.find('.child').getDOMNode(); + const event = new MouseEvent('contextmenu', { + bubbles: true, + cancelable: true, + }); + targetDOMNode.dispatchEvent(event); + + assert.isFalse(interceptorHandlerCalled); + assert.isTrue(rootHandlerCalled); + }); +}); From d99db74aace80410ac849a102d20055921507f43 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 19 May 2017 15:30:09 -0400 Subject: [PATCH 2/9] Dispose any accumulated contextmenu handlers after each test --- test/helpers.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/helpers.js b/test/helpers.js index d0d3c14411..3a4fa8b398 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -9,6 +9,7 @@ import sinon from 'sinon'; import Repository from '../lib/models/repository'; import GitShellOutStrategy from '../lib/git-shell-out-strategy'; import WorkerManager from '../lib/worker-manager'; +import ContextMenuInterceptor from '../lib/context-menu-interceptor'; assert.autocrlfEqual = (actual, expected, ...args) => { const newActual = actual.replace(/\r\n/g, '\n'); @@ -186,6 +187,9 @@ beforeEach(function() { afterEach(function() { activeRenderers.forEach(r => r.unmount()); activeRenderers = []; + + ContextMenuInterceptor.dispose(); + global.sinon.restore(); }); From 8aa1cac8b9e02e8d11c93113168b663040115866 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 19 May 2017 15:33:51 -0400 Subject: [PATCH 3/9] It's delete() not remove() --- lib/context-menu-interceptor.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/context-menu-interceptor.js b/lib/context-menu-interceptor.js index 78ba4a846a..44586668a0 100644 --- a/lib/context-menu-interceptor.js +++ b/lib/context-menu-interceptor.js @@ -32,7 +32,7 @@ export default class ContextMenuInterceptor extends React.Component { } componentWillUnmount() { - ContextMenuInterceptor.registration.remove(this.element); + ContextMenuInterceptor.registration.delete(this.element); } } From 86169eaa1b5c4ef65b1e76348408207c0a86f655 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 19 May 2017 15:40:36 -0400 Subject: [PATCH 4/9] Dispose any context menu handlers on package deactivation --- lib/github-package.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/github-package.js b/lib/github-package.js index 7f5ee37649..5c99f96dbb 100644 --- a/lib/github-package.js +++ b/lib/github-package.js @@ -18,6 +18,7 @@ import StubItem from './atom-items/stub-item'; import Switchboard from './switchboard'; import yardstick from './yardstick'; import GitTimingsView from './views/git-timings-view'; +import ContextMenuInterceptor from './context-menu-interceptor'; import AsyncQueue from './async-queue'; import WorkerManager from './worker-manager'; @@ -71,6 +72,7 @@ export default class GithubPackage { this.setActiveContext(WorkdirContext.absent()); } }), + ContextMenuInterceptor, ); this.setupYardstick(); From af9b623c9c5dbf7e595bf6f3337247724d44e2ba Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 19 May 2017 15:41:52 -0400 Subject: [PATCH 5/9] Use a ContextMenuInterceptor component within LineView --- lib/views/hunk-view.js | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/lib/views/hunk-view.js b/lib/views/hunk-view.js index 088b80a786..a52171f7f2 100644 --- a/lib/views/hunk-view.js +++ b/lib/views/hunk-view.js @@ -2,6 +2,8 @@ import React from 'react'; import PropTypes from 'prop-types'; import {autobind} from 'core-decorators'; +import ContextMenuInterceptor from '../context-menu-interceptor'; + export default class HunkView extends React.Component { static propTypes = { hunk: PropTypes.object.isRequired, @@ -102,19 +104,21 @@ class LineView extends React.Component { const lineSelectedClass = this.props.isSelected ? 'is-selected' : ''; return ( -
this.props.mousedown(event, line)} - onMouseMove={event => this.props.mousemove(event, line)} - onContextMenu={event => this.props.contextMenuOnItem(event, line)} - ref={e => this.props.registerLineElement(line, e)}> -
{oldLineNumber}
-
{newLineNumber}
-
- {line.getOrigin()} - {line.getText()} + this.props.contextMenuOnItem(event, line)}> +
this.props.mousedown(event, line)} + onMouseMove={event => this.props.mousemove(event, line)} + onContextMenu={event => this.props.contextMenuOnItem(event, line)} + ref={e => this.props.registerLineElement(line, e)}> +
{oldLineNumber}
+
{newLineNumber}
+
+ {line.getOrigin()} + {line.getText()} +
-
+ ); } } From f0983e67155bc38f116a608f050e21c3ca20ee28 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 19 May 2017 15:42:53 -0400 Subject: [PATCH 6/9] Call .stopPropagation() before setState is invoked --- lib/views/file-patch-view.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/views/file-patch-view.js b/lib/views/file-patch-view.js index c09e6f3ae3..65abd5786b 100644 --- a/lib/views/file-patch-view.js +++ b/lib/views/file-patch-view.js @@ -193,8 +193,6 @@ export default class FilePatchView extends React.Component { @autobind contextMenuOnItem(event, hunk, line) { const resend = () => { - event.stopPropagation(); - const newEvent = new MouseEvent(event.type, event); requestAnimationFrame(() => { event.target.parentNode.dispatchEvent(newEvent); @@ -203,10 +201,14 @@ export default class FilePatchView extends React.Component { const mode = this.state.selection.getMode(); if (mode === 'hunk' && !this.state.selection.getSelectedHunks().has(hunk)) { + event.stopPropagation(); + this.setState(prevState => { return {selection: prevState.selection.selectHunk(hunk, event.shiftKey)}; }, resend); } else if (mode === 'line' && !this.state.selection.getSelectedLines().has(line)) { + event.stopPropagation(); + this.setState(prevState => { return {selection: prevState.selection.selectLine(line, event.shiftKey)}; }, resend); From c8fd02c57bbddd86063f9a1d21630fb1dab6c7f5 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 19 May 2017 15:43:21 -0400 Subject: [PATCH 7/9] Use setImmediate instead of requestAnimationFrame for the context menu re-dispatch --- lib/views/file-patch-view.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/views/file-patch-view.js b/lib/views/file-patch-view.js index 65abd5786b..3c427e0d1a 100644 --- a/lib/views/file-patch-view.js +++ b/lib/views/file-patch-view.js @@ -194,9 +194,7 @@ export default class FilePatchView extends React.Component { contextMenuOnItem(event, hunk, line) { const resend = () => { const newEvent = new MouseEvent(event.type, event); - requestAnimationFrame(() => { - event.target.parentNode.dispatchEvent(newEvent); - }); + setImmediate(() => event.target.parentNode.dispatchEvent(newEvent)); }; const mode = this.state.selection.getMode(); From 7f0f43baba4f06dabc94659e1ac24ae0fecbabd0 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 23 May 2017 09:14:19 -0400 Subject: [PATCH 8/9] :fire: unnecessary onContextMenu attribute --- lib/views/hunk-view.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/views/hunk-view.js b/lib/views/hunk-view.js index a52171f7f2..23e34cb670 100644 --- a/lib/views/hunk-view.js +++ b/lib/views/hunk-view.js @@ -109,7 +109,6 @@ class LineView extends React.Component { className={`github-HunkView-line ${lineSelectedClass} is-${line.getStatus()}`} onMouseDown={event => this.props.mousedown(event, line)} onMouseMove={event => this.props.mousemove(event, line)} - onContextMenu={event => this.props.contextMenuOnItem(event, line)} ref={e => this.props.registerLineElement(line, e)}>
{oldLineNumber}
{newLineNumber}
From b1c950ce3a9a45be040b0ac3711f943082bb6969 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 23 May 2017 10:07:40 -0400 Subject: [PATCH 9/9] Dispatch simulated mouse events to the line element within a LineView --- test/controllers/file-patch-controller.test.js | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/test/controllers/file-patch-controller.test.js b/test/controllers/file-patch-controller.test.js index 4148beeddf..d8b706bcb5 100644 --- a/test/controllers/file-patch-controller.test.js +++ b/test/controllers/file-patch-controller.test.js @@ -286,8 +286,8 @@ describe('FilePatchController', function() { const opPromise0 = switchboard.getFinishStageOperationPromise(); const hunkView0 = wrapper.find('HunkView').at(0); - hunkView0.find('LineView').at(1).simulate('mousedown', {button: 0, detail: 1}); - hunkView0.find('LineView').at(3).simulate('mousemove', {}); + hunkView0.find('LineView').at(1).find('.github-HunkView-line').simulate('mousedown', {button: 0, detail: 1}); + hunkView0.find('LineView').at(3).find('.github-HunkView-line').simulate('mousemove', {}); window.dispatchEvent(new MouseEvent('mouseup')); hunkView0.find('button.github-HunkView-stageButton').simulate('click'); await opPromise0; @@ -329,9 +329,11 @@ describe('FilePatchController', function() { await updatePromise2; const hunkView2 = wrapper.find('HunkView').at(0); - hunkView2.find('LineView').at(1).simulate('mousedown', {button: 0, detail: 1}); + hunkView2.find('LineView').at(1).find('.github-HunkView-line') + .simulate('mousedown', {button: 0, detail: 1}); window.dispatchEvent(new MouseEvent('mouseup')); - hunkView2.find('LineView').at(2).simulate('mousedown', {button: 0, detail: 1, metaKey: true}); + hunkView2.find('LineView').at(2).find('.github-HunkView-line') + .simulate('mousedown', {button: 0, detail: 1, metaKey: true}); window.dispatchEvent(new MouseEvent('mouseup')); const opPromise2 = switchboard.getFinishStageOperationPromise(); @@ -442,7 +444,8 @@ describe('FilePatchController', function() { })); const hunkView0 = wrapper.find('HunkView').at(0); - hunkView0.find('LineView').at(1).simulate('mousedown', {button: 0, detail: 1}); + hunkView0.find('LineView').at(1).find('.github-HunkView-line') + .simulate('mousedown', {button: 0, detail: 1}); window.dispatchEvent(new MouseEvent('mouseup')); // stage lines in rapid succession @@ -467,7 +470,8 @@ describe('FilePatchController', function() { await line1PatchPromise; const hunkView1 = wrapper.find('HunkView').at(0); - hunkView1.find('LineView').at(2).simulate('mousedown', {button: 0, detail: 1}); + hunkView1.find('LineView').at(2).find('.github-HunkView-line') + .simulate('mousedown', {button: 0, detail: 1}); window.dispatchEvent(new MouseEvent('mouseup')); const line2StagingPromise = switchboard.getFinishStageOperationPromise();