Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/lightbox-pause-on-close.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@astryxdesign/core': patch
---

[fix] Closing a Lightbox stops a video that was playing. The viewer stays mounted, so the picture and its sound would otherwise continue after Close, Escape, or a tap outside. A viewer that is already closed does not start playback either. Opening it again plays the current video when `hasAutoPlay` is set. A video inside the caption stays stopped.

@SebTardif
151 changes: 151 additions & 0 deletions packages/core/src/Lightbox/Lightbox.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -620,6 +620,157 @@ describe('Lightbox', () => {
expect(pause).toHaveBeenCalledTimes(1);
});

it('pauses a playing video when the viewer closes', () => {
const items = [
{src: '/clip.mp4', alt: 'A clip', type: 'video' as const},
{src: '/b.jpg', alt: 'Image B'},
];
const view = (isOpen: boolean) => (
<Lightbox
isOpen={isOpen}
onOpenChange={() => {}}
media={items}
index={0}
/>
);
const {rerender} = render(view(true));
const video = document.querySelector('video')!;
// Started by someone: playing, with its sound. Closing keeps the
// element mounted inside the closed dialog.
const pause = vi.fn();
video.pause = pause;
Object.defineProperty(video, 'paused', {
configurable: true,
get: () => pause.mock.calls.length > 0,
});
rerender(view(false));
expect(document.querySelector('video')).toBe(video);
expect(pause).toHaveBeenCalledTimes(1);
});

it('pauses a single playing video when the viewer closes', () => {
const media = {src: '/clip.mp4', alt: 'A clip', type: 'video' as const};
const view = (isOpen: boolean) => (
<Lightbox isOpen={isOpen} onOpenChange={() => {}} media={media} />
);
const {rerender} = render(view(true));
const video = document.querySelector('video')!;
const pause = vi.fn();
video.pause = pause;
Object.defineProperty(video, 'paused', {
configurable: true,
get: () => pause.mock.calls.length > 0,
});
rerender(view(false));
expect(document.querySelector('video')).toBe(video);
expect(pause).toHaveBeenCalledTimes(1);
});

it('plays a single video again when the viewer reopens, with hasAutoPlay', () => {
const play = vi
.spyOn(HTMLMediaElement.prototype, 'play')
.mockImplementation(async () => {});
try {
const media = {src: '/clip.mp4', alt: 'A clip', type: 'video' as const};
const view = (isOpen: boolean) => (
<Lightbox
isOpen={isOpen}
onOpenChange={() => {}}
media={media}
hasAutoPlay
/>
);
const {rerender} = render(view(true));
expect(play).toHaveBeenCalledTimes(1);
const video = document.querySelector('video')!;
const pause = vi.fn();
video.pause = pause;
Object.defineProperty(video, 'paused', {
configurable: true,
get: () => pause.mock.calls.length > 0,
});
rerender(view(false));
expect(pause).toHaveBeenCalledTimes(1);
rerender(view(true));
expect(play).toHaveBeenCalledTimes(2);
expect(play.mock.contexts.at(-1)).toBe(video);
} finally {
play.mockRestore();
}
});

it('does not start a video inside a single photo caption, with hasAutoPlay', () => {
const play = vi
.spyOn(HTMLMediaElement.prototype, 'play')
.mockImplementation(async () => {});
try {
render(
<Lightbox
isOpen={true}
onOpenChange={() => {}}
media={{
src: '/photo.jpg',
alt: 'Photo',
caption: <video src="/caption.mp4" />,
}}
hasAutoPlay
/>,
);
expect(document.querySelector('video')).not.toBeNull();
expect(play).not.toHaveBeenCalled();
} finally {
play.mockRestore();
}
});

it('does not start a video inside a gallery caption, with hasAutoPlay', () => {
const play = vi
.spyOn(HTMLMediaElement.prototype, 'play')
.mockImplementation(async () => {});
try {
render(
<Lightbox
isOpen={true}
onOpenChange={() => {}}
media={[
{
src: '/photo.jpg',
alt: 'Photo',
caption: <video src="/caption.mp4" />,
},
{src: '/b.jpg', alt: 'Image B'},
]}
hasAutoPlay
/>,
);
const captionVideo = document.querySelector(
'[data-lightbox-caption] video',
);
expect(captionVideo).not.toBeNull();
expect(play).not.toHaveBeenCalled();
} finally {
play.mockRestore();
}
});

it('withholds autoplay while the viewer is mounted closed', () => {
const media = {src: '/clip.mp4', alt: 'A clip', type: 'video' as const};
const view = (isOpen: boolean) => (
<Lightbox
isOpen={isOpen}
onOpenChange={() => {}}
media={media}
hasAutoPlay
/>
);
const {rerender} = render(view(false));
// The element stays mounted. The attribute would start playback once
// the file loads, after the close effect has already run.
expect(document.querySelector('video')).not.toHaveAttribute('autoplay');
rerender(view(true));
expect(document.querySelector('video')).toHaveAttribute('autoplay');
});

it("keeps a paged-away video's element and time once the viewer pages back", () => {
const items = [
{src: '/clip.mp4', alt: 'A clip', type: 'video' as const},
Expand Down
27 changes: 22 additions & 5 deletions packages/core/src/Lightbox/Lightbox.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -632,26 +632,43 @@ export function Lightbox({
// A video left on another page stops: the pager keeps the neighbours
// mounted, and a playing one would go on, with its sound, off screen. With
// `hasAutoPlay`, the video paged to plays again, as a newly shown one does.
// The closed dialog keeps them mounted too, so closing stops a playing
// 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.
Comment on lines +636 to +637

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

useEffect(() => {
const container = containerRef.current;
if (container == null) {
return;
}
if (!isOpen) {
for (const video of container.querySelectorAll<HTMLVideoElement>(
'video',
)) {
if (!video.paused) {
video.pause();
}
}
return;
}
Comment thread
astracat-bot[bot] marked this conversation as resolved.
Resolved
for (const video of container.querySelectorAll<HTMLVideoElement>(
'[data-lightbox-page]:not([data-current]) video',
)) {
if (!video.paused) {
video.pause();
}
}
const current = container.querySelector<HTMLVideoElement>(
'[data-lightbox-page][data-current] video',
);
const onAPage = container.querySelector('[data-lightbox-page]') != null;
const current = onAPage
? container.querySelector<HTMLVideoElement>(
'[data-lightbox-page][data-current] [data-lightbox-page-media] video',
)
: (imageWrapperRef.current?.querySelector<HTMLVideoElement>('video') ??
null);
if (hasAutoPlay && current != null && current.paused) {
// A browser can refuse playback without a user's gesture.
void current.play()?.catch(() => {});
}
}, [index, hasAutoPlay]);
}, [index, hasAutoPlay, isOpen]);

// The backdrop, the controls and the pages a dismissal faded, to restore
// exactly those. A page faded as a neighbour may be the current one when
Expand Down Expand Up @@ -1338,7 +1355,7 @@ export function Lightbox({
src={src}
aria-label={alt}
controls
autoPlay={isCurrent && hasAutoPlay}
autoPlay={isOpen && isCurrent && hasAutoPlay}
preload={isCurrent ? 'auto' : 'metadata'}
{...stylex.props(styles.video)}
/>
Expand Down
Loading