Repository navigation
chore(gestures): a Storybook harness to check presented hit testing on a phone - #9400
Conversation
/lab/presented-hit-testing, not linked and not indexed, springs a sheet up over a full-page scrim on a spring slowed to a 3 s response, with Done in the sheet and a page behind it that scrolls. It reads out what the engine hit-tested at each touch-down, Done's clicks, the scrim's taps, the sheet pan's translation and how far the page scrolled during the last touch on the sheet, and a switch turns the gesture host's correction off as the control: a phone opening a preview can now exercise the presented hit testing of spec:AST-094 FR11 and FR19. The page drives Core's gesture host and animator through @astryxdesign/core/internal/touch, which now also re-exports GestureHost, PanGestureRecognizer, createAnimator and isIosWebKit by name (no root or public export changes); the entry's test checks each is the value Core imports by path, and failed before them. eslint.config.js lets that one route import the internal entry; any other docsite file still cannot.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
PR Analysis Report🔎 Preview ·
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested: the lab page is a useful instrument for the AST-094 device check, but it needs two fixes first.
- The ESLint exemption lets a docsite route import
@astryxdesign/core/internal/touch, whichspec:AST-017FR21 andarchitecture:react-component-runtimeINV13 limit to Core and Lab, so either keep the harness inside Core or Lab (for example a Lab-owned harness that the preview's Storybook renders) or amend both records with an engineering owner's approval in this pull request. - The "page scrolled" readout listens only on the sheet, so on iOS WebKit, where a touch on the moving sheet targets the scrim, it never updates; measure every touch at the page root instead.
- Smaller: three of the new Effects have handler or ref homes, and the status line should say when Reduce Motion is on, since the sheet then never moves.
[Automated review]
| ignores: [ | ||
| 'packages/core/**', | ||
| 'packages/lab/**', | ||
| 'apps/docsite/src/app/lab/presented-hit-testing/**', |
There was a problem hiding this comment.
This exempts the route from @astryx/no-core-internal-import, so a docsite file imports @astryxdesign/core/internal/touch. spec:AST-017 FR21 and architecture:react-component-runtime INV13 both limit that subpath to Core and Lab, and neither is amended here. Either keep the harness inside Core or Lab, or carry an engineering-owner-approved amendment of both records (and their verification rows) in this PR.
| onTouchStart={() => { | ||
| scrollAtTouchRef.current = window.scrollY; | ||
| }} | ||
| onTouchEnd={() => { | ||
| const start = scrollAtTouchRef.current; | ||
| if (start != null) { | ||
| setScrolled(Math.round(Math.abs(window.scrollY - start))); | ||
| } | ||
| scrollAtTouchRef.current = null; | ||
| }} |
There was a problem hiding this comment.
On iOS WebKit a finger on the moving sheet targets the scrim, and every later event of that touch goes to the scrim too, so these handlers never run for the drag the FR19 check reads and "page scrolled" keeps its last value. Measuring at the page root (onTouchStartCapture / onTouchEndCapture / onTouchCancelCapture on the outer <div>) would see every touch.
…at Storybook renders Per review: a docsite route may not import Core's internal entries (AST-017 FR21, react-component-runtime INV13), so the device check moves into Core as a harness that a Storybook story renders, and a phone opens it on the preview's Storybook. The harness imports the gesture host, the motion animator and the engine check by relative path, so internal/touch, its test, eslint.config.js and the changeset are back to main's. Also from review: - Every touch is measured at the page root in the capture phase: on iOS WebKit a touch on the moving sheet targets the scrim, so the sheet's own handlers missed it. - The engine and Reduce Motion are read on the first render, and the animator lives in the sheet's ref callback, instead of three Effects. - The status line says when Reduce Motion is on, since the sheet then does not move. - A test checks the scrim-targeted touch is logged and measured, and the Reduce Motion line. Co-authored-by: vjeux <vjeux@users.noreply.github.com>
cixzhang
changed the title
github-actions
Bot
removed
the
needs:design-review
There was a problem hiding this comment.
Changes requested: the harness is the right shape for the AST-094 phone check, but as written it cannot show the correction on an iPhone.
- The Correction control is Core's
Switch, which attaches to the document's shared gesture host; on iOS WebKit that host handles each touch first and resolves the moving sheet, so the harness's own host sees no moving element and every tap and drag still goes to the scrim with Correction on, exactly as with it off; use a native checkbox (or any control that does not use the gesture host) so the harness's host is the only one on the page, and add a test that sends a scrim-targeted touch at Done's drawn point with the engine check stubbed to iOS WebKit and expects "Done clicked".
Everything else checks out: the harness stays inside Core, ships in no package, and its readouts work in Chromium with touch emulation. A small comment fix is noted inline.
[Automated review]
| <HStack gap={3} vAlign="center" hAlign="between" wrap="wrap"> | ||
| <Heading level={1}>Presented hit testing</Heading> | ||
| <HStack gap={3} vAlign="center"> | ||
| <Switch |
There was a problem hiding this comment.
This Switch attaches a press consumer to the document's shared gesture host (useSwitchTouch.ts calls getGestureHost) in a layout effect, so that host hears every touch before the host built in the Effect above. On iOS WebKit its presentedTarget resolves the moving sheet first, which zeroes the sheet's presented offset; this harness's host then finds no moving element and keeps the engine's target, the scrim. With an iOS target emulated (engine check stubbed, touches targeted at the scrim), Correction on reads "Done clicked 0 · scrim took 1"; with a native <input type="checkbox"> here it reads "Done clicked 1 · scrim took 0" and a drag catches the pan.
| // The page root sees every touch in the capture phase, before the host | ||
| // corrects anything. On iOS WebKit a touch on the moving sheet targets the | ||
| // scrim, so a handler on the sheet would miss it. |
There was a problem hiding this comment.
Non-blocking: the gesture hosts listen in the window's capture phase, so they run before this root handler, not after it. The log is still right because no host changes event.target. Suggested: "The page root logs the engine's own target: no host changes it. On iOS WebKit a touch on the moving sheet targets the scrim, so a handler on the sheet would miss it."
Per review: Core's Switch attaches to the document's shared gesture host, which on iOS WebKit resolves the moving sheet before the harness's own host, so the correction never showed. A native checkbox keeps the harness's host the only one on the page. A test stubs the engine check to iOS WebKit, sends a scrim-targeted touch at Done's drawn point, and expects Done to click (it fails with the Switch). A comment says the root logs the engine's own target. Co-authored-by: vjeux <vjeux@users.noreply.github.com>
There was a problem hiding this comment.
Changes requested: the Correction fix works, but the story's title breaks the repository's story-tree check.
- The title
'Core/Gestures/Presented hit testing'has three segments, whichapps/storybook/.storybook/story-tree.test.ts("never nests a component deeper than /") rejects, so thebuild-storybookjob and thebuildcheck fail once CI runs; use a two-segment title such astitle: 'Core/Gestures'with the story named'Presented hit testing', and update the story link in the description.
The native checkbox resolves the earlier finding: with an iOS target emulated, a tap on the rising Done now clicks it with Correction on and reaches the scrim with it off, and the new test guards that. Everything else checks out.
[Automated review]
| import {PresentedHitTestingLab} from '../../../packages/core/src/__tests__/PresentedHitTestingLab'; | ||
|
|
||
| const meta: Meta<typeof PresentedHitTestingLab> = { | ||
| title: 'Core/Gestures/Presented hit testing', |
There was a problem hiding this comment.
This three-segment title fails apps/storybook/.storybook/story-tree.test.ts ("never nests a component deeper than /": a third segment is allowed only under Hooks, Themes, Internationalization and the chart groups), which the build-storybook job runs. A two-segment title fixes it, for example title: 'Core/Gestures' with name: 'Presented hit testing' on the story (id core-gestures--presented-hit-testing); the story link in the description would change with it.
Per review: the story tree allows <Package>/<Component> only, so the story is "Presented hit testing" under "Core/Gestures" (core-gestures--presented-hit-testing). The story-tree test passes on a Storybook built from this head. Co-authored-by: vjeux <vjeux@users.noreply.github.com>
There was a problem hiding this comment.
Recommending approval: the story-tree failure from the last round is fixed, and nothing else needs to change.
- The two-segment title
Core/Gestureswith the story named "Presented hit testing" passes the repository's story-tree check, and the story opens atcore-gestures--presented-hit-testingas the description says. - The harness stays inside Core and ships in no package, and its tests, strict lint and the gesture import-boundary tests pass on this head.
Real-device evidence for presented hit testing follows merge, as spec:AST-094 allows, so the iPhone run is the next step rather than a condition here.
[Automated review]
|
/approve-spec c80d196 |
cixzhang
added
the
ci:run
There was a problem hiding this comment.
Recommending approval: nothing needs to change.
- The harness stays inside Core and ships in no package. Its tests, strict lint and the gesture import-boundary tests pass on this head, and the iOS-stubbed tap case fails as it should with Correction unticked.
- The story opens at
core-gestures--presented-hit-testingand passes the story-tree check.
spec:AST-094 lets real-device evidence for presented hit testing follow merge, so the iPhone run comes next rather than being a condition here.
[Automated review]
… name meta#9754 renamed Core's internal createMotionAnimator to createMotionEngine, with no alias. The presented hit-testing lab still called the old name, so main's type check would fail once it merged.
|
/approve-spec 5adbd62 |
There was a problem hiding this comment.
Recommending approval, pending CI: CI has not finished on this head yet.
- The harness now calls Core's motion engine as
createMotionEngine, the name main gave it, and Core's type check passes with it. - The harness tests, the gesture import-boundary tests, strict lint and the story-tree check pass on this head, and the iOS-stubbed tap case reads the control correctly with Correction unticked.
spec:AST-094 lets real-device evidence for presented hit testing follow merge, so the iPhone run comes next rather than being a condition here.
[Automated review]
There was a problem hiding this comment.
Recommending approval: nothing needs to change.
- Main is merged in with no change to the pull request's own files, and on this head Core's type check passes and the harness tests pass, including the iOS-stubbed tap that reaches the drawn Done.
- The harness stays inside Core and ships in no package, and the story passes the story-tree check.
spec:AST-094 lets real-device evidence for presented hit testing follow merge, so the iPhone run comes next rather than being a condition here.
[Automated review]
Main is merged into this branch. Open this pull request's Storybook preview on an iPhone:
/storybook/iframe.html?id=core-gestures--presented-hit-testing&viewMode=story.Maintainer impact
#8360's presented hit testing (
spec:AST-094FR11, FR19) is proven in CI's WebKit lane and the iOS Simulator. But no story, sandbox or docs page attaches the gesture host, so a phone that opens a preview cannot exercise it. A check on a physical iPhone needs a page that does.Intended invariant
spec:AST-017FR21 andarchitecture:react-component-runtimeINV13: Core's internal entries and gesture source stay with Core and Lab. This pull request adds no import of them from outside Core: the harness is a Core file, and the story renders it the way the a11y stories render Core's__tests__renderings.Change
packages/core/src/__tests__/PresentedHitTestingLab.tsx(not exported): a sheet springs up over a full-page scrim on a spring slowed to a 3 s response, with Done in the sheet and a page behind it that scrolls. It reads out:A native checkbox turns the gesture host's correction off as the control; it is not Core's Switch, which would attach the document's shared gesture host and hide the correction. Replay restarts the motion. Every touch is measured at the page root in the capture phase, because on iOS WebKit a touch on the moving sheet targets the scrim. When Reduce Motion is on, the status line says so, since the sheet then does not move.
apps/storybook/stories/PresentedHitTestingLab.stories.tsx: one full-screen story, "Presented hit testing" under "Core/Gestures".No change to
internal/touch,eslint.config.js, any public entry, or any record. No changeset: story and test files only.On a phone
Scope
Testing
At 5adbd62: main 9e559e9 merged in, and the lab calls
createMotionEngine, the name #9754 gave Core's internal animator.tsc --noEmitpasses. On the plain merge without the rename, it fails with TS2305 on the old import.review:pre: clean.Earlier evidence:
After the maintainer update (main merged; the harness moved into Core; the review fixes):
Chromium with iPhone-sized touch emulation, against a Storybook built from this head:
Chromium already hit-tests the drawn sheet, so the phone is the check of the correction itself.
PresentedHitTestingLab.test.tsx:Story tree:
story-tree.test.tspasses on a Storybook built from this head (two-segment title).Static checks: strict ESLint (no
no-core-internal-importexemption), Prettier, core typecheck andpnpm review:pre(no findings).