Repository navigation
fix(Lightbox): stop a playing video when the viewer closes - #10692
Conversation
A mounted Lightbox keeps its video element after the dialog closes. The playback effect paused only a gallery neighbour, so Close, Escape, or a tap outside left the sound running. Closing now pauses every video. Opening again plays the current one when hasAutoPlay is set, including a single video that has no page. The new close cases failed before the effect watched isOpen. vitest run packages/core/src/Lightbox/Lightbox.test.tsx --project ui passed 91 tests after the fix.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Hi @SebTardif! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
github-actions
Bot
added
community
|
AI review status for this pull request.
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval waits on an engineering owner's decision, not on changes from you.
- The Lightbox spec covers pausing a gallery video on a neighbouring page while another item shows (FR18), but says nothing about what a closed viewer does with playback, so an engineering owner needs to confirm that closing should pause a playing video where it stopped (and that reopening with
hasAutoPlayplays it on).
The new close and reopen tests fail on main and pass here, and in Chromium playback continues behind the closed dialog on main and stops on this branch. CI has not run on this head yet.
Optional, non-blocking:
- The added comment sentence "Opening again plays the current one when
hasAutoPlayis set." restates the code below it and could go. - Not caused by this pull request: a Lightbox mounted closed with
hasAutoPlaystill plays its video behind the closed dialog; renderingautoPlay={isOpen && isCurrent && hasAutoPlay}would close that gap, since the effect already plays the current video on open.
[Automated review]
The closed dialog still mounts the video. hasAutoPlay set the autoPlay attribute even when isOpen was false, so a viewer mounted closed started playback once the file loaded. The attribute now follows isOpen. The close comment no longer restates the play check. The new attribute test failed before this change because the closed viewer rendered autoplay. The playback tests passed after it.
|
The two optional notes are in 43c7d2a. The close comment no longer restates the play check, and The owner decision is unchanged. FR18 covers a video on a neighbouring page. This pull request still needs an engineering owner to confirm that closing pauses playback. |
There was a problem hiding this comment.
Approval waits on an engineering owner's decision, not on changes from you.
- The Lightbox spec covers pausing a gallery video on a neighbouring page while another item shows (FR18), but says nothing about what a closed viewer does with playback, so an engineering owner needs to confirm that closing pauses a playing video where it stopped, that a viewer mounted closed with
hasAutoPlaywaits until it opens, and that reopening withhasAutoPlayplays the current video on.
Both earlier optional notes are addressed: the effect comment keeps only the reason, and autoPlay now follows isOpen. The four new tests fail on main and pass here, and in Chromium playback stops on close on this branch while it continues behind the closed dialog on main. CI has not run on this head yet; a maintainer adds the ci:run label so it runs.
[Automated review]
meta-cla
Bot
added
the
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
/approve-spec 43c7d2a |
cixzhang
added
ci:run
There was a problem hiding this comment.
Approved. Closing the Lightbox now pauses a playing video where it stopped, a viewer mounted closed no longer starts its video, and opening with hasAutoPlay plays the current video on; an engineering owner confirmed this behaviour for this head.
- The four new tests fail against
main's Lightbox and pass here, the existing gallery paging tests still pass, and required CI passed on this head. - Optional: with
hasAutoPlay, the single-item fallback (Lightbox.tsx:650-655) also plays a<video>inside a single image's caption, as the gallery selector already does on a page; scoping both selectors to the media element would avoid that.
[Automated review]
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| lab/Comments | 357.5 kB (+39 B) · 2.5 kB CSS (0) | — | — · — · — |
| Lightbox · Storybook | 125.8 kB (+32 B) · 10.6 kB CSS (0) | 3,418 (+14) | ✓ · ✓ · ✓ |
| lab/CommentThread | 350.7 kB (+30 B) · 2.4 kB CSS (0) | — | — · — · — |
Package size: @astryxdesign/core 7.32 MB packed (+275 B); no new dependencies.
Touched components checked; add ci:run-visual or ci:run-rtl to run the full suites.
Generated by PR Enrichment workflow | View full report
github-actions
Bot
removed
the
needs:code-review
|
/approve-spec 3deab00 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet, because its workflow runs are waiting for a maintainer to approve them.
Closing the Lightbox now pauses a playing video where it stopped, a viewer mounted closed no longer starts its video, and opening with hasAutoPlay plays the current video on; an engineering owner confirmed this behaviour.
- The four new tests fail against
main's Lightbox and pass here, and the existing gallery paging tests still pass. - Optional: with
hasAutoPlay, the single-item fallback (Lightbox.tsx:659-664) also plays a<video>inside a single photo's caption, as the gallery selector already does on a page; scoping both lookups to the media element would avoid that.
[Automated review]
hasAutoPlay played the first video in the dialog. A caption can contain one, and a single photo has no page element to tell them apart. Play only the media element. Closing still pauses every video in the viewer. The two caption cases failed before this change because play() ran once. vitest run packages/core/src/Lightbox/Lightbox.test.tsx --project ui passed 102 tests after the fix.
github-actions
Bot
added
the
needs:code-review
There was a problem hiding this comment.
Approved. Required CI has one failure this change does not cause: the WebKit paging proof the busy proof fails a slowed page settle that stays still 450 ms into the block was starved of frames on a photo gallery, and main has since fixed that test, so update the branch onto main and re-run CI before merging.
Closing the Lightbox now pauses a playing video where it stopped, a viewer mounted closed starts no video, and opening with hasAutoPlay plays the current video on from there; a <video> inside a caption is no longer autoplayed. An engineering owner confirmed the closed-viewer playback for this head, and the new tests fail before the change and pass with it.
One optional note: the new comment at Lightbox.tsx:636-637 uses an idiom ("stays put"). "Play starts only the media's video. A caption can hold its own video, and that video does not start." keeps only the reason.
[Automated review]
| // video. A single video has no page element. Play looks only in the media | ||
| // box: a caption can contain its own video, and that one stays put. |
There was a problem hiding this comment.
Non-blocking (comment style): "stays put" is an idiom, and "Play looks" gives the sentence a vague subject. A shorter version that keeps only the reason:
// video. A single video has no page element. Play starts only the media's
// video. A caption can hold its own video, and that video does not start.
|
/approve-spec 4914d79 |
github-actions
Bot
removed
the
needs:code-review
|
/approve-spec 2ddec11 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet, because its workflow runs are waiting for a maintainer to approve them (the ci:run label is already on).
Closing the Lightbox now pauses a playing video where it stopped, a closed viewer starts no video, and opening with hasAutoPlay plays the current one on; an engineering owner confirmed this behaviour for this head, and the new tests fail before the change and pass with it.
Optional: the comment at Lightbox.tsx:636-637 uses an idiom ("stays put"); "Play starts only the media's video. A caption can hold its own video, and that video does not start." keeps only the reason.
[Automated review]
|
/approve-spec 3b102bb |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet, because its workflow runs are waiting for a maintainer to approve them (the ci:run label is already on).
Closing the Lightbox now pauses a playing video where it stopped, a closed viewer starts no video, and opening with hasAutoPlay plays the current one on; an engineering owner confirmed this behaviour, and the new tests fail before the change and pass with it.
Optional: the comment at Lightbox.tsx:636-637 uses an idiom ("stays put"); "Play starts only the media's video. A caption can hold its own video, and that video does not start." keeps only the reason.
|
/approve-spec 890ebe8 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet, because its workflow runs are waiting for a maintainer to approve them (the ci:run label is already on).
Closing the Lightbox now pauses a playing video where it stopped, a closed viewer starts no video, and opening with hasAutoPlay plays the current one on; an engineering owner confirmed this behaviour, and the new tests fail before the change and pass with it.
Optional: the comment at Lightbox.tsx:636-637 uses an idiom ("stays put"); "Play starts only the media's video. A caption can hold its own video, and that video does not start." keeps only the reason.
User impact
Someone watching a video in Lightbox hears it keep playing after they close the viewer. Close, Escape, and a tap outside leave the component mounted. The picture is hidden and the sound continues.
Problem and solution fit
The playback effect pauses a gallery video only when that video is no longer the current page. Closing does not change the current page, and a single video is not on a page, so nothing calls
pause. The closed dialog keeps the element, and a hidden video keeps its audio. The effect now pauses every video while the viewer is closed, and plays the current one again on open whenhasAutoPlayis set.Expected behavior and authority
Gallery paging already stops a video that leaves the screen, and plays it again with
hasAutoPlay(Lightbox FR18). Closing hides that same mounted element. Autoplay still followshasAutoPlay.Smallest restoration
The existing playback effect now also depends on
isOpen. While closed it pauses every video and returns. While open it keeps the neighbour pause and plays the current video, including a single video that has no page element. No new prop and no visual change.Evidence
pnpm exec vitest run packages/core/src/Lightbox/Lightbox.test.tsx --project ui -t "pauses a playing video when the viewer closes"failed becausepausewas called 0 times. The video element was still in the document.hasAutoPlaystill plays that video.pauses a playing video when the viewer closes,pauses a single playing video when the viewer closes, andplays a single video again when the viewer reopens, with hasAutoPlayinLightbox.test.tsx.Scope
Testing
pnpm exec vitest run packages/core/src/Lightbox/Lightbox.test.tsx --project ui(91 passed). The new cases mockHTMLMediaElement.pauseandplayin jsdom, the same way the existing paging tests observe playback.The neighbour pause landed with the gallery pager in #8197. This covers the dismiss path.