Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
8b544b7 to
f4e5e1f
Compare
f4e5e1f to
9ad18b5
Compare
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This pull request can't be approved yet. Several of its additions have no current spec authority, and two of its changes contradict current records. An engineering owner needs to decide on: the draft spec:AST-094 requirements; the TabBar's 10 pt drag slop; the new @astryxdesign/core/motion/experimental and @astryxdesign/core/gestures/experimental subpaths; the new presentation="adaptive" threshold, which contradicts component:Selector DEC-1; and delivering onChange three frames after the lift, which contradicts architecture:react-component-runtime INV2. An engineering owner records approval by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner; the two contradicted records need an amendment on main, or the change reverted. Whatever the owners decide, these fixes are needed:
- Land the stacked predecessors, or retarget onto #8129, so this diff is its own 1,342-line commit instead of 28,746 authored lines.
- Either ship the adaptive threshold as
[breaking]in a declared minor or keep the documented 768px rule foradaptive, because it changes documented interaction behavior. - Add a Changeset naming
@astryxdesign/clifor the new discover entries; the lint job's Changeset coverage check fails without it. - Cancel the deferred lift report when the TabBar unmounts.
- Run the interaction proofs inside an existing CI lane instead of the separately routed
pr-interactionjob.
[Automated review]
| } | ||
| }; | ||
| pendingReportRef.current = report; | ||
| afterHandOff(report); |
There was a problem hiding this comment.
Nothing cancels this deferred report. afterHandOff keeps no frame or timeout handle, and no cleanup clears pendingReportRef, so a lift within about 50 ms of the bar unmounting still calls onChange/onReselect (architecture:react-component-runtime INV5). Keep the ids, cancel them in an effect cleanup, and add a test that unmounts inside that window. The three-frame delay itself contradicts INV2 and is waiting on an owner decision.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] `presentation="adaptive"` on DropdownMenu, ContextMenu, MoreMenu, Selector and MultiSelector chooses the bottom sheet on a compact touch screen: a coarse primary pointer and a viewport narrower than 744px or shorter than 500px (it was 768px wide and below). An iPad mini in portrait keeps the anchored popover; a phone in landscape now gets the sheet. |
There was a problem hiding this comment.
This changes documented interaction behavior of the released presentation="adaptive": the docs promised the sheet at 768px and below with a coarse pointer, and an iPad mini in portrait now gets a popover while a landscape phone gets a sheet. spec:AST-017 FR3 and FR25 classify that as breaking outside a declared minor. Ship it as [breaking] in a declared minor, saying how to keep the old rule, or keep 768px for adaptive.
| }, | ||
| { | ||
| "kind": "component", | ||
| "name": "TabBar" |
There was a problem hiding this comment.
This changes what astryx discover lists, but no Changeset names @astryxdesign/cli, so the lint job's "Check Changeset coverage" step fails on this file. A [feat] Changeset for @astryxdesign/cli fixes it.
| # them, so a change to an instrument is proven by its negative controls | ||
| # before a component spec relies on it. Chromium only on these runners; | ||
| # see the harness README for running WebKit. | ||
| pr-interaction: |
There was a problem hiding this comment.
pr-interaction routes itself through its own scope check, but the specs it runs live inside the Core and Lab surfaces. spec:AST-030 FR11 says new coverage inside an existing surface joins that surface's owner lane and must not create a separately routed lane, and FR10 says a new test type is not a new surface. Run these specs from the lane that already owns the Storybook build they read, or admit this lane through an owner-approved FR10 amendment.
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: No accessibility violations detected. Visual Regression55 of 502 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 35 more. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
9ad18b5 to
35806b6
Compare
There was a problem hiding this comment.
This pull request can't be approved yet: several of its additions have no current spec authority, and two of its changes contradict current records. An engineering owner needs to decide on the draft spec:AST-094 requirements, the TabBar's 10 pt drag slop, the new @astryxdesign/core/motion/experimental and @astryxdesign/core/gestures/experimental subpaths, the new presentation="adaptive" threshold (which contradicts component:Selector DEC-1) and delivering onChange three frames after the lift (which contradicts architecture:react-component-runtime INV2); approval is recorded by commenting /approve-spec <full head SHA> on this pull request, which the author may do as an engineering owner, and the two contradicted records need an amendment on main or the change reverted. Whatever the owners decide, these fixes are needed:
- Land the stacked predecessors, or retarget onto #8129, so this diff is its own 1,342-line commit instead of 28,753 authored lines.
- Either ship the adaptive threshold as
[breaking]in a declared minor or keep the documented 768px rule foradaptiveand sub-menu drill-in. - Let a finger choose a tab when none is selected: the press now claims the touch and swallows the click, but
beginandendreturn early when nothing is selected. - Cancel the deferred lift report when the TabBar unmounts, and capture the chosen tab at the lift.
- Run the interaction proofs inside an existing CI lane instead of the separately routed
pr-interactionjob.
[Automated review]
| const element = capsuleRef.current; | ||
| const selected = selectedRef.current; | ||
| const root = element?.parentElement; | ||
| if (element == null || root == null || selected < 0) { |
There was a problem hiding this comment.
With no tab selected (selectedIndex < 0, which TabBar.tsx handles by giving the first tab the tab stop and rendering no lens), this returns before pressRef is set, and end returns too, so onLift never runs. The zero-delay long press has still begun and claimed the touch, and with cancelsTouchesInView defaulting to true the gesture host swallows the tap's click, so a finger can no longer choose any tab in that state, where it reached onClick before. Commit the nearest tab on the lift without the lens motion (or leave the touch to the click), and add an interaction test for a tap with no selection.
35806b6 to
5ac0476
Compare
There was a problem hiding this comment.
This pull request can't be approved yet: several of its additions have no current spec authority, and two of its changes contradict current records. An engineering owner needs to decide on the draft spec:AST-094 requirements, the TabBar's 10 pt drag slop, the new @astryxdesign/core/motion/experimental and @astryxdesign/core/gestures/experimental subpaths, the new presentation="adaptive" threshold (which contradicts component:Selector DEC-1) and delivering onChange three frames after the lift (which contradicts architecture:react-component-runtime INV2); an engineering owner records approval by commenting /approve-spec <full head SHA> on this pull request, which the author may do as an engineering owner, and the two contradicted records need an amendment on main or the change reverted. Whatever the owners decide, these fixes are needed:
- Land the stacked predecessors, or retarget onto #8129, so this diff is its own 1,349-line commit instead of 29,149 authored lines, and restack onto current
main, whose workflow now runs a test this branch predates (thepr-a11yfailure). - Either ship the adaptive threshold as
[breaking]in a declared minor or keep the documented 768px rule foradaptiveand sub-menu drill-in. - Let a finger choose a tab when none is selected: the press now claims the touch and swallows the click, but
beginandendreturn early when nothing is selected. - Cancel the deferred lift report when the TabBar unmounts, and capture the chosen tab at the lift.
- Run the interaction proofs inside an existing CI lane instead of the separately routed
pr-interactionjob.
[Automated review]
|
Posted by Cindy's agent on her behalf. Parts of this PR re-implement behavior that already exists, or is being specified, as a shared part of core. Converging keeps one implementation per behavior:
This is about where the behavior lives, not about the PR's intent. |
|
Agreed: one implementation.
|
…-touch-model restack)
5ac0476 to
a750d0f
Compare
github-actions
Bot
added
the
needs:design-review
…th its last marked element The touch-action writer records which elements it marked in each document or shadow root, and when the last one is unmarked (its last recognizer detached) it removes that root's adopted sheet or style element, so nothing touch-related remains after the last instance unmounts (spec:AST-094 FR24). A later mark writes the rules again; an element marked in another root first lets go of the old one.
… the inline style it reads - The motion registry, the host's engine gate and the README name the presented hit test as an engine workaround (spec:AST-013 FR6) with its review condition (re-measure on each iOS release; remove it once iOS WebKit hit-tests a compositor-moved element where it is drawn) and its regression fixture. - While a curve runs the animator holds the value it starts from inline, so presentation.ts and the README no longer say the inline style holds the end.
…core/internal/touch Core ships the internal entry @astryxdesign/core/internal/touch, which re-exports the gesture primitives for Astryx's own packages, and the Lab-import guard fails a Lab import of core outside core's exports (architecture:react-component-runtime INV13). The README and the barrel's header named internal workspace imports and said nothing here is exported; they now name that entry.
…ill needs the element, and the listeners that cancel The release note, the stream's header, the host's comment and the README said the stream is one passive listener per event type and that a document-capture stopPropagation no longer starves the recognizers. Such a stop never hid a listener on the same node, and some subscriptions cancel. They now say what changed: an app's window-capture stopPropagation, or a document-capture stopImmediatePropagation registered before core, no longer hides a touch; a recognizer that must claim still needs the touch to reach its element; the gesture host's touchend, click and contextmenu cancel while a claimer is attached and the page hold's touchmove while it holds; and the stream stays ahead of an app's window-capture listener only until a type's listener is re-added. The README's list of blocking touchmove listeners gains the claimed touch's target and the document while a claiming element moves.
…default it moves
The {stylexOptions} form of astryxStylex now passes StyleX
treeshakeCompensation: true unless the caller sets it. spec:AST-017 FR3
asks the release note to name the old and new default and the option
that restores the old one; it now does.
…er and for the two stops the input stream fixes - A still finger with sub-pixel jitter over a scroll view keeps its tap and scrolls nothing (spec:AST-094 FR18–FR22). - A tap is still recognized under an app's window-capture stopPropagation, and under a document-capture stopImmediatePropagation registered before core's: the recognizers hear touches on the window's capture phase. Chromium and WebKit pass.
…s once Where nothing that claims is attached, the host's touchend listener is passive and cannot cancel the engine's own click after a retargeted tap clicks what is drawn. The shared click guard swallows that late click whatever is attached; this case pins it, as asked on meta#8293.
…esture host's subscriptions The gesture host subscribes no click: the one cancelling click subscriber is the click guard's, added at a window's first arm and kept. The release note, the stream's header and the gestures README listed click with the host's touchend and contextmenu; they now name the guard's own.
…changed) bbc0a4a is the merge of the heads this pull request builds on. With it as an ancestor, GitHub's compare from it shows exactly this pull request's own files.
…ps reach it Red first: with no label the tab list has no name, and aria-label or aria-labelledby passed to TabBar land on its root, where they name nothing.
As TabList does, the tab list is named by aria-labelledby, else aria-label,
else label, else the translated `@astryx.tabList.label` ("Tabs"), and the
aria naming props go to the tab list instead of the root.
spec:AST-094 FR4 names none of the tab bar's values, and Core's roles test pins the registered roles to exactly FR4's list, so registerTabBarTable() lists no motion role and each spring plays as measured, as the scroll and navigation tables do. The comments and the changeset no longer call them motion roles.
The comments it rewrote say, at this layer's wording too, that the tab bar's springs play as measured; the lens's selection Effect says what it synchronizes.
…nges under it
Red first: four tabs shrinking to three under a finger held on the last one
throw at the lift ("Cannot read properties of undefined (reading 'value')"),
since the lift resolves its index against the new list.
…ges under it A press measured the tab centres of the list it began on. When the list changes under a held finger, the press now ends there as a cancel and the lens settles back to rest, so no move chases the old centres and the lift never hands onLift an index into the new list.
The merge of meta#8129's fixes ran Prettier over every file it staged, which reformatted 17 files this stack does not change (docs, scripts and a test from main and the gesture foundation). They are back to the foundation's text.
The tap and the second finger's landing went out 20 ms apart on the test's wall clock, which a loaded box stretched until the lens had reached Profile before the finger landed. They now go back to back, and the hold is a wait after the landing.
There was a problem hiding this comment.
Approval waits on an engineering-owner decision about TabBar's lens copy: drawing every tab a second time inside the lens while a finger is down needs an architecture:component-style-authoring INV7 / spec:AST-017 FR60 exception recorded with its counts (176 to 198 elements on the Default story), or dropping the half-tint. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Whatever the decision, these still need changes:
family:action-groupsnow counts a tab bar as a member, so TabBar's press should run on Core'sactionGroupTracker(architecture:interaction-modalityINV13) for every pointer (FR5,spec:AST-094FR43), and a lift off the bar should act on nothing (FR3).- The gesture host attaches nothing under a fine pointer and tells authors to keep gestures touch-only, which FR19 as amended and FR43 rule out, and
perEventPointerrests on an FR19 exception that was never added. - Most of the 31,986 changed lines are the stack beneath #8129; landing it separately would leave this pull request its own TabBar layer.
[Automated review]
| } | ||
| }, | ||
| { | ||
| isEnabled: isTouch, |
There was a problem hiding this comment.
Under a fine pointer this turns the press off, so a mouse, a trackpad, or a pen or finger on a fine-pointer device gets only the click, and a press slid from one tab to another chooses nothing. family:action-groups names a tab bar among the components that join by its rule, and its FR5 and spec:AST-094 FR43 keep press, slide and lift on for every pointer; architecture:interaction-modality INV13 gives that tracking to Core's actionGroupTracker, whose resolveTarget and onMove already describe a row of segments and a lens that chases the finger.
| await expect(tab(page, 'Library')).toHaveAttribute('aria-selected', 'true'); | ||
| }); | ||
|
|
||
| test('a drag that leaves the bar upward still commits where it lifts', async ({ |
There was a problem hiding this comment.
This pins a lift above the bar committing a tab, while family:action-groups FR3 says a press that lifts outside the group acts on nothing; this case and motion/uikit/tabBar.ts:26-28 ("leaving the bar does not cancel") would change with the move to the shared tracker.
| ref={lens.clip} | ||
| data-tab-bar-lens-clip="" | ||
| {...stylex.props(styles.lensClip)}> | ||
| {lens.isCopyShown ? ( |
There was a problem hiding this comment.
While isCopyShown is true every tab is drawn a second time inside the lens (176 to 198 elements on the Default story by your count). architecture:component-style-authoring INV7 and spec:AST-017 FR60 allow that only through an owner exception recorded with those counts; the open question asks an engineering owner to record it or to drop the half-tint.
| } | ||
|
|
||
| /** Something attached where the primary pointer is fine: say so once. */ | ||
| private warnFinePointer(): void { |
There was a problem hiding this comment.
spec:AST-094 FR19 now reads "Recognizers attach for every pointer, with FR43's exceptions", and DEC-21 rejects keeping gestures and press tracking touch-only, so this warning, the gate at :12-22 and :911-912, and the coarse-only selection lock point authors the other way. Attaching for every pointer, with FR43's exceptions, would match the record.
| * browser, which still pans, and no mouse or pen press. Everything else | ||
| * stays inactive there. @default false | ||
| */ | ||
| perEventPointer?: boolean; |
There was a problem hiding this comment.
The FR19 press-feedback exception this cites was never added: FR19 was amended to attach recognizers for every pointer instead. No component passes this option, and its press tracker takes no mouse or pen press, the touch-only press tracking DEC-21 rejects; removing it is the smallest fix.
| } | ||
| }, [isTouch]); | ||
|
|
||
| // The tab list changed under a held finger: the press ends here, as a |
There was a problem hiding this comment.
architecture:react-component-runtime INV3 asks a new Effect to name the external system it synchronizes; naming the recognizer's press and the lens's animations here, as the selection Effect below does, would cover it.
|
Thanks, fixed in this push:
The copy of the items and the INV7 count are an owner decision. |
The Effect that ends a press when the recognizer turns off, and the one that ends it when the tab list changes under a finger, now say what they synchronize, as the selection Effect does.
|
Thanks. Fixed in this push: both press Effects now say what they synchronize, as the selection Effect does. The lens copy is an owner decision. The press under a mouse comes with #8703, and whether TabBar follows the action-groups family is asked on #8129. |
There was a problem hiding this comment.
This pull request can't be approved yet: two departures from current spec wait on an engineering-owner decision, which any engineering owner, the author included, records by commenting /approve-spec <full head SHA> on this pull request. The owners need to settle whether the lens may draw every tab a second time while a finger is down (an architecture:component-style-authoring INV7 / spec:AST-017 FR60 exception with its counts, 176 to 198 elements on the Default story) or should drop the half-tint, and whether TabBar may keep its own press tracker and commit a lift above the bar (an exception to family:action-groups FR2 and FR3 and architecture:interaction-modality INV13) or should follow the family. Whatever they decide, these still need changes:
- The press is touch-only, while
spec:AST-094FR43 and DEC-21 keep gestures on for every pointer; bring the mouse and pen press into this pull request or land it together with #8703. - The gesture host's coarse-only gate and its development warning follow
spec:AST-094FR19 as it read before its amendment, andperEventPointercites an FR19 exception that was never added. - The diff carries 31,988 authored lines, 30,484 of them the stacked predecessors; land those first or retarget onto #8129's branch, and split the
astryxStylexchange.
[Automated review]
Depends on #8129.
Review only this PR's change: vjeux/astryx@94bba42...ff6c905
User need
Part of matching Astryx's touch components to iOS 26, so apps built with it feel native on phones. On iOS a finger drags the tab bar's lens: it lifts, follows the finger and drops on another tab as the bar flexes. #8129 answers only a finished tap; a drag from the bar scrolls the page. This adds it, measured on iOS 26.2.
Problem and solution fit
Current specification
spec:AST-094FR17 (pans begin at 10 pt), FR13, FR25, FR8;spec:AST-017FR21, DEC-27;architecture:interaction-modalityINV10;architecture:react-component-runtimeINV2 (the lift reports the choice).Public delta and caller burden
No API change;
uikit.tabBargains the press values andinternal/touchgainslockSelectionanduseGestureRecognizer.Size, gzip against #8129's head: TabBar's import 40.3 → 59.4 KB JavaScript, 1,524 → 1,648 B CSS.
Design and alternatives
Evidence
iOS 26.2 simulator (a drag; Reduce Motion), then Android 15 Chrome.
Scope
Stack: #8360 → #8129 → this PR.
Testing
Try it:
pnpm storybook, Lab › TabBar › Default, on a phone or Chrome's device mode.Browser tests: the motion follows UIKit's springs; a drag from the bar never scrolls the page; the lift reports the tab as the settle starts.
Known limit: a main-thread block as the finger lands holds the press until it ends.
Passing:
pnpm test:interaction TabBar,pnpm vitest run packages/lab/src/TabBar,pnpm typecheck, lint.