Skip to content

fix: polish mobile history lightbox swipe UX #41

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Simplereally merged 1 commit into from
Mar 10, 2026
Merged

fix: polish mobile history lightbox swipe UX #41

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
139 changes: 137 additions & 2 deletions components/images/image-lightbox.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -864,7 +864,9 @@ describe("ImageLightbox - Mobile Swipe Navigation", () => {
clientY: 180,
});

expect(onNext).toHaveBeenCalledTimes(1);
return waitFor(() => {
expect(onNext).toHaveBeenCalledTimes(1);
});
});

it("navigates to the previous media on downward touch swipe", () => {
Expand Down Expand Up @@ -900,7 +902,140 @@ describe("ImageLightbox - Mobile Swipe Navigation", () => {
clientY: 320,
});

expect(onPrevious).toHaveBeenCalledTimes(1);
return waitFor(() => {
expect(onPrevious).toHaveBeenCalledTimes(1);
});
});

it("drags the media with the finger before release", () => {
render(
<ImageLightbox
image={mockImage}
isOpen={true}
onClose={vi.fn()}
mediaNavigation={{
hasNext: true,
hasPrevious: true,
onNext: vi.fn(),
onPrevious: vi.fn(),
}}
/>,
);

const swipeRegion = screen.getByTestId("lightbox-swipe-region");
const swipeMotion = screen.getByTestId("lightbox-swipe-motion");
const initialTransform = swipeMotion.style.transform;

fireEvent.pointerDown(swipeRegion, {
pointerId: 1,
pointerType: "touch",
isPrimary: true,
clientX: 120,
clientY: 180,
});
fireEvent.pointerMove(swipeRegion, {
pointerId: 1,
pointerType: "touch",
isPrimary: true,
clientX: 122,
clientY: 300,
});

expect(swipeMotion.style.transform).not.toBe(initialTransform);
expect(swipeMotion.style.transform).toContain("translate3d");
});

it("ignores mostly horizontal drags", async () => {
const onNext = vi.fn();
const onPrevious = vi.fn();

render(
<ImageLightbox
image={mockImage}
isOpen={true}
onClose={vi.fn()}
mediaNavigation={{
hasNext: true,
hasPrevious: true,
onNext,
onPrevious,
}}
/>,
);

const swipeRegion = screen.getByTestId("lightbox-swipe-region");
fireEvent.pointerDown(swipeRegion, {
pointerId: 1,
pointerType: "touch",
isPrimary: true,
clientX: 120,
clientY: 180,
});
fireEvent.pointerMove(swipeRegion, {
pointerId: 1,
pointerType: "touch",
isPrimary: true,
clientX: 280,
clientY: 210,
});
fireEvent.pointerUp(swipeRegion, {
pointerId: 1,
pointerType: "touch",
isPrimary: true,
clientX: 280,
clientY: 210,
});

await waitFor(() => {
expect(onNext).not.toHaveBeenCalled();
expect(onPrevious).not.toHaveBeenCalled();
});
});

it("snaps back when swiping beyond a gallery boundary", async () => {
render(
<ImageLightbox
image={mockImage}
isOpen={true}
onClose={vi.fn()}
mediaNavigation={{
hasNext: false,
hasPrevious: false,
onNext: vi.fn(),
onPrevious: vi.fn(),
}}
/>,
);

const swipeRegion = screen.getByTestId("lightbox-swipe-region");
const swipeMotion = screen.getByTestId("lightbox-swipe-motion");
const initialTransform = swipeMotion.style.transform;

fireEvent.pointerDown(swipeRegion, {
pointerId: 1,
pointerType: "touch",
isPrimary: true,
clientX: 120,
clientY: 240,
});
fireEvent.pointerMove(swipeRegion, {
pointerId: 1,
pointerType: "touch",
isPrimary: true,
clientX: 120,
clientY: 80,
});
fireEvent.pointerUp(swipeRegion, {
pointerId: 1,
pointerType: "touch",
isPrimary: true,
clientX: 120,
clientY: 80,
});

await waitFor(() => {
expect(swipeMotion.style.transform).toBe(initialTransform);
});
});

it("ignores short touch drags", () => {
Expand Down
96 changes: 61 additions & 35 deletions components/images/image-lightbox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,10 @@ export function ImageLightbox({

// Zoom state for single-image mode (updated via callback from LightboxMediaDisplay)
const [singleModeZoomed, setSingleModeZoomed] = React.useState(false);
const suppressBackdropClickRef = React.useRef(false);
const suppressBackdropClickUntilRef = React.useRef(0);
const suppressBackdropClick = React.useCallback(() => {
suppressBackdropClickUntilRef.current = Date.now() + 180;
}, []);
const resetEditSessionState = React.useCallback(() => {
setEditChain([]);
setSelectedVersionIndex(0);
Expand All @@ -138,13 +141,13 @@ export function ImageLightbox({

React.useEffect(() => {
// Reset local-only edit state whenever the opened image changes.
suppressBackdropClickRef.current = false;
suppressBackdropClickUntilRef.current = 0;
resetEditSessionState();
}, [imageSessionKey, resetEditSessionState]);

React.useEffect(() => {
if (!isOpen) {
suppressBackdropClickRef.current = false;
suppressBackdropClickUntilRef.current = 0;
resetEditSessionState();
}
}, [isOpen, resetEditSessionState]);
Expand Down Expand Up @@ -259,8 +262,7 @@ export function ImageLightbox({
}, [onClose]);

const handleBackdropCloseAttempt = React.useCallback(() => {
if (suppressBackdropClickRef.current) {
suppressBackdropClickRef.current = false;
if (Date.now() < suppressBackdropClickUntilRef.current) {
return;
}

Expand Down Expand Up @@ -422,23 +424,33 @@ export function ImageLightbox({

const swipeNavigationHandlers = useVerticalSwipeNavigation({
enabled: isSwipeNavigationEnabled,
itemKey: imageSessionKey ? String(imageSessionKey) : null,
onSwipeIntent: suppressBackdropClick,
onSwipeUp: mediaNavigation?.hasNext
? () => {
suppressBackdropClickRef.current = true;
suppressBackdropClick();
mediaNavigation.onNext();
}
: undefined,
onSwipeDown: mediaNavigation?.hasPrevious
? () => {
suppressBackdropClickRef.current = true;
suppressBackdropClick();
mediaNavigation.onPrevious();
}
: undefined,
});
const {
touchAction: swipeTouchAction,
overlayStyle: swipeOverlayStyle,
mediaStyle: swipeMediaStyle,
isDragging: isSwipeDragging,
isAnimating: isSwipeAnimating,
...swipeGestureHandlers
} = swipeNavigationHandlers;
const isSwipeInteractionActive = isSwipeDragging || isSwipeAnimating;
const lightboxBackdropStyle = isSwipeNavigationEnabled
? swipeOverlayStyle
: { backgroundColor: "rgba(0, 0, 0, 0.8)" };

return (
<>
Expand All @@ -456,7 +468,10 @@ export function ImageLightbox({
</VisuallyHidden>

{displayImage && activeImage && (
<div className="w-full h-full bg-black/80 backdrop-blur-md cursor-default flex items-center justify-center animate-in fade-in duration-150">
<div
className="w-full h-full backdrop-blur-md cursor-default flex items-center justify-center animate-in fade-in duration-150"
style={lightboxBackdropStyle}
>
<div className="relative w-full h-full">
{isVideo ? (
<div
Expand All @@ -465,26 +480,31 @@ export function ImageLightbox({
style={{ touchAction: swipeTouchAction }}
{...swipeGestureHandlers}
>
<div className="relative w-full h-full flex items-center justify-center p-4">
<button
type="button"
aria-label="Close lightbox"
className="absolute inset-0 cursor-default"
onClick={handleBackdropCloseAttempt}
onMouseEnter={() => setIsHovering(true)}
onMouseLeave={() => setIsHovering(false)}
onFocus={() => setIsHovering(true)}
onBlur={() => setIsHovering(false)}
/>
<button
type="button"
aria-label="Close lightbox"
className="absolute inset-0 cursor-default"
onClick={handleBackdropCloseAttempt}
onMouseEnter={() => setIsHovering(true)}
onMouseLeave={() => setIsHovering(false)}
onFocus={() => setIsHovering(true)}
onBlur={() => setIsHovering(false)}
/>
<div
className="relative w-full h-full flex items-center justify-center p-4"
style={swipeMediaStyle}
Comment on lines +493 to +495
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Mar 10, 2026

Choose a reason for hiding this comment

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

P1 Badge Keep video backdrop close button clickable

In video lightbox mode, the full-screen swipe-motion container now sits above the absolute close button, so taps on the backdrop hit the motion layer instead of the button. This effectively breaks tap-to-close for videos when swipe navigation is enabled, forcing users to rely on alternate dismissal paths.

Useful? React with 👍 / 👎.

data-testid="lightbox-swipe-motion"
>
<div className="relative shadow-[0_0_50px_rgba(0,0,0,0.5)] rounded-sm group/video z-10">
<MediaPlayer
key={displayImage.url}
url={displayImage.url}
alt={displayImage.prompt || "Generated video"}
contentType={displayImage.contentType}
controls={true}
autoPlay={true}
alt={displayImage.prompt || "Generated video"}
contentType={displayImage.contentType}
controls={true}
autoPlay={true}
loop={true}
muted={false}
muted={true}
className="w-auto h-auto max-w-full max-h-full object-contain select-none"
draggable={false}
/>
Expand Down Expand Up @@ -517,20 +537,26 @@ export function ImageLightbox({
style={{ touchAction: swipeTouchAction }}
{...swipeGestureHandlers}
>
<LightboxMediaDisplay
image={activeImage}
isOpen={isOpen}
isLoadingDetails={isLoadingDetails}
isGenerating={isGenerating}
onHoverChange={setIsHovering}
onBackdropClick={handleBackdropCloseAttempt}
onZoomChange={setSingleModeZoomed}
/>
<div
className="w-full h-full"
style={swipeMediaStyle}
data-testid="lightbox-swipe-motion"
>
<LightboxMediaDisplay
image={activeImage}
isOpen={isOpen}
isLoadingDetails={isLoadingDetails}
isGenerating={isGenerating}
onHoverChange={setIsHovering}
onBackdropClick={handleBackdropCloseAttempt}
onZoomChange={setSingleModeZoomed}
/>
</div>
</div>
)}

{/* Bottom hover zone to trigger overlay (only in single mode) */}
{!hasEdits && !isZoomed && (
{!hasEdits && !isZoomed && !isSwipeInteractionActive && (
<button
type="button"
aria-hidden="true"
Expand All @@ -548,7 +574,7 @@ export function ImageLightbox({
<LightboxInfoOverlay
image={activeImage}
isLoadingDetails={isLoadingDetails}
isVisible={!isZoomed && isHovering}
isVisible={!isZoomed && isHovering && !isSwipeInteractionActive}
onHoverChange={setIsHovering}
footer={
<LightboxVersionStrip
Expand Down
12 changes: 12 additions & 0 deletions components/ui/media-player.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ describe("MediaPlayer", () => {
render(<MediaPlayer url={videoUrl} autoPlay={true} contentType="video/mp4" />)
const video = screen.getByTestId("media-video")
expect(video).toHaveAttribute("preload", "auto")
expect(video).toHaveAttribute("autoplay")
})

it("uses preload=metadata when autoPlay is false", () => {
Expand Down Expand Up @@ -110,6 +111,17 @@ describe("MediaPlayer", () => {
expect(handleLoad).toHaveBeenCalled()
})

it("removes the loading spinner when canplay fires for video", () => {
render(<MediaPlayer url={videoUrl} contentType="video/mp4" />)
const video = screen.getByTestId("media-video")

expect(video.parentElement?.querySelector(".animate-spin")).toBeInTheDocument()

fireEvent(video, new Event("canplay"))

expect(video.parentElement?.querySelector(".animate-spin")).not.toBeInTheDocument()
})

it("calls onLoad when image loads", () => {
const handleLoad = vi.fn()
render(<MediaPlayer url={imageUrl} onLoad={handleLoad} />)
Expand Down
Loading