From dd71536316ec819caeb418b8635d544ac80e58ad Mon Sep 17 00:00:00 2001 From: undergroundwires Date: Wed, 8 May 2024 15:24:12 +0200 Subject: [PATCH] Fix misaligned tooltip positions in modal dialogs This commit fixes a bug that causes tooltips to be slightly misaligned. Tooltip positioning was incorrect during modal transitions due to their initial movement, causing tooltips to align incorrectly at the start of the animation rather than the end. One way to solve this would be using `autoUpdate` from `floating-ui` with `animationFrame: true`. However, this recalculates positions tens of times per second, impacting performance. This is a monkey solution. This commit adopts a more efficient approach by updating tooltip positions only at the end of the transitions, which reduces calculations and conserves resources. Key changes: - Addd transition end event listener for updating tooltip positions. - Use throttling to eliminate excessive position recalculations. Other supporting changes: - Improve throttle function to support efficient recalculations of positions: - Add ability to optionally exclude the first execution (leading call). - Refactor to simplify it make it easier to follow and read. - Fix a bug where initial calls were incorrectly throttled if `dateNow()` returned `0`. - Introduce and use a global hook for efficient DOM event management. This greatily introduce safety, reuse and testability of event listening. --- src/application/Common/Timing/Throttle.ts | 146 ++++++++- .../bootstrapping/DependencyProvider.ts | 5 + .../Scripts/View/Cards/CardList.vue | 13 +- .../Node/UseKeyboardInteractionState.ts | 25 +- .../TreeView/UseTreeKeyboardNavigation.ts | 20 +- .../Hooks/UseAutoUnsubscribedEventListener.ts | 79 +++++ .../Modal/Hooks/UseEscapeKeyListener.ts | 19 +- .../Shared/Modal/ModalContainer.vue | 12 +- .../components/Shared/TooltipWrapper.vue | 32 +- src/presentation/injectionSymbols.ts | 8 +- ...bileSafariActivePseudoClassEnabler.spec.ts | 8 +- .../Modal/Hooks/UseEscapeKeyListener.spec.ts | 74 +++++ .../Node/UseKeyboardInteractionState.spec.ts | 63 ++++ .../UseAutoUnsubscribedEventListener.spec.ts | 67 ++++ .../Shared/Modal/ModalContainer.spec.ts | 29 ++ tests/shared/Spies/EventTargetSpy.ts | 70 +++++ tests/shared/Spies/WindowEventSpies.ts | 67 ---- .../Common/Timing/Throttle.spec.ts | 297 +++++++++++++++--- .../bootstrapping/DependencyProvider.spec.ts | 1 + .../Scripts/View/Cards/CardList.spec.ts | 2 + .../Node/UseKeyboardInteractionState.spec.ts | 141 +++------ .../UseAutoUnsubscribedEventListener.spec.ts | 172 ++++++++++ .../Modal/Hooks/UseEscapeKeyListener.spec.ts | 95 +++--- .../Shared/Modal/ModalContainer.spec.ts | 37 ++- tests/unit/shared/Stubs/TimerStub.ts | 3 +- .../unit/shared/Stubs/UseEventListenerStub.ts | 15 + 26 files changed, 1165 insertions(+), 335 deletions(-) create mode 100644 src/presentation/components/Shared/Hooks/UseAutoUnsubscribedEventListener.ts create mode 100644 tests/integration/presentation/components/Scripts/View/Tree/Shared/Modal/Hooks/UseEscapeKeyListener.spec.ts create mode 100644 tests/integration/presentation/components/Scripts/View/Tree/TreeView/Node/UseKeyboardInteractionState.spec.ts create mode 100644 tests/integration/presentation/components/Shared/Hooks/UseAutoUnsubscribedEventListener.spec.ts create mode 100644 tests/integration/presentation/components/Shared/Modal/ModalContainer.spec.ts create mode 100644 tests/shared/Spies/EventTargetSpy.ts delete mode 100644 tests/shared/Spies/WindowEventSpies.ts create mode 100644 tests/unit/presentation/components/Shared/Hooks/UseAutoUnsubscribedEventListener.spec.ts create mode 100644 tests/unit/shared/Stubs/UseEventListenerStub.ts diff --git a/src/application/Common/Timing/Throttle.ts b/src/application/Common/Timing/Throttle.ts index 8fc575c2..da3bb732 100644 --- a/src/application/Common/Timing/Throttle.ts +++ b/src/application/Common/Timing/Throttle.ts @@ -1,44 +1,156 @@ +/* eslint-disable max-classes-per-file */ import { PlatformTimer } from './PlatformTimer'; import type { Timer, TimeoutType } from './Timer'; export type CallbackType = (..._: readonly unknown[]) => void; +export interface ThrottleOptions { + /** Skip the immediate execution of the callback on the first invoke */ + readonly excludeLeadingCall: boolean; + readonly timer: Timer; +} + +const DefaultOptions: ThrottleOptions = { + excludeLeadingCall: false, + timer: PlatformTimer, +}; + export function throttle( callback: CallbackType, waitInMs: number, - timer: Timer = PlatformTimer, + options: Partial = DefaultOptions, ): CallbackType { - const throttler = new Throttler(timer, waitInMs, callback); + const defaultedOptions: ThrottleOptions = { + ...DefaultOptions, + ...options, + }; + const throttler = new Throttler(waitInMs, callback, defaultedOptions); return (...args: unknown[]) => throttler.invoke(...args); } class Throttler { - private queuedExecutionId: TimeoutType | undefined; + private lastExecutionTime: number | null = null; - private previouslyRun: number; + private executionScheduler: DelayedCallbackScheduler; constructor( - private readonly timer: Timer, private readonly waitInMs: number, private readonly callback: CallbackType, + private readonly options: ThrottleOptions, ) { if (!waitInMs) { throw new Error('missing delay'); } if (waitInMs < 0) { throw new Error('negative delay'); } + this.executionScheduler = new DelayedCallbackScheduler(options.timer); } public invoke(...args: unknown[]): void { - const now = this.timer.dateNow(); - if (this.queuedExecutionId !== undefined) { - this.timer.clearTimeout(this.queuedExecutionId); - this.queuedExecutionId = undefined; - } - if (!this.previouslyRun || (now - this.previouslyRun >= this.waitInMs)) { - this.callback(...args); - this.previouslyRun = now; - } else { - const nextCall = () => this.invoke(...args); - const nextCallDelayInMs = this.waitInMs - (now - this.previouslyRun); - this.queuedExecutionId = this.timer.setTimeout(nextCall, nextCallDelayInMs); + switch (true) { + case this.isLeadingCallWithinThrottlePeriod(): { + if (this.options.excludeLeadingCall) { + this.scheduleNext(args); + return; + } + this.executeNow(args); + return; + } + case this.isAlreadyScheduled(): { + this.updateNextScheduled(args); + return; + } + case !this.isThrottlePeriodPassed(): { + this.scheduleNext(args); + return; + } + default: + throw new Error('Throttle logical error: no conditions for execution or scheduling were met.'); } } + + private isLeadingCallWithinThrottlePeriod(): boolean { + return this.isThrottlePeriodPassed() + && !this.isAlreadyScheduled(); + } + + private isThrottlePeriodPassed(): boolean { + if (this.lastExecutionTime === null) { + return true; + } + const timeSinceLastExecution = this.options.timer.dateNow() - this.lastExecutionTime; + const isThrottleTimePassed = timeSinceLastExecution >= this.waitInMs; + return isThrottleTimePassed; + } + + private isAlreadyScheduled(): boolean { + return this.executionScheduler.getNext() !== null; + } + + private scheduleNext(args: unknown[]): void { + if (this.executionScheduler.getNext()) { + throw new Error('An execution is already scheduled.'); + } + this.executionScheduler.resetNext( + () => this.executeNow(args), + this.waitInMs, + ); + } + + private updateNextScheduled(args: unknown[]): void { + const nextScheduled = this.executionScheduler.getNext(); + if (!nextScheduled) { + throw new Error('A non-existent scheduled execution cannot be updated.'); + } + const nextDelay = nextScheduled.scheduledTime - this.dateNow(); + this.executionScheduler.resetNext( + () => this.executeNow(args), + nextDelay, + ); + } + + private executeNow(args: unknown[]): void { + this.callback(...args); + this.lastExecutionTime = this.dateNow(); + } + + private dateNow(): number { + return this.options.timer.dateNow(); + } +} + +interface ScheduledCallback { + readonly scheduleTimeoutId: TimeoutType; + readonly scheduledTime: number; +} + +class DelayedCallbackScheduler { + private scheduledCallback: ScheduledCallback | null = null; + + constructor( + private readonly timer: Timer, + ) { } + + public getNext(): ScheduledCallback | null { + return this.scheduledCallback; + } + + public resetNext( + callback: () => void, + delayInMs: number, + ) { + this.clear(); + this.scheduledCallback = { + scheduledTime: this.timer.dateNow() + delayInMs, + scheduleTimeoutId: this.timer.setTimeout(() => { + this.clear(); + callback(); + }, delayInMs), + }; + } + + private clear() { + if (this.scheduledCallback === null) { + return; + } + this.timer.clearTimeout(this.scheduledCallback.scheduleTimeoutId); + this.scheduledCallback = null; + } } diff --git a/src/presentation/bootstrapping/DependencyProvider.ts b/src/presentation/bootstrapping/DependencyProvider.ts index 1231e6c3..ab9056b9 100644 --- a/src/presentation/bootstrapping/DependencyProvider.ts +++ b/src/presentation/bootstrapping/DependencyProvider.ts @@ -16,6 +16,7 @@ import { useCodeRunner } from '@/presentation/components/Shared/Hooks/UseCodeRun import { CurrentEnvironment } from '@/infrastructure/RuntimeEnvironment/RuntimeEnvironmentFactory'; import { useDialog } from '@/presentation/components/Shared/Hooks/Dialog/UseDialog'; import { useScriptDiagnosticsCollector } from '@/presentation/components/Shared/Hooks/UseScriptDiagnosticsCollector'; +import { useAutoUnsubscribedEventListener } from '@/presentation/components/Shared/Hooks/UseAutoUnsubscribedEventListener'; export function provideDependencies( context: IApplicationContext, @@ -77,6 +78,10 @@ export function provideDependencies( InjectionKeys.useScriptDiagnosticsCollector, useScriptDiagnosticsCollector, ), + useAutoUnsubscribedEventListener: (di) => di.provide( + InjectionKeys.useAutoUnsubscribedEventListener, + useAutoUnsubscribedEventListener, + ), }; registerAll(Object.values(resolvers), api); } diff --git a/src/presentation/components/Scripts/View/Cards/CardList.vue b/src/presentation/components/Scripts/View/Cards/CardList.vue index 249bc021..dd831327 100644 --- a/src/presentation/components/Scripts/View/Cards/CardList.vue +++ b/src/presentation/components/Scripts/View/Cards/CardList.vue @@ -40,7 +40,7 @@