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.
|
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.
Approval waits on an engineering owner's decision: the touch timing here rests only on the draft spec:AST-094 FR1, FR2 and FR4 that this pull request adds, and FR4 would narrow the current theme-tokens INV3 (components consume motion roles) to pointer presentations. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and you may do so yourself as an engineering owner. Independent of that decision, these need changes:
- Land or drop the #8226 stack and rebase, so this pull request is the 367-line SelectableCard commit rather than 13,990 authored lines (the rebase also clears the
pr-a11y"No tests found" failure). - Make the new
pr-interactionjob part of the existing UI test owner and itstestjoin, or leave it out, since AST-030 FR11 and FR7 rule out a separately routed lane whose failure fails no join. - Add the writing-rubric results for
spec:AST-094to the description, as the knowledge-contracts record requires for a new record. - Describe the touch arm as following a coarse primary pointer, not "a finger" versus "a mouse", in the Indicator docs and both changesets.
[Automated review]
| # 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.
This job routes itself (Check interaction scope) and is not in the test join, so a failing interaction proof fails none of the required joins. AST-030 FR11 asks new coverage inside an existing surface to join that surface's test owner instead of adding a separately routed lane, and FR7 asks an owned lane's failure to fail its join.
| // do (motion/uikit/selection.ts). The ring cannot interpolate with the | ||
| // card's resting shadow, so a fade would split it from the border. A | ||
| // mouse keeps the theme's duration. | ||
| '@media (pointer: coarse)': MARK_CHANGE, |
There was a problem hiding this comment.
(pointer: coarse) follows the device's primary pointer, not the input that made the selection, so a trackpad on a coarse-primary device also gets 0 ms and a finger on a fine-primary laptop keeps the fade. Whether touch marks may leave --duration-fast at all is the open decision on draft AST-094 FR1 and FR4.
feacfda to
c68e13e
Compare
|
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.
|
c68e13e to
a782f5b
Compare
vjeux
changed the title
|
Pushed in a782f5b: the card's mark keys on |
a782f5b to
7bb7354
Compare
|
Pushed in 7bb7354: the card's touch and Reduce Motion arms now import the shared keys ( |
7bb7354 to
c30794b
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision under spec:AST-094 FR4, which is current on main: under a coarse pointer the SelectableCard mark (and the checkbox and radio marks from #8226) plays a build-time 0 ms constant, while FR4 requires every touch duration to be a theme motion role and forbids a literal. An engineering owner records whether this one-frame mark sits outside FR4 or must become a registered motion role by commenting /approve-spec <full head SHA> on this pull request, and you may do so yourself as an engineering owner. Independent of that decision, these need changes:
- Land or drop the stack beneath the card commit and rebase on current
main, which also clears the conflicts in seven gesture and motion files that keep CI from running, so this pull request is the 598-line card change rather than 31,417 authored lines. - Add mouse before-and-after evidence for the card's new Reduce Motion behaviour, as AST-017 FR59 asks.
- Type the root theme
motionduration scale so a partial scale fails type-checking, remove the build README paragraph about the reverted undeclared-variable warning, and correct the checkbox and radio Reduce Motion sentence in the changeset and theisSelecteddoc.
[Automated review]
| // once (design:reduced-motion DR3); a finger's change already is. | ||
| // The resolver's and the policy's keys, spelled: an imported at-rule | ||
| // key lands in a different StyleX priority layer than a literal one. | ||
| '@media (pointer: coarse)': MARK_CHANGE, |
There was a problem hiding this comment.
Under a coarse pointer this arm plays a build-time 0 ms constant (selection.mark.changeMs through selectionMotionConsts). Current spec:AST-094 FR4 requires every duration a touch presentation plays to be a theme motion role and forbids a literal, and selection is not in the generated role table, so this needs an engineering owner's ruling: either the one-frame mark sits outside FR4, or it becomes a role read through its --motion-* variable.
| export interface MotionConfig | ||
| extends Partial<MotionScaleConfig>, MotionRoleValues {} |
There was a problem hiding this comment.
Partial<MotionScaleConfig> lets a root motion: {fast: 150, medium: 300} type-check, but motionScaleOf throws unless fast, medium and ratio come together (lines 89-92); the base type rejected it at compile time (TS2741). Typing the root input as all-or-none, e.g. MotionRoleValues & (MotionScaleConfig | {fast?: never; medium?: never; ratio?: never; slow?: never; easing?: never}), keeps the error in the editor; the adaptation value can stay partial since it completes from the root.
|
|
||
| Unsupported declarations fail the build by default with StyleX's replacement guidance. Set `propertyValidationMode` explicitly in `stylexOverrides` (modern API) or `stylexOptions` (legacy API) only when a migration needs the upstream `warn` or `silent` behavior. | ||
|
|
||
| A production build also checks the StyleX stylesheet for generated custom properties (`--<prefix><hash>`) that it reads but no stylesheet in the build declares, which browsers drop: a `.stylex.ts` export read in a style that is not a `defineVars`/`defineConsts` value, or one whose module never reached the build. It warns, naming the variable and the rule that reads it. Only stylesheets count as declarations, so a property the app sets at runtime (an inline style) is reported too: give it a name outside the StyleX class-name prefixes. |
There was a problem hiding this comment.
This describes the production warning for undeclared generated custom properties, which this branch reverted (vite.ts has no such check), so build users would wait for a diagnostic that never appears. Suggest removing the paragraph.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] Under a finger (a coarse primary pointer, on every touch device), a checkbox or radio mark now changes in the frame the selection lands, the way UIKit shows a selection mark. The box or circle used to fade between its two colors over the theme duration, so a tap first showed half-filled frames. With a mouse the marks keep the theme duration and easing, and under Reduce Motion a mouse's change is now immediate. |
There was a problem hiding this comment.
The base already gave CheckboxIndicator and RadioIndicator '@media (prefers-reduced-motion: reduce)': '0s', so a mouse's change under Reduce Motion was already immediate; only the coarse-pointer change is new for these marks.
| name: 'isSelected', | ||
| type: 'boolean', | ||
| description: | ||
| 'Controlled selection state. Under a finger (a coarse primary pointer) the selection ring and border change in one frame; with a mouse they fade over the theme’s fast duration.', |
There was a problem hiding this comment.
With a mouse only the border fades: the ring switches at once (SelectableCard.tsx:90-91), and under Reduce Motion neither fades. Suggested: "…with a mouse the border fades over the theme's fast duration, except under Reduce Motion."
There was a problem hiding this comment.
Approval waits on an engineering owner's decision under spec:AST-094 FR4, which is current on main: under a coarse pointer the SelectableCard mark (and the checkbox and radio marks from #8226) plays a build-time 0 ms constant, while FR4 requires every touch duration to be a motion role and forbids a literal. An engineering owner records whether this one-frame mark sits outside FR4 or must become a registered motion role by commenting /approve-spec <full head SHA> on this pull request, and you may do so yourself if you are an engineering owner. Whatever that decision, these need changes:
- Land or drop the stack beneath the card and rebase, so this pull request carries the 612-line card change rather than 23,846 authored lines.
- Add mouse before-and-after evidence for the card's new Reduce Motion behavior, as
spec:AST-017FR59 asks. - Correct the
isSelecteddoc (with a mouse only the border fades, and not under Reduce Motion) and the changeset sentence announcing a mouse Reduce Motion change the checkbox and radio marks already had.
[Automated review]
| @@ -71,3 +72,13 @@ export const navigation = { | |||
| */ | |||
| interactivePopFinishThreshold: 0.5, | |||
| } as const; | |||
|
|
|||
| /** | |||
| * Makes each spring, duration and curve above a motion role a theme may | |||
There was a problem hiding this comment.
This table passes no roles, so registerMotionTable registers no role here, and current spec:AST-094 FR4 says a theme cannot override a touch motion role yet (DEC-11). Say the values stay internal constants instead of "a motion role a theme may retune".
| @@ -122,3 +124,16 @@ export const scroll = { | |||
| */ | |||
| scrollToTopRate: 0.99035, | |||
| } as const; | |||
|
|
|||
| /** | |||
| * Makes each spring, duration and curve above a motion role a theme may | |||
There was a problem hiding this comment.
Same as registerNavigationTable: no roles are listed, so nothing here becomes a role, and FR4 rules out theme retuning for now; the comment should say so.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision under spec:AST-094 FR4: under a coarse pointer the SelectableCard mark (and the checkbox and radio marks from #8226) plays a build-time 0 ms constant rather than a registered motion role, and an engineering owner records whether that is acceptable by commenting /approve-spec <full head SHA> on this pull request, which you may do yourself if you are an engineering owner. Whatever that decision, these need changes:
- Land or drop #8226 and its predecessors and rebase, so this pull request carries the card change instead of 23,099 authored lines.
- Add mouse before-and-after evidence for the card's new Reduce Motion behaviour, as
spec:AST-017FR59 asks, and correct theisSelecteddoc and the selection-marks changeset, which misdescribe what a mouse user sees.
[Automated review]
| // once (design:reduced-motion DR3); a finger's change already is. | ||
| // The resolver's and the policy's keys, spelled: an imported at-rule | ||
| // key lands in a different StyleX priority layer than a literal one. | ||
| '@media (pointer: coarse)': MARK_CHANGE, |
There was a problem hiding this comment.
Under a coarse pointer this arm plays MARK_CHANGE, the build-time 0 ms mirror of selection.mark.changeMs, and motion/uikit/selection.ts registers no motion role. Current spec:AST-094 FR4 says "A component MUST read touch motion through these roles and MUST NOT play a literal", so an engineering owner needs to rule whether a one-frame mark is outside FR4 or must become a registered role.
| // The resolver's and the policy's keys, spelled: an imported at-rule | ||
| // key lands in a different StyleX priority layer than a literal one. | ||
| '@media (pointer: coarse)': MARK_CHANGE, | ||
| '@media (prefers-reduced-motion: reduce) and (not (pointer: coarse))': |
There was a problem hiding this comment.
This arm changes what mouse users see: on the base the card's border fades over 125 ms under Reduce Motion, and here it changes at once (the design:reduced-motion DR3 repair). spec:AST-017 FR59 asks the pull request to show fine-pointer evidence before and after such a change; the PR's evidence covers touch only.
| name: 'isSelected', | ||
| type: 'boolean', | ||
| description: | ||
| 'Controlled selection state. Under a finger (a coarse primary pointer) the selection ring and border change in one frame; with a mouse they fade over the theme’s fast duration.', |
There was a problem hiding this comment.
With a mouse only the border fades: the ring switches at once (see SelectableCard.tsx:90-91), and under Reduce Motion nothing fades. Suggested: "with a mouse the border fades over the theme's fast duration (at once under Reduce Motion)".
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] Under a finger (a coarse primary pointer, on every touch device), a checkbox or radio mark now changes in the frame the selection lands, the way UIKit shows a selection mark. The box or circle used to fade between its two colors over the theme duration, so a tap first showed half-filled frames. With a mouse the marks keep the theme duration and easing, and under Reduce Motion a mouse's change is now immediate. |
There was a problem hiding this comment.
The base CheckboxIndicator and RadioIndicator already set '@media (prefers-reduced-motion: reduce)': '0s' for every pointer, so "under Reduce Motion a mouse's change is now immediate" announces a change readers will not see. Drop that clause.
| * Roles: a spring or curve that is one of a registered table's motion roles | ||
| * plays the value its CSS variable carries on the element's scope, or the | ||
| * table's value when none is set (resolveMotion.ts; nothing public sets one | ||
| * while touch motion stays internal): `springs.settle` once | ||
| * `registerSpringsTable()` has run, the Reduce Motion fade's curve (the | ||
| * animator registers that role alone). Every other value, and a table never | ||
| * registered, plays its own value, and the fade's length is not a role. A | ||
| * role is read once, when the motion starts, from the scope's computed style: no layout, and the start, the promotion | ||
| * and the browser-set start time are unchanged. A spring written inline at the | ||
| * call site is played as given. |
There was a problem hiding this comment.
This paragraph (and the comment at line 1701) says a role plays its CSS variable read from the scope's computed style, but resolveMotion.ts returns the table value and reads no style while touch motion stays internal (spec:AST-094 DEC-11).
| * DELEGATED CONSUMERS. A consumer type registered with an attribute (a | ||
| * pressable row) is found at touch-down by walking up from the touch target, | ||
| * and an instance is created for that one touch: a list of a thousand rows | ||
| * costs nothing until a finger lands on one. |
There was a problem hiding this comment.
The host no longer registers consumer types by attribute (only attachConsumer remains), so this "DELEGATED CONSUMERS" paragraph describes a capability that is gone.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision under spec:AST-094 FR4: under a coarse pointer the SelectableCard mark (and the checkbox and radio marks from #8226) plays a build-time 0 ms constant rather than a registered motion role, and an engineering owner records whether that is acceptable by commenting /approve-spec <full head SHA> on this pull request, which you may do yourself if you are an engineering owner. Whatever that decision, these need changes:
- Land or drop #8226 and its predecessors and rebase, so this pull request carries the card change instead of 21,774 authored lines.
- In the gesture host those predecessors add, intersect the inline
touch-actionwith the element's own value and count a scroll as the browser taking a finger only after 10 px of travel, asspec:AST-094FR18 and FR20 require. - Add mouse before-and-after evidence for the card's new Reduce Motion behaviour, as
spec:AST-017FR59 asks, and correct theisSelecteddoc and the selection-marks changeset, which misdescribe what a mouse user sees.
[Automated review]
| element.style.getPropertyValue(property), | ||
| ); | ||
| } | ||
| element.style.setProperty(property, value); |
There was a problem hiding this comment.
This inline touch-action overrides the element's own value, and nothing reads or intersects it first. spec:AST-094 FR18 says the derived value "is intersected with the element's own author touch-action and is never wider than it (DEC-8)": an element styled touch-action: none with a tap recognizer would become manipulation here. Reading the element's computed value before writing, keeping the author's value when it omits pinch-zoom, and a test for a narrower author value would bring it in line.
| } | ||
| } | ||
| if (taken.length > 0) { | ||
| this.arena.browserTookOver(taken, time); |
There was a problem hiding this comment.
Any scroll of a container under a live, unclaimed finger hands that finger to the browser here, with no travel check (the test at GestureHost.test.ts:802 asserts it for a finger that never moved). spec:AST-094 FR20 counts a native scroll "only once that finger has moved the scroll take distance (FR2), so sub-pixel movement under a still finger does not cancel a tap (DEC-8)", and FR2 sets that distance at 10 px.
There was a problem hiding this comment.
This needs an engineering-owner decision before it can be approved: under a finger, SelectableCard's mark now plays a 0 ms build-time constant, while spec:AST-094 FR4 requires touch motion to read a named motion role and not play a literal. An engineering owner (the author included, if they are one) can accept the one-frame mark as outside FR4 by commenting /approve-spec <full head SHA> on this pull request, or ask for selection.mark.changeMs to become a registered motion role. Whatever the ruling, these need fixing:
- Narrow the pull request to the card change by landing or dropping #8226 and its predecessors, since it carries 21,497 authored lines today.
- The gesture host replaces a narrower author
touch-action(GestureHost.ts:467) and lets a scroll cancel a still finger's tap (GestureHost.ts:1460), against AST-094 FR18 and FR20. - Add fine-pointer before-and-after evidence for the Reduce Motion change, as AST-017 FR59 requires.
- Fix the
isSelectedtext atSelectableCard.doc.mjs:62(with a mouse only the border fades) and drop the Reduce Motion clause from.changeset/selection-marks-touch-one-frame.md:5, which describes what the base already did.
[Automated review]
There was a problem hiding this comment.
Approval needs an engineering owner to provide or identify the governing spec for the one-frame (0 ms) selection mark that SelectableCard, CheckboxIndicator and RadioIndicator play under a coarse pointer: accept it as it stands under spec:AST-094 FR4, or require selection.mark.changeMs to become a registered motion role first. 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 changes are needed:
- Land #8226 and its stack first so this pull request carries only the card change (21,115 authored lines today), and fix
GestureHostwriting its derivedtouch-actionover the author's (FR18) and cancelling a still finger's tap on any scroll (FR20). - Correct
SelectableCard.doc.mjs, since a mouse fades only the border, not the ring, and add before-and-after mouse evidence for the Reduce Motion change (spec:AST-017FR59).
[Automated review]
| name: 'isSelected', | ||
| type: 'boolean', | ||
| description: | ||
| 'Controlled selection state. Under a finger (a coarse primary pointer) the selection ring and border change in one frame; with a mouse they fade over the theme’s fast duration.', |
There was a problem hiding this comment.
With a mouse only the border fades: a click starts eight 125 ms border-*-color transitions and none on box-shadow, and the ring switches at once (the comment at SelectableCard.tsx:90–91 says the ring cannot interpolate). Suggest: "with a mouse the border fades over the theme’s fast duration and the ring changes at once."
| element.style.getPropertyValue(property), | ||
| ); | ||
| } | ||
| element.style.setProperty(property, value); |
There was a problem hiding this comment.
This writes the recognizers’ derived touch-action over the element’s own stylesheet value without intersecting it, so an author’s none or pan-y can widen to manipulation or pinch-zoom. spec:AST-094 FR18 requires the derived value to be "intersected with the element’s own author touch-action and … never wider than it (DEC-8)".
| } | ||
| } | ||
| if (taken.length > 0) { | ||
| this.arena.browserTookOver(taken, time); |
There was a problem hiding this comment.
Every unclaimed touch inside the scroller is handed to the browser here, however far its finger moved, so a sub-pixel or programmatic scroll cancels a still finger’s tap. spec:AST-094 FR20 counts a native scroll "only once that finger has moved the scroll take distance (FR2)", which is 10 px.
There was a problem hiding this comment.
Approval needs an engineering owner to decide whether the one-frame (0 ms) selection mark that SelectableCard, CheckboxIndicator and RadioIndicator play under a coarse pointer is acceptable under spec:AST-094 FR4 as it stands, or whether selection.mark.changeMs must first become a registered motion role. 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 changes are needed:
- Land #8226 and its stack first so this pull request carries only the card change (17,795 authored lines today).
- In
GestureHost, intersect the derivedtouch-actionwith the element's own (FR18) and let a scroll take a touch only after 10 px of that finger's travel (FR20). - Add before-and-after mouse evidence for the Reduce Motion change (
spec:AST-017FR59), and correctSelectableCard.doc.mjs, since a mouse fades only the border.
[Automated review]
| // once (design:reduced-motion DR3); a finger's change already is. | ||
| // The resolver's and the policy's keys, spelled: an imported at-rule | ||
| // key lands in a different StyleX priority layer than a literal one. | ||
| '@media (pointer: coarse)': MARK_CHANGE, |
There was a problem hiding this comment.
Under a coarse pointer this arm plays MARK_CHANGE, the build-time 0 ms mirror of selection.mark.changeMs, and motion/uikit/selection.ts registers no motion role. Current spec:AST-094 FR4 says "A component MUST read touch motion through these roles and MUST NOT play a literal", and its non-goals leave each component's touch presentation to its own record, which SelectableCard does not have. This waits on an engineering owner's decision: accept the one-frame mark as it stands, or register the value as a role first.
| name: 'isSelected', | ||
| type: 'boolean', | ||
| description: | ||
| 'Controlled selection state. Under a finger (a coarse primary pointer) the selection ring and border change in one frame; with a mouse they fade over the theme’s fast duration.', |
There was a problem hiding this comment.
With a mouse only the border fades: a click starts four 125 ms border-*-color transitions and none on box-shadow (the ring cannot interpolate, SelectableCard.tsx:90–91), and under Reduce Motion nothing fades. Suggested: "with a mouse the border fades over the theme’s fast duration (at once under Reduce Motion) and the ring changes at once."
| element.style.getPropertyValue(property), | ||
| ); | ||
| } | ||
| element.style.setProperty(property, value); |
There was a problem hiding this comment.
This writes the recognizers' derived touch-action over the element's own stylesheet value without intersecting it, so an author's none or pan-y can widen to manipulation or pan-y pinch-zoom. spec:AST-094 FR18: "The derived value is intersected with the element's own author touch-action and is never wider than it (DEC-8)".
| } | ||
| } | ||
| if (taken.length > 0) { | ||
| this.arena.browserTookOver(taken, time); |
There was a problem hiding this comment.
Every unclaimed touch inside the scroller is handed to the browser here, however far its finger moved, so a sub-pixel or programmatic scroll cancels a still finger's tap (GestureHost.test.ts:802 asserts it). spec:AST-094 FR20 counts a native scroll "only once that finger has moved the scroll take distance (FR2)", which is 10 px.
There was a problem hiding this comment.
Approval needs an engineering owner to provide or identify the governing spec for the one-frame (0 ms) selection mark that SelectableCard, CheckboxIndicator and RadioIndicator play under a coarse pointer: accept it as it stands under spec:AST-094 FR4, or require selection.mark.changeMs to become a registered motion role, with each component's own record stating its touch presentation, first. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner. Whatever the decision, these changes are needed:
- Land #8226 and its stack first, so this pull request carries only the 612-line card change (17,370 authored lines today).
- In
GestureHost, intersect the derivedtouch-actionwith the element's own (FR18), and let a scroll take a touch only after 10 px of that finger's travel (FR20). - Add before-and-after mouse evidence for the Reduce Motion change (
spec:AST-017FR59), and correctSelectableCard.doc.mjs, since a mouse fades only the border.
[Automated review]
| name: 'isSelected', | ||
| type: 'boolean', | ||
| description: | ||
| 'Controlled selection state. Under a finger (a coarse primary pointer) the selection ring and border change in one frame; with a mouse they fade over the theme’s fast duration.', |
There was a problem hiding this comment.
With a mouse only the border fades: the ring (box-shadow) never transitions, on the base or this head, and under Reduce Motion nothing fades. Suggested: "with a mouse the border fades over the theme’s fast duration (at once under Reduce Motion) and the ring changes at once."
| element.style.getPropertyValue(property), | ||
| ); | ||
| } | ||
| element.style.setProperty(property, value); |
There was a problem hiding this comment.
This writes the recognizers' derived touch-action over the element's own stylesheet value, so an author's none or pan-y comes back as manipulation or pan-y pinch-zoom. spec:AST-094 FR18 (DEC-8): the derived value is intersected with the author's and is never wider; when the author's value leaves out pinch-zoom, the author's value wins.
| } | ||
| } | ||
| if (taken.length > 0) { | ||
| this.arena.browserTookOver(taken, time); |
There was a problem hiding this comment.
Every unclaimed touch inside the scroller is handed to the browser here, however far the finger moved, so a sub-pixel or programmatic scroll under a resting finger cancels its tap. spec:AST-094 FR20 (DEC-8) counts a native scroll only once that finger has moved 10 px from touch-down; GestureHost.test.ts:823 asserts the opposite.
| // True while an Effect drives the recognizer, so a transition it causes | ||
| // reaches no caller callback (react-component-runtime INV2, INV3). | ||
| const isSyncingRef = useRef(false); | ||
| useIsomorphicLayoutEffect(() => { |
There was a problem hiding this comment.
architecture:react-component-runtime INV3: name what this effect synchronizes (the latest onGesture the recognizer's listener reads), as the neighbouring effects do; the same applies to the options ref at :133.
There was a problem hiding this comment.
The one-frame selection mark under a finger works as described, but approval needs an engineering owner to provide or identify the governing spec for it: spec:AST-094 FR4 requires every touch duration to be a registered motion role, and no current record gives SelectableCard, CheckboxIndicator or RadioIndicator a touch presentation, so an owner must settle whether the 0 ms mark read from the measured selection table stands as it is or first needs a registered role and component records. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as one.
- Whatever the decision, land #8226's stack first so this diff is the card change alone, and fix
GestureHostso it intersects itstouch-actionwith the author's (FR18) and lets a scroll take a touch only after 10 px of that finger's travel (FR20). - Add before-and-after mouse evidence for the Reduce Motion change (
spec:AST-017FR59), and correct theisSelecteddocs, since with a mouse only the border fades.
[Automated review]
| name: 'isSelected', | ||
| type: 'boolean', | ||
| description: | ||
| 'Controlled selection state. Under a finger (a coarse primary pointer) the selection ring and border change in one frame; with a mouse they fade over the theme’s fast duration.', |
There was a problem hiding this comment.
With a mouse only the border fades: the ring is a box-shadow that cannot interpolate with the card's resting shadow (as SelectableCard.tsx:90–91 says), and under Reduce Motion nothing fades. Suggested wording: "…with a mouse the border fades over the theme’s fast duration (at once under Reduce Motion) and the ring changes at once."
| element.style.getPropertyValue(property), | ||
| ); | ||
| } | ||
| element.style.setProperty(property, value); |
There was a problem hiding this comment.
This inline write replaces the element's own touch-action instead of intersecting with it, so a tap recognizer turns an author's stylesheet touch-action: none into manipulation. spec:AST-094 FR18: "The derived value is intersected with the element's own author touch-action and is never wider than it (DEC-8)". Read the author's value before the first write and intersect it, keeping the author's value when it leaves out pinch-zoom.
| } | ||
| } | ||
| if (taken.length > 0) { | ||
| this.arena.browserTookOver(taken, time); |
There was a problem hiding this comment.
Any scroll inside the container hands every unclaimed touch to the browser here, even a finger that has not moved, so a sub-pixel or programmatic scroll cancels a resting finger's tap. spec:AST-094 FR20 counts a native scroll as the browser taking the touch "only once that finger has moved the scroll take distance (FR2)", which is 10 px. Check each touch's travel from touch-down before adding it to taken, and turn GestureHost.test.ts:823 around.
| // True while an Effect drives the recognizer, so a transition it causes | ||
| // reaches no caller callback (react-component-runtime INV2, INV3). | ||
| const isSyncingRef = useRef(false); | ||
| useIsomorphicLayoutEffect(() => { |
There was a problem hiding this comment.
This layout effect, and the one at :133, names no external system it synchronizes, unlike the neighbouring effects' "Syncs …" comments. architecture:react-component-runtime INV3: "A new or changed Effect MUST name the external system it synchronizes." Name the listener each ref feeds.
There was a problem hiding this comment.
Approval waits on an engineering owner providing or identifying the governing spec for the one-frame (0 ms) selection mark under a coarse pointer: spec:AST-094 FR4 requires every duration a touch presentation plays to be a motion role, and no current component record gives SelectableCard, CheckboxIndicator or RadioIndicator a touch presentation, so an owner either accepts the mark as it stands or asks for selection.mark.changeMs to become a registered role, with the component records stating the presentation, first. 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 changes are needed:
- Land #8226 and the rest of its stack first and rebase, so this pull request carries the 612-line card change instead of 28,981 lines.
- Correct
SelectableCard.doc.mjs: with a mouse only the border fades (and not under Reduce Motion), while the ring changes at once. - Add before-and-after mouse evidence for the Reduce Motion change, as
spec:AST-017FR59 asks.
[Automated review]
| name: 'isSelected', | ||
| type: 'boolean', | ||
| description: | ||
| 'Controlled selection state. Under a finger (a coarse primary pointer) the selection ring and border change in one frame; with a mouse they fade over the theme’s fast duration.', |
There was a problem hiding this comment.
With a mouse only the border fades: the inset ring cannot interpolate with the card's resting shadow, so it changes at once (the comment at SelectableCard.tsx:90–91 says the same), and under Reduce Motion the border changes at once too on this head. Suggested wording: "with a mouse the border fades over the theme’s fast duration (at once under Reduce Motion) and the ring changes at once."
| // once (design:reduced-motion DR3); a finger's change already is. | ||
| // The resolver's and the policy's keys, spelled: an imported at-rule | ||
| // key lands in a different StyleX priority layer than a literal one. | ||
| '@media (pointer: coarse)': MARK_CHANGE, |
There was a problem hiding this comment.
This 0 ms arm is the part that waits on an engineering-owner decision: spec:AST-094 FR4 asks every duration a touch presentation plays to be a motion role read through its role, and selectionMotionConsts.markChangeMs mirrors a table value that is not a registered role. The literal @media (pointer: coarse) key itself conforms to FR13 (DEC-6).
iOS has no checkbox or radio control; a choice is marked on its row, and on iOS 26.2 the mark changes in the frame the selection lands with no animation of its own: the checkmark accessory of a single-choice list in Settings, the completion circle in Reminders, a collection-view list's multiselect accessory, with and without Reduce Motion. CheckboxIndicator and RadioIndicator faded their box or circle between its two colors over the theme's fast duration under every pointer, so a tap showed half-filled frames first. Under a coarse primary pointer the fade now takes the UIKit table's value, one frame (motion/uikit/selection.ts, mirrored for the stylesheet in Indicator/selectionMotion.stylex.ts and held to it by selectionMotion.test.ts). A mouse keeps the theme's duration and easing tokens; under Reduce Motion a mouse's change is immediate, in the instant key of the reduced-motion policy (design:reduced-motion DR3), and a finger's already is. The browser spec proves both in Chromium and WebKit: every frame of a tap shows the mark wholly in its old or its new colour, a busy main thread cannot stretch the change, and a mouse keeps the fade.
…ange in one frame A selected card's mark is an inset ring, drawn in the card's shadow slot, and the accent border. The ring cannot interpolate with the card's resting shadow, so it appeared or vanished in one frame while the border faded over the theme's fast duration: the mark changed in two steps. UIKit changes a selection mark in the frame the selection lands (motion/uikit/selection.ts, selection.mark.changeMs). Under a finger (a coarse primary pointer, the touch resolver's TOUCH_QUERY, on every touch device) the card's box-shadow and border-color transition now takes that clock, through the same build-time mirror the checkbox and radio marks read (Indicator/selectionMotion.stylex.ts), so ring and border change together. A mouse keeps the theme's fade, and under Reduce Motion changes at once (design:reduced-motion DR3).
f69f26c to
3161c95
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about the one-frame selection mark. Under a coarse pointer, SelectableCard, CheckboxIndicator and RadioIndicator now play 0 ms from a mirror of the measured selection.mark.changeMs, while spec:AST-094 FR4 asks for every touch duration to be a registered motion role and leaves each component's touch presentation to that component's own record, which none of these records states yet. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request (you may do so yourself as an engineering owner), or asks for selection.mark.changeMs to become a registered motion role first. No blocking defects were found, and the Reduce Motion fix for mouse users matches design:reduced-motion DR3; if the mark is accepted, please add real-device iOS Safari and Android Chrome evidence for the one-frame claim, as spec:AST-094 asks.
[Automated review]
| /** Written by the press controller for the press's release. */ | ||
| const PRESSED_FADING = '[data-astryx-press="fading"]'; | ||
|
|
||
| /** How long the selection mark takes to change under a finger: one frame. */ |
There was a problem hiding this comment.
Non-blocking (source comments): this restates MARK_CHANGE and copies the table's value, so it goes stale if selection.mark.changeMs changes. Suggest deleting it; the same line sits in CheckboxIndicator.tsx:27 and RadioIndicator.tsx:27.
| // split it from the border. Under Reduce Motion a mouse's change is | ||
| // immediate too. |
There was a problem hiding this comment.
Non-blocking (source comments): "Under Reduce Motion a mouse's change is immediate too." narrates the 0s arm below. Suggest dropping the sentence and keeping the reason and the StyleX key quirk; the same sentence is in CheckboxIndicator.tsx:44-45 and RadioIndicator.tsx:48-49.
| displayName: 'Checkbox Indicator', | ||
| description: | ||
| 'The checkbox visual: a square box with a checkmark or an indeterminate bar. Decorative (aria-hidden); the owning control keeps the input, role, accessible name, focus, and keyboard behavior.', | ||
| 'The checkbox visual: a square box with a checkmark or an indeterminate bar. Decorative (aria-hidden); the owning control keeps the input, role, accessible name, focus, and keyboard behavior. Under a finger (a coarse primary pointer) the box changes in one frame, as UIKit selection marks do; with a mouse its colors fade over the theme duration.', |
There was a problem hiding this comment.
Non-blocking: under Reduce Motion a mouse's change is immediate, so "with a mouse its colors fade over the theme duration" is not true there. Suggest ending with "; under Reduce Motion it changes at once", as SelectableCard.doc.mjs does. The RadioIndicator description at line 100 has the same sentence.
|
Superseded: under the owner's ruling, main's indicators already play the theme duration on every pointer and change at once under Reduce Motion; SelectableCard's Reduce Motion arm lands in #8326. |
Stacked on #8226. Review only this PR's change: vjeux/astryx@4707533...3161c95
User need
On a phone, tapping a SelectableCard changes its selection mark in two steps. The inset ring appears or vanishes at once, while the border fades over the theme's fast duration (125 ms), so for several frames the card that lost the selection still shows a gray outline. UIKit changes a selection mark in the frame the selection lands; under a finger the card now does too.
Evidence
A tap moving the selection from Basic to Pro, ¼ speed, recorded on this PR's earlier code: iOS 26.2 Mobile Safari (the magenta ring is the finger; before is #8226) and Android 15 Chrome (before is main). Before, Basic's border fades after Pro's ring appears; after, both cards change in one frame. The engine runs in Testing prove this head:
With a mouse under Reduce Motion, Chromium storyboards of a click moving the selection to Pro. Before (main), the borders fade; after, both cards change in one frame:
Problem and solution fit
selection.mark.changeMs(0), through the mirror that feat(core): under a finger a selection mark changes in one frame #8226 adds, so the ring and the border change together.Current specification
spec:AST-094FR1, FR13, FR24.design:reduced-motionDR2, DR3.design:reduced-motionDR2. This stays a draft until it is answered. It follows fix(core): no press scale or card color fade under reduced motion #8326's option (A): under Reduce Motion a fine pointer gets no transition. With (B), this drops its Reduce Motion arm.Public delta and caller burden
Under a finger only, the card's ring and border change in one frame. No API changes; the press overlay, colours and elevation are unchanged.
Design and alternatives
The card's touch press paint is separate press-layer work (#8057, #8118).
Scope
Testing
Local gates at 1d2f527c7f, which differs from this head (3161c95) only in the
isSelecteddoc sentence, on #8226's 4707533:pnpm build,pnpm api:report --check,pnpm typecheckandpnpm review:pre: clean.SelectableCard.doc.mjs, which main also leaves unformatted.check:repo,check:changesets,check:knowledgeandcheck:i18n-runtime: clean.SelectableCardMark.test.tsxfails on feat(core): under a finger a selection mark changes in one frame #8226's SelectableCard.