Repository navigation
chore(core): resolve a component's presentation so a caller's choice wins over touch defaults - #8298
Conversation
|
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.
|
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewComponent Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: The accessibility audit did not run on this pull request. It runs after merges to Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval is on hold: besides the resolver commit, this pull request's compare carries the whole unmerged touch-presentation stack (28,468 authored lines), including the draft spec:AST-094 and additions no current spec covers: theme motion roles, the motion/experimental and gestures/experimental subpaths, and the new 744px/500px compact-touch rule. An engineering owner needs to approve or reject those draft requirements and identify the governing spec for the additions, or confirm that those decisions stay with #8052–#8076; an engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Independently of that decision, please:
- merge the stack first, so this pull request contains only the resolver commit;
- drop the
platformadaptation condition or amendspec:AST-012FR2, whosewhenvocabulary is closed; - keep the experimental hooks out of stable
astryx searchandastryx hookoutput without an opt-in (spec:AST-017FR27) and restore theno-icon-setgoldens; - reconcile
spec:AST-094FR4 and DEC-1 with the theme-tunable touch motion the stack ships.
[Automated review]
| * is everything else. Pair `ios` with `pointer: 'coarse'` for a touch | ||
| * profile, such as touch motion tuned per platform. | ||
| */ | ||
| platform?: 'ios' | 'other'; |
There was a problem hiding this comment.
platform is a fifth when field, but spec:AST-012 FR2 closes that vocabulary to width, pointer, contrast and motion ("Future environment fields require explicit additions to this closed vocabulary"). Please remove it here or amend AST-012 FR2 through an owner-approved record change first.
| - **FR4 — Physics are not themable in touch presentations.** A touch | ||
| presentation MUST read its motion from the table on every theme. Themes keep | ||
| color, shape, type and spacing; the theme's duration and easing tokens keep |
There was a problem hiding this comment.
FR4 says a touch presentation reads its motion from the table on every theme, while the same stack lets themes retune touch springs, durations and curves that the animator plays (packages/core/src/motion/resolveMotion.ts). One of the two needs to change before either lands.
| @@ -1,4 +1,24 @@ | |||
| Results for "experimental" (3) | |||
| Results for "experimental" (7) | |||
| name: useAnimator | |||
There was a problem hiding this comment.
These goldens now list four experimental hooks in stable astryx search output with no opt-in. The header of packages/cli/test/experimental-isolation.test.mjs calls a golden diff that names experimental the leak the test exists to catch (spec:AST-017 FR27).
08063f5 to
53cf41a
Compare
github-actions
Bot
added
the
needs:design-review
53cf41a to
0d8bc30
Compare
0d8bc30 to
470434a
Compare
There was a problem hiding this comment.
Approval is on hold: besides its two resolver commits, this pull request's compare carries the whole unmerged touch-presentation stack (30,652 authored lines), including the draft spec:AST-094 and additions no current spec covers: theme motion roles, the motion/experimental and gestures/experimental subpaths and a new motion/reducedMotion.stylex export. An engineering owner needs to approve or reject those draft requirements and identify the governing spec for the additions, or confirm that those decisions stay with #8052–#8076; an engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Independently of that decision, please:
- merge the stack first, so this pull request contains only the resolver commits;
- build the compact-touch query from the theme's effective
mdpoint, asspec:AST-012FR9 requires (packages/core/src/utils/touchPresentation.ts); - align
spec:AST-094FR8 with the 200 ms limit indesign:reduced-motionDR2, reconcile FR4 and DEC-1 with the theme-tunable touch motion the stack ships, and citespec:AST-090only once it exists; - keep the experimental hooks out of
astryx hookoutput without an opt-in (spec:AST-017FR27), and announce or withhold themotion/reducedMotion.stylexexport.
[Automated review]
| * CSS: a style that implements a touch surface or input mode is applied with | ||
| * `touchModes` (`stylex.props(touchModes && styles.touchMode)`), never as a | ||
| * bare media key on a base style, so a pinned `'popover'` keeps none of it; | ||
| * touch-only rules inside it stay nested as IOS_TOUCH_STYLE_KEYS spell. On |
There was a problem hiding this comment.
This clause predates the one-touch-model refactor: IOS_TOUCH_STYLE_KEYS adds @supports (-webkit-touch-callout: none), which touchPresentation.ts reserves for a style iOS WebKit itself requires, so an author following it would ship a touch mode's rules to iOS Safari only. Drop it, or say that touch-only rules key on TOUCH_QUERY.
| export const COMPACT_QUERY = DEFAULT_COMPACT_DEFINITION.query; | ||
|
|
||
| /** Both: presentations such as a sheet in place of a popover. */ | ||
| export const COMPACT_TOUCH_QUERY = | ||
| DEFAULT_COMPACT_DEFINITION.coarsePointerQuery; |
There was a problem hiding this comment.
These are built once from DEFAULT_COMPACT_DEFINITION, so a theme that overrides md (for example adaptations: {widthBreakpoints: {md: 700}}, which AppShell honours) never moves adaptive, the menu drill-in or this resolver. spec:AST-012 FR9 defines compact as below the effective md point; compactDefinition(points) with the nearest theme's effective breakpoints would follow it.
| place, and any other move fades through. A navigation push or pop | ||
| cross-fades over 0.5 s on the ease-in-out curve; a sheet, a full-screen | ||
| modal or an alert fades on its own presentation spring and drops its travel | ||
| and scale; menus open and close on UIKit's Reduce Motion spring. Motion that |
There was a problem hiding this comment.
A 0.5 s cross-fade and spring-timed fades exceed what design:reduced-motion DR2 allows ("MUST change opacity only and MUST finish in 200 ms or less"), and DR4 forbids a longer animation under reduced motion. That record is current and this one neither names nor amends it, while the stack's animator already uses the 200 ms fade. Please restate FR8 as DR2 and DR3, or propose the amendment to that record's owner.
| follows. | ||
| - **FR7 — Exits animate as UIKit's do.** A touch presentation MUST play the | ||
| exit its UIKit counterpart plays. **Proposed; pending owner review:** this | ||
| departs from the draft `spec:AST-090` FR1 (exits off by default) and FR10 |
There was a problem hiding this comment.
spec:AST-090 is not in the repository on main or at this head, so its owners have nothing to rule on and an approver cannot check what FR7 overrides. Cite it once it lands, or drop the amendment until then.
| // Proposed; pending owner review (spec:AST-094 OQ4): the Reduce Motion | ||
| // media keys and fade constants, for a transition in another package. | ||
| './motion/reducedMotion.stylex': { | ||
| source: './src/motion/reducedMotion.stylex.ts', | ||
| types: './dist/motion/reducedMotion.stylex.d.ts', | ||
| default: './dist/motion/reducedMotion.stylex.js', |
There was a problem hiding this comment.
spec:AST-094 has no OQ4, and no Changeset announces @astryxdesign/core/motion/reducedMotion.stylex; .changeset/touch-experimental-subpaths.md says nothing else is added to any subpath. Outside an /experimental boundary this becomes a stable import path on release. Keep it out of the exports map until an owner decides it, or announce it with that decision.
| // An experimental hook (spec:AST-017 FR27) is never a search result: | ||
| // search answers as it would without it. | ||
| if (doc?.stability === 'experimental') { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Search now skips experimental hooks, but astryx hook --list and astryx hook <name> still find them: discoverHooks scans every src/ directory except utils, __tests__ and node_modules, and the hook list reads no stability. That changes stable output without an opt-in (spec:AST-017 FR27); the same skip in hook discovery, plus a hook --list golden, would close it.
There was a problem hiding this comment.
Changes requested: the presentation resolver now keeps touch modes out of the server and hydrating render, and its CSS note no longer sends touch-mode rules to the iOS-only gate, so the resolver itself matches public-component-api INV16 and AST-094 FR13. This branch still carries #8076's gesture, motion, harness and build stack (15,626 of the 16,048 lines it changes outside the lockfile), so please land #8076 first and rebase onto main, or put the resolver's commits on main by themselves. Separately, #8076's gesture host listens only under a coarse pointer (GestureHost.ts:580-583), which contradicts react-component-runtime INV13 and AST-094 FR43 and should be fixed there before it lands.
[Automated review]
This pull request sat on meta#8076 and carried its old copies of the harness meta#8045 and meta#8046 landed on main. meta#8360 carries meta#8076 with main's harness, so after the merge this pull request's own diff is the presentation resolver's commits. The one conflict, the gestures README, keeps both sides: this pull request's paragraph on the presentation prop ends the touch presentation section, and meta#8360's migration section follows it.
There was a problem hiding this comment.
Changes requested: the presentation resolver itself matches architecture:public-component-api INV16, but this branch now merges #8360 as well and carries 27,232 lines of the open #8076 and #8360 stack around the 422-line resolver. Please land #8360 and the pull requests it merges first and rebase onto main, or move the resolver's commits onto main by themselves. Before that stack lands, its gesture host should attach for every pointer, as architecture:react-component-runtime INV13 and spec:AST-094 FR19 and FR43 require, rather than only under a coarse primary pointer, and PressTracker should be built by a create… factory rather than a class.
[Automated review]
| * ONLY WHERE THE PRIMARY POINTER IS COARSE | ||
| * | ||
| * - Recognizers attach, and the host registers listeners, only while the | ||
| * primary pointer is coarse (`spec:AST-094` FR19, FR24; coarsePointer.ts). | ||
| * Where it is fine, an attach is recorded but nothing is installed: no | ||
| * document, window or element listener, no `touch-action` or other style, | ||
| * nothing delivered, so a finger on a touchscreen laptop gets the | ||
| * browser's defaults. The host follows the query: turning coarse installs | ||
| * it and styles every registration; turning fine cancels every touch in | ||
| * flight and removes it all. A development warning names the first attach | ||
| * under a fine pointer. |
There was a problem hiding this comment.
This rule is the opposite of CURRENT: spec:AST-094 FR19 says "Recognizers attach for every pointer, with FR43's exceptions", FR43 says "Gestures attach for every pointer, a mouse, a pen, and a finger, not only a coarse primary pointer", DEC-21 rejects "keeping gestures and press tracking touch-only", and architecture:react-component-runtime INV13 says "The gesture host attaches for every pointer, with no touch-only query". As written, a mouse or pen on a fine primary pointer gets no pan, swipe, pinch, rotation or hover. Attach for every pointer, express FR43's exceptions as per-recognizer pointer filters, keep fine-pointer document listeners to the press (FR24), and reverse the tests at GestureHost.test.ts:3037-3158, in the pull request that owns the host.
| return tap.numberOfTapsRequired === 1 && tap.numberOfTouchesRequired === 1; | ||
| } | ||
|
|
||
| export class PressTracker implements TouchConsumer { |
There was a problem hiding this comment.
architecture:react-component-runtime INV13 keeps classes to the recognizer family ("its base, the arena, and the host") and builds "Every other module ... from create… factories"; its verification fails "a class outside the recognizer family". A createPressTracker factory, used by usePressTracker.ts:100, would meet it.
| * | ||
| * Every touch motion passes through here, so the role a value plays is named | ||
| * in one place. Until the public motion-token set is decided, a role plays | ||
| * its table value: touch motion stays internal (`spec:AST-094` DEC-11), so |
There was a problem hiding this comment.
spec:AST-094 DEC-15 superseded this: "This supersedes DEC-11's "all internal" for exactly these three knobs". The same citation appears at :29, motion/README.md:23, :49, :73, :244 and animator.ts:149, :2244; citing DEC-15 and spec:AST-108 FR3 (roles play their table value until the knobs ship) would keep them accurate.
… as a dependency meta#8627 is now one commit on main; meta#8360 carries meta#8076, meta#8073 and meta#8446. useMenuPress.ts: the trigger half lives in menuTriggerPress.ts, so it takes meta#8360's trigger changes there. The trigger's holds run on the long-press detector and pass FR29's 13 px drift allowance as each press starts, under every primary pointer (FR43). The hold duration comes from motion/uikit/longPress.ts. MENU_PRESS_TRIGGER_DRIFT_PX is re-exported beside MENU_PRESS_LONG_PRESS_MS. The stray-click window is the shared click guard's, from meta#8360.
meta#8089 now takes meta#8080, which takes meta#8078 (with meta#8627, meta#8360 and meta#8298) and meta#8186. The DropdownMenu spec's FR5 row takes meta#8080's list (FR26–FR32, FR34 and FR35) and its state names FR35 verified by this PR; FR28's cancel clear and FR34 stay pending on this line. Prettier re-pads the table.
meta#8089 now takes meta#8080, which takes meta#8078 (with meta#8627, meta#8360 and meta#8298) and meta#8186. DropdownMenuTouch.ts registers UIKit's two menu springs for the grow, from meta#8080, and keeps the springs table for the in-place sub-menu; its header says so. The usage text takes meta#8080's wording, meta#8089's popover sentence and this PR's in-place sub-menu sentence.
…wins over touch defaults usePresentation and readPresentation resolve a component's `presentation` prop: a caller's explicit value always wins over the touch default (architecture:public-component-api INV16). The surface is the popover or the sheet, and touchModes says whether the component's touch input modes apply, the same on every touch device; a server or hydrating render takes no touch modes. meta#8298's change, rebased onto main as one commit; its gestures README paragraph waits for that README, and the resolver's file header carries the same contract.
8cb7c3a to
24eb9bc
Compare
github-actions
Bot
removed
the
needs:design-review
There was a problem hiding this comment.
Changes requested: the resolver itself matches architecture:public-component-api INV16 and spec:AST-094 FR13, but its FR13 test renders only on the default desktop device, so it would still pass if usePresentation put touch modes into a phone's hydrating render. Please make setDevice(IPHONE) the first line of that test, and consider a hydrateRoot case as well. Separately, and not blocking, subscribing to the pointer and compact queries only for the requests that read them, as useAdaptivePresentation does, would keep a pinned 'popover' free of listeners.
[Automated review]
| expect(result.current).toMatchObject({touchModes: true}); | ||
| }); | ||
|
|
||
| it('renders no touch surface or input mode on the server or while hydrating, and takes no hint that would (spec:AST-094 FR13)', () => { |
There was a problem hiding this comment.
This case runs on the desktop device beforeEach sets, where a live read and the server snapshot both answer popover/false, so a usePresentation that seeded its server snapshot from the live window (touch modes in a phone's hydrating render, which spec:AST-094 FR13 rules out) would still pass. Making setDevice(IPHONE); the first line makes that variant fail while the current resolver still passes; a hydrateRoot case would cover the hydrating render directly.
| presentation: AdaptivePresentation | undefined, | ||
| {whenOmitted = 'popover'}: UsePresentationOptions = {}, | ||
| ): ResolvedPresentation { | ||
| const touch = useTouchPresentation(); |
There was a problem hiding this comment.
Non-blocking: this subscribes to both the pointer and the compact query for every value, so a pinned 'popover' holds two listeners, and the omitted 'touch' default and 'bottom-sheet' hold one they never read. useAdaptivePresentation holds none for a fixed value (architecture:react-component-runtime INV5: optional behavior SHOULD acquire local resources only while enabled); a no-op subscription for the queries a request does not read would match it.
…wins over touch defaults usePresentation and readPresentation resolve a component's `presentation` prop: a caller's explicit value always wins over the touch default (architecture:public-component-api INV16). The surface is the popover or the sheet, and touchModes says whether the component's touch input modes apply, the same on every touch device; a server or hydrating render takes no touch modes, and the hook listens only to the queries a request reads (architecture:react-component-runtime INV5). meta#8298's change, rebased onto main as one commit; its gestures README paragraph waits for that README, and the resolver's file header carries the same contract.
24eb9bc to
8026021
Compare
|
Fixed in 8026021: the FR13 test now starts with |
There was a problem hiding this comment.
Changes requested: the resolver matches architecture:public-component-api INV16 and spec:AST-094 FR13, and the FR13 test and scoped subscriptions now hold up, but the surface-parity matrix in presentation.test.ts would still pass a resolver that drops the coarse-pointer check for 'adaptive', reads compact by width only, or ignores the theme's md. Please add a fine-pointer narrow window (600×800), a phone in landscape (852×393) and the 767/768 and 499/500 edges to that matrix, plus a usePresentation('adaptive') case under a theme with md: 1000 like the one in touchPresentation.test.ts. Those cases catch all three variants and pass on this head.
[Automated review]
| }); | ||
|
|
||
| it("renders useAdaptivePresentation's surface for every value on every device", () => { | ||
| for (const next of [IPHONE, IPAD, ANDROID, DESKTOP, MAC_SAFARI]) { |
There was a problem hiding this comment.
This matrix has no short screen, no fine-pointer compact screen and no compact edge, so a usePresentation that gives 'adaptive' the sheet without checking the coarse pointer (a desktop window under 768px wide), or that reads compact by width only (a phone in landscape), still passes every case. spec:AST-094 FR13 is verified at "coarse and fine pointer; narrow, wide and short" and spec:AST-012 FR9 at "width 767/768 at height 800; width 1024 at height 499/500; overridden md". Adding {coarse: false, width: 600, height: 800}, {coarse: true, width: 852, height: 393}, 767/768 by 800 and 1024 by 499/500 here, plus a usePresentation('adaptive') case under a theme with md: 1000 (as touchPresentation.test.ts:285-308 does), catches those variants and passes on this head.
…wins over touch defaults usePresentation and readPresentation resolve a component's `presentation` prop: a caller's explicit value always wins over the touch default (architecture:public-component-api INV16). The surface is the popover or the sheet, and touchModes says whether the component's touch input modes apply, the same on every touch device; a server or hydrating render takes no touch modes, and the hook listens only to the queries a request reads (architecture:react-component-runtime INV5). meta#8298's change, rebased onto main as one commit; its gestures README paragraph waits for that README, and the resolver's file header carries the same contract.
|
Added in 9f0595f: the surface matrix now covers a fine-pointer narrow window (600×800), a phone in landscape (852×393) and the 767/768 and 499/500 edges, with each new device's |
There was a problem hiding this comment.
This looks ready to approve: the resolver matches architecture:public-component-api INV16, spec:AST-094 FR13 and spec:AST-012 FR9, and the added fine-narrow, landscape, edge and themed-md cases now fail for a resolver that drops the coarse-pointer check, reads compact by width only, or ignores the theme's md. One optional addition: the "keeps the sheet meaning of adaptive and bottom-sheet" case checks 'bottom-sheet' only on a desktop, so an iPhone assertion of {surface: 'bottom-sheet', touchModes: true} would pin that row of the description's table.
[Automated review]
| expect(resolve('bottom-sheet')).toMatchObject({ | ||
| surface: 'bottom-sheet', | ||
| touchModes: false, | ||
| }); |
There was a problem hiding this comment.
Non-blocking: this is the only client-side check of 'bottom-sheet''s touchModes, and it runs on the desktop, so a resolver that never gives 'bottom-sheet' touch modes still passes every case. An iPhone assertion would pin the "with touch modes under a finger" half of this case's title and the description's table:
setDevice(IPHONE);
expect(resolve('bottom-sheet')).toEqual({surface: 'bottom-sheet', touchModes: true});
Small batch of the merge plan: one commit on main holding only this change. Its paragraph for the gestures README waits for that README, which comes with #8076; the resolver's file header carries the same contract.
Maintainer impact
A component that behaves differently under a finger, such as a menu that tracks the finger or becomes a sheet, must still let a caller pin the desktop behavior. Today components read
presentation ?? 'popover', which cannot tell an omitted prop from an explicit'popover', so a component that adopts a touch default would override a caller who asked for the popover. The menu, context menu and picker work that adopts touch modes builds on this resolver (#8104, #8627, #8080, #8125, #8201, #8159, #8162).Intended invariant
A caller's explicit
presentationalways wins over the touch default (architecture:public-component-apiINV16), and the surface equalsuseAdaptivePresentation's for every value on every device, so no caller sees a change:presentation'popover''bottom-sheet''adaptive'whenOmitted: 'touch'The modes follow the pointer, never the platform. The server and hydrating render never present touch modes, and no hint changes that (
spec:AST-094FR13). The hook listens only to the queries a request reads (architecture:react-component-runtimeINV5): none for a pinned'popover', the pointer for'bottom-sheet'and the omitted'touch'default, and the pointer and compact size for'adaptive'. A touch-mode style is applied throughtouchModes, never nested under the iOS-only@supportsgate.Change
utils/presentation.ts:usePresentationandreadPresentation(and the pureresolvePresentation) return the surface to render andtouchModes, whether the component's touch input modes apply;usePresentationsubscribes to each media query only while the request reads it. Internal: no component reads it yet.hooks/useAdaptivePresentation.ts: its header points a component that also switches input modes tousePresentation, whose surface is its own.Evidence
presentationand an explicit'popover'resolve the same, so a touch default cannot tell them apart; a server render given a touch hint must still answer the popover with no touch modes, a case that failed first. On an iPhone, a resolver that seeded its server snapshot from the live window fails both the server-render case and thehydrateRootcase; listening to both queries for every value fails the subscription case. A resolver that gives'adaptive'the sheet without the coarse pointer, or reads compact by width only, fails the surface matrix; one that ignores the theme'smdfails the themed case.'adaptive'under a theme whosemdis 1000) covers each row above, a live pointer change and the one-moment read; on an iPhone, the server render and the hydrating render answer the popover with no touch modes, and after hydration the phone's sheet with touch modes; each request holds only the subscriptions it reads.useAdaptivePresentation's surface on every device, and no component reads the resolver yet.Scope
Testing
Local gates at 9f0595f, one commit on main d73908f holding only this change (4 files, +571/−1):
pnpm test:ui(core UI): 654 files, 16,459 tests passed.pnpm exec vitest run --project node: 804 files passed; the only failures are main's own story-tree and release-cache reds, tracked in test(storybook): run the story-tree check where Storybook is built, and bring the tree back to its shape #8509 and fix(cli): a crashed run's release install is recognized by the Node it ran on, whatever its name #8520.tsc --project packages/core/tsconfig.json --noEmit, strict ESLint and Prettier on every changed file: clean.pnpm check:repo: clean.