fix: Stop PTZ movement and browser key handling from fighting (#2674)

- Closes: #2623

src/card-controller/actions/actions/ptz-digital.ts

@@ -14,14 +14,14 @@ import type { CardActionsAPI } from '../../types';
 import { ZoomRequestViewModifier } from '../../view/modifiers/zoom-request';
 import type { TargetedActionContext } from '../types';
 import {
-  setInProgressForThisTarget,
-  stopInProgressForThisTarget,
+  clearInProgressForThisTarget,
+  replaceInProgressForThisTarget,
 } from '../utils/action-state';
 import { AdvancedCameraCardAction } from './base';
 
 const STEP_DELAY_SECONDS = 0.1;
 const STEP_ZOOM = 0.1;
-const STEP_PAN = 5;
+export const STEP_PAN = 5;
 
 declare module 'action' {
   interface ActionContext {
@@ -31,6 +31,7 @@ declare module 'action' {
 
 export class PTZDigitalAction extends AdvancedCameraCardAction<PTZDigitialActionConfig> {
   private _timer = new Timer();
+  private _stopped = false;
 
   private async _stepChange(api: CardActionsAPI, targetID: string): Promise<void> {
     api
@@ -46,6 +47,7 @@ export class PTZDigitalAction extends AdvancedCameraCardAction<PTZDigitialAction
   }
 
   public async stop(): Promise<void> {
+    this._stopped = true;
     this._timer.stop();
   }
 
@@ -72,16 +74,20 @@ export class PTZDigitalAction extends AdvancedCameraCardAction<PTZDigitialAction
 
     /* v8 ignore else: the else path cannot be reached -- @preserve */
     if (action.ptz_phase === 'start') {
-      await stopInProgressForThisTarget(targetID, this._context.ptzDigital);
-      setInProgressForThisTarget(targetID, this._context, 'ptzDigital', this);
+      this._stopped = false;
+      await replaceInProgressForThisTarget(targetID, this._context, 'ptzDigital', this);
 
       await this._stepChange(api, targetID);
-      this._timer.startRepeated(STEP_DELAY_SECONDS, () =>
-        this._stepChange(api, targetID),
-      );
+
+      // The steps are repeated only once the first step returns, and only if
+      // this action has not been stopped.
+      if (!this._stopped) {
+        this._timer.startRepeated(STEP_DELAY_SECONDS, () =>
+          this._stepChange(api, targetID),
+        );
+      }
     } else if (action.ptz_phase === 'stop') {
-      await stopInProgressForThisTarget(targetID, this._context.ptzDigital);
-      delete this._context.ptzDigital?.[targetID];
+      await clearInProgressForThisTarget(targetID, this._context, 'ptzDigital');
     }
   }
 

src/card-controller/actions/actions/ptz.ts

@@ -4,21 +4,16 @@ import { PTZMovementType } from '../../../types';
 import { getPTZTarget, ptzActionToCapabilityKey } from '../../../utils/ptz';
 import { Timer } from '../../../utils/timer';
 import type { CardActionsAPI } from '../../types';
+import type { TargetedActionContext } from '../types';
 import {
-  setInProgressForThisTarget,
-  stopInProgressForThisTarget,
+  clearInProgressForThisTarget,
+  replaceInProgressForThisTarget,
 } from '../utils/action-state';
 import { AdvancedCameraCardAction } from './base';
 
-interface PTZContext {
-  [cameraID: string]: {
-    inProgressAction?: PTZAction;
-  };
-}
-
 declare module 'action' {
   interface ActionContext {
-    ptz?: PTZContext;
+    ptz?: TargetedActionContext;
   }
 }
 
@@ -96,8 +91,8 @@ export class PTZAction extends AdvancedCameraCardAction<PTZActionConfig> {
 
     if (action.ptz_phase === 'start') {
       // Scenario: Asked to start a continuous move, camera only supports relative moves natively.
-      await stopInProgressForThisTarget(ptzCameraID, this._context.ptz);
-      setInProgressForThisTarget(ptzCameraID, this._context, 'ptz', this);
+      this._stopped = false;
+      await replaceInProgressForThisTarget(ptzCameraID, this._context, 'ptz', this);
 
       const singleStep = async (): Promise<void> => {
         /* v8 ignore else: the else path cannot be reached as ptz_action
@@ -108,10 +103,10 @@ export class PTZAction extends AdvancedCameraCardAction<PTZActionConfig> {
           });
         }
 
+        // The next step is scheduled only once this step returns, and only if
+        // this action has not been stopped.
+        // See: https://github.com/dermotduffy/advanced-camera-card/issues/1967
         if (!this._stopped) {
-          // Only start the timer for the next step after this step returns, and
-          // only if this action has not been stopped.
-          // See: https://github.com/dermotduffy/advanced-camera-card/issues/1967
           this._timer.start(
             ptzConfiguration.r2c_delay_between_calls_seconds,
             singleStep,
@@ -119,11 +114,10 @@ export class PTZAction extends AdvancedCameraCardAction<PTZActionConfig> {
         }
       };
 
-      this._stopped = false;
       await singleStep();
     } else if (action.ptz_phase === 'stop') {
       // Scenario: Asked to stop continuous move, camera only supports relative moves natively.
-      await stopInProgressForThisTarget(ptzCameraID, this._context.ptz);
+      await clearInProgressForThisTarget(ptzCameraID, this._context, 'ptz');
     } else {
       this._stopped = false;
 

src/card-controller/actions/utils/action-state.ts

@@ -1,16 +1,9 @@
 import type { ActionContext } from 'action';
 import { merge } from 'lodash-es';
 
-import type { Action, TargetedActionContext } from '../types';
+import type { Action } from '../types';
 
-export const stopInProgressForThisTarget = async (
-  targetID: string,
-  context?: TargetedActionContext,
-): Promise<void> => {
-  await context?.[targetID]?.inProgressAction?.stop();
-};
-
-export const setInProgressForThisTarget = (
+const setInProgressForThisTarget = (
   targetID: string,
   context: ActionContext,
   contextKey: keyof ActionContext,
@@ -24,3 +17,28 @@ export const setInProgressForThisTarget = (
     },
   });
 };
+
+// `action` is registered before the stop is awaited, so an action that starts
+// for this target meanwhile sees `action`.
+export const replaceInProgressForThisTarget = async (
+  targetID: string,
+  context: ActionContext,
+  contextKey: keyof ActionContext,
+  action: Action,
+): Promise<void> => {
+  const replaced = context[contextKey]?.[targetID]?.inProgressAction;
+  setInProgressForThisTarget(targetID, context, contextKey, action);
+  await replaced?.stop();
+};
+
+// The removal is made before the stop is awaited, so an action that registers
+// for this target meanwhile is left in place.
+export const clearInProgressForThisTarget = async (
+  targetID: string,
+  context: ActionContext,
+  contextKey: keyof ActionContext,
+): Promise<void> => {
+  const stopped = context[contextKey]?.[targetID]?.inProgressAction;
+  delete context[contextKey]?.[targetID];
+  await stopped?.stop();
+};

src/card-controller/automations-manager.ts

@@ -3,6 +3,7 @@ import { createConditionEvaluator } from '../condition-trigger/conditions/factor
 import { TriggersManager } from '../condition-trigger/triggers/manager.js';
 import type { TriggerData } from '../condition-trigger/triggers/types.js';
 import type { Automation, AutomationActions } from '../config/schema/automations.js';
+import type { Trigger } from '../config/schema/condition-trigger/triggers/types.js';
 import { localize } from '../localize/localize.js';
 import type { CardAutomationsAPI, TaggedAutomation } from './types.js';
 
@@ -21,6 +22,10 @@ export class AutomationsManager {
     this._api = api;
   }
 
+  public getTriggers(): Trigger[] {
+    return [...this._automations.keys()].flatMap((automation) => automation.triggers);
+  }
+
   public deleteAutomations(tag?: unknown) {
     for (const [automation, triggers] of this._automations) {
       if (automation.tag === tag) {

src/card-controller/config/load-keyboard-shortcuts.ts

@@ -88,6 +88,15 @@ const convertKeyboardShortcutsToAutomations = (
           trigger: 'key' as const,
           key: shortcut.key,
           state: 'up',
+
+          // The same modifiers as the start above, so that the pair claims the
+          // same presses. A key is recorded with the modifiers it was pressed
+          // with, so a release still matches when a modifier is taken up while
+          // the key is held.
+          shift: shortcut.shift,
+          ctrl: shortcut.ctrl,
+          alt: shortcut.alt,
+          meta: shortcut.meta,
         },
       ],
       actions: [

src/card-controller/keyboard-state-manager.ts

@@ -1,8 +1,12 @@
 import { isEqual } from 'lodash-es';
 
+import { KeyConditionEvaluator } from '../condition-trigger/conditions/conditions/key';
+import type { Trigger } from '../config/schema/condition-trigger/triggers/types';
 import { isFocusWithin } from '../utils/focus';
 import type { CardKeyboardStateAPI, KeysState } from './types';
 
+const KEY_STATES = ['down', 'up'] as const;
+
 export class KeyboardStateManager {
   private _api: CardKeyboardStateAPI;
   private _state: KeysState = {};
@@ -33,16 +37,20 @@ export class KeyboardStateManager {
       capture: true,
     });
 
-    // Clear state on disconnect. Without listeners the card cannot know
-    // whether a key was released while detached, and stale "down" state
-    // would suppress the next real keydown (e.g. PTZ stop shortcuts).
-    if (Object.keys(this._state).length) {
-      this._state = {};
-      this._processStateChange();
-    }
+    this._releaseHeldKeys();
   }
 
   private _handleKeydown = (ev: KeyboardEvent): void => {
+    if (this._isKeyEventOwnedElsewhere(ev)) {
+      return;
+    }
+
+    // If the card acts on this key, the browser must NOT act on it also (e.g.
+    // 'down' should pan the camera without also scrolling the dashboard).
+    if (this._isKeyEventClaimedByAnyTrigger(ev)) {
+      ev.preventDefault();
+    }
+
     const keyObj = {
       state: 'down' as const,
       ctrl: ev.ctrlKey,
@@ -57,6 +65,64 @@ export class KeyboardStateManager {
     }
   };
 
+  private _isKeyEventOwnedElsewhere(ev: KeyboardEvent): boolean {
+    // A key press belongs to something other than the card when:
+    return (
+      // ... something within the card has already answered it ...
+      ev.defaultPrevented ||
+      // ... a character is mid-composition, e.g. choosing a Japanese character
+      // from an input method's candidate list with the arrows ...
+      ev.isComposing ||
+      // ... or it landed on an element with keys of its own.
+      this._isKeyHandlingElement(ev)
+    );
+  }
+
+  private _isKeyEventClaimedByAnyTrigger(ev: KeyboardEvent): boolean {
+    return this._api
+      .getAutomationsManager()
+      .getTriggers()
+      .some((trigger) => this._isKeyEventClaimedByTrigger(ev, trigger));
+  }
+
+  private _isKeyHandlingElement(ev: KeyboardEvent): boolean {
+    const target = ev.composedPath()[0];
+
+    return (
+      target instanceof HTMLInputElement ||
+      target instanceof HTMLSelectElement ||
+      target instanceof HTMLTextAreaElement ||
+      (target instanceof HTMLElement && target.isContentEditable)
+    );
+  }
+
+  private _isKeyEventClaimedByTrigger(ev: KeyboardEvent, trigger: Trigger): boolean {
+    // A trigger with no key of its own (i.e. undefined `trigger.key` field)
+    // watches *every* key without "claiming" any, as the card would otherwise
+    // swallow every press.
+    if (trigger.trigger !== 'key' || trigger.enabled === false) {
+      return false;
+    }
+    const evaluator = new KeyConditionEvaluator(trigger);
+
+    // Must count both directions, since the browser acts on a key as it goes
+    // down and so a trigger that acts on the way up must claim it then too.
+    return KEY_STATES.some(
+      (state) =>
+        evaluator.evaluate({
+          keys: {
+            [ev.key]: {
+              state: state,
+              ctrl: ev.ctrlKey,
+              alt: ev.altKey,
+              meta: ev.metaKey,
+              shift: ev.shiftKey,
+            },
+          },
+        }).result,
+    );
+  }
+
   private _handleKeyup = (ev: KeyboardEvent): void => {
     if (ev.key in this._state && this._state[ev.key].state === 'down') {
       this._state[ev.key] = { ...this._state[ev.key], state: 'up' as const };
@@ -88,12 +154,27 @@ export class KeyboardStateManager {
       return;
     }
 
-    if (Object.keys(this._state).length) {
-      // State is emptied if the element loses focus.
-      this._state = {};
+    this._releaseHeldKeys();
+  };
+
+  // Report every held key as newly released. The card receives key events only
+  // while it has focus, so it may never see the key release itself without
+  // this, and a condition that matches a released key would thus never
+  // evaluate.
+  private _releaseHeldKeys(): void {
+    let released = false;
+
+    for (const [key, keyObj] of Object.entries(this._state)) {
+      if (keyObj.state === 'down') {
+        this._state[key] = { ...keyObj, state: 'up' as const };
+        released = true;
+      }
+    }
+
+    if (released) {
       this._processStateChange();
     }
-  };
+  }
 
   // Clone before passing to ConditionStateManager so that subsequent
   // in-place mutations to this._state don't affect the stored reference,

src/card-controller/types.ts

@@ -251,6 +251,7 @@ export interface CardInteractionAPI {
 }
 
 export interface CardKeyboardStateAPI {
+  getAutomationsManager(): AutomationsManager;
   getCardElementManager(): CardElementManager;
   getConditionStateManager(): ConditionStateManager;
   getConfigManager(): ConfigManager;

src/condition-trigger/triggers/triggers/key.ts

@@ -4,6 +4,9 @@ import type { TriggerOfType } from './types';
 
 export class KeyTrigger extends ConditionStateTriggerBase<TriggerOfType<'key'>> {
   protected _getValue(state: ConditionState): unknown {
-    return state.keys;
+    const key = this._trigger.key;
+
+    // Without a key the trigger is the any-change form, and watches every key.
+    return key === undefined ? state.keys : state.keys?.[key];
   }
 }

src/config/schema/view.ts

@@ -12,15 +12,19 @@ const keyboardShortcut = z.object({
 });
 export type KeyboardShortcut = z.infer<typeof keyboardShortcut>;
 
+// Only claim the keys if there are NO modifiers pressed, otherwise they are
+// allowed to fall through to browser handling.
+const UNMODIFIED = { ctrl: false, alt: false, meta: false };
+
 const keyboardShortcutsDefault = {
   enabled: true,
-  ptz_left: { key: 'ArrowLeft' },
-  ptz_right: { key: 'ArrowRight' },
-  ptz_up: { key: 'ArrowUp' },
-  ptz_down: { key: 'ArrowDown' },
-  ptz_zoom_in: { key: '+' },
-  ptz_zoom_out: { key: '-' },
-  ptz_home: { key: 'h' },
+  ptz_left: { key: 'ArrowLeft', ...UNMODIFIED },
+  ptz_right: { key: 'ArrowRight', ...UNMODIFIED },
+  ptz_up: { key: 'ArrowUp', ...UNMODIFIED },
+  ptz_down: { key: 'ArrowDown', ...UNMODIFIED },
+  ptz_zoom_in: { key: '+', ...UNMODIFIED },
+  ptz_zoom_out: { key: '-', ...UNMODIFIED },
+  ptz_home: { key: 'h', ...UNMODIFIED },
 };
 
 const keyboardShortcutsSchema = z.object({