fix(speed): preserve native rate when extension has no authoritative target#1537
Open
rdavidwu wants to merge 1 commit into
Open
fix(speed): preserve native rate when extension has no authoritative target#1537rdavidwu wants to merge 1 commit into
rdavidwu wants to merge 1 commit into
Conversation
…target When lastSpeed is null and no per-site rule applies, lifecycle restore on play, seeked, or loadedmetadata silently overwrote the video's current playbackRate with the 1.0 baseline. This dropped any speed the user set via Chrome's native HTML5 controls (or that a site set pre-metadata-load) the moment the user paused, played, or scrubbed. Add an early-return guard at the top of mediaEventAction and initializeSpeed: when lastSpeed is null AND siteDefaultSpeed is unset, the extension has no opinion and must not act. This preserves the "null = no opinion" invariant established by 12a4fcd end-to-end. Three regression tests cover the play, seeked, and deferred loadedmetadata paths. The existing igrigorik#1494 regression and event-manager gesture tests are unaffected. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a bug where Chrome's native HTML5 playback-speed control gets silently undone when the extension is loaded.
Reproduction (current behavior):
<video>page with the extension installed (e.g., w3schools video sample) on a fresh browser session —lastSpeed=null, no per-site rule,rememberSpeed=off.Root cause: When
lastSpeed === nulland nositeDefaultSpeedrule applies,getTargetSpeed()returns the 1.0 baseline as a fallback. Lifecycle handlers (mediaEventActiononplay/seeked, andinitializeSpeed's deferredloadedmetadatahandler) then calladjustSpeed(video, 1.0, init), force-overwriting whatever rate the user set via native controls.Fix: Add an early-return guard at the top of
mediaEventActionandinitializeSpeed: when the extension has no authoritative target (lastSpeed null AND siteDefaultSpeed unset), do nothing. The video keeps whatever rate it has. This preserves thenull = no opinioninvariant established by 12a4fcd end-to-end.The fix targets the lifecycle restore path rather than trying to detect user gestures upstream — gesture-detection in
event-manager.jswould require a DOMclickto reachdocumentduring Chrome's native context-menu flow, which isn't guaranteed (clicks dispatch in browser chrome) and would also widen the trust boundary in ways that could regress 12a4fcd's protection against autonomous site rate changes (ad transitions, stream switches).Test Coverage
Three new tests in
tests/unit/core/video-controller.test.js:play event is no-op when lastSpeed is null and no per-site ruleseeked event is no-op when lastSpeed is null and no per-site ruleinitializeSpeed is no-op when lastSpeed is null and no per-site rule (deferred init path)Existing tests unchanged and still passing:
play event restore does not overwrite lastSpeed (#1494)— verified the guard does NOT fire whenlastSpeedis set, so the Regression: keyboard s/d shortcuts use 1.0x baseline after tab backgrounding #1494 background-tab restore behavior is preserved.tests/unit/utils/event-manager.test.js:351-441— user-gesture window tests forlastSpeed=1.5paths. Noevent-manager.jschange in this PR.Tests: 377 → 378 (+1 for the deferred-init regression). All 378 vitest tests pass.
Manual verification
npm run buildand loaddist/unpacked in Chrome.<video>page) withlastSpeed=nullandrememberSpeed=off.rememberSpeedon (lastSpeed previously set to 1.8): the existing Regression: keyboard s/d shortcuts use 1.0x baseline after tab backgrounding #1494 behavior is preserved — rate is restored to 1.8 on play even after a background-tab reset.Test plan
npx vitest run)npx eslint src/)lastSpeedis set🤖 Generated with Claude Code