fix(wheel): improve cross-platform wheel intent and suppress text selection on drag#304
Conversation
…ection on drag Detect legacy mouse wheelDelta (±120) to prevent Windows mouse scroll from being misclassified as trackpad pan, extend modifier+PIXEL rule for Mac Cmd zoom, and add optional text selection suppression during camera/block drags. Includes WheelIntentProbe Storybook for live wheel event debugging. Co-authored-by: Cursor <cursoragent@cursor.com>
Reviewer's GuideRefines wheel intent classification to better distinguish trackpad vs mouse behavior across platforms, adds modifier-based pinch detection fixes for Mac/Windows, introduces optional suppression of text selection during drag, and provides a new Storybook WheelIntentProbe dev story with logging utilities and docs updates. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Preview is ready. |
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In dragListener, text selection suppression is installed immediately when the listener is created and only removed in cleanup; consider deferring installTextSelectionSuppression until the drag actually starts to avoid globally blocking selection when a drag is never initiated.
- hasLegacyMouseWheelDelta is recomputed multiple times (in isClassicMouseWheelStep, isIntegerPixelTrackpadScroll, and buildWheelSignals); you could cache this flag in TWheelContext to avoid repeated property access and to keep all heuristics using a single computed value.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In dragListener, text selection suppression is installed immediately when the listener is created and only removed in cleanup; consider deferring installTextSelectionSuppression until the drag actually starts to avoid globally blocking selection when a drag is never initiated.
- hasLegacyMouseWheelDelta is recomputed multiple times (in isClassicMouseWheelStep, isIntegerPixelTrackpadScroll, and buildWheelSignals); you could cache this flag in TWheelContext to avoid repeated property access and to keep all heuristics using a single computed value.
## Individual Comments
### Comment 1
<location path="docs/system/wheel-intent.md" line_range="53" />
<code_context>
+| `deltaMode === 0` + **small integer** `deltaX`/`deltaY` (< 20 px) | **Trackpad** | **Pan** (I3) |
+| `deltaMode === 0` + **large integer** in rapid stream (< 38 ms) | **Trackpad** fast scroll | **Pan** (I3) |
+| `deltaMode === 0` + **isolated large integer** (≥ 20 px, not rapid) | **Mouse** (Chromium) | **I4** per `MOUSE_WHEEL_BEHAVIOR` |
+| `deltaMode === 0` + **legacy wheelDelta ≈ ±120** | **Mouse** (Chromium/Edge) | **I4** (never I3, even in rapid stream) |
| `deltaMode === 0` + **fractional** deltas | **Mouse** smooth-scroll | **I4** per `MOUSE_WHEEL_BEHAVIOR` |
</code_context>
<issue_to_address>
**nitpick (typo):** Consider changing “even in rapid stream” to “even in a rapid stream” for smoother grammar.
The current wording is slightly awkward; adding “a” makes it read more naturally without changing the meaning.
```suggestion
| `deltaMode === 0` + **legacy wheelDelta ≈ ±120** | **Mouse** (Chromium/Edge) | **I4** (never I3, even in a rapid stream) |
```
</issue_to_address>
### Comment 2
<location path="docs/system/wheel-intent.md" line_range="264" />
<code_context>
Debug hooks are explicit opt-in. Each wheel event logs two plain-text lines: a one-line summary and a `JSON.stringify` payload you can copy from the console.
+**Storybook dev stand:** run `npm run storybook` → **Dev / WheelIntentProbe** — live table of raw `WheelEvent` fields plus resolver rule/signals; copy JSON after reproducing on your OS/device.
+
See also [Camera](./camera.md) for `MOUSE_WHEEL_BEHAVIOR` and camera constants.
</code_context>
<issue_to_address>
**suggestion (typo):** “Storybook dev stand” is an unusual phrase and may be a typo or unclear wording.
"Dev stand" isn’t a standard term in this context and may confuse readers. Consider a clearer alternative like "Storybook dev panel" or "dev sandbox," or another phrase that better matches what this view is for.
```suggestion
**Storybook dev panel:** run `npm run storybook` → **Dev / WheelIntentProbe** — live table of raw `WheelEvent` fields plus resolver rule/signals; copy JSON after reproducing on your OS/device.
```
</issue_to_address>
### Comment 3
<location path="src/utils/functions/wheelIntent.ts" line_range="190" />
<code_context>
return delta;
}
+/** Minimum |wheelDelta| / |wheelDeltaY| for legacy mouse-wheel detection (Chromium ≈ 120). */
+const LEGACY_MOUSE_WHEEL_DELTA_MIN = 100;
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider centralizing wheel device detection into a single classifier and using it to drive simpler predicates and resolver branches instead of scattering legacy and stream checks across multiple helpers and conditions.
You’ve added useful heuristics, but they’re now spread across multiple layers and intertwined with stream-speed and legacy detection. You can keep all behavior while reducing complexity by centralizing “device kind” and simplifying predicates.
### 1. Centralize legacy + stream heuristics into a single classifier
Instead of:
- `hasLegacyMouseWheelDelta` used inside `isClassicMouseWheelStep`
- `hasLegacyMouseWheelDelta` used inside `isIntegerPixelTrackpadScroll`
- `signals.hasLegacyMouseWheelDelta` used in the main resolver
…create a single classifier that decides `"trackpad"` vs `"mouse"` (or similar) using `ctx` + `isRapidStream`, and then use that everywhere.
```ts
type TWheelDeviceKind = "trackpad" | "mouse";
function classifyWheelDevice(ctx: TWheelContext, isRapidStream: boolean): TWheelDeviceKind {
const { event, isPixelDeltaMode, hasFractionalDelta, absX, absY } = ctx;
if (!isPixelDeltaMode) {
return "mouse";
}
if (hasLegacyMouseWheelDelta(event)) {
return "mouse";
}
const peak = Math.max(absX, absY);
if (hasFractionalDelta) {
// fractional PIXEL = mouse inertia / wheel behavior
return "mouse";
}
// integer PIXEL logic:
if (peak < MOUSE_WHEEL_DISCRETE_MIN_PX) {
return "trackpad"; // small ticks
}
return isRapidStream ? "trackpad" : "mouse"; // large, isolated vs burst
}
```
Then expose this from your signals (either directly or via `ctx`):
```ts
function createWheelSignals(ctx: TWheelContext, isRapidStream: boolean): TWheelSignals {
const deviceKind = classifyWheelDevice(ctx, isRapidStream);
return {
// existing fields...
hasFractionalDelta: ctx.hasFractionalDelta,
isPixelDeltaMode: ctx.isPixelDeltaMode,
// optionally:
deviceKind,
};
}
```
### 2. Make `isIntegerPixelTrackpadScroll` geometric again
Once classification is centralized, `isIntegerPixelTrackpadScroll` can go back to being a simple geometric predicate, or disappear entirely in favor of `deviceKind === "trackpad"`.
For example, if you keep it:
```ts
function isIntegerPixelTrackpadScroll(ctx: TWheelContext): boolean {
return ctx.isPixelDeltaMode && !ctx.hasFractionalDelta;
}
```
And in the resolver:
```ts
const deviceKind = classifyWheelDevice(ctx, isRapidStream);
if (signals.isPinchZoom) {
...
} else if (signals.isDiagonalScroll || signals.isPredominantHorizontalScroll) {
...
} else if (deviceKind === "trackpad" && isIntegerPixelTrackpadScroll(ctx)) {
intent = EWheelIntent.Pan;
rule = isRapidStream ? "I3:integer-trackpad" : "I3:integer-trackpad-slow";
} else if (deviceKind === "mouse" && (signals.isDominantAxisLargeWheel || signals.isClassicMouseWheelStep)) {
intent = intentFromMouseWheelBehavior(mouseWheelBehavior);
rule = signals.isClassicMouseWheelStep ? "I4:mouse-wheel-step" : "I4:large-step";
markMouseWheelBurst(now);
} else if (isTrackpadLikeRapidSmall(ctx, isRapidStream)) {
...
}
```
This:
- Keeps all your “small vs large integer PIXEL + rapid stream + legacy” behavior,
- But moves the logic into a single `classifyWheelDevice` function instead of scattering `hasLegacyMouseWheelDelta` and stream checks across three helpers and the main branch.
### 3. Simplify `isClassicMouseWheelStep`
With a device classifier, you can remove the embedded legacy check from `isClassicMouseWheelStep` and keep it focused on shape:
```ts
function isClassicMouseWheelStep(ctx: TWheelContext): boolean {
const { event, absY, isPixelDeltaMode, hasFractionalDelta } = ctx;
if (Math.abs(event.deltaX) >= 0.5) return false;
if (event.deltaMode === WheelEvent.DOM_DELTA_LINE || event.deltaMode === WheelEvent.DOM_DELTA_PAGE) return true;
if (absY < MOUSE_WHEEL_DISCRETE_MIN_PX) return false;
if (isPixelDeltaMode && !hasFractionalDelta) return false;
return true;
}
```
The “integer PIXEL + legacy ≈ ±120 means mouse” rule is then enforced in `classifyWheelDevice` only, avoiding repeated `hasLegacyMouseWheelDelta` checks at multiple layers.
### 4. Clean up the I4 branch condition
The updated I4 branch:
```ts
} else if (
!signals.isDominantAxisLargeWheel ||
signals.isClassicMouseWheelStep ||
signals.hasLegacyMouseWheelDelta
) {
...
}
```
is logically hard to follow: the leading `!` combined with `||` makes it non-obvious when this path is taken, and mixes shape (`isDominantAxisLargeWheel` / `isClassicMouseWheelStep`) with legacy state again.
With a device classifier, this becomes both clearer and more robust:
```ts
} else if (deviceKind === "mouse" && (signals.isDominantAxisLargeWheel || signals.isClassicMouseWheelStep)) {
intent = intentFromMouseWheelBehavior(mouseWheelBehavior);
rule = signals.isClassicMouseWheelStep ? "I4:mouse-wheel-step" : "I4:large-step";
markMouseWheelBurst(now);
}
```
This keeps behavior but reduces the cognitive load: legacy detection, stream heuristics, and shape-based rules are clearly separated and each lives in a single, well-defined function.
</issue_to_address>
### Comment 4
<location path="src/stories/examples/wheelIntentProbe/WheelEventLogPanel.tsx" line_range="32" />
<code_context>
+ const [copyFeedback, setCopyFeedback] = useState<string | null>(null);
+ const jsonPreRef = useRef<HTMLPreElement>(null);
+ const hiddenCopyRef = useRef<HTMLTextAreaElement>(null);
+ const copyFeedbackTimerRef = useRef<number | null>(null);
+
+ useEffect(() => {
</code_context>
<issue_to_address>
**issue (complexity):** Consider centralizing clipboard logic in the shared helper and simplifying in-component copy/feedback handling to reduce local complexity.
You can reduce complexity around clipboard/selection and transient feedback without changing behavior by:
1. **Centralizing clipboard behavior in `copyTextToClipboard`**
2. **Dropping the component-level hidden `<textarea>` as a copy mechanism**
3. **Simplifying the “Copied” feedback timer**
### 1. Use `copyTextToClipboard` as the single copy primitive
Instead of maintaining `copyFromHiddenTextarea` and a separate hidden textarea, delegate all auto-copy attempts to the shared helper and only handle “success vs fallback” in the component:
```ts
// wheelEventCapture.ts
// adjust helper to expose success/failure
export function copyTextToClipboard(text: string): boolean {
// existing logic:
// - try navigator.clipboard if available
// - fallback to hidden textarea + execCommand
// return true/false based on success
}
```
```ts
// WheelEventLogPanel.tsx
const runCopy = useCallback(
(text: string, selectJsonFallback: boolean): void => {
// single sync attempt via helper, keeps user-gesture chain
const success = copyTextToClipboard(text);
if (success) {
showCopyFeedback("Copied to clipboard");
} else {
selectForManualCopy(selectJsonFallback);
}
},
[selectForManualCopy, showCopyFeedback],
);
```
Now the panel has no knowledge of textareas, `execCommand`, or `navigator.clipboard` – it only decides what to do on failure.
### 2. Simplify manual selection (no hidden textarea needed)
Since this tool is for Storybook/debug, selecting the JSON `<pre>` is sufficient for manual copy. You can drop `hiddenCopyRef` and only select the JSON:
```ts
const jsonPreRef = useRef<HTMLPreElement>(null);
const selectForManualCopy = useCallback(
(selectJsonFallback: boolean): void => {
if (selectJsonFallback && jsonPreRef.current) {
const range = document.createRange();
range.selectNodeContents(jsonPreRef.current);
const selection = window.getSelection();
selection?.removeAllRanges();
selection?.addRange(range);
}
showCopyFeedback("Auto-copy blocked — text selected, press Ctrl/Cmd+C");
},
[showCopyFeedback],
);
```
Remove:
```ts
const hiddenCopyRef = useRef<HTMLTextAreaElement>(null);
// and in JSX:
<textarea
ref={hiddenCopyRef}
className="wheel-probe-hidden-copy"
readOnly
tabIndex={-1}
aria-hidden="true"
/>
```
Along with the entire `copyFromHiddenTextarea` callback.
### 3. Simplify the copy feedback timer
You don’t need `copyFeedbackTimerRef` + explicit cleanup; one effect keyed by `copyFeedback` is enough:
```ts
const [copyFeedback, setCopyFeedback] = useState<string | null>(null);
const showCopyFeedback = useCallback((message: string): void => {
setCopyFeedback(message);
}, []);
useEffect(() => {
if (!copyFeedback) return;
const id = window.setTimeout(() => setCopyFeedback(null), 4000);
return () => window.clearTimeout(id);
}, [copyFeedback]);
```
This keeps the same UX (“Copied” or “Auto-copy blocked…” for 4s) while removing refs and manual cleanup.
---
Result: the panel only orchestrates UI state and manual selection; all clipboard intricacies are handled in one shared helper, which should make the Storybook code easier to follow and maintain.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return delta; | ||
| } | ||
|
|
||
| /** Minimum |wheelDelta| / |wheelDeltaY| for legacy mouse-wheel detection (Chromium ≈ 120). */ |
There was a problem hiding this comment.
issue (complexity): Consider centralizing wheel device detection into a single classifier and using it to drive simpler predicates and resolver branches instead of scattering legacy and stream checks across multiple helpers and conditions.
You’ve added useful heuristics, but they’re now spread across multiple layers and intertwined with stream-speed and legacy detection. You can keep all behavior while reducing complexity by centralizing “device kind” and simplifying predicates.
1. Centralize legacy + stream heuristics into a single classifier
Instead of:
hasLegacyMouseWheelDeltaused insideisClassicMouseWheelStephasLegacyMouseWheelDeltaused insideisIntegerPixelTrackpadScrollsignals.hasLegacyMouseWheelDeltaused in the main resolver
…create a single classifier that decides "trackpad" vs "mouse" (or similar) using ctx + isRapidStream, and then use that everywhere.
type TWheelDeviceKind = "trackpad" | "mouse";
function classifyWheelDevice(ctx: TWheelContext, isRapidStream: boolean): TWheelDeviceKind {
const { event, isPixelDeltaMode, hasFractionalDelta, absX, absY } = ctx;
if (!isPixelDeltaMode) {
return "mouse";
}
if (hasLegacyMouseWheelDelta(event)) {
return "mouse";
}
const peak = Math.max(absX, absY);
if (hasFractionalDelta) {
// fractional PIXEL = mouse inertia / wheel behavior
return "mouse";
}
// integer PIXEL logic:
if (peak < MOUSE_WHEEL_DISCRETE_MIN_PX) {
return "trackpad"; // small ticks
}
return isRapidStream ? "trackpad" : "mouse"; // large, isolated vs burst
}Then expose this from your signals (either directly or via ctx):
function createWheelSignals(ctx: TWheelContext, isRapidStream: boolean): TWheelSignals {
const deviceKind = classifyWheelDevice(ctx, isRapidStream);
return {
// existing fields...
hasFractionalDelta: ctx.hasFractionalDelta,
isPixelDeltaMode: ctx.isPixelDeltaMode,
// optionally:
deviceKind,
};
}2. Make isIntegerPixelTrackpadScroll geometric again
Once classification is centralized, isIntegerPixelTrackpadScroll can go back to being a simple geometric predicate, or disappear entirely in favor of deviceKind === "trackpad".
For example, if you keep it:
function isIntegerPixelTrackpadScroll(ctx: TWheelContext): boolean {
return ctx.isPixelDeltaMode && !ctx.hasFractionalDelta;
}And in the resolver:
const deviceKind = classifyWheelDevice(ctx, isRapidStream);
if (signals.isPinchZoom) {
...
} else if (signals.isDiagonalScroll || signals.isPredominantHorizontalScroll) {
...
} else if (deviceKind === "trackpad" && isIntegerPixelTrackpadScroll(ctx)) {
intent = EWheelIntent.Pan;
rule = isRapidStream ? "I3:integer-trackpad" : "I3:integer-trackpad-slow";
} else if (deviceKind === "mouse" && (signals.isDominantAxisLargeWheel || signals.isClassicMouseWheelStep)) {
intent = intentFromMouseWheelBehavior(mouseWheelBehavior);
rule = signals.isClassicMouseWheelStep ? "I4:mouse-wheel-step" : "I4:large-step";
markMouseWheelBurst(now);
} else if (isTrackpadLikeRapidSmall(ctx, isRapidStream)) {
...
}This:
- Keeps all your “small vs large integer PIXEL + rapid stream + legacy” behavior,
- But moves the logic into a single
classifyWheelDevicefunction instead of scatteringhasLegacyMouseWheelDeltaand stream checks across three helpers and the main branch.
3. Simplify isClassicMouseWheelStep
With a device classifier, you can remove the embedded legacy check from isClassicMouseWheelStep and keep it focused on shape:
function isClassicMouseWheelStep(ctx: TWheelContext): boolean {
const { event, absY, isPixelDeltaMode, hasFractionalDelta } = ctx;
if (Math.abs(event.deltaX) >= 0.5) return false;
if (event.deltaMode === WheelEvent.DOM_DELTA_LINE || event.deltaMode === WheelEvent.DOM_DELTA_PAGE) return true;
if (absY < MOUSE_WHEEL_DISCRETE_MIN_PX) return false;
if (isPixelDeltaMode && !hasFractionalDelta) return false;
return true;
}The “integer PIXEL + legacy ≈ ±120 means mouse” rule is then enforced in classifyWheelDevice only, avoiding repeated hasLegacyMouseWheelDelta checks at multiple layers.
4. Clean up the I4 branch condition
The updated I4 branch:
} else if (
!signals.isDominantAxisLargeWheel ||
signals.isClassicMouseWheelStep ||
signals.hasLegacyMouseWheelDelta
) {
...
}is logically hard to follow: the leading ! combined with || makes it non-obvious when this path is taken, and mixes shape (isDominantAxisLargeWheel / isClassicMouseWheelStep) with legacy state again.
With a device classifier, this becomes both clearer and more robust:
} else if (deviceKind === "mouse" && (signals.isDominantAxisLargeWheel || signals.isClassicMouseWheelStep)) {
intent = intentFromMouseWheelBehavior(mouseWheelBehavior);
rule = signals.isClassicMouseWheelStep ? "I4:mouse-wheel-step" : "I4:large-step";
markMouseWheelBurst(now);
}This keeps behavior but reduces the cognitive load: legacy detection, stream heuristics, and shape-based rules are clearly separated and each lives in a single, well-defined function.
| const [copyFeedback, setCopyFeedback] = useState<string | null>(null); | ||
| const jsonPreRef = useRef<HTMLPreElement>(null); | ||
| const hiddenCopyRef = useRef<HTMLTextAreaElement>(null); | ||
| const copyFeedbackTimerRef = useRef<number | null>(null); |
There was a problem hiding this comment.
issue (complexity): Consider centralizing clipboard logic in the shared helper and simplifying in-component copy/feedback handling to reduce local complexity.
You can reduce complexity around clipboard/selection and transient feedback without changing behavior by:
- Centralizing clipboard behavior in
copyTextToClipboard - Dropping the component-level hidden
<textarea>as a copy mechanism - Simplifying the “Copied” feedback timer
1. Use copyTextToClipboard as the single copy primitive
Instead of maintaining copyFromHiddenTextarea and a separate hidden textarea, delegate all auto-copy attempts to the shared helper and only handle “success vs fallback” in the component:
// wheelEventCapture.ts
// adjust helper to expose success/failure
export function copyTextToClipboard(text: string): boolean {
// existing logic:
// - try navigator.clipboard if available
// - fallback to hidden textarea + execCommand
// return true/false based on success
}// WheelEventLogPanel.tsx
const runCopy = useCallback(
(text: string, selectJsonFallback: boolean): void => {
// single sync attempt via helper, keeps user-gesture chain
const success = copyTextToClipboard(text);
if (success) {
showCopyFeedback("Copied to clipboard");
} else {
selectForManualCopy(selectJsonFallback);
}
},
[selectForManualCopy, showCopyFeedback],
);Now the panel has no knowledge of textareas, execCommand, or navigator.clipboard – it only decides what to do on failure.
2. Simplify manual selection (no hidden textarea needed)
Since this tool is for Storybook/debug, selecting the JSON <pre> is sufficient for manual copy. You can drop hiddenCopyRef and only select the JSON:
const jsonPreRef = useRef<HTMLPreElement>(null);
const selectForManualCopy = useCallback(
(selectJsonFallback: boolean): void => {
if (selectJsonFallback && jsonPreRef.current) {
const range = document.createRange();
range.selectNodeContents(jsonPreRef.current);
const selection = window.getSelection();
selection?.removeAllRanges();
selection?.addRange(range);
}
showCopyFeedback("Auto-copy blocked — text selected, press Ctrl/Cmd+C");
},
[showCopyFeedback],
);Remove:
const hiddenCopyRef = useRef<HTMLTextAreaElement>(null);
// and in JSX:
<textarea
ref={hiddenCopyRef}
className="wheel-probe-hidden-copy"
readOnly
tabIndex={-1}
aria-hidden="true"
/>Along with the entire copyFromHiddenTextarea callback.
3. Simplify the copy feedback timer
You don’t need copyFeedbackTimerRef + explicit cleanup; one effect keyed by copyFeedback is enough:
const [copyFeedback, setCopyFeedback] = useState<string | null>(null);
const showCopyFeedback = useCallback((message: string): void => {
setCopyFeedback(message);
}, []);
useEffect(() => {
if (!copyFeedback) return;
const id = window.setTimeout(() => setCopyFeedback(null), 4000);
return () => window.clearTimeout(id);
}, [copyFeedback]);This keeps the same UX (“Copied” or “Auto-copy blocked…” for 4s) while removing refs and manual cleanup.
Result: the panel only orchestrates UI state and manual selection; all clipboard intricacies are handled in one shared helper, which should make the Storybook code easier to follow and maintain.
Fixes TypeScript build error that prevented e2e webServer from starting in CI. Co-authored-by: Cursor <cursoragent@cursor.com>
Install user-select/selectstart guards in startDrag instead of on mousedown so text in React blocks stays selectable before the drag threshold is exceeded. Also cache hasLegacyMouseWheelDelta in wheel context and tidy wheel-intent docs. Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
wheelDelta±120 mouse wheel no longer treated as trackpad pan during rapid scroll streams)suppressTextSelectionto drag operations (defaulttrue) to prevent text selection in React blocks while panning/dragging the cameraTest plan
npm test -- wheelIntent.test.ts— 27 tests passMOUSE_WHEEL_BEHAVIOR: "zoom"→ consistent zoomMOUSE_WHEEL_BEHAVIOR: "scroll"→ consistent panMade with Cursor
Summary by Sourcery
Refine wheel intent heuristics for better differentiation between trackpad and mouse wheel across platforms, introduce optional suppression of text selection during drag operations, and add a Storybook dev tool for capturing and inspecting wheel events.
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores: