Skip to content

fix: use desktop scroll events on macOS#863

Merged
thymikee merged 7 commits into
mainfrom
fix/macos-desktop-scroll-wheel
Jun 25, 2026
Merged

fix: use desktop scroll events on macOS#863
thymikee merged 7 commits into
mainfrom
fix/macos-desktop-scroll-wheel

Conversation

@thymikee

@thymikee thymikee commented Jun 24, 2026

Copy link
Copy Markdown
Member

Summary

Route macOS app-session scroll through a desktop XCTest scroll command instead of the Apple fused mobile drag path, and keep iOS/tvOS on their existing mobile/focus scroll behavior.
Add --duration-ms for scroll and thread pixels/duration through CLI, batch input, daemon/request context, runtime/provider contracts, and the macOS runner command. Non-macOS backends accept the shared duration option shape but ignore timing and no longer echo durationMs as honored.
Closes #856

Touched-file count: 37.

Validation

Focused contract and route coverage passed: pnpm exec vitest run --project unit src/core/__tests__/dispatch-scroll.test.ts src/commands/interaction/runtime/interactions.test.ts src/commands/batch/cli.test.ts src/platforms/ios/__tests__/index.test.ts; pnpm exec vitest run --project provider-integration test/integration/provider-scenarios/macos-desktop.test.ts.
Latest-head focused regression passed on 1981502: pnpm exec vitest run src/platforms/ios/__tests__/index.test.ts, covering iOS fused scroll and tvOS remote scroll with ignored durationMs omitted from results.
Required local gates passed on 1981502: pnpm build, pnpm check:quick, ./node_modules/.bin/fallow audit --base origin/main, direct full oxfmt --check, git diff --check. pnpm build:xcuitest passed earlier for the Swift runner changes; the latest commit only touched TypeScript Apple normalization/tests.
GitHub Actions on 1981502: Fallow Code Quality, Integration Tests, Unit Tests, Coverage, Lint & Format, Typecheck, Bundle Size, Web Platform Smoke, iOS Runner Swift Compatibility, CodeQL, and 3/4 Smoke Tests were green when CI watching was stopped; 1 Smoke Tests matrix job was still pending.
Manual native macOS verification passed earlier in System Settings, session issue-856-scroll-coordinate (closed). Before scroll down --pixels 200 --duration-ms 200, Accessibility showed top rows like Vision, VoiceOver, Zoom, Display, Motion. The command returned pixels: 200, durationMs: 200, x1: 1150.5, y1: 495.5; the next snapshot showed later rows including Captions, Live Captions, Name Recognition, and Motor. A batch with four scroll down steps at pixels: 200, durationMs: 50 executed 4/4 and advanced further to Motor, Voice Control, Keyboard, Pointer Control, Switch Control, Speech, and Siri.

Known residual risk: I did not verify a react-native-macos sample screen in this run; the requested live verification was against native System Settings. The PR remains draft until the final pending Smoke Tests job completes green.

@github-actions

github-actions Bot commented Jun 24, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 1.3 MB 1.3 MB -2.5 kB
JS gzip 430.3 kB 430.1 kB -253 B
npm tarball 566.5 kB 567.7 kB +1.1 kB
npm unpacked 1.9 MB 1.9 MB +5.7 kB

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 26.3 ms 26.7 ms +0.4 ms
CLI --help 45.9 ms 46.0 ms +0.1 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/find.js +8.8 kB +3.5 kB
dist/src/selector-runtime.js -2.5 kB -757 B
dist/src/2948.js -2.2 kB -293 B
dist/src/9722.js +581 B +184 B
dist/src/cli-help.js +416 B +119 B

@thymikee

Copy link
Copy Markdown
Member Author

Review: use desktop scroll events on macOS

Nice — the wiring is thorough (contract, traits, fixtures, runner-session allowlist, provider integration all updated), and I confirmed the coordinate space is right: resolvedTouchReferenceFrame returns screen-global, top-left-origin coordinates, which is exactly what CGEvent expects, so CGPoint(x: frame.midX, y: frame.midY) is correct (points, no Retina mismatch).

A few things to address:

1. Direction may invert with "natural scrolling" (please verify)

desktopScrollWheelDeltas hard-codes down → negative vertical, up → positive. Events posted to .cghidEventTap can be subject to the system's "natural scrolling" transform, which would flip that mapping on machines with the opposite setting. Your manual test passed on one machine, and the unit test only asserts the sign mapping — not the on-screen result. Please confirm scroll down actually moves content the same way with natural scrolling toggled on vs. off, and decide whether to normalize for it. This is the one thing that could behave inconsistently across environments.

2. The mouseMoved post relocates the user's cursor

You post a .mouseMoved event and set scroll.location = point. Since the scroll event carries its own location, is the separate mouseMoved needed for targeting — or is it there to establish hover state for apps that scroll the view under the pointer? If the latter, a one-line comment would help; if not, dropping it avoids physically yanking the user's cursor (a visible side effect with no restore).

3. Minor

  • device.target === 'desktop' in the runAppleScroll gate is always true for macOS (buildHostMacDevice hard-codes it; nothing else assigns a macOS target). Harmless/defensive, but platform === 'macos' alone is sufficient today.
  • A single large pixel delta (e.g. pixels=400 for amount 0.5) is one big wheel event vs. the many small deltas a real trackpad emits — some views won't momentum/animate, or may clamp it. Consider chunking if you find apps that ignore large single deltas.
  • pixels is computed twice — TS buildScrollGesturePlan (reported value) and Swift runnerScrollGesturePlan (actual travel). Consistent with the existing fused-scroll pattern; just keep the two plans in sync so the reported pixels match real travel.

Otherwise this is in good shape.

🤖 Automated review via Claude Code

@thymikee thymikee marked this pull request as draft June 24, 2026 16:16
@thymikee thymikee marked this pull request as ready for review June 24, 2026 16:31
@thymikee thymikee marked this pull request as draft June 24, 2026 16:34
@thymikee

Copy link
Copy Markdown
Member Author

Follow-up: deeper re-check

Traced two of the points above through the full code path; here's the refined read.

Natural-scroll direction — confirmed by mechanism, not just "maybe"

macOS inverts scrollingDeltaX/Y per the Scroll direction: Natural preference, and per the NSEvent docs that inversion is applied before the event reaches the app ("the setting is applied before the event is given to the app"). CGEventPost(.cghidEventTap) injects below that layer, so a synthesized scroll posted there is subject to the same inversion — meaning the hard-coded signs in desktopScrollWheelDeltas will flip on a machine whose setting differs from your test box, and Natural is the macOS default. Your manual validation only confirmed "content moved," not that down moved content down. Please test with the setting toggled both ways; if it flips, read the preference (or honor NSEvent.isDirectionInvertedFromDevice) and normalize. This is the one item I'd treat as blocking until verified — it's a silent per-machine correctness flip.

The provider integration test doesn't exercise the real reported coordinates

desktopScroll reports its touch point from frame.midX/midY, which are global screen coordinates — your own manual note shows x1=737.5. But the provider fixture mocks the runner result as { x: 200, y: 400, referenceWidth: 400, referenceHeight: 800 }, i.e. a point that sits inside a 400-wide frame. So the green test validates the TS normalization wiring but not the real reporting shape the runner emits. Worth a fixture whose x reflects the global window-center value so a future change to the frame math is actually caught.

Reported frame is hand-built instead of using the shared helper

Every other gesture builds its reported frame via resolvedTouchVisualizationFrame / resolvedDragVisualizationFrame. desktopScroll instead constructs TouchVisualizationFrame(x: frame.midX, y: frame.midY, referenceWidth: frame.width, …) directly. Mostly equivalent — except the helper applies app.frame.minX/minY as the origin while frame.midX is derived from the window frame (resolvedTouchReferenceFrame returns the window). On macOS where app.frame and the main window frame can differ (multi-window, or app.frame spanning the screen), those two origins diverge and the reported point drifts. Keep posting the CGEvent at the global frame.midX/midY (that part is correct), but consider building the reported frame via resolvedTouchVisualizationFrame(app: activeApp, x: frame.width / 2, y: frame.height / 2) so it stays identical to every other gesture. At minimum, confirm app.frame == window.frame for the single-window macOS case.

Correction to my earlier "pixels computed twice" note

That's already handled deliberately — RunnerTests+ScrollGesture.swift documents the TS/Swift duplication as a "two-place invariant" with parity vectors mirroring the vitest suite. The only gap: no vector trips the maxTravelPixels clamp (requestedPixels > axisLength − 2·edgePadding), which is exactly where reported-vs-actual travel could silently diverge. Adding one clamp vector to both languages closes it.

🤖 Automated review via Claude Code

@thymikee thymikee marked this pull request as ready for review June 24, 2026 16:58
@thymikee thymikee marked this pull request as draft June 24, 2026 17:09
@thymikee thymikee marked this pull request as ready for review June 24, 2026 17:22
@thymikee

Copy link
Copy Markdown
Member Author

Addressed the scroll review points in a11fc65:

  • macOS desktop scroll remains on XCUICoordinate.scroll(byDeltaX:deltaY:); no raw CGEventPost or mouseMoved cursor relocation is used. Added an inline comment documenting that choice.
  • Removed the redundant device.target === "desktop" gate for the macOS Apple path.
  • The runner response now uses resolvedTouchVisualizationFrame while preserving the actual event point at the resolved global window center.
  • Provider and runner-route tests now use global-looking center coordinates instead of local frame coordinates.
  • Added explicit pixels > safe band clamp parity vectors in both TS and Swift.

Local validation passed (pnpm build, pnpm build:xcuitest, pnpm check:quick, pnpm check:fallow --base origin/main, pnpm format, focused unit/provider tests), and GitHub Actions are green on a11fc65.

@thymikee thymikee marked this pull request as draft June 24, 2026 17:39
@thymikee

Copy link
Copy Markdown
Member Author

Second-pass review found two contract issues to handle before this comes out of draft:

  1. durationMs is echoed in successful scroll results even on platforms that ignore it. Since only macOS desktop scroll currently applies the timing, either reject/warn outside that path or only include durationMs when the backend actually honored it.
  2. Structured/batch input can bypass the CLI 10s cap. The schema/daemon dispatch currently only require a non-negative integer, while Swift sleeps for the requested duration. Please enforce the same 10000ms maximum for structured input too.

After patching, please reply with what changed and which focused route/batch validation proves the cap/reporting behavior.

@thymikee thymikee marked this pull request as ready for review June 24, 2026 18:09
@thymikee

Copy link
Copy Markdown
Member Author

Addressed the second-pass contract comments in 99e2f75:

  • durationMs is no longer generically echoed by direct dispatch/runtime scroll results. It is reported only when the backend result includes it, which is the macOS desktop path that actually honors timing.
  • Added a shared SCROLL_DURATION_MAX_MS cap of 10000 and enforce it in structured command metadata, runtime scroll input, daemon dispatch, the Apple adapter, and the Swift desktopScroll command.
  • Added focused regression coverage for ignored-duration reporting, honored-duration reporting, dispatch/runtime cap rejection, structured batch pre-projection rejection, and Apple adapter pre-runner rejection.

Validation passed locally: focused unit/provider tests, pnpm build, pnpm build:xcuitest, pnpm check:quick, pnpm check:fallow --base origin/main, pnpm format, and git diff --check. GitHub Actions are green on 99e2f75, and the PR is ready again.

@thymikee thymikee marked this pull request as draft June 24, 2026 18:44
@thymikee

Copy link
Copy Markdown
Member Author

Latest-head review found one remaining blocker in the duration reporting fix:

durationMs can still be reported on non-macOS Apple scroll even though it is not honored. In src/platforms/ios/interactions.ts, the iOS/fused-scroll path intentionally does not send durationMs to the runner, but then calls normalizeScrollResultWithResolvedFrame(..., options), and normalizeIosScrollResult echoes options.durationMs into the result. So scroll down --duration-ms 50 --platform ios can still return durationMs: 50 while timing was ignored.

Please move duration reporting behind the macOS desktop result only, and add an iOS/tvOS regression test that passes durationMs and proves it is not reported when ignored. Keep the PR draft until this is fixed and CI is green again.

@thymikee

Copy link
Copy Markdown
Member Author

Addressed the latest duration-reporting blocker in 1981502:

  • macOS desktop scroll is now the only Apple path that includes durationMs in normalized scroll results.
  • iOS fused scroll still accepts the shared option shape but does not send durationMs to the runner and no longer reports it as honored.
  • tvOS remote scroll also omits ignored durationMs from the result.
  • While touching the normalizer, simplified normalizeIosScrollResult to keep Fallow clean.

Focused validation: pnpm exec vitest run src/platforms/ios/__tests__/index.test.ts now includes iOS and tvOS regression cases passing durationMs: 50 and asserting it is absent from both ignored runner/reporting paths. Local gates also passed: pnpm check:quick, pnpm build, ./node_modules/.bin/fallow audit --base origin/main, direct full oxfmt --check, and git diff --check.

CI status when I stopped watching per request: all listed jobs green except one Smoke Tests matrix job still pending, so the PR remains draft until that last job completes green.

@thymikee thymikee marked this pull request as ready for review June 24, 2026 19:53
@thymikee thymikee merged commit ba825d8 into main Jun 25, 2026
20 checks passed
@thymikee thymikee deleted the fix/macos-desktop-scroll-wheel branch June 25, 2026 05:38
@github-actions

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-06-25 05:38 UTC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Scroll does not work on macOS

1 participant