feat: modal video player - #359
Merged
Merged
Conversation
lstein
force-pushed
the
lstein/feature/video-player-modal
branch
from
August 16, 2026 19:59
e089533 to
7efd8c2
Compare
lstein
force-pushed
the
lstein/feature/video-badges
branch
from
August 17, 2026 00:33
683cf6b to
4aa8cf5
Compare
lstein
force-pushed
the
lstein/feature/video-player-modal
branch
from
August 17, 2026 00:33
786c215 to
19d77e8
Compare
lstein
force-pushed
the
lstein/feature/video-badges
branch
from
August 17, 2026 01:56
4aa8cf5 to
0c8ec09
Compare
lstein
force-pushed
the
lstein/feature/video-player-modal
branch
from
August 17, 2026 01:56
19d77e8 to
2ae86d8
Compare
lstein
force-pushed
the
lstein/feature/video-badges
branch
from
August 17, 2026 02:53
0c8ec09 to
2888a99
Compare
lstein
force-pushed
the
lstein/feature/video-player-modal
branch
from
August 17, 2026 02:53
2ae86d8 to
c6b1c34
Compare
lstein
force-pushed
the
lstein/feature/video-badges
branch
from
August 17, 2026 02:58
2888a99 to
ad612e5
Compare
lstein
force-pushed
the
lstein/feature/video-player-modal
branch
2 times, most recently
from
August 17, 2026 03:38
0685d6f to
6aded63
Compare
Consumes the videoPlayRequested event from the play badge and plays the video in a modal over a dimmed backdrop, reusing the shared .modal-overlay machinery with a much wider content box — the base .modal-content caps at 600px, which would letterbox a 1080p clip into a stamp. The modal owns its own <video> and never borrows one from a slide. Swiper destroys slide DOM nodes as the user navigates (trimShuffleBacklog, enforceHighWaterMark, resetAllSlides), and a detached <video> goes on playing audio. Lifecycle details that are each a real bug if missed: - On close the src is removed and load() called. Merely hiding the overlay leaves the browser streaming and the audio audible. - The slideshow's running state is snapshotted on open and *restored* on close, not force-started, so opening a video from a paused slideshow does not silently start it. - Swiper keyboard nav is disabled while open, and shouldIgnoreKeyEvent now suppresses global shortcuts, so Space pauses the video instead of being swallowed by handleSpacebarToggle's preventDefault, and the arrows scrub instead of changing slides. Escape is exempt — it dismisses the player, and now takes priority over hideMetadataOverlay. - Backspace dismisses the player rather than moving the slideshow underneath it; the modal is a transient overlay and pushes no history entry. - slideChanged and albumChanged close it, so the modal can never describe a different slide than the drawer and the UMAP marker. - handleFullscreenChange bails out while the player is open. <video controls> has its own fullscreen button, and leaving *that* fullscreen would otherwise read as "the app left fullscreen" and un-hide every panel behind the still-open modal. - touch.js no longer treats a tap on a button or the badge as a tap on the slide, which in fullscreen would both open the player and toggle the slideshow. Containers browsers cannot play get an explanation and a download link rather than a black rectangle. That is belt and braces: the static playable flag covers the common case, but the <video> error listener is the load-bearing half, since no extension list predicts an HEVC .mp4 (Safari yes, Firefox no) correctly in either direction. Tests: 22 new covering open/close, the badge-to-player seam, slideshow restore semantics, navigation teardown, both fallback paths, and a page with no modal markup. Frontend 501 passed, backend 576 passed, lint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The modal is opened from a click on the play badge, and window events dispatch synchronously, so the play() call still carries that gesture's transient user activation — which is what lets it start with sound rather than being refused by the browser's autoplay policy. A rejection is deliberately not treated as a playback failure and must not raise the error fallback: it means either the browser declined anyway (NotAllowedError under a stricter policy) or the load was torn down while still pending (AbortError, from closing the modal immediately). In both cases the native controls are right there, so the rejection is swallowed and logged at debug level. Left unhandled it would also surface as an unhandled promise rejection in the console on every quick close. Playback is only attempted when there is actually something to play — not on the no-URL path, and not for a container already declared unplayable, where the fallback is showing instead. The <video> also gains playsinline. Without it iOS Safari hijacks playback into its own fullscreen view the moment the video starts, which would drop the user out of the modal entirely — a problem that only appears once playback begins on its own. Tests: 6 new covering autoplay firing, both rejection paths, the two no-playback paths, and a browser with no play() at all. Frontend 510 passed, lint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lstein
force-pushed
the
lstein/feature/video-player-modal
branch
from
August 17, 2026 03:52
6aded63 to
3ae210e
Compare
Panel visibility desync (the reachable one). handleFullscreenChange suppressed itself while the player was open, to stop the video's own fullscreen button from un-hiding the panels behind the modal. Pressing Escape in fullscreen exits fullscreen without delivering the keydown to the page, so the app leaves fullscreen with the modal still open, the handler is skipped, and the control panel, search panel and score display keep .hidden-fullscreen (opacity:0 + visibility:hidden, both !important) after the modal closes. Nothing restores them short of toggling fullscreen twice. The gate turns out to be unnecessary as well as harmful: video fullscreen is entered and left in matched pairs, so the class ends where it started, and the modal backdrop covers the panels at z-index 99999 meanwhile. Both cases now verified by test; removing it also drops the control-panel -> video-player dependency edge. Space dismissed the player instead of pausing. shouldIgnoreKeyEvent was extended to hand Space to the native controls, and then the open path focused the close button — which activates on Space. Verified in Chromium: close button focused, Space fires its click; video focused, Space toggles playback. Focus now goes to the video, falling back to the close button when the fallback panel is showing (a hidden element cannot take focus). The template gains an explicit tabindex="0", a no-op in browsers (focus order and Space-to-pause unchanged, verified) that states the contract the open path relies on. Backspace guard was dead code. shouldIgnoreKeyEvent returns before KEYBOARD_SHORTCUTS is consulted, so the isVideoPlayerOpen() check inside handleBackKey could never run and Backspace silently did nothing while the player was open. Escape and Backspace are now both exempt. Mark the modal visible before starting playback, so a close arriving mid-open is not a silent no-op that leaves the clip playing, audible, behind a modal the user never saw. Tests: new video-player-integration.test.js covers the seams the module reaches into rather than the module alone — the fullscreen cases, and a parity check that the shipped template still provides every id the module queries. The existing suite builds its own fixture, which would pass unchanged if an id were renamed on one side only.
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
1 participant
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.
PR 5 of 8 in the video-support stack. Stacked on #358.
Consumes the
videoPlayRequestedevent from the play badge and plays the video in a modal over a dimmed backdrop, reusing the shared.modal-overlaymachinery with a much wider content box — the base.modal-contentcaps at 600px, which would letterbox a 1080p clip into a stamp.The modal owns its own
<video>and never borrows one from a slide. Swiper destroys slide DOM nodes as the user navigates (trimShuffleBacklog,enforceHighWaterMark,resetAllSlides), and a detached<video>goes on playing audio.Lifecycle — each row is a real bug if missed
pause(), removesrc,load(). Merely hiding the overlay leaves the browser streaming and audible.autoplay.running, pause, and restore on close — never force-start, so opening a video from a paused slideshow doesn't silently start itSpaceshouldIgnoreKeyEventnow suppresses globals while open, so Space pauses the video rather than being swallowed byhandleSpacebarToggle'spreventDefaultEscapehideMetadataOverlayBackspaceslideChanged/albumChangedhandleFullscreenChangebails while open —<video controls>has its own fullscreen button, and leaving that fullscreen would otherwise read as "the app left fullscreen" and un-hide every panel behind the still-open modaltouch.jsno longer treats a tap on a button or the badge as a tap on the slide, which in fullscreen would both open the player and toggle the slideshowFormats browsers cannot play
An explanation and a download link rather than a black rectangle. Belt and braces: the static
playableflag covers the common case, but the<video>errorlistener is the load-bearing half — no extension list predicts an HEVC.mp4(Safari yes, Firefox no) correctly in either direction, so the player always tries and reacts to what actually happened. The error fired by teardown clearingsrcis distinguished from a real failure.Tests: 22 new — open/close, the badge→player seam, slideshow restore semantics, navigation teardown, both fallback paths, error-recovery on reopen, and a page with no modal markup at all. Frontend 501 passed / 35 suites; backend 576 passed; eslint + prettier clean.
🤖 Generated with Claude Code