Repository navigation
chore(core): Core's internal touch entry re-exports the action-group tracking hook, extended for Lab's TabBar, and a tab-bar string joins the catalog - #9699
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
Generated by PR Enrichment workflow | View current CI run |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes. The Core part itself, seven named re-exports on the internal touch entry and three catalog strings, follows the current internal-entry and catalog rules, and its tests pass. What blocks is what the diff against main carries.
- The diff includes #9399's two commits, which make it 7,261 changed lines (51 of them this pull request's own); land #9399 first and rebase, so this pull request contains only its own commit.
- SearchField's
armSwallow(packages/lab/src/SearchField/SearchFieldTouch.ts) adds its own document listeners to swallow a claimed tap's late mouse events, a second stray-click swallower beside Core's click guard, which INV13 indocs/architecture/react-component-runtime.mdallows only once; extendutils/clickGuard.tsto cover the compatibilitymousedownandmouseupand call it instead. .changeset/core-internal-entries-lab-tabbar.mdsays Lab's TabBar reaches these modules and that TabBar's strings join the catalog, but no TabBar exists at this head; describe the re-exports and the strings themselves.
Two non-blocking comment suggestions are inline.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [experimental] Lab's experimental TabBar reaches Core's pan recognizer, press tracker, finger-scroll reader, selection lock, touch query, rubber band and UIKit gesture values through Core's internal `@astryxdesign/core/internal/touch` entry. That entry stays outside the public API and its compatibility promise. TabBar's strings join Core's message catalog. |
There was a problem hiding this comment.
No TabBar exists at this head (it arrives in #9700 and #9701), so this release note announces a consumer the release may not contain. Suggested wording:
[experimental] Core's internal `@astryxdesign/core/internal/touch` entry re-exports the pan recognizer, press tracker, finger-scroll reader, selection lock, touch query, rubber band and UIKit gesture values for Lab components. Three tab-bar strings join Core's message catalog.
There was a problem hiding this comment.
Fixed in b5b0aaa with your wording: the note describes the entry's re-exports and the three strings, and names no consumer.
| const armSwallow = useCallback((doc: Document) => { | ||
| disarmRef.current?.(); | ||
| let sawGuard = false; | ||
| const swallow = (event: Event) => { | ||
| if (event.type === 'click' && (event as MouseEvent).detail === 0) { | ||
| return; | ||
| } | ||
| if (isClickGuardArmed(doc)) { | ||
| sawGuard = true; | ||
| } else if (sawGuard) { | ||
| disarm(); | ||
| return; | ||
| } | ||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| if (event.type === 'click') { | ||
| disarm(); | ||
| } | ||
| }; | ||
| const types = ['mousedown', 'mouseup', 'click'] as const; | ||
| const ends = ['touchstart', 'pointerdown', 'keydown'] as const; | ||
| const disarm = () => { | ||
| for (const type of types) { | ||
| doc.removeEventListener(type, swallow, true); | ||
| } | ||
| for (const type of ends) { | ||
| doc.removeEventListener(type, disarm, true); | ||
| } | ||
| if (disarmRef.current === disarm) { | ||
| disarmRef.current = null; | ||
| } | ||
| }; | ||
| for (const type of types) { | ||
| doc.addEventListener(type, swallow, true); | ||
| } | ||
| for (const type of ends) { | ||
| doc.addEventListener(type, disarm, true); | ||
| } | ||
| disarmRef.current = disarm; | ||
| }, []); | ||
| // Unmount releases a pending swallow's document listeners. | ||
| useEffect(() => () => disarmRef.current?.(), []); |
There was a problem hiding this comment.
These document listeners are a second stray-click swallower beside Core's click guard (packages/core/src/utils/clickGuard.ts), and INV13 in docs/architecture/react-component-runtime.md allows only one. Extending the click guard to cover a claimed tap's compatibility mousedown and mouseup, and arming it from this tap through the internal entry, would let armSwallow, disarmRef and the unmount Effect go.
| * @input The gesture core: the arena, the recognizers and their geometry, | ||
| * React hook, press tracker, finger-scroll reader and text-selection lock. | ||
| * The animator, the cubic-bézier solver, the rubber band and UIKit's gesture | ||
| * and timing tables. The touch-presentation hook and its query, the | ||
| * writing-direction read, and whether the click guard is armed. |
There was a problem hiding this comment.
Non-blocking: INV29 keeps a file header to about 12 lines, and this one is now 23. A shorter @input:
* @input Core's gesture, motion and touch modules that Lab components useThere was a problem hiding this comment.
Taken in b5b0aaa: @input is now your one line, and the header is down from 23 to 19 lines. The rest is the @position block's record citations, which stay until a record links the file.
| // A tap on the text of a field at the bottom of the screen (`bottom`, | ||
| // `tab`) focuses it without the browser's reveal (`preventScroll`). The | ||
| // browser would otherwise pan the whole page up to show a field the | ||
| // keyboard is about to cover. The tap is the gesture system's: recognizing | ||
| // it cancels the browser's own focus for that touch. Taps on the buttons, | ||
| // and taps once the text has focus, stay the browser's. | ||
| // The claim applies only where the keyboard covers the page, Chrome on | ||
| // Android included. There the browser's own tap scrolls the document once | ||
| // its keyboard is up. The claimed tap's focus brings the keyboard up and | ||
| // pans nothing (measured on the Android emulator). | ||
| // The recognizer attaches only where it claims: any other field keeps the | ||
| // browser's own touch-action and tap. | ||
| // It takes a finger and a pen, because Safari reveals a field for both. A | ||
| // mouse press stays the browser's, which places the caret and selects text | ||
| // where it lands. The claim is touch work, so it attaches only where the | ||
| // primary pointer is coarse. Under a fine one, as in desktop Safari or | ||
| // Firefox, no software keyboard rises. There a mouse or a pen tablet keeps | ||
| // the browser's own focus and click. | ||
| // The keyboard check reads the window that the touch presentation reads. | ||
| // `isTouch` is false on the server and while hydrating, so no markup | ||
| // differs. |
There was a problem hiding this comment.
Non-blocking: this comment covers seven topics in 21 lines (INV29: one topic per sentence). A shorter version that keeps the reasons:
// A tap on the text focuses the field with preventScroll. The browser's own
// focus would pan the page under the keyboard. The claim attaches only under
// a coarse pointer where the keyboard covers the page. It takes touch and pen.
// A mouse keeps the browser's focus and caret.There was a problem hiding this comment.
This comment is #9399's (SearchField), carried here only by the stack. I've passed your shorter version to that PR's thread rather than change its code here.
b86bd24 to
b5b0aaa
Compare
|
On the stack: this PR's own commit is the 51-line change at vjeux/astryx@1748182...b5b0aaa. The rest of the diff is #9399, which this branch sits on because both change |
There was a problem hiding this comment.
Requesting changes. The Core part itself, seven named re-exports on the internal touch entry and three catalog strings, follows the internal-entry and catalog rules, its tests pass, and the TabBar changeset now describes exactly what the head contains. What blocks is what the diff against main carries from #9399.
- The diff includes #9399's two commits, which make it 7,257 changed lines (47 of them this pull request's own); land #9399 first and rebase, so this pull request contains only its own commit.
- SearchField's
armSwallow(packages/lab/src/SearchField/SearchFieldTouch.ts) adds its own document listeners to swallow a claimed tap's late mouse events, a second stray-click swallower beside Core's click guard, which INV13 indocs/architecture/react-component-runtime.mdallows only once; extendutils/clickGuard.tsto cover the compatibilitymousedownandmouseupand call it instead. - SearchField's resize handling in the same file creates its own
ResizeObserverper field instead of callingobserveResizefrom@astryxdesign/core/utils; subscribe through the shared observer, and extend it if the field's own window is the reason.
Both SearchField items live in #9399's code and leave this diff with the rebase.
[Automated review]
| useLayoutEffect(() => { | ||
| const field = fieldRef.current; | ||
| const Observer = field?.ownerDocument.defaultView?.ResizeObserver; | ||
| // The tab bar lays out and moves a tab field's box; it has no parts. | ||
| if (field == null || Observer == null || placement === 'tab') { | ||
| return; | ||
| } | ||
| const observer = new Observer(() => { | ||
| if (isAnimating.current || settledRef.current === null) { | ||
| return; | ||
| } | ||
| if (isBottom) { | ||
| placeBottom(bottomPoseFor(settledRef.current, bottomShape().width)); | ||
| } else { | ||
| place(poseFor(settledRef.current, geometry())); | ||
| } | ||
| }); | ||
| observer.observe(field); | ||
| return () => observer.disconnect(); |
There was a problem hiding this comment.
This creates one ResizeObserver per field. observeResize in @astryxdesign/core/utils (packages/core/src/utils/sharedResizeObserver.ts) already owns resize observation, and SearchField.tsx already imports from that entry. Subscribing with observeResize(field, …) and returning its unsubscribe would drop the per-instance observer; if the field's own window is the reason, extending observeResize for it keeps one path.
There was a problem hiding this comment.
This is #9399's code too (SearchField), carried here only by the stack, and I've passed it to that PR's author. This PR's own commit has no observer.
b5b0aaa to
62b989d
Compare
There was a problem hiding this comment.
Requesting changes. The Core part of this pull request (eight named re-exports on the internal touch entry, the widened useFingerScroll type and three catalog strings) follows the internal-entry and catalog rules, and its tests pass. What blocks is what the diff against main still carries from #9399.
- The diff includes #9399's two commits, which make it 7,416 changed lines (76 of them this pull request's own); land #9399 first and rebase, so this pull request contains only its own commit.
- Those commits bring Lab's SearchField with its strings,
astryx discoverentry, stories and tests, which this pull request's title and body do not cover and which belong in #9399, so they need to leave this diff.
The two earlier SearchField findings (the second stray-click swallower and the per-field ResizeObserver) are fixed at this head. Two non-blocking notes are inline.
[Automated review]
| * the document (or a ref that holds it) for the page's own scroll. Returns a | ||
| * reader, stable across renders: `true` while the scroller's scroll is a | ||
| * finger's. | ||
| * Internal: not exported from the package. |
There was a problem hiding this comment.
Non-blocking: this commit re-exports the hook from @astryxdesign/core/internal/touch, so "not exported from the package" no longer holds. Suggested:
* Internal: outside Core, only `@astryxdesign/core/internal/touch` reaches it.There was a problem hiding this comment.
Good catch. I'll take your wording in the push that rebases this onto main once #9399 lands, so it rides with the change this round can't make yet.
| return {duration, at, velocity: t => slope(at, t)}; | ||
| } | ||
|
|
||
| /** A curve times a constant (a direction). */ |
62b989d to
53c7f49
Compare
There was a problem hiding this comment.
Requesting changes. This pull request's own commit (eight named re-exports on the internal touch entry, the widened useFingerScroll type and three catalog strings) follows the internal-entry and catalog rules, and its new test and type cases fail before the change and pass after it. What blocks is what the diff against main still carries from #9399.
- The diff includes #9399's two commits, 7,371 of its 7,449 changed lines, and this copy is already behind #9399's own head; land #9399 first and rebase onto
main, so this pull request holds only its own 78-line commit. - Lab's SearchField, its stories, strings, discover entry and registry route belong to #9399 and have no place in a pull request about the touch entry's re-exports; they leave with that rebase.
Non-blocking, for #9399: the local translate(...) helper in SearchFieldBottom.interaction.browser.spec.ts makes the i18n catalog check list nine unverifiable key references; renaming it removes the noise.
[Automated review]
Internal entries land with their first user, per meta#8562. Lab's NavigationStack, PageControl and TabBar animate with Core's animator, solve UIKit's timing curves with Core's cubic-bézier solver, listen for taps and pans with Core's gesture hook and pan recognizer, stretch past their ends with Core's rubber band and read the writing direction as Core does, so `@astryxdesign/core/internal/touch` re-exports, by name: - the motion engine, `createMotionEngine`, for its jobs beyond the public `createAnimator` (architecture:react-component-runtime INV13), with its types `MotionEngine` and `MotionEngineOptions` (the engine's `Animator` and `AnimatorOptions`), `AnimateOptions`, `MotionCurve` and `MotionHandle`. The engine's factory is renamed from `createMotionAnimator` at its source and in every internal caller, so the engine and the public animator differ by more than one word; - `cubicBezier` and UIKit's `timing` table; - `useGestureRecognizer`, `useTouchPresentation` and `isRtlElement`; - `PanGestureRecognizer`, `rubberBand`, UIKit's `UIKIT_GESTURES` table and `TOUCH_QUERY`, which more than one of those components reads. Named re-exports of code already on main, plus that rename: no behavior changes, no docs, no root re-export, and the `@astryx/no-core-internal-import` lint rule already limits the entry to Core and Lab. The entry's test checks it re-exports the same values Core imports by path, and that it carries no public factory's name. One Lab module that those components share comes with them: `internal/useMotionEngine`, a stable motion engine bound to an element through a callback ref.
53c7f49 to
918491f
Compare
|
Restacked on #9754 instead of #9399, since #9399 won't merge. This PR's own commit is now only what TabBar adds to Core's internal touch entry: the press tracker, the action-group tracker, the finger-scroll reader and the selection lock, plus the widened |
There was a problem hiding this comment.
Requesting changes for one small fix. The new internal re-exports, the widened useFingerScroll type and the three catalog strings follow the internal-entry and catalog rules.
.changeset/core-internal-entries-lab-tabbar.mdis tagged[experimental], which lists this internal-entry change under "Experimental APIs" in Core's release notes, while AST-017 FR21 and FR22 keep./internal/*outside experimental status; please retag it[chore], as the stacked changeset does.
Non-blocking: the @output header in packages/core/src/internal/touch.ts and the @input header in packages/core/src/gestures/useFingerScroll.ts no longer describe everything they now cover.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [experimental] Core's internal `@astryxdesign/core/internal/touch` entry re-exports the press tracker, action-group tracker, finger-scroll reader and selection lock for Lab components. The finger-scroll reader also takes a ref that holds a document. Three tab-bar strings join Core's message catalog. |
There was a problem hiding this comment.
[experimental] files this under "Experimental APIs" in Core's release notes, but an ./internal/* entry is outside experimental status (AST-017 FR21 and FR22). Please retag it [chore], as core-internal-touch-entry-lab-animator.md does; the patch bump stays.
There was a problem hiding this comment.
Retagged [chore], as #9754's changeset is; the patch bump stays.
918491f to
a88504b
Compare
|
Changes in this round:
|
|
/approve-spec a88504b |
There was a problem hiding this comment.
Requesting changes for one fix. The new hook options, the internal re-export and the catalog string follow the internal-entry, action-group and catalog rules, and the new tests fail before the change and pass after it.
- Turning tracking off mid-press (
isEnabledgoing false) calls neitheronPressEndnoronPressCancel, so a group usingpaintsHighlight: falsekeeps its own highlight lit although theonPressCanceldocs promise a report whenever a press ends with nothing acting; report it as a cancel in the cleanup that already cancels the tracker, with a test, or document that the caller clears its own highlight.
Two small notes that do not block: the hook's header names Lab's TabBar as a consumer before TabBar exists, and the title says "strings" for the one key the change adds.
[Automated review]
| onPressMove?: (x: number, y: number, action: HTMLElement | null) => void; | ||
| /** The lift, before its action runs, with the tracked action (null: none). */ | ||
| onPressEnd?: (action: HTMLElement | null) => void; | ||
| /** The press ended with nothing acting: a cancel, or a second finger. */ |
There was a problem hiding this comment.
Turning tracking off mid-press (isEnabled going false) ends the press with nothing acting, but the cleanup at lines 128-134 only calls tracker.cancel(), so neither onPressEnd nor this callback runs. With paintsHighlight: false the caller's own highlight stays lit. Report it here too (for example if (tracker.isTracking) optionsRef.current.onPressCancel?.() before tracker.cancel() in that cleanup), with a test beside the three new ones, or state in this JSDoc that turning tracking off ends a press with no report.
There was a problem hiding this comment.
Done: the cleanup that cancels the tracker now reports a press it drops as a cancel (onPressCancel, after tracker.cancel(), only if a press was live). A new case turns tracking off mid-press and expects begin then cancel, with nothing run; it fails on the previous head. The header now says "Lab components", and the title says "a tab-bar string".
…tracking hook, extended for Lab's TabBar, and a tab-bar string joins the catalog Re-exports only on the entry: @astryxdesign/core/internal/touch adds useActionGroupTracking and its options type. So Lab's TabBar presses through the hook AlertDialog uses: a second finger ends the press with nothing acting, and focus follows the lift. The hook gains options for a group that draws its own highlight. All are optional, and without them the hook runs as before, so AlertDialog does not change. A no-paint option leaves the highlight to the group. Press callbacks report the action under the press as it begins and after each move, the lift before its action runs, and a cancel. Core's message catalog gains TabBar's badged-tab name. It lands ahead of the Lab component, so the Lab batch changes only Lab. The entry stays outside the public API: no docs, no root re-export, and the @astryx/no-core-internal-import rule keeps it to Core and Lab.
a88504b to
aa755bc
Compare
vjeux
changed the title
|
/approve-spec aa755bc |
There was a problem hiding this comment.
Recommending approval. The hook's new paintsHighlight option and press callbacks, the internal re-export and the catalog string follow the internal-entry, action-group and catalog rules. Turning tracking off mid-press now reports a cancel, with a test that fails on the previous version.
No blocking findings remain. One optional cleanup is noted inline: an orphaned JSDoc in packages/core/src/motion/animator.ts. This pull request lands after #9754.
[Automated review]
|
|
||
| /** | ||
| * Options of `createMotionAnimator`. | ||
| * Options of `createMotionEngine`. |
There was a problem hiding this comment.
Non-blocking: this JSDoc documents nothing. It sits directly above SetOptions' own JSDoc, and it restates what the name AnimatorOptions already says. Delete it, or move it above AnimatorOptions.
Stacked on #9754 (Core's internal touch entry and Lab's motion-engine hook). Review only this PR's change: vjeux/astryx@4e363d8...aa755bc
Maintainer impact
Lab's TabBar lands as two Lab-only batches, drafts that follow this one. Its press needs Core's action-group tracking hook, which Lab can't reach or extend itself, and its badged tabs need one catalog string. A Lab-only batch carries no Core code, so those parts land here first, on their own.
Intended invariant
No behavior change for today's caller: the hook's new options are optional, and AlertDialog passes none. Core's internal touch entry stays internal: named re-exports only, outside the public API, and limited to Core and Lab by
@astryx/no-core-internal-import. No Core module reads the new catalog key.Change
useActionGroupTrackinggains options for a group that draws its own highlight.paintsHighlight: falsewrites no press attribute. Press callbacks report the press:onPressBeginandonPressMovewith the action under it,onPressEndbefore the lift's action runs, andonPressCancelwhen it ends with nothing acting: a cancel, a second finger, or tracking turned off mid-press.@astryxdesign/core/internal/touchre-exports the hook and its options type. TabBar's press goes through it, so a second finger ends a press with nothing chosen (family:action-groupsFR3) and focus follows the lift (FR1), as in AlertDialog.packages/core/locales/en.jsongains@astryx.tabBar.badgedItem, a badged tab's name.[chore]) names the re-export, the options and the string.Evidence
expected 'on' to be null; no callback runs, also when tracking turns off mid-press). The entry's new case fails on the current entry (useActionGroupTracking: expected undefined).Scope
Testing
Local gates at aa755bc, on #9754's 4e363d8, node 24.22.1 from
.nvmrc: Core typecheck; the hook's 30 cases, with its four new ones red on the current hook first; AlertDialog's, the shared tracker's and the entry's units (84), with the entry's new case red first; strict ESLint, 0 problems; Prettier. At a88504b (this head without the cancel report on turning tracking off): Lab typecheck; 130 unit tests, including the internal-entry lint and bundle tests; AlertDialog's interaction spec on the built Storybook, 12/12 in each engine; Changesets and coverage; i18n runtime and catalog;review:pre.Stack: #9754 → this PR → TabBar's Lab batches #9700 and #9701.