Fix Alt+Click in desktop sessions

web/packages/shared/components/DesktopSession/DesktopSession.tsx

@@ -49,7 +49,7 @@ import {
 } from 'shared/libs/tdp';
 import { TdpError } from 'shared/libs/tdp/client';
 
-import { KeyboardHandler } from './KeyboardHandler';
+import { InputHandler } from './InputHandler';
 import TopBar from './TopBar';
 import useDesktopSession, {
   clipboardSharingMessage,
@@ -109,9 +109,9 @@ export function DesktopSession({
   const [tdpConnectionStatus, setTdpConnectionStatus] =
     useState<TdpConnectionStatus>({ status: '' });
 
-  const keyboardHandler = useRef(new KeyboardHandler());
+  const inputHandler = useRef(new InputHandler());
   useEffect(() => {
-    return () => keyboardHandler.current.dispose();
+    return () => inputHandler.current.dispose();
   }, []);
 
   const [
@@ -243,7 +243,7 @@ export function DesktopSession({
   }, [client, shouldConnect, keyboardLayout]);
 
   function handleKeyDown(e: React.KeyboardEvent) {
-    keyboardHandler.current.handleKeyboardEvent({
+    inputHandler.current.handleInputEvent({
       cli: client,
       e: e.nativeEvent,
       state: ButtonState.DOWN,
@@ -262,15 +262,15 @@ export function DesktopSession({
   }
 
   function handleKeyUp(e: React.KeyboardEvent) {
-    keyboardHandler.current.handleKeyboardEvent({
+    inputHandler.current.handleInputEvent({
       cli: client,
       e: e.nativeEvent,
       state: ButtonState.UP,
     });
   }
 
   function handleBlur() {
-    keyboardHandler.current.onFocusOut();
+    inputHandler.current.onFocusOut();
   }
 
   function handleMouseMove(e: React.MouseEvent) {
@@ -281,9 +281,11 @@ export function DesktopSession({
   }
 
   function handleMouseDown(e: React.MouseEvent) {
-    if (e.button === 0 || e.button === 1 || e.button === 2) {
-      client.sendMouseButton(e.button, ButtonState.DOWN);
-    }
+    inputHandler.current.handleInputEvent({
+      cli: client,
+      e: e.nativeEvent,
+      state: ButtonState.DOWN,
+    });
 
     // Opportunistically sync local clipboard to remote while
     // transient user activation is in effect.
@@ -292,9 +294,11 @@ export function DesktopSession({
   }
 
   function handleMouseUp(e: React.MouseEvent) {
-    if (e.button === 0 || e.button === 1 || e.button === 2) {
-      client.sendMouseButton(e.button, ButtonState.UP);
-    }
+    inputHandler.current.handleInputEvent({
+      cli: client,
+      e: e.nativeEvent,
+      state: ButtonState.UP,
+    });
   }
 
   function handleMouseWheel(e: WheelEvent) {

web/packages/shared/components/DesktopSession/KeyboardHandler.tsx → web/packages/shared/components/DesktopSession/InputHandler.tsx

@@ -17,14 +17,14 @@
  */
 
 import { getPlatform, Platform } from 'design/platform';
-import { ButtonState, SyncKeys, TdpClient } from 'shared/libs/tdp';
+import { ButtonState, MouseButton, SyncKeys, TdpClient } from 'shared/libs/tdp';
 
 import { Withholder } from './Withholder';
 
 /**
- * Handles keyboard events.
+ * Handles mouse and keyboard events.
  */
-export class KeyboardHandler {
+export class InputHandler {
   private withholder: Withholder = new Withholder();
   /**
    * Tracks whether the next keydown or keyup event should sync the
@@ -38,33 +38,37 @@ export class KeyboardHandler {
   private static isMac: boolean = getPlatform() === Platform.macOS;
 
   constructor() {
-    // Bind finishHandlingKeyboardEvent to this instance so it can be passed
+    // Bind finishHandlingInputEvent to this instance so it can be passed
     // as a callback to the Withholder.
-    this.finishHandlingKeyboardEvent =
-      this.finishHandlingKeyboardEvent.bind(this);
+    this.finishHandlingInputEvent = this.finishHandlingInputEvent.bind(this);
   }
 
   /**
-   * Primary method for handling keyboard events.
+   * Primary method for handling input events.
    */
-  public handleKeyboardEvent(params: KeyboardEventParams) {
+  public handleInputEvent(params: InputEventParams) {
     const { e, cli } = params;
-    e.preventDefault();
-    this.handleSyncBeforeNextKey(cli, e);
-    this.withholder.handleKeyboardEvent(
-      params,
-      this.finishHandlingKeyboardEvent
-    );
+    if (e instanceof KeyboardEvent) {
+      // Only prevent default for KeyboardEvents.
+      // If preventDefault is done on MouseEvents,
+      // it breaks focus and keys won't be registered.
+      e.preventDefault();
+      this.handleSyncBeforeNextKey(cli, e);
+    }
+    this.withholder.handleInputEvent(params, this.finishHandlingInputEvent);
   }
 
-  private handleSyncBeforeNextKey(cli: TdpClient, e: KeyboardEvent) {
+  private handleSyncBeforeNextKey(
+    cli: TdpClient,
+    e: KeyboardEvent | MouseEvent
+  ) {
     if (this.syncBeforeNextKey === true) {
       cli.sendSyncKeys(this.getSyncKeys(e));
       this.syncBeforeNextKey = false;
     }
   }
 
-  private getSyncKeys = (e: KeyboardEvent): SyncKeys => {
+  private getSyncKeys = (e: KeyboardEvent | MouseEvent): SyncKeys => {
     return {
       scrollLockState: this.getModifierState(e, 'ScrollLock'),
       numLockState: this.getModifierState(e, 'NumLock'),
@@ -80,7 +84,7 @@ export class KeyboardHandler {
    * @param keyArg The key to check the state of. Valid values can be found [here](https://www.w3.org/TR/uievents-key/#keys-modifier)
    */
   private getModifierState = (
-    e: KeyboardEvent,
+    e: KeyboardEvent | MouseEvent,
     keyArg: string
   ): ButtonState => {
     return e.getModifierState(keyArg) ? ButtonState.DOWN : ButtonState.UP;
@@ -93,10 +97,17 @@ export class KeyboardHandler {
    * For withheld or delayed keys, this is called as the callback when
    * another key is pressed or released (withheld) or after a delay (delayed).
    */
-  private finishHandlingKeyboardEvent(params: KeyboardEventParams): void {
+  private finishHandlingInputEvent(params: InputEventParams): void {
     const { cli, e, state } = params;
+
+    // If this is a mouse event no special handling is needed.
+    if (e instanceof MouseEvent) {
+      cli.sendMouseButton(e.button as MouseButton, state);
+      return;
+    }
+
     // Special handling for CapsLock on Mac.
-    if (e.code === 'CapsLock' && KeyboardHandler.isMac) {
+    if (e.code === 'CapsLock' && InputHandler.isMac) {
       // On Mac, every UP or DOWN given to us by the browser corresponds
       // to a DOWN + UP on the remote machine for CapsLock.
       cli.sendKeyboardInput('CapsLock', ButtonState.DOWN);
@@ -108,7 +119,7 @@ export class KeyboardHandler {
   }
 
   /**
-   * Must be called when the element associated with the KeyboardHandler loses focus.
+   * Must be called when the element associated with the InputHandler loses focus.
    */
   public onFocusOut() {
     // Sync toggle keys when we come back into focus.
@@ -118,16 +129,16 @@ export class KeyboardHandler {
   }
 
   /**
-   * Should be called when the element associated with the KeyboardHandler goes away.
+   * Should be called when the element associated with the InputHandler goes away.
    */
   public dispose() {
     // Make sure we cancel any withheld keys, particularly we want to cancel the timeouts.
     this.withholder.cancel();
   }
 }
 
-export type KeyboardEventParams = {
+export type InputEventParams = {
   cli: TdpClient;
-  e: KeyboardEvent;
+  e: KeyboardEvent | MouseEvent;
   state: ButtonState;
 };

web/packages/shared/components/DesktopSession/Withholder.test.ts

@@ -59,7 +59,7 @@ describe('withholder', () => {
       state: ButtonState.DOWN,
       cli: new TdpClient(() => null, selectDirectoryInBrowser),
     };
-    withholder.handleKeyboardEvent(params, mockHandleKeyboardEvent);
+    withholder.handleInputEvent(params, mockHandleKeyboardEvent);
     expect(mockHandleKeyboardEvent).toHaveBeenCalledWith(params);
   });
 
@@ -82,12 +82,12 @@ describe('withholder', () => {
       cli: new TdpClient(() => null, selectDirectoryInBrowser),
     };
 
-    withholder.handleKeyboardEvent(metaDown, mockHandleKeyboardEvent);
-    withholder.handleKeyboardEvent(metaUp, mockHandleKeyboardEvent);
+    withholder.handleInputEvent(metaDown, mockHandleKeyboardEvent);
+    withholder.handleInputEvent(metaUp, mockHandleKeyboardEvent);
 
     expect(mockHandleKeyboardEvent).not.toHaveBeenCalled();
 
-    withholder.handleKeyboardEvent(enterDown, mockHandleKeyboardEvent);
+    withholder.handleInputEvent(enterDown, mockHandleKeyboardEvent);
 
     expect(mockHandleKeyboardEvent).toHaveBeenCalledTimes(3);
     expect(mockHandleKeyboardEvent).toHaveBeenNthCalledWith(1, metaDown);
@@ -107,8 +107,8 @@ describe('withholder', () => {
       cli: new TdpClient(() => null, selectDirectoryInBrowser),
     };
 
-    withholder.handleKeyboardEvent(metaParams, mockHandleKeyboardEvent);
-    withholder.handleKeyboardEvent(altParams, mockHandleKeyboardEvent);
+    withholder.handleInputEvent(metaParams, mockHandleKeyboardEvent);
+    withholder.handleInputEvent(altParams, mockHandleKeyboardEvent);
 
     expect(mockHandleKeyboardEvent).not.toHaveBeenCalled();
 
@@ -125,13 +125,32 @@ describe('withholder', () => {
       state: ButtonState.UP,
       cli: new TdpClient(() => null, selectDirectoryInBrowser),
     };
-    withholder.handleKeyboardEvent(metaParams, mockHandleKeyboardEvent);
-    expect((withholder as any).withheldKeys).toHaveLength(1);
+    withholder.handleInputEvent(metaParams, mockHandleKeyboardEvent);
+    expect((withholder as any).withheldInputs).toHaveLength(1);
 
     withholder.cancel();
     jest.advanceTimersByTime(10);
 
     expect(mockHandleKeyboardEvent).not.toHaveBeenCalled();
-    expect((withholder as any).withheldKeys).toHaveLength(0);
+    expect((withholder as any).withheldInputs).toHaveLength(0);
+  });
+
+  it('flushes keys on mouse event', () => {
+    const metaParams = {
+      e: { key: 'Meta' } as KeyboardEvent,
+      state: ButtonState.DOWN,
+      cli: new TdpClient(() => null, selectDirectoryInBrowser),
+    };
+    withholder.handleInputEvent(metaParams, mockHandleKeyboardEvent);
+
+    const mouseEvent = {
+      e: { button: 0 } as MouseEvent,
+      state: ButtonState.DOWN,
+      cli: new TdpClient(() => null, selectDirectoryInBrowser),
+    };
+    withholder.handleInputEvent(mouseEvent, mockHandleKeyboardEvent);
+
+    expect(mockHandleKeyboardEvent).toHaveBeenCalledWith(metaParams);
+    expect(mockHandleKeyboardEvent).toHaveBeenCalledWith(mouseEvent);
   });
 });

web/packages/shared/components/DesktopSession/Withholder.tsx

@@ -18,7 +18,7 @@
 
 import { ButtonState } from 'shared/libs/tdp';
 
-import { KeyboardEventParams } from './KeyboardHandler';
+import { InputEventParams } from './InputHandler';
 
 /**
  * The Withholder class manages keyboard events, particularly for alt/cmd keys. It delays handling these keys to determine the user's intent:
@@ -39,25 +39,26 @@ export class Withholder {
    */
   private keysToWithhold: string[] = ['Meta', 'Alt'];
   /**
-   * The internal array of keystrokes that are currently
+   * The internal array of inputs that are currently
    * being withheld.
    */
-  private withheldKeys: Array<WithheldKeyboardEventHandler> = [];
+  private withheldInputs: WithheldInputEventHandler[] = [];
 
   /**
-   * All keyboard events should be handled via this function.
+   * All input events should be handled via this function.
    */
-  public handleKeyboardEvent(
-    params: KeyboardEventParams,
-    handleKeyboardEvent: (params: KeyboardEventParams) => void
+  public handleInputEvent(
+    params: InputEventParams,
+    handleInputEvent: (params: InputEventParams) => void
   ) {
-    const key = params.e.key;
-
-    // If this is not a key we withhold, immediately flush any withheld keys
+    // If this is not a key we withhold or a mouse event, immediately flush any withheld keys
     // and handle this key.
-    if (!this.keysToWithhold.includes(key)) {
+    if (
+      params.e instanceof MouseEvent ||
+      !this.keysToWithhold.includes(params.e.key)
+    ) {
       this.flush();
-      handleKeyboardEvent(params);
+      handleInputEvent(params);
       return;
     }
 
@@ -84,32 +85,32 @@ export class Withholder {
       }, UP_DELAY_MS);
     }
 
-    this.withheldKeys.push({
+    this.withheldInputs.push({
       params,
-      handler: handleKeyboardEvent,
+      handler: handleInputEvent,
       timeout,
     });
   }
 
   // Cancel all withheld keys.
   public cancel() {
-    this.withheldKeys.forEach(w => clearTimeout(w.timeout));
-    this.withheldKeys = [];
+    this.withheldInputs.forEach(w => clearTimeout(w.timeout));
+    this.withheldInputs = [];
   }
 
   // Flush all withheld keys.
   private flush() {
-    this.withheldKeys.forEach(w => {
+    this.withheldInputs.forEach(w => {
       clearTimeout(w.timeout);
       w.handler(w.params);
     });
-    this.withheldKeys = [];
+    this.withheldInputs = [];
   }
 }
 
-type WithheldKeyboardEventHandler = {
-  handler: (params: KeyboardEventParams) => void;
-  params: KeyboardEventParams;
+type WithheldInputEventHandler = {
+  handler: (params: InputEventParams) => void;
+  params: InputEventParams;
   timeout?: NodeJS.Timeout;
 };