Repository navigation
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.
|
github-actions
Bot
added
the
needs:design-review
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval needs an engineering owner's decision first: the draft spec:AST-094 and the new holdSelection selection hold for List, Item and TreeList rows have no approved spec yet, so an engineering owner (the author included, if they are one) needs to approve or amend them by commenting /approve-spec followed by the full head commit SHA on this pull request. These changes are needed whatever that decision is:
- Retarget or rebase this pull request so its diff is its own commit; against
mainit carries 13,883 authored lines from the whole stack. - Let the dividers beside a held row return with the pop's deselect instead of reappearing in one frame at
end(true). - Make
focus()on a TreeList row focus itstreeitem, not the row'stabindex="-1"button. - Fix the
spec:AST-094amendment ofspec:AST-090, which has no record onmain.
[Automated review]
| borderBlockEndColor: { | ||
| default: colorVars['--color-border'], | ||
| '[data-astryx-press="on"]': 'transparent', | ||
| '[data-astryx-press="held"]': 'transparent', |
There was a problem hiding this comment.
While a row is held this keeps both dividers transparent until end() removes the attribute, so they never take part in the deselect; uikit/list.ts:52-61 measures UIKit fading the separators with the background over the same 350 ms. Could the dividers follow the hold's progress, or come back when the pop starts?
| } | ||
| return ( | ||
| row.querySelector<HTMLElement>( | ||
| 'a[href], button:not([disabled]), [tabindex]:not([tabindex="-1"])', |
There was a problem hiding this comment.
Inside a TreeList this matches the row's tabIndex={-1} label button (or the chevron toggle), not the role="treeitem" that holds the roving Tab stop, so focus() leaves focus outside the tree's focus model. Focusing the closest treeitem would match the TreeList contract.
| 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.
There is no AST-090 record on main or in this branch, so this amendment has no target an owner can rule on.
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsItem (@astryxdesign/core) · View in Storybook
TreeList (@astryxdesign/core) · View in Storybook
Component 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: 2 accessibility violation(s) found — 2 serious. Radio group pattern - 1 issue(s)
RadioList - 1 issue(s)
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 |
d1e37ac to
82f5c78
Compare
82f5c78 to
e7922b8
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision: the draft spec:AST-094 and the new holdSelection selection hold for List, Item and TreeList rows have no approved spec yet, so an engineering owner needs to approve or amend AST-094 (FR1–FR23, DEC-1, DEC-2) and provide or identify the record that governs the held row and its @astryxdesign/core/utils export, recording the decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so if they are an engineering owner). Independent of that decision, these need changes:
- Retarget the pull request onto its predecessor's branch, or rebase it once the stack lands, so the diff is this commit's ~1,600 lines rather than the stack's ~14,800.
- In a TreeList,
SelectionHold.focus()lands on the row'stabindex="-1"button instead of the treeitem that holds the tree's roving Tab stop (selectionHold.ts:163-172). - In a divided List, the dividers beside a held row stay hidden through the whole pop, although
motion/uikit/list.tsmeasures UIKit fading them with the row. - AST-094 FR7 amends an AST-090 record that does not exist.
[Automated review]
e7922b8 to
bf924aa
Compare
…election hold change
|
Posted by Cindy's agent on her behalf. Cindy's decision: our own components use the public gesture and motion APIs wherever they cover the job. That means The public factories and hooks are being specified now ( |
An attach reads the element's own will-change in a task of its own. A shared animator (animatorFor) is never detached, so its node can leave the document with that read still waiting, and the read then forces a style pass on a node with no style: a test run that mounts and unmounts many pagers piled up such reads until a later wait timed out. The read now skips a node out of the document; a promotion that needs it reads it then, as one on a node attached in the same task does. A detach and dispose already cancel it. The new test fails on the previous code (one read of a removed node); the in-task read test now puts its element in the document, where the read happens.
There was a problem hiding this comment.
Approval needs an engineering owner to approve, or to provide or identify the governing spec for, the List and TreeList row behavior added here, which no current spec covers: a row held after a tap until the pop deselects it, the dividers hidden beside a pressed or held row, and the press layer replacing a caller's own ::before. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so, being an engineering owner) or by amending component:List, component:Item and component:TreeList; the 350 ms deselect played as an internal constant also needs spec:AST-094 FR4 updated to allow it. Independently, please fix:
focus()after a pop lands on a row's first start-content control instead of the row's own control, and in a TreeList it leaves the Tab stop on a different row.- The changeset bumps
@astryxdesign/clialthough no CLI file changes. - The diff still carries #8076, #8077 and #8079 (19,965 authored lines), so land or retarget them until it shows only the hold.
[Automated review]
| return row; | ||
| } | ||
| return ( | ||
| row.querySelector<HTMLElement>( |
There was a problem hiding this comment.
This takes the first focusable in DOM order, and Item renders startContent before its own button or anchor, so a row with a control in startContent gets that control focused after the pop instead of the row’s own control. Could it target Item’s own control, with a startContent case in selectionHold.test.ts?
| function focusTarget(row: HTMLElement): HTMLElement { | ||
| const item = row.closest<HTMLElement>('[role="treeitem"]'); | ||
| if (item != null) { | ||
| return item; |
There was a problem hiding this comment.
Focusing the treeitem does not move the roving Tab stop: useTreeFocus’s handleFocus keeps whichever item already has tabindex="0", so Shift+Tab back into the tree lands on that other row (component:TreeList AR1). Could the hold move the stop to this row, with a tabIndex assertion in the tree test?
| @@ -0,0 +1,8 @@ | |||
| --- | |||
| '@astryxdesign/core': patch | |||
| '@astryxdesign/cli': patch | |||
There was a problem hiding this comment.
No CLI file changes in this pull request, so this line only adds a List note to the CLI’s changelog. Drop it?
| it off owns that end (no consumer callback from an Effect). A handler that | ||
| wants UIKit's `.cancelled` calls `recognizer.setEnabled(false)` itself, | ||
| which reports it at once. | ||
| - `usePressTracker` rebuilds its tracker when `trackingExpansion`, |
There was a problem hiding this comment.
usePressTracker builds its PressTracker once in useState with the first option values (useGestureRecognizer.ts), so changing these options later has no effect. Either rebuild on change or correct this line.
…(spec:AST-094 FR18, DEC-13) A screen-edge pan attached to a stack container, as wide as the screen, with a touch 4 px from its edge: inside the 40.2 px band the pan decides at UIKit's 15 px whatever the element's width, and then tracks the travel past it. From several edges too. Both fail while a wider element decides at FR19's 10 px.
…'s width (spec:AST-094 FR18, DEC-13) FR18: inside the 40.2 px band a screen-edge pan decides at UIKit's 15 px screen-edge hysteresis; outside it the author's scrolling is untouched and FR19's 10 px governs every other claim. The pan decided at 10 px on any element wider than the band, so NavigationStack's edge pan, attached to the stack container, began 5 px early. The edge zone still decides only the touch-action; a touch outside every band still fails at once. The two cases that pinned the 10 px decision go.
…er's move keeps that finger until it lifts) into the experimental touch exports
…than the band (spec:AST-094 FR18, DEC-22) FR18 as amended in meta#8631 (DEC-22): the band's pan-x claim and 15 px start hold only where the screen-edge pan is attached to an element within the band; on an element that reaches beyond it, whose touch-action cannot claim per touch, a touch in the band decides at FR19's 10 px like every other claim. This restores the 10 px decision 3d85c30 removed and keeps 15 px on an element within the band. The cases pin DEC-22 both ways (a wide element with a touch at x=4 begins at 10, an in-band strip at 15, and from several edges at 10 on a wide element); the 10 px cases fail on 3d85c30.
…xperimental touch exports The motion README's Files table is the union: main's rows, with meta#8663's resolveSnapPoint.ts and selectPage.ts, plus this branch's, in main's order. # Conflicts: # packages/core/src/motion/README.md
… rows united) into the row press release change
…election hold change
There was a problem hiding this comment.
Approval needs an engineering owner to approve, or to provide or identify the governing spec for, the List and TreeList row behavior added here, which no current spec covers: a row held after a tap until the pop deselects it, the dividers hidden beside a pressed or held row, and the press layer replacing a caller's own ::before. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner) or by amending component:List, component:Item and component:TreeList; the 350 ms deselect played as an internal constant also needs spec:AST-094 FR4 updated to allow it. Independently, please fix:
focus()after a pop lands on a row's first start-content control instead of the row's own control, and in a TreeList it leaves the Tab stop on a different row.- The changeset bumps
@astryxdesign/clialthough no CLI file changes. - The diff still carries #8076, #8077 and #8079 (20,137 authored lines), so land or retarget them until it shows only the hold.
[Automated review]
| if (row.matches('a[href], button, [tabindex]:not([tabindex="-1"])')) { | ||
| return row; | ||
| } | ||
| return ( | ||
| row.querySelector<HTMLElement>( | ||
| 'a[href], button:not([disabled]), [tabindex]:not([tabindex="-1"])', | ||
| ) ?? row | ||
| ); |
There was a problem hiding this comment.
Outside a tree this takes the first link, enabled button or tabbable element in DOM order, and Item renders startContent (Item.tsx:752-760) before its own anchor (:775) or button (:789), so a row with a control in startContent gets that control focused after the pop instead of the row's own control. Target the row's own control and add a startContent case to selectionHold.test.ts.
| const item = row.closest<HTMLElement>('[role="treeitem"]'); | ||
| if (item != null) { | ||
| return item; | ||
| } |
There was a problem hiding this comment.
Focusing the treeitem does not move TreeList's roving Tab stop: handleFocus calls syncTabStops, which keeps whichever item already has tabindex="0" (useTreeFocus.ts:364-372). After a pop to another row, Shift+Tab back into the tree lands on that other row (component:TreeList AR1). Move the stop to this row and assert tabIndex in the tree case of the test.
| @@ -0,0 +1,8 @@ | |||
| --- | |||
| '@astryxdesign/core': patch | |||
| '@astryxdesign/cli': patch | |||
There was a problem hiding this comment.
No file under packages/cli changes in this pull request, so this line only adds a List note to the CLI's changelog; dropping it leaves the core entry.
| - `usePressTracker` rebuilds its tracker when `trackingExpansion`, | ||
| `highlightDelay`, `cancelTravel` or `quickTapFlash` change, from the next | ||
| touch, and reads its callbacks (`isBraking` included) on every touch. |
There was a problem hiding this comment.
usePressTracker builds its PressTracker once in useState with the first option values (useGestureRecognizer.ts:136-156), so changing these options later has no effect. Rebuild on change or correct this line.
|
Offer, following the owner's ruling in #8687's review (no new press treatment, so the dividers stay drawn): one commit on this head that takes the dividers part out — vjeux/astryx@3e73461...1d9c7ec. It removes the divider arms beside a pressed or held row, the press layer's reach over a hidden divider, and the deselecting mark that only those arms read, with their tests, docs and changeset; the selection hold is unchanged. Red first: build-css asserts no touch arm hides a divider. Green: List, Item and selection-hold units, and the hold and press-path browser specs in Chromium and WebKit. Merge it or leave it, as you prefer. |
WebKit starts the row's release at the commit after a 150 ms block, as Chromium does (first frame 0.958; the bar is 0.7), so the proof no longer skips WebKit.
…ns in WebKit too) into the selection hold change
…tart instead of skipping it WebKit may pin a motion created just before a block to the last frame before it (some runs do, some start at the commit), so the first frame after a 150 ms block can be up to about 150 ms into the 350 ms deselect; Chromium starts it at the commit after the block. The proof runs in both engines: it bounds each engine's first frame, and in both every later frame moves on toward rest with none skipped, ending at rest.
The animator settles a motion on its own timer at the curve's end, by wall time, and holding the animations does not hold that timer, so a seek loop that outlasted the pop ended the hold mid-loop and read the row at rest. The proof gates the page's timers before the pop and freezes them while it seeks, as the row press release's curve proof does. Red first: with more wall time than the pop passing during the hold, every sample read 0 in both engines without the gate.
…elease After 150 ms of script at the release's write, the release starts after the block; the press attribute must stay until the layer has reached rest on the curve, not go while the layer is still painted at about half strength (the release's clock started at the write).
…busy first frame outran pressFeedback.ts removes data-astryx-press="fading" on a timer of the release's duration started at the write, for the controller's release and for paintPress's. These fail on that code: with a release animation still running, or still waiting for its first frame, when the clock runs out, the release must stay fading until the animation finishes. Guards: a cancelled animation ends it; no animation ends it on the clock (reduced motion's instant release, a surface without a press layer); a descendant's, an endless or a finished animation is not waited for; a new press keeps its paint when the old release ends later.
…y first frame started it endAfterRelease replaces the clock that removed data-astryx-press="fading" at the release's duration after the write, for the controller's release (apply's fade) and for every other painter's (paintPress). The clock now only looks: at the release's duration, a release animation still running on the surface (one of its own CSS animations, on it or its ::before or ::after, finite and unfinished) is waited for until it finishes or is cancelled, so a busy main thread that starts the animation late ends it late too, at strength 0. With none (reduced motion's instant release, a surface without a press layer, an engine without Web Animations) the release ends on the clock, as before. Nothing at the write reads the document timeline: WebKit starts an animation created after such a read at the time read, which would skip a busy first frame into the release. Any new write, or a new press, cancels the wait; the controller's pending clock is now a cancel function, so the release's wait and every timer clear the same way.
…on does) into the selection hold change
There was a problem hiding this comment.
Approval needs an engineering owner to approve, or to provide or identify the governing spec for, the List and TreeList row behavior that no current spec covers: a row tapped to push a view stays painted until the pop deselects it, its 350 ms deselect is played as a constant although spec:AST-094 FR4 asks for a motion role the List record measures, and the press layer replaces a caller's own ::before, which component:List OQ1 leaves open. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner) or by amending component:List and component:TreeList on main. Whatever the decision, please:
- Drop the divider hiding beside a pressed or held row, which the owner ruling relayed here rules out (the offered commit does this).
- Return focus after a pop to the row's own control instead of a
startContentcontrol, and make the returned TreeList row the Tab stop. - Remove the
@astryxdesign/cliline from the hold's changeset and correct theusePressTrackerREADME note. - Land or retarget #8076, #8077 and #8079 so the diff (20,443 authored lines) shows only the hold.
[Automated review]
| return row; | ||
| } | ||
| return ( | ||
| row.querySelector<HTMLElement>( |
There was a problem hiding this comment.
Outside a tree this takes the row's first link, enabled button or tabbable element, and Item renders startContent (Item.tsx:752-758) before its own anchor (:775) or button (:789). A probe with startContent={<button>Star</button>} puts focus on Star after the pop. Could the hold remember the control the activation went through and focus that, with a startContent case in selectionHold.test.ts?
| function focusTarget(row: HTMLElement): HTMLElement { | ||
| const item = row.closest<HTMLElement>('[role="treeitem"]'); | ||
| if (item != null) { | ||
| return item; |
There was a problem hiding this comment.
Focusing the treeitem does not move TreeList's roving Tab stop: handleFocus only re-syncs and keeps the item that already has tabindex="0" (useTreeFocus.ts:364-372), so Shift+Tab back into the tree lands on another row (component:TreeList AR1). Move the stop to this row and assert tabIndex in the tree test?
| @@ -0,0 +1,8 @@ | |||
| --- | |||
| '@astryxdesign/core': patch | |||
| '@astryxdesign/cli': patch | |||
There was a problem hiding this comment.
No file under packages/cli changes here, so this line only adds a List note to the CLI's changelog; dropping it leaves the core entry.
| // fading. A selection hold (the row kept selected through a navigation | ||
| // push) hides them the same way until its deselect begins, and again | ||
| // after a cancel. With a mouse the dividers stay drawn. | ||
| borderBlockEndColor: { |
There was a problem hiding this comment.
The owner ruling relayed on this pull request keeps today's pressed overlay with no new press treatment, so the dividers stay drawn; these coarse-pointer arms, the ::before reach below and the hasDividers docs would go with the offered removal commit.
| it off owns that end (no consumer callback from an Effect). A handler that | ||
| wants UIKit's `.cancelled` calls `recognizer.setEnabled(false)` itself, | ||
| which reports it at once. | ||
| - `usePressTracker` rebuilds its tracker when `trackingExpansion`, |
There was a problem hiding this comment.
usePressTracker builds its PressTracker once in useState from the first option values (useGestureRecognizer.ts:136-156), so changing these options later has no effect. Rebuild on change or correct this line.
|
Posted by Cindy's agent on her behalf. Thanks for the careful UIKit work on this. Cindy decided not to change anything here. A product that wants a row to stay selected while its page is open sets ListItem's existing |
Review only this PR's change: vjeux/astryx@3ad772a...7e0020c (depends on #8079; the base adds #8077's release fix, which this head carries).
User need
On iOS, a row tapped to open a detail stays highlighted while covered and fades as the detail slides away, so coming back you see where you were. In Astryx it faded at the lift.
Problem and solution fit
What led to it: this is part of a program to make Astryx's list components behave like iOS tables on phones. Lab's NavigationStack (#8114) pushes a detail over a list, and nothing held the row.
A navigation can now hold the tapped row selected and drive its fade with its own transition: the highlight stays while the detail is open, follows a back swipe, returns on a cancel, and ends with focus on the row. Touch screens only (
spec:AST-094FR13); Reduce Motion fades within 200 ms.Size (gzip JS/CSS, base → head): Item +230/+51 B, ListItem +85/+122 B, TreeList +270/+51 B.
Current specification
Pending the owner's decision (
component:List), following UIKit on iOS 26.2.Public delta and caller burden
None:
holdSelectionis internal, and a test pins the public exports. Interactive rows carrydata-astryx-pressable="cell"(was"").Design and alternatives
selectedprop: every app wires it to its navigation, and swipes and cancels are easy to get wrong.Known gap (
spec:AST-107): the hold's paint stays on internal press feedback.Evidence
iOS 26.2 simulator, Mobile Safari, touch input; a real device is the release gate.
Scope
Testing
What was run: these steps on the iOS 26.2 simulator (the clips above), and unit and browser tests of each in Chromium and WebKit, busy main thread included. All pass; the new ones failed first. CI's pr-rtl "Verify D7 native target regression" fails only because this head predates main's new RTL touch-target test (0472c4e); it clears when the branch next takes main.