feat(pptx): add full-screen slideshow with keyboard controls - #197
Conversation
F5 or P enters a full-screen slideshow that moves the existing scale box into an overlay and hides inactive slide slots with CSS, so the engine's scoped slide styles keep applying and no nodes are cloned. A placeholder comment marks the original position so exiting restores the deck exactly. Includes the slideshow button in the demo renderer, the three new i18n keys across zh/en/ja, and a Playwright verification script.
wybaby168
left a comment
There was a problem hiding this comment.
Thanks for the substantial work here. The happy path is promising, and I verified that the current 20-slide demo can enter, navigate, and exit when a server is started manually. I can't merge this head yet because the review found several reproducible correctness issues:
-
The new public
@file-viewer/pptxAPI does not work with its default, documented configuration.slideCountonly countsslideRecords, but the non-windowed path appends slides without creating those records, soenterPresentation()silently returns. With the bundled 20-slide sample I observed20rendered.slidenodes,slideCount === 0, and no overlay afterenterPresentation(). -
Virtualization can remove the active slide during a presentation. After jumping to slide 20, a resize/window update can unmount that record because the windowing code still uses scroll-view geometry while the scale box is inside the fixed overlay. The active slot remains selected but has no child slide, producing a black presentation.
-
Each standard renderer instance installs its own document-level
F5/Plistener. With two PPTX viewers on one page, onePkeypress opens two overlays. Please scope the shortcut to a focused/active viewer or use one global arbiter. -
Native fullscreen ownership is detected incorrectly inside Shadow DOM. Chromium exposes the overlay through
shadowRoot.fullscreenElementwhiledocument.fullscreenElementis the host, soownsFullscreenstays false and a browser-native fullscreen exit leaves the fixed overlay open. -
Exiting after entering from a scrolled deck loses the original scroll position because moving the scale box collapses the scroller and clamps
scrollTopto zero. The placeholder restores DOM position, but not view position. -
When the exit button has focus, Enter/Space is intercepted by the document presentation handler and advances the slide instead of activating the button.
The regression gate also needs hardening: pnpm verify:pptx-slideshow assumes an external server at 127.0.0.1:8081, is not run by Public CI, and only asserts slot markers, so an active but empty slot can pass. Please make it self-host the built demo and cover default/non-windowed API use, active-slide content after resize, two instances, Shadow DOM native fullscreen exit, focusable controls, unmount/failure cleanup, and exact scroll/transform restoration.
Locally passed on this exact head: full pnpm type-check, PPTX and presentation builds, pnpm docs:build, pnpm test:i18n, pnpm verify:browser-smoke, and the existing slideshow script after manually starting the built demo. These failures are runtime coverage gaps rather than compile failures.
- slideCount now counts every slide the worker sends, so the default (non-windowed) API reports the deck size and enterPresentation() works without lazySlides. Non-windowed decks also get the active-slide toggle and hide rules, since they append slides directly instead of in slots. - The windowing code keeps the active slide and its neighbour mounted while presenting, so a resize cannot unmount the slide that is on screen. - A module-level arbiter makes the last-mounted/last-focused renderer shell the only one that answers F5/P, so one keypress opens one slideshow even with several viewers on a page. - Fullscreen ownership is detected through the shadow root as well as the document, so a browser-native fullscreen exit closes the overlay inside Shadow DOM. presentationFullscreen option lets hosts opt out. - The deck's scroll position is captured on enter and restored on exit after the scale box height is recomputed, and the original transform is kept. - Enter/Space on a focused control (the exit button) activates that control instead of advancing the slide. - verify-pptx-slideshow now self-hosts the built demo and covers the non-windowed API, active-slide content after resize, two instances, shadow fullscreen exit, focusable controls, unmount cleanup and exact scroll/transform restoration. Wired into Public CI.
|
Addressed all six findings plus the regression-gate hardening. Each fix has a dedicated check in
The verify script now self-hosts the built demo (no external server), asserts active-slide content rather than slot markers, and covers unmount/failure cleanup (check 15) plus the two-instance and shadow-fullscreen paths. |
- layout() measures the slide itself in non-windowed decks instead of the slide's first child, so the fit scale is computed from the right element. - If exit() runs while fullscreen is being requested, the browser is left out of fullscreen instead of stranded with no overlay. - With two slideshows open, a key pressed inside the other overlay belongs to that overlay; only the focused one advances. - verify-pptx-slideshow trims the MIME map to what the test serves, asserts the non-windowed slide is scaled, and covers the two-overlay focus case.
There was a problem hiding this comment.
Thanks — the original runtime blockers are fixed, and the browser gate passes. Two host-boundary blockers remain on the latest head:
P/F5opens the last-mounted viewer even when focus is on an unrelated host control, and prevents the host key event. Restrict shortcuts to an explicitly active/focused viewer and add an outside-focus regression.- The document
keydownlistener is installed beforePptxViewer.open()completes. If initialization rejects, no instance is returned, so the listener and shell remain. Clean up listeners, active state, and DOM on failure/abort, with a regression test.
Please also rebase onto current main before landing so the PPTX security fixes stay intact.
# Conflicts: # package.json # packages/renderers/pptx/src/viewer.ts
wybaby168
left a comment
There was a problem hiding this comment.
Maintainer fixes applied on the latest security baseline. Slideshow, sanitizer, build, type-check, unit, and browser smoke gates pass.
|
Merged after maintainer fixes and a clean main-branch CI run. Thanks for the contribution. |
Summary
F5orP.←/→navigate,Home/Endjump,Escexits; click zones navigate, and a "Start slideshow" button with a keyboard hint sits above the deck.presentation.slideshow.*) across zh/en/ja.Verification
pnpm type-checkpnpm docs:buildpnpm test:i18npnpm verify:pptx-slideshow- self-hosts the built demo and runs 16 checks:slideCountreports the deck,enterPresentation()opens, navigation and active-slide content workPopens exactly one overlaypnpm verify:browser-smokeNotes
pptxpresentationFullscreen: falselets hosts opt out of native fullscreen (the overlay staysposition:fixed).