Track and sync modifier states

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

@@ -303,6 +303,8 @@ export function DesktopSession({
 
   function handleMouseWheel(e: WheelEvent) {
     e.preventDefault();
+    // Check modifier states are correct to avoid unintentional zooming whilst scrolling.
+    inputHandler.current.synchronizeModifierState(client, e);
     // We only support pixel scroll events, not line or page events.
     // https://developer.mozilla.org/en-US/docs/Web/API/WheelEvent/deltaMode
     if (e.deltaMode === WheelEvent.DOM_DELTA_PIXEL) {

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

@@ -0,0 +1,192 @@
+/**
+ * Teleport
+ * Copyright (C) 2024 Gravitational, Inc.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+import {
+  ButtonState,
+  selectDirectoryInBrowser,
+  TdpClient,
+} from 'shared/libs/tdp';
+
+import { InputHandler } from './InputHandler';
+
+// Mock the TdpClient class
+jest.mock('shared/libs/tdp', () => {
+  const originalModule = jest.requireActual('shared/libs/tdp');
+  return {
+    ...originalModule,
+    TdpClient: jest.fn().mockImplementation(() => {
+      return {
+        sendKeyboardInput: jest.fn(),
+        sendMouseButton: jest.fn(),
+        sendSyncKeys: jest.fn(),
+      };
+    }),
+  };
+});
+
+describe('InputHandler', () => {
+  let inputHandler: InputHandler;
+  let mockTdpClient: jest.Mocked<TdpClient>;
+
+  beforeEach(() => {
+    inputHandler = new InputHandler();
+    mockTdpClient = new TdpClient(
+      () => null,
+      selectDirectoryInBrowser
+    ) as jest.Mocked<TdpClient>;
+  });
+
+  afterEach(() => {
+    inputHandler.dispose();
+  });
+
+  describe('synchronizeModifierState', () => {
+    it('sends modifier sync when local and remote states differ', () => {
+      // Create event with Shift pressed but don't track it in remote state first
+      const event = new KeyboardEvent('keydown', {
+        code: 'KeyA',
+        shiftKey: true,
+      });
+
+      const params = {
+        e: event,
+        state: ButtonState.DOWN,
+        cli: mockTdpClient,
+      };
+
+      inputHandler.handleInputEvent(params);
+
+      // Should send Shift DOWN to sync states since remote state defaults to UP
+      expect(mockTdpClient.sendKeyboardInput).toHaveBeenCalledWith(
+        'ShiftLeft',
+        ButtonState.DOWN
+      );
+    });
+
+    it('does not send sync when states are already synchronized', () => {
+      // First, set Shift as DOWN in remote state
+      const shiftDownEvent = new KeyboardEvent('keydown', {
+        code: 'ShiftLeft',
+      });
+      inputHandler.handleInputEvent({
+        e: shiftDownEvent,
+        state: ButtonState.DOWN,
+        cli: mockTdpClient,
+      });
+
+      mockTdpClient.sendKeyboardInput.mockClear();
+
+      // Now press a key with Shift
+      const event = new KeyboardEvent('keydown', {
+        code: 'KeyA',
+        shiftKey: true,
+      });
+
+      inputHandler.handleInputEvent({
+        e: event,
+        state: ButtonState.DOWN,
+        cli: mockTdpClient,
+      });
+
+      // Should not send any Shift synchronization events
+      const shiftCalls = mockTdpClient.sendKeyboardInput.mock.calls.filter(
+        call => call[0].includes('Shift')
+      );
+      expect(shiftCalls).toHaveLength(0);
+    });
+
+    it('synchronizes multiple modifier states correctly', () => {
+      // Set Alt as DOWN in remote state (to test it gets synced to UP)
+      const altDownEvent = new KeyboardEvent('keydown', { code: 'AltLeft' });
+      inputHandler.handleInputEvent({
+        e: altDownEvent,
+        state: ButtonState.DOWN,
+        cli: mockTdpClient,
+      });
+
+      // Press event with multiple modifiers active but not previously tracked
+      const event = new KeyboardEvent('keydown', {
+        code: 'KeyA',
+        shiftKey: true,
+        ctrlKey: true,
+        altKey: false,
+        metaKey: false,
+      });
+
+      inputHandler.handleInputEvent({
+        e: event,
+        state: ButtonState.DOWN,
+        cli: mockTdpClient,
+      });
+
+      // Should sync Shift and Control to DOWN, and Alt to UP
+      expect(mockTdpClient.sendKeyboardInput).toHaveBeenCalledWith(
+        'ShiftLeft',
+        ButtonState.DOWN
+      );
+      expect(mockTdpClient.sendKeyboardInput).toHaveBeenCalledWith(
+        'ControlLeft',
+        ButtonState.DOWN
+      );
+      expect(mockTdpClient.sendKeyboardInput).toHaveBeenCalledWith(
+        'AltLeft',
+        ButtonState.UP
+      );
+    });
+
+    it('handles modifier key release correctly', () => {
+      // First press Shift
+      const shiftDownEvent = new KeyboardEvent('keydown', {
+        code: 'ShiftLeft',
+      });
+      inputHandler.handleInputEvent({
+        e: shiftDownEvent,
+        state: ButtonState.DOWN,
+        cli: mockTdpClient,
+      });
+
+      // Then release Shift
+      const shiftUpEvent = new KeyboardEvent('keyup', { code: 'ShiftLeft' });
+      inputHandler.handleInputEvent({
+        e: shiftUpEvent,
+        state: ButtonState.UP,
+        cli: mockTdpClient,
+      });
+
+      mockTdpClient.sendKeyboardInput.mockClear();
+
+      // Now press a key without Shift
+      const normalKeyEvent = new KeyboardEvent('keydown', {
+        code: 'KeyA',
+        shiftKey: false,
+      });
+
+      inputHandler.handleInputEvent({
+        e: normalKeyEvent,
+        state: ButtonState.DOWN,
+        cli: mockTdpClient,
+      });
+
+      // Should not send additional Shift events since it's already UP
+      const shiftCalls = mockTdpClient.sendKeyboardInput.mock.calls.filter(
+        call => call[0].includes('Shift')
+      );
+      expect(shiftCalls).toHaveLength(0);
+    });
+  });
+});

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

@@ -37,6 +37,18 @@ export class InputHandler {
   private syncBeforeNextKey: boolean = true;
   private static isMac: boolean = getPlatform() === Platform.macOS;
 
+  /**
+   * Keep track of the remote state of modifier keys.
+   * This is used to mitigate stuck key issues when local
+   * and remote modifier states are out of sync.
+   */
+  private remoteModifierState = new Map<string, ButtonState>([
+    ['Shift', ButtonState.UP],
+    ['Control', ButtonState.UP],
+    ['Alt', ButtonState.UP],
+    ['Meta', ButtonState.UP],
+  ]);
+
   constructor() {
     // Bind finishHandlingInputEvent to this instance so it can be passed
     // as a callback to the Withholder.
@@ -47,13 +59,14 @@ export class InputHandler {
    * Primary method for handling input events.
    */
   public handleInputEvent(params: InputEventParams) {
-    const { e, cli } = params;
+    const { e, cli, state } = params;
     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.updateModifierState(e.code, state);
     }
     this.withholder.handleInputEvent(params, this.finishHandlingInputEvent);
   }
@@ -100,6 +113,9 @@ export class InputHandler {
   private finishHandlingInputEvent(params: InputEventParams): void {
     const { cli, e, state } = params;
 
+    // Synchronize local and remote modifier state before sending the input event.
+    this.synchronizeModifierState(cli, e);
+
     // If this is a mouse event no special handling is needed.
     if (e instanceof MouseEvent) {
       cli.sendMouseButton(e.button as MouseButton, state);
@@ -118,6 +134,78 @@ export class InputHandler {
     }
   }
 
+  /**
+   * Updates the locally tracked remote modifier state.
+   *
+   * @param keyCode The key code of the key.
+   * @param state The state of the key.
+   */
+  private updateModifierState(keyCode: string, state: ButtonState) {
+    switch (keyCode) {
+      case 'ShiftLeft':
+      case 'ShiftRight':
+        this.remoteModifierState.set('Shift', state);
+        break;
+      case 'ControlLeft':
+      case 'ControlRight':
+        this.remoteModifierState.set('Control', state);
+        break;
+      case 'AltLeft':
+      case 'AltRight':
+        this.remoteModifierState.set('Alt', state);
+        break;
+      case 'MetaLeft':
+      case 'MetaRight':
+        this.remoteModifierState.set('Meta', state);
+        break;
+    }
+  }
+
+  /**
+   * Checks if the given key is a modifier key.
+   *
+   * @param key The key to check.
+   * @returns True if the key is a modifier key, false otherwise.
+   */
+  private isModifierKey(key: string): boolean {
+    return ['Shift', 'Control', 'Alt', 'Meta'].some(modifier =>
+      key.startsWith(modifier)
+    );
+  }
+
+  /**
+   * Synchronizes the local modifier state with the remote machine.
+   * This is called when key or mouse click/scroll events occur to ensure
+   * that the remote machine has the correct state of modifier keys.
+   * If not, it sends the current state.
+   *
+   * @param cli The TdpClient instance used to send the state.
+   * @param e The KeyboardEvent or MouseEvent that triggered the synchronization.
+   */
+  public synchronizeModifierState(
+    cli: TdpClient,
+    e: KeyboardEvent | MouseEvent
+  ) {
+    // Don't process modifier keys themselves
+    if (e instanceof KeyboardEvent && this.isModifierKey(e.code)) {
+      return;
+    }
+
+    this.remoteModifierState.forEach((state, modifier) => {
+      const localState = e.getModifierState(modifier)
+        ? ButtonState.DOWN
+        : ButtonState.UP;
+
+      if (localState !== state) {
+        // If the local state is different from the remote state, send the updates.
+        cli.sendKeyboardInput(modifier + 'Left', localState);
+        cli.sendKeyboardInput(modifier + 'Right', localState);
+        // Update the remote state to match the local state.
+        this.remoteModifierState.set(modifier, localState);
+      }
+    });
+  }
+
   /**
    * Must be called when the element associated with the InputHandler loses focus.
    */