From d0274886e6e8e7d61f4a237f583eab2cde5cdac6 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Wed, 2 Sep 2026 14:15:05 +0200 Subject: [PATCH] fix(gallery): decide the overlay by pointer capability, not viewport width Codex review round 1 on #1272. Both findings are consequences of extending the Grid/Justified tap-to-reveal model to every layout: what those two layouts got away with, because they were the only ones using it, becomes wrong once Masonry, Mosaic and Timeline inherit it. The hover variants no longer hide behind `md:`. On a fine pointer under 768px `isTouchDevice` is false, so nothing reveals the overlay, and the `md:` prefix disabled the only hover variants there were -- the controls stayed `opacity-0 pointer-events-none` with no way to reach them. Grid and Justified already behaved that way, but Masonry, Mosaic and Timeline had unprefixed `group-hover:` and revealed at any width, so this was a regression for them. Width was never the real question: what the breakpoint was standing in for is that :hover latches on a touchscreen once a tile is tapped. So the variants are emitted for pointer devices only and withheld on touch, which says that directly. detectCoarsePointer no longer ORs the touch fallbacks over matchMedia. matchMedia describes the PRIMARY pointer; `ontouchstart` and `maxTouchPoints` only say a touchscreen exists somewhere, which is equally true of a touchscreen laptop or a docked tablet being driven by its mouse. OR-ing them classified those as touch-only, so an ordinary click merely revealed the overlay and opening a photo took two clicks. The fallbacks now stand in only where matchMedia is absent, which is what the comment already claimed. 3 more tests, all 3 failing before this commit. --- frontend/src/components/gallery/PhotoCard.tsx | 36 +++++++++++----- .../__tests__/PhotoCard.touchTargets.test.tsx | 42 ++++++++++++++++++- 2 files changed, 67 insertions(+), 11 deletions(-) diff --git a/frontend/src/components/gallery/PhotoCard.tsx b/frontend/src/components/gallery/PhotoCard.tsx index 9b53fb2b..24c16d5f 100644 --- a/frontend/src/components/gallery/PhotoCard.tsx +++ b/frontend/src/components/gallery/PhotoCard.tsx @@ -17,16 +17,21 @@ const COARSE_POINTER_QUERY = '(hover: none) and (pointer: coarse)'; * change here would land an extra render between the tile measurement in * useLayoutEffect and the image mount it gates, remounting each card once. * - * matchMedia is missing in some test environments and old embedded webviews, - * so the other two signals stand in for it. + * matchMedia is authoritative wherever it exists, because it describes the + * PRIMARY pointer. `ontouchstart` and `maxTouchPoints` only say a touchscreen + * is present somewhere, which is equally true of a touchscreen laptop being + * driven by its mouse -- OR-ing them in would classify that as touch-only and + * turn every ordinary click into a two-step reveal. They stand in only where + * matchMedia is absent (old embedded webviews, some test environments). */ function detectCoarsePointer(): boolean { if (typeof window === 'undefined') return false; + if (typeof window.matchMedia === 'function') { + return window.matchMedia(COARSE_POINTER_QUERY).matches; + } const hasNavigator = typeof navigator !== 'undefined'; - const fallback = ('ontouchstart' in window) + return ('ontouchstart' in window) || (hasNavigator && navigator.maxTouchPoints > 0); - if (typeof window.matchMedia !== 'function') return fallback; - return window.matchMedia(COARSE_POINTER_QUERY).matches || fallback; } export interface PhotoCardFeedbackOptions { @@ -241,11 +246,22 @@ export const PhotoCard: React.FC = ({ // so a tap in the middle of a tile silently downloaded or liked instead of // opening the photo. Every visibility toggle below therefore moves // pointer-events with it, in both the tap-to-reveal and the hover branch. - const revealed = (visible: boolean) => - (visible - ? 'opacity-100 md:opacity-100 pointer-events-auto md:pointer-events-auto' - : 'opacity-0 md:opacity-0 pointer-events-none md:pointer-events-none') - + ' md:group-hover:opacity-100 md:group-hover:pointer-events-auto'; + // + // The hover variants are emitted for pointer devices only, rather than being + // gated behind `md:`. Width is the wrong proxy for hover: a mouse user with + // a window under 768px got no overlay at all, and in Masonry, Mosaic and + // Timeline -- whose `group-hover:` used to be unprefixed -- that made + // download and like unreachable at any narrow width. Withholding the classes + // on touch is what the breakpoint was really for, since :hover latches on a + // touchscreen once a tile has been tapped. + const revealed = (visible: boolean) => { + const base = visible + ? 'opacity-100 pointer-events-auto' + : 'opacity-0 pointer-events-none'; + return isTouchDevice + ? base + : `${base} group-hover:opacity-100 group-hover:pointer-events-auto`; + }; const overlayClassName = `${overlayBaseClassName} ${revealed(overlayVisible)}`; diff --git a/frontend/src/components/gallery/__tests__/PhotoCard.touchTargets.test.tsx b/frontend/src/components/gallery/__tests__/PhotoCard.touchTargets.test.tsx index b199e717..b8f39517 100644 --- a/frontend/src/components/gallery/__tests__/PhotoCard.touchTargets.test.tsx +++ b/frontend/src/components/gallery/__tests__/PhotoCard.touchTargets.test.tsx @@ -168,6 +168,46 @@ describe('PhotoCard overlay hit-testing (#1263)', () => { fireEvent.click(container.querySelector('.tile')!); expect(onClick).toHaveBeenCalledTimes(1); - expect(tokens(overlayOf(container))).toContain('md:group-hover:pointer-events-auto'); + expect(tokens(overlayOf(container))).toContain('group-hover:pointer-events-auto'); + }); + + it('reaches the hover overlay below the md breakpoint', () => { + // Codex review round 1. The hover variants used to be `md:group-hover:*`, + // so a mouse user with a window under 768px saw no overlay at all — and in + // Masonry, Mosaic and Timeline, whose `group-hover:` was unprefixed before + // this branch, that made download and like unreachable. Viewport width is + // not what decides whether a device can hover. + stubTouchDevice(false); + const { container } = renderCard(); + const overlay = tokens(overlayOf(container)); + + expect(overlay).toContain('group-hover:opacity-100'); + expect(overlay).toContain('group-hover:pointer-events-auto'); + expect(overlay.some((c) => c.startsWith('md:'))).toBe(false); + }); + + it('withholds the hover variants on touch, where :hover latches after a tap', () => { + // The reason the breakpoint was there in the first place. Emitting + // `group-hover:` on a touchscreen leaves the overlay stuck open on the + // last tile tapped. + const { container } = renderCard(); + expect(tokens(overlayOf(container)).some((c) => c.startsWith('group-hover:'))).toBe(false); + }); + + it('treats a touchscreen laptop driven by a mouse as a pointer device', () => { + // Codex review round 1. matchMedia describes the PRIMARY pointer; + // maxTouchPoints only says a touchscreen exists. OR-ing them classified a + // hybrid device as touch-only, so an ordinary click merely revealed the + // overlay and opening a photo took two clicks — a regression for the three + // layouts that had no tap-to-reveal step before. + stubTouchDevice(false); + Object.defineProperty(navigator, 'maxTouchPoints', { configurable: true, value: 10 }); + (window as any).ontouchstart = null; + + const onClick = vi.fn(); + const { container } = renderCard({ onClick }); + + fireEvent.click(container.querySelector('.tile')!); + expect(onClick).toHaveBeenCalledTimes(1); }); });