File patch selection and context menu interaction

[smashwilson]

Strictly speaking, the "actual fix" for #822 is a missing event.persist() call. In FilePatchView.contextMenuOnItem(), the MouseEvent is used in the resend callback, which is passed to setState() and may or may not be invoked asynchronously depending on React's scheduling mechanism. If you reproduce in dev mode you'll see a bunch of warnings to that effect in the console.

However... while I was reproducing this, I discovered that the right-click behavior no longer matched what the code was intended to do. We were intending to add the right-clicked line or hunk to the selection if it was not already selected, but what was actually happening was that the context menu was appearing before the selection was modified.

This likely regressed in #617, because React's synthetic event system relies on event handlers added to the document, but Atom implements context menus internally by adding an event handler to document first. React's synthetic event callback fires too late to cancel the event.

To fix this, I've introduced a ContextMenuInterceptor component that installs a single native handler on the document with {capture: true} so that it'll trigger before Atom's handler.

I also had to change the event re-fire from a requestAnimationFrame to a setImmediate to actually allow the repaint with the updated selection to occur before the context menu is shown. I'm not entirely sure why that happens, but I suspect it has to do with this bit from the documentation:

The window.requestAnimationFrame() method tells the browser that you wish to perform an animation and requests that the browser call a specified function to update an animation before the next repaint. The method takes as an argument a callback to be invoked before the repaint.

If I'm reading that right I bet two nested requestAnimationFrame() calls would do the trick as well... but setImmediate() works reliably for me and is somewhat cleaner.

[BinaryMuse]

I think this looks great; nice job tracking this down, and 👍 for a nice elegant solution.

Some questions below, but otherwise 🚢

[BinaryMuse]

I'm not 100% sure I understand why this is necessary, if registrations only happen on mount and are removed on unmount.

[smashwilson]

The ContextMenuInterceptor uses a single native event listener attached to the document that's shared among all instances. Each instance registers and deregisters an element in a Map, but the event listener itself isn't removed until the static ContextMenuInterceptor.dispose() method is called. This bit ☝️ ensures that we don't leak listeners on package deactivation.

... Although, this might be overkill. I could just register a new document listener for each instance and check element.includes(event.target)? I was intending to minimize the number of listeners we register but maybe a single listener that does an O(n) operation on a Map isn't any better.

[smashwilson]

I gave the simplification here a shot and our test suite leaked a ton of event listeners, because Enzyme doesn't trigger componentWillUnmount. I think I'm just going to leave it as -is with the static Map.

[BinaryMuse]

I'm not sure so thought I'd ask — can this be removed now that ContextMenuInterceptor wraps it?

[smashwilson]

👍 🔥 Totally.