Skip to content

Commit 658fd42

Browse files
committed
fix(ui): harden compact touch behavior
1 parent 05e606d commit 658fd42

9 files changed

Lines changed: 189 additions & 32 deletions

File tree

packages/editor/App.tsx

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -57,11 +57,8 @@ import {
5757
ScrollViewportProvider,
5858
} from '@plannotator/ui/hooks/useScrollViewport';
5959
import { useOverlayViewport } from '@plannotator/ui/hooks/useOverlayViewport';
60-
import { useIsMobile } from '@plannotator/ui/hooks/useIsMobile';
61-
import {
62-
hasCoarsePointer,
63-
useViewportEnvironment,
64-
} from '@plannotator/ui/hooks/useViewportEnvironment';
60+
import { useCompactTouchLayout, useIsMobile } from '@plannotator/ui/hooks/useIsMobile';
61+
import { useViewportEnvironment } from '@plannotator/ui/hooks/useViewportEnvironment';
6562
import {
6663
getPermissionModeSettings,
6764
needsPermissionModeSetup,
@@ -523,8 +520,7 @@ const App: React.FC = () => {
523520
});
524521
const [showLookAndFeelAnnouncement, setShowLookAndFeelAnnouncement] = useState(needsLookAndFeelAnnouncement);
525522
const isMobile = useIsMobile();
526-
const isMobilePageScrollWidth = useIsMobile(1025);
527-
const usesDocumentScroll = isMobilePageScrollWidth && hasCoarsePointer();
523+
const usesDocumentScroll = useCompactTouchLayout();
528524

529525
const viewerRef = useRef<ViewerHandle>(null);
530526
// Desktop uses the main document element as its native scroll viewport.
@@ -4255,6 +4251,7 @@ const App: React.FC = () => {
42554251
<div
42564252
data-print-region="root"
42574253
data-pn-browser-canvas={browserCanvas}
4254+
data-pn-compact-touch-layout={usesDocumentScroll ? 'true' : undefined}
42584255
data-pn-document-scroll={usesDocumentScroll ? 'true' : undefined}
42594256
className={`pn-app-viewport flex flex-col ${usesDocumentScroll ? 'overflow-visible' : 'overflow-hidden'} ${browserCanvas === 'card' ? 'bg-card' : 'bg-background'}`}
42604257
>

packages/review-editor/components/AnnotationToolbar.tsx

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import type { ConventionalLabel, ConventionalDecoration } from '@plannotator/ui/
1010
import type { AIChatEntry } from '../hooks/useAIChat';
1111
import { useDraggable } from '@plannotator/ui/hooks/useDraggable';
1212
import {
13-
hasCoarsePointer,
13+
hasPrimaryCoarsePointer,
1414
useVisibleViewportBounds,
1515
} from '@plannotator/ui/hooks/useViewportEnvironment';
1616

@@ -79,7 +79,7 @@ export const AnnotationToolbar: React.FC<AnnotationToolbarProps> = ({
7979
onViewAIResponse,
8080
aiHistoryMessages = [],
8181
}) => {
82-
const coarsePointer = hasCoarsePointer();
82+
const coarsePointer = hasPrimaryCoarsePointer();
8383
const visibleBounds = useVisibleViewportBounds(coarsePointer ? 16 : 0);
8484
const horizontalInset = coarsePointer ? Math.min(160, visibleBounds.width / 2) : 150;
8585
const suggestedCodeRef = useRef<HTMLTextAreaElement>(null);
@@ -206,7 +206,7 @@ export const AnnotationToolbar: React.FC<AnnotationToolbarProps> = ({
206206
value={commentText}
207207
onChange={(e) => setCommentText(e.target.value)}
208208
placeholder="Leave feedback..."
209-
className="w-full min-h-[4.5rem] max-h-[calc(var(--pn-viewport-height,100dvh)-16rem)] px-3 py-2 bg-muted rounded-lg text-xs leading-6 resize-y border-0 focus:outline-none focus:ring-1 focus:ring-primary/50 placeholder:text-muted-foreground"
209+
className="w-full min-h-[4.5rem] max-h-[calc(var(--pn-viewport-height,100vh)-16rem)] px-3 py-2 bg-muted rounded-lg text-xs leading-6 resize-y border-0 focus:outline-none focus:ring-1 focus:ring-primary/50 placeholder:text-muted-foreground"
210210
rows={3}
211211
autoFocus={!coarsePointer}
212212
onKeyDown={(e) => {

packages/review-editor/hooks/useAnnotationToolbar.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { useState, useCallback, useRef, useEffect } from 'react';
22
import { CodeAnnotation, SelectedLineRange, CodeAnnotationType, TokenAnnotationMeta, ConventionalLabel, ConventionalDecoration } from '@plannotator/ui/types';
33
import { useDismissOnOutsideAndEscape } from '@plannotator/ui/hooks/useDismissOnOutsideAndEscape';
44
import {
5-
hasCoarsePointer,
5+
hasPrimaryCoarsePointer,
66
shouldUseExpandedComposer,
77
useVisibleViewportBounds,
88
} from '@plannotator/ui/hooks/useViewportEnvironment';
@@ -62,7 +62,7 @@ export function useAnnotationToolbar({ patch, filePath, isFocused, onLineSelecti
6262
const visibleBounds = useVisibleViewportBounds(16);
6363
const expandedComposerRequired = shouldUseExpandedComposer({
6464
bounds: visibleBounds,
65-
coarsePointer: hasCoarsePointer(),
65+
coarsePointer: hasPrimaryCoarsePointer(),
6666
});
6767
const toolbarRef = useRef<HTMLDivElement>(null);
6868
const lastMousePosition = useRef<{ x: number; y: number }>({ x: 0, y: 0 });

packages/ui/components/CommentPopover.mobile.test.tsx

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,4 +153,51 @@ describe('CommentPopover mobile composition', () => {
153153
expect(document.querySelector<HTMLTextAreaElement>('[data-pn-mobile-editable]')?.value)
154154
.toBe('Keep this draft');
155155
});
156+
157+
test.skipIf(!hasDom)('keeps focus on a newly tapped outside target', async () => {
158+
// SAFETY: The test double implements the MediaQueryList surface consumed by the component.
159+
window.matchMedia = ((query: string): MediaQueryList => ({
160+
...coarseMatchMedia(query),
161+
matches: false,
162+
})) as typeof window.matchMedia;
163+
164+
function Harness() {
165+
const triggerRef = useRef<HTMLButtonElement>(null);
166+
const [open, setOpen] = useState(false);
167+
return (
168+
<>
169+
<button ref={triggerRef} onClick={() => setOpen(true)}>Open</button>
170+
<button>Next target</button>
171+
{open && (
172+
<CommentPopover
173+
anchorEl={triggerRef.current ?? undefined}
174+
anchorRect={new DOMRect(40, 40, 80, 24)}
175+
contextText="selected text"
176+
isGlobal={false}
177+
onSubmit={() => {}}
178+
onClose={() => setOpen(false)}
179+
/>
180+
)}
181+
</>
182+
);
183+
}
184+
185+
await mount(<Harness />);
186+
const buttons = Array.from(document.querySelectorAll<HTMLButtonElement>('button'));
187+
const trigger = buttons.find((button) => button.textContent === 'Open');
188+
const nextTarget = buttons.find((button) => button.textContent === 'Next target');
189+
expect(trigger).not.toBeUndefined();
190+
expect(nextTarget).not.toBeUndefined();
191+
192+
await act(async () => trigger?.click());
193+
await act(async () => {
194+
nextTarget?.dispatchEvent(new PointerEvent('pointerdown', { bubbles: true }));
195+
nextTarget?.focus();
196+
nextTarget?.dispatchEvent(new MouseEvent('click', { bubbles: true }));
197+
});
198+
await act(async () => new Promise<void>(resolve => requestAnimationFrame(() => resolve())));
199+
200+
expect(document.querySelector('[data-comment-popover]')).toBeNull();
201+
expect(document.activeElement).toBe(nextTarget);
202+
});
156203
});

packages/ui/components/CommentPopover.tsx

Lines changed: 19 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import { useSkillReferenceAutocomplete } from '../hooks/useSkillReferenceAutocom
1010
import { HumanOnlySkillNotice, SkillReferenceMenu } from './SkillReferenceMenu';
1111
import type { SkillReferenceToken } from '../utils/skillReferences';
1212
import {
13-
hasCoarsePointer,
13+
hasPrimaryCoarsePointer,
1414
shouldUseExpandedComposer,
1515
useVisibleViewportBounds,
1616
type VisibleViewportBounds,
@@ -165,7 +165,7 @@ export const CommentPopover: React.FC<CommentPopoverProps> = ({
165165
yieldState,
166166
}) => {
167167
const visibleBounds = useVisibleViewportBounds(16);
168-
const coarsePointer = hasCoarsePointer();
168+
const coarsePointer = hasPrimaryCoarsePointer();
169169
const prefersExpandedComposer = shouldUseExpandedComposer({
170170
bounds: visibleBounds,
171171
coarsePointer,
@@ -293,17 +293,20 @@ export const CommentPopover: React.FC<CommentPopoverProps> = ({
293293
});
294294
}, []);
295295

296-
const handleClose = useCallback(() => {
297-
if (draftKey) {
298-
if (hasUnsavedCommentContent(text, allowImages ? images : [])) {
299-
draftStore.set(draftKey, { text, images: allowImages ? images : [] });
300-
} else {
301-
draftStore.delete(draftKey);
296+
const handleClose = useCallback(
297+
(focusDisposition: 'restore-opener' | 'preserve-pointer-target' = 'restore-opener') => {
298+
if (draftKey) {
299+
if (hasUnsavedCommentContent(text, allowImages ? images : [])) {
300+
draftStore.set(draftKey, { text, images: allowImages ? images : [] });
301+
} else {
302+
draftStore.delete(draftKey);
303+
}
302304
}
303-
}
304-
onClose();
305-
restoreOpeningFocus();
306-
}, [allowImages, draftKey, images, onClose, restoreOpeningFocus, text]);
305+
onClose();
306+
if (focusDisposition === 'restore-opener') restoreOpeningFocus();
307+
},
308+
[allowImages, draftKey, images, onClose, restoreOpeningFocus, text],
309+
);
307310

308311
// Click-outside for popover mode
309312
useEffect(() => {
@@ -322,7 +325,7 @@ export const CommentPopover: React.FC<CommentPopoverProps> = ({
322325
// Preserve the existing draft so the following Shift-click can extend
323326
// it instead of silently replacing it with a new one.
324327
if (shiftSelectionActive && e.shiftKey) return;
325-
handleClose();
328+
handleClose('preserve-pointer-target');
326329
};
327330

328331
document.addEventListener('pointerdown', handlePointerDown, true);
@@ -512,7 +515,7 @@ export const CommentPopover: React.FC<CommentPopoverProps> = ({
512515
type="button"
513516
aria-label="Dismiss comment"
514517
className="absolute inset-0 bg-background/80 backdrop-blur-sm"
515-
onClick={handleClose}
518+
onClick={() => handleClose()}
516519
/>
517520

518521
{/* Dialog card */}
@@ -561,7 +564,7 @@ export const CommentPopover: React.FC<CommentPopoverProps> = ({
561564
</button>
562565
)}
563566
<button
564-
onClick={handleClose}
567+
onClick={() => handleClose()}
565568
className="p-1 rounded hover:bg-muted text-muted-foreground hover:text-foreground transition-colors"
566569
title="Close"
567570
>
@@ -706,7 +709,7 @@ export const CommentPopover: React.FC<CommentPopoverProps> = ({
706709
<ExpandIcon />
707710
</button>
708711
<button
709-
onClick={handleClose}
712+
onClick={() => handleClose()}
710713
className="p-1 rounded hover:bg-muted text-muted-foreground hover:text-foreground transition-colors"
711714
title="Close"
712715
>
Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
import { afterEach, describe, expect, test } from 'bun:test';
2+
import React, { act } from 'react';
3+
import { createRoot, type Root } from 'react-dom/client';
4+
import {
5+
COMPACT_TOUCH_LAYOUT_MEDIA_QUERY,
6+
useCompactTouchLayout,
7+
} from './useIsMobile';
8+
9+
const hasDom = typeof document !== 'undefined';
10+
let root: Root | null = null;
11+
let host: HTMLElement | null = null;
12+
let originalMatchMedia: typeof window.matchMedia | undefined;
13+
14+
function Harness() {
15+
const compact = useCompactTouchLayout();
16+
return <output data-compact={compact ? 'true' : 'false'} />;
17+
}
18+
19+
afterEach(async () => {
20+
if (root) await act(async () => root?.unmount());
21+
root = null;
22+
host?.remove();
23+
host = null;
24+
if (hasDom && originalMatchMedia) window.matchMedia = originalMatchMedia;
25+
originalMatchMedia = undefined;
26+
});
27+
28+
describe('useCompactTouchLayout', () => {
29+
test('uses compact width and the primary coarse pointer', () => {
30+
expect(COMPACT_TOUCH_LAYOUT_MEDIA_QUERY)
31+
.toBe('(max-width: 1024px) and (pointer: coarse)');
32+
expect(COMPACT_TOUCH_LAYOUT_MEDIA_QUERY).not.toContain('any-pointer');
33+
});
34+
35+
test.skipIf(!hasDom)('follows media changes and removes its listener', async () => {
36+
originalMatchMedia = window.matchMedia;
37+
let matches = false;
38+
const listeners = new Set<(event: MediaQueryListEvent) => void>();
39+
40+
// SAFETY: The test double implements the MediaQueryList surface consumed by the hook.
41+
window.matchMedia = ((query: string): MediaQueryList => ({
42+
matches,
43+
media: query,
44+
onchange: null,
45+
addEventListener: (_type, listener) => listeners.add(listener),
46+
removeEventListener: (_type, listener) => listeners.delete(listener),
47+
addListener: () => {},
48+
removeListener: () => {},
49+
dispatchEvent: () => true,
50+
})) as typeof window.matchMedia;
51+
52+
host = document.createElement('div');
53+
document.body.appendChild(host);
54+
root = createRoot(host);
55+
await act(async () => root?.render(<Harness />));
56+
57+
expect(document.querySelector('output')?.getAttribute('data-compact')).toBe('false');
58+
expect(listeners.size).toBe(1);
59+
60+
matches = true;
61+
await act(async () => {
62+
// SAFETY: The hook reads only `matches` from this synthetic media event.
63+
for (const listener of listeners) listener({ matches } as MediaQueryListEvent);
64+
});
65+
expect(document.querySelector('output')?.getAttribute('data-compact')).toBe('true');
66+
67+
await act(async () => root?.unmount());
68+
root = null;
69+
expect(listeners.size).toBe(0);
70+
});
71+
});

packages/ui/hooks/useIsMobile.ts

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,17 @@
11
import { useState, useEffect } from 'react';
22

3+
/** Maximum CSS viewport width that can enter Plannotator's compact touch shell. */
4+
export const COMPACT_TOUCH_LAYOUT_MAX_WIDTH = 1024;
5+
6+
/**
7+
* Canonical media query for the compact application shell.
8+
*
9+
* The primary pointer is intentional: `any-pointer: coarse` also matches
10+
* touchscreen laptops whose primary mouse or trackpad needs the desktop shell.
11+
*/
12+
export const COMPACT_TOUCH_LAYOUT_MEDIA_QUERY =
13+
`(max-width: ${COMPACT_TOUCH_LAYOUT_MAX_WIDTH}px) and (pointer: coarse)`;
14+
315
export function useIsMobile(breakpoint = 768): boolean {
416
const [isMobile, setIsMobile] = useState(
517
() => typeof window !== 'undefined' ? window.innerWidth < breakpoint : false
@@ -15,3 +27,28 @@ export function useIsMobile(breakpoint = 768): boolean {
1527

1628
return isMobile;
1729
}
30+
31+
/**
32+
* Reports whether the current viewport needs Plannotator's compact touch shell.
33+
* Plan and Code Review must share this decision so responsive chrome and scroll
34+
* ownership cannot diverge on hybrid devices.
35+
*/
36+
export function useCompactTouchLayout(): boolean {
37+
const [isCompactTouchLayout, setIsCompactTouchLayout] = useState(
38+
() => typeof window !== 'undefined'
39+
&& window.matchMedia(COMPACT_TOUCH_LAYOUT_MEDIA_QUERY).matches,
40+
);
41+
42+
useEffect(() => {
43+
const mediaQuery = window.matchMedia(COMPACT_TOUCH_LAYOUT_MEDIA_QUERY);
44+
const onChange = (event: MediaQueryListEvent) => {
45+
setIsCompactTouchLayout(event.matches);
46+
};
47+
48+
mediaQuery.addEventListener('change', onChange);
49+
setIsCompactTouchLayout(mediaQuery.matches);
50+
return () => mediaQuery.removeEventListener('change', onChange);
51+
}, []);
52+
53+
return isCompactTouchLayout;
54+
}

packages/ui/hooks/useViewportEnvironment.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -183,11 +183,11 @@ export function shouldUseExpandedComposer({
183183
return coarsePointer || bounds.width < 640 || bounds.height < 420;
184184
}
185185

186-
export function hasCoarsePointer(targetWindow?: Window): boolean {
186+
/** Returns whether the device's primary pointing input is coarse. */
187+
export function hasPrimaryCoarsePointer(targetWindow?: Window): boolean {
187188
const resolvedWindow = targetWindow ?? (typeof window === 'undefined' ? undefined : window);
188189
if (!resolvedWindow?.matchMedia) return false;
189-
return resolvedWindow.matchMedia('(any-pointer: coarse)').matches
190-
|| resolvedWindow.matchMedia('(pointer: coarse)').matches;
190+
return resolvedWindow.matchMedia('(pointer: coarse)').matches;
191191
}
192192

193193
function readViewportEnvironment(targetWindow: Window): ViewportEnvironment {

packages/ui/theme.css

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -745,6 +745,7 @@ body:has([data-pn-document-scroll="true"]) > #root {
745745

746746
.pn-app-viewport[data-pn-document-scroll="true"] {
747747
height: auto;
748+
min-height: 100vh;
748749
min-height: 100dvh;
749750
}
750751

@@ -790,7 +791,7 @@ body:has([data-pn-document-scroll="true"]) > #root {
790791
left: var(--pn-viewport-offset-left, 0px);
791792
box-sizing: border-box;
792793
width: var(--pn-viewport-width, 100vw);
793-
height: var(--pn-viewport-height, 100dvh);
794+
height: var(--pn-viewport-height, 100vh);
794795
padding-top: max(1rem, var(--pn-safe-top));
795796
padding-right: max(1rem, var(--pn-safe-right));
796797
padding-bottom: max(1rem, var(--pn-safe-bottom));
@@ -802,6 +803,7 @@ body:has([data-pn-document-scroll="true"]) > #root {
802803
* strand the footer below an ornamental desktop margin. */
803804
.pn-responsive-composer-dialog {
804805
width: 100%;
806+
height: min(36rem, 85vh);
805807
height: min(36rem, 85dvh);
806808
max-width: 42rem;
807809
max-height: 100%;

0 commit comments

Comments
 (0)