Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
github-actions
Bot
added
the
needs:design-review
|
AI review status for this pull request.
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: the draft spec:AST-094 adds new touch-presentation requirements, and the two row changes you marked "Proposed; pending owner review" (an interactive row's ::before becoming the press layer, and the untinted hasDividers divider) have no governing spec yet. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, which you can do yourself as an engineering owner, or by amending the Item, List and TreeList records. Whatever that decision, these need changes before merge:
RadioListoptions now tint the whole row under a finger and their indicator loses the pressed overlay thatdesign:user-statesDR1 requires, becauseItemadds the press layer toRadioListItem's delegate row.- The changeset says a caller's
::beforereplaces the press paint, but withxstylethe caller's::beforeinherits the layer'sopacity: 0and positioning, so the caller's decoration disappears instead. - At 12,456 authored lines this carries ten separable predecessor commits; landing them first would leave only the row fix here.
[Automated review]
| // The touch press on a compositor layer (after the overlay, whose | ||
| // own touch paint it turns off): the release keeps fading while | ||
| // script runs. | ||
| isInteractive && pressLayerStyles.cell, |
There was a problem hiding this comment.
This also reaches RadioListItem, whose row is interactive through interactiveRef. There --_press-paint: transparent empties the radio indicator's press layer (it reads --_press-paint-image) and the new ::before, which rowStyles.root does not neutralize, tints the whole row, so under a finger the indicator no longer paints --color-overlay-pressed as design:user-states DR1 requires for RadioList. Keeping the layer off rows whose press paints elsewhere, plus a RadioList case in pressLayer.test.ts, would fix it.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] A list row's touch press release keeps fading while the page runs script. `ListItem`, `Item` and `TreeList` rows used to stay fully pressed after the finger lifted until the main thread was free, and every frame of the fade repainted the row; the release now runs on the compositor, with the same color, clock and curve as before. On an interactive row the press paints on the row's `::before`, so a row's own `::before` set through `xstyle` or `className` replaces the press paint. |
There was a problem hiding this comment.
With xstyle, StyleX merges a caller's ::before with the layer per property: the caller's pseudo-element keeps the layer's opacity: 0, position: absolute, inset: 0, z-index: -1 and pressed background. So it is the caller's decoration that disappears (and turns into a press tint), not the press paint that is replaced; the note should say that.
| 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 does not exist on main, so this amendment (and the matching line in Current-state impact) points at no record; cite the record that exists or drop the clause until it lands.
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 504 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 |
d104ec1 to
de861e0
Compare
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: the draft spec:AST-094 adds new touch-presentation requirements, and the two row changes you marked "Proposed; pending owner review" (an interactive row's ::before becoming the press layer, and the untinted hasDividers divider) have no governing spec yet. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, which you can do yourself as an engineering owner, or by amending the Item, List and TreeList records. Whatever that decision, these need changes before merge:
RadioListoptions now tint the whole row under a finger and their indicator loses the pressed overlay thatdesign:user-statesDR1 requires, becauseItemadds the press layer toRadioListItem's delegated row.- The changeset says a caller's
::beforereplaces the press paint, but withxstylethe caller's::beforeinherits the layer'sopacity: 0and positioning, so the caller's decoration disappears instead. lintruns out of heap at this head, andspec:AST-094and the new motion docs cite records and files that do not exist (spec:AST-090,packages/core/src/gestures/README.md,sources.test.ts).- At 13,376 authored lines this carries ten separable predecessor commits; landing them first would leave only the row fix here.
[Automated review]
| * tap-to-click, long press, double tap and the start of a page scroll, so | ||
| * many guesses can be settled by measuring Safari itself. | ||
| * | ||
| * SYNC: When modified, update packages/core/src/gestures/README.md (the |
There was a problem hiding this comment.
packages/core/src/gestures/README.md and the ../../gestures module named in this header (line 7) do not exist at this head, and scroll.ts, springs.ts and timing.ts credit a sources.test.ts that is really uikit/uikit.test.ts. Point these at files that exist, or drop the clauses until the gesture recognizers land.
de861e0 to
0987165
Compare
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: the draft spec:AST-094 adds new touch-presentation requirements, and the two row changes you marked "Proposed; pending owner review" (an interactive row's ::before becoming the press layer, and the untinted hasDividers divider) have no governing spec yet. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, which you can do yourself as an engineering owner, or by amending the Item, List and TreeList records. Whatever that decision, these need changes before merge:
RadioListoptions now tint the whole row under a finger and their indicator loses the pressed overlay thatdesign:user-statesDR1 requires, becauseItemadds the press layer toRadioListItem's delegated row; on its own this pull request ships that regression.- The changeset says a caller's
::beforereplaces the press paint, but withxstylethe caller's::beforeinherits the layer'sopacity: 0and positioning, so the caller's decoration disappears instead. spec:AST-094and the new motion docs cite records and files that do not exist (spec:AST-090,packages/core/src/gestures/README.md,sources.test.ts,uikit/switch.ts,uikit/contextMenu.ts), and the description has no## Writing rubric resultsfor the new record.- At 13,532 authored lines this carries ten separable predecessor commits; landing them first would leave only the row fix here.
[Automated review]
…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.
There was a problem hiding this comment.
Changes requested: the list-row release fix itself holds up (under a finger the highlight now keeps fading through script, held, resting and mouse rows match main, and the ::before and divider trade-offs follow the engineering owner's decision on this pull request), but the branch still carries all of #8890 with #8360 and #8076, about 27,600 of its 29,400 changed lines. Please rebase onto main once #8360 and #8890 land (or retarget the pull request) so the diff is the 18-file, 1,799-line row-press change.
[Automated review]
…tion constant From review on meta#8890: spec:AST-094 FR4 lists the 200 ms Reduce Motion cap among the touch motion roles and says a component never plays a literal. The release capped its clock at a literal 200 in pressFeedback.ts (REDUCED_MOTION_MAX_MS) and at a literal 200ms in the overlay's reduced release duration. Both now read the shared constant: the controller timing.reducedMotionCrossFadeMs, the stylesheet reducedMotionFadeConsts.duration. Both are 200 ms, so nothing on screen changes; the built stylesheet's cap is still 200 ms. Red first: a unit case that stubs timing.reducedMotionCrossFadeMs at 120 ms found the release still fading at 120 ms on the change before this one; it now ends there.
…Motion constant too The row press layer capped its release under reduced motion at a literal 200ms (CELL_RELEASE_REDUCED), the same cap the review on meta#8890 asked the overlay to read from the shared constant (spec:AST-094 FR4: the 200 ms Reduce Motion cap is a role, never a literal). It now reads reducedMotionFadeConsts.duration, as the overlay does; both are 200 ms, and the built stylesheet's cap is still 200 ms. A unit case holds every reduced-motion cap in the layer and the overlay to the constant. Red first: it found the layer's literal on the change before this one.
…ame window Both 'a dip held before its first frame lands once the hold is …' proofs waited for no running animation, then one frame and a timeout, assuming the next step starts within a frame. The dip lands on a timer due on the call's own clock, though, and the proofs passed only because the evaluate round trips covered the gap: in WebKit on a loaded box the window closed first and the proof read the element mid-dip (opacity 0, translate none; reproduced 1 in 4 with the round trips removed). They now wait on the call's own finished, then on the browser's own animations still running or waiting for their first frame, and read nothing from the animator.
…ment at once and settles then A Reduce Motion dip whose landing runs more than its whole fade back in late (a hold released long after the dip was due) played a 1 ms opacity animation, which WebKit keeps pending until its next frame, and settled the call on a 1 ms timer: the call's finished could resolve while the element still showed opacity 0. The animator's notes say the element appears at once there. Fails now: the landing plays one opacity animation and the call has not settled.
…element at once and settles A Reduce Motion landing that runs at least the whole fade back in late had nothing left of the cross-fade, but still played a 1 ms opacity animation and settled the call 1 ms later. WebKit keeps that animation pending until its next frame, so the call's finished could resolve with the element still at opacity 0. Such a landing now writes the end opacity at once (handing a borrowed one back to the stylesheet), plays nothing, and settles the call then, as the animator's notes say.
There was a problem hiding this comment.
Changes requested: the list-row release fix itself holds up (under a finger the highlight keeps fading through script, mouse and trackpad rows are unchanged, and an engineering owner has accepted the ::before and divider behaviour), but this pull request still carries all of #8890 with #8360 and #8076, about 27,600 of its 29,500 authored lines. Please rebase onto main once #8360 and #8890 land so the diff is the 18-file row-press change, and add a touch browser case that holds and releases a row between two divided rows of a hasDividers list and checks the divider stays drawn, which component:List FR11's verification asks for.
[Automated review]
| import {TOUCH_QUERY} from './touchPresentation'; | ||
| import {timing} from '../motion/uikit/timing'; |
There was a problem hiding this comment.
This import brings the whole UIKit timing table, and through timing.ts's own import the motion role registry module, into the press controller every pressable component loads, for one value. architecture:component-style-authoring INV6 says a shared value table "ships to a component only the rows that component uses". The line comes from #8890 and leaves this pull request with the rebase; it is worth fixing there, for example by reading the one constant without the table, as PRESS_FADE_MS reads pressConsts.
There was a problem hiding this comment.
| expect(look.layer).toBe('none'); | ||
| }); | ||
|
|
||
| test('under a finger, a row at rest is as it was: its own ::before keeps its look and it is no stacking context; the press layer takes the ::before only while pressed', async ({ |
There was a problem hiding this comment.
component:List FR11's verification also asks for "a pressed row between two divided rows" ("a divider hidden during a press" fails), and none of these cases uses a hasDividers list. A touch case that holds and releases a row between two divided rows and checks the divider stays drawn, by pixels and computed border-block-end, through the press and the release would close it.
There was a problem hiding this comment.
Thanks, added at 0d989ae: a touch case holds and releases the middle row of the Interactive story with hasDividers, and checks the dividers above and below it by computed border-bottom and by pixels, while held, halfway through the release, and after it. A page style that hides a pressed row's divider fails it.
…ot the timing table From review (an inline comment on meta#8077 about this line): the press controller, which every pressable component loads, imported the whole UIKit timing table for one value, its reduced-motion cap, while architecture:component-style-authoring INV6 ships a component only the rows of a shared table it uses. It now reads the fade's own export, reducedMotionCrossFade.durationMs, as the animator does; the value is the same 200 ms. Red first: a bundle of pressFeedback.ts carried the table's other rows (stateChangeMs, buttonReleaseMs) on the change before this one; it now carries the fade alone.
…th dividers drawn From review on meta#8077: component:List FR11's verification asks for a pressed row between two divided rows, and no case used a list with dividers. A touch case opens the Interactive story with hasDividers, holds the middle row and releases it, and checks the divider above it and its own divider below, by computed border and by the pixels of each divider's strip, while held, halfway through the release fade and after it; each strip must differ from the row's surface just inside it at rest, so a match is a drawn divider. It passes in Chromium and WebKit. A page style that hides a pressed row's divider fails it while the row is held.
… judges its first frame on that frame's time 'a spring started right before a block plays in full after it' waited a fixed 300 ms of wall clock after the block, then read the screencast. In the full interaction run on a loaded box no moving frame had reached the screencast by then, and the proof failed on an empty read. It now waits until the page shows the box a quarter of the way along and three more frames reach the screencast. Its bound changes form, not strength: the curve's own start (the handle's started) must come after the block, and the first moving frame may be no further along than a curve started a frame before the block ended, at that frame's own time, which is the old 30 px for a frame right after the block and still holds when a loaded machine sends frames sparsely. A curve dated at the call is 107 px in.
…tor's notes drop an undefined law The animator spec's header cited 'law M1' for compositor-only motion and useAnimator.ts cited 'law M4' for an animator that never re-renders, and no document defines either. The spec now cites spec:AST-094 FR5, which keeps touch motion to transform, translate, scale, rotate and opacity; useAnimator's sentence says what it means without the citation.
There was a problem hiding this comment.
Changes requested: the list-row release fix itself holds up (under a finger the highlight keeps fading through script, mouse and trackpad rows are unchanged, an engineering owner has accepted the ::before and divider behaviour, and the new divided-row case covers the last verification case of component:List FR11), but this pull request still carries all of #8890 with #8360 and #8076, about 27,700 of its 29,600 authored lines. Please rebase onto main once #8360 and #8890 land, so the diff is the 18-file row-press change.
[Automated review]
…e surface releaseAnimationsOf counted only the press owner's own animations (on it, or its ::before or ::after). A component that draws its release on a descendant, such as CheckboxInput's release on its box wrapper's ::after, then lost the end of it: fading went at the 200 ms clock while the 250 ms release still ran, and the frame it went relaid and repainted the box. It now also counts a CSS release animation on a descendant whose nearest press owner (closest [data-astryx-pressable]) is this element, the same nearest-owner rule the layers read by, so a press owner nested inside plays its own press's release and never extends this one. Red first: a unit case whose release runs on a box inside the row found fading gone at the clock on the change before this one; it now holds to the release's end. A second case keeps a nested press owner's release, on it or inside it, out of the row's.
…ide lands when its retire runs first Reported as a WebKit hang of the held-dip proof under an 8-worker run: the proof's wait for the call's finished never settled. Instrumented, the page kept drawing frames (about 60 a second) while the element sat at opacity 0 where it started, with no animation left: the held fade out had been finished from outside, then retired (cancelled) by its timer before the frame that reads the call's start, so the call never started and its landing was never armed. This case orders the retire timer before that frame and fails on this head: the move never lands.
… that cannot come [skip ci] The retire timer cancels a finished animation once the main thread runs past its curve. When a sampler finished a call's animations before the browser started them, a loaded page could run that timer before the frame that reads the call's start: the cancel left only idle animations, the start was never read, and the call's Reduce Motion landing was never armed, so the element stayed hidden where it started and the call never settled. The retire now reads such a call's start from the finished animation before cancelling it, and with nothing left to wait on the call keeps its own clock, so its landing still runs.
There was a problem hiding this comment.
Changes requested: the list-row release fix itself holds up (under a finger the highlight keeps fading through script, mouse and trackpad rows are unchanged, and an engineering owner has accepted the ::before and divider behaviour), but this pull request still carries all of #8890 with #8360 and #8076, about 27,800 of its 29,800 authored lines. Please rebase onto main once #8360 and #8890 land, so the diff is the 18-file row-press change. The description's compare link for this pull request's own change also stops before its last two commits.
[Automated review]
… curve, so only rows ship the row layer's constants The shared press overlay (utils/interactionOverlay.stylex.ts) read its release curve from the row layer's constants (cellPressConsts in rowPressLayer.stylex.ts). The overlay reaches nearly every component, and .stylex modules are side effects in core's package.json, so every component then shipped the row layer's constants (the repository's size report: +17-47 B gzip on each component without a press row), against architecture:component-style-authoring INV6. The overlay goes back to its own copy of the curve, main's literal; it and the row layer's copy are both the constants table's timing.press.highlightOutCurve, and a unit case fails if the overlay's drifts. Red first: a source bundle of Card carried the row layer's constants (cellRelease) on the change before this one; it now carries the overlay and none of them.
The changeset, the release lookup's comment and its unit case named a checkbox's box as a part that draws its release inside the surface. No component in this change does: the checkbox's release runs on the row around it. The lookup still counts a release on a descendant whose nearest press owner is the surface; the text now says that without the example.
…ease change [skip ci]
… change [skip ci]
There was a problem hiding this comment.
Changes requested: the list-row release fix itself holds up (under a finger the highlight keeps fading through script, mouse and trackpad rows are unchanged, and an engineering owner has accepted the ::before and divider behaviour), and the newest commit keeps the row constants out of components that render no press row. This pull request still carries all of #8890 with #8360 and #8076, about 27,900 of its 29,900 authored lines, so please rebase onto main once #8360 and #8890 land, leaving the 18-file row-press change. While there, refresh the description's compare link and Size line, which still describe the previous revision.
[Automated review]
Review only this PR's change: https://github.com/vjeux/astryx/compare/af9e82db13..83cc9619d7 (depends on #8890, which depends on #8360; the base adds main). Start with
fix(Item): a list row's touch press release runs on the compositor.User impact
Under a finger (
ListItem, an actionableItem, aTreeListrow), a pressed row's highlight should fade over 200 ms after the lift; it stayed pressed while the app's script ran, then snapped off.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. A tap usually starts work (navigating, loading), so the main thread is busiest exactly when the release plays, and the release animated a gradient that only the main thread can run.
The press now paints on a layer under the row's content and fades its opacity, which the browser animates off the main thread, on the same clock and curve (
spec:AST-094FR13,component:ListFR11). #8890, split out of this pull request, makes the press machine end the press when that fade has played, not on a clock from the lift, which cut the fade short when script delayed its first frame. A mouse or trackpad row is unchanged (spec:AST-017FR59).Size (gzip, base → head, the repository's size report run against #8890): Item +481 B JS and +338 B CSS, ListItem +357/+326 B, TreeList +320/+335 B, and the 18 menus, tables, selectors and other components built on Item +364–554 B JS and +326–347 B CSS; every other core component 0 B, ContextMenuItem +11 B JS; the package stylesheet +348 B.
Expected behavior and authority
usePressFeedback("fades the release over 200 ms"),design:user-statesDR1/DR5.Smallest restoration
Alternatives: keep the gradient (it freezes) or fade in JavaScript (it freezes the same way). An opacity layer is the one paint the compositor runs alone. Rows compose it from
utils/rowPressLayer.ts, reading the press stateutils/pressFeedback.tswrites: no second press machine.Evidence
iOS 26.2 simulator, Mobile Safari, touch input (recorded at an earlier head; at this head the block and Reduce Motion paths were recorded again and match):
Testing
addEventListener('pointerup', () => { const end = performance.now() + 700; while (performance.now() < end); }, {once: true}). Press and lift a row: the highlight keeps fading through the freeze (before, it stayed pressed, then snapped off).What was run: steps 1–3 on the iOS 26.2 simulator (the clips above), and unit and browser tests of each step in Chromium and WebKit, including a row held and released between two divided rows, whose dividers stay drawn by pixels and computed border (
component:ListFR11). All pass; the new ones failed before the change, and a page style that hides a pressed row's divider fails the divider case. These are local runs; no CI run is expected for this pull request.