Repository navigation
feat(core): a checkbox's or radio option's hit area grows toward 44 px - #8241
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
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| MultiSelector | 🔴 75.9 kB (+2.7 kB) · 15.5 kB CSS (+159 B) | — | — · — · — |
| TableHeaderCell | 🔴 78.8 kB (+2.6 kB) · 16.3 kB CSS (+161 B) | — | — · — · — |
| Table | 🔴 87.3 kB (+2.6 kB) · 16.8 kB CSS (+165 B) | — | — · — · — |
18 more components
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| TableCell | 🔴 79.2 kB (+2.6 kB) · 16.5 kB CSS (+166 B) | — | — · — · — |
| ContextMenu | 🔴 78.0 kB (+2.6 kB) · 16.1 kB CSS (+166 B) | — | — · — · — |
| Markdown | 🔴 148.7 kB (+2.6 kB) · 19.0 kB CSS (+156 B) | — | — · — · — |
| CheckboxInput · Storybook | 🔴 51.3 kB (+2.5 kB) · 8.8 kB CSS (+161 B) | 424 (+52) | — · ✓ · — |
| CheckboxListItem | 🔴 56.6 kB (+2.5 kB) · 10.6 kB CSS (+159 B) | — | — · — · — |
| richtext/RichTextEditor | 🔴 281.0 kB (+2.7 kB) · 2.6 kB CSS (0) | — | — · — · — |
| ListItem | 🔴 55.9 kB (+2.5 kB) · 10.6 kB CSS (+159 B) | — | — · — · — |
| richtext/RichTextView | 🔴 261.4 kB (+2.6 kB) · 2.3 kB CSS (0) | — | — · — · — |
| lab/CommentThread | 🔴 262.5 kB (+2.6 kB) · 2.4 kB CSS (0) | — | — · — · — |
| lab/Comments | 🔴 267.8 kB (+2.5 kB) · 2.5 kB CSS (0) | — | — · — · — |
| RadioListItem | 🔴 54.3 kB (+2.3 kB) · 9.5 kB CSS (+144 B) | — | — · — · — |
| PowerSearch | 244.8 kB (0) · 22.4 kB CSS (+165 B) | — | — · — · — |
| MoreMenu | 74.3 kB (+1 B) · 16.2 kB CSS (+156 B) | — | — · — · — |
| DropdownMenu | 74.1 kB (0) · 16.2 kB CSS (+156 B) | — | — · — · — |
| CheckboxIndicator | 2.7 kB (0) · 813 B CSS (0) | — | — · — · — |
| CheckIndicator | 8.0 kB (0) · 681 B CSS (0) | — | — · — · — |
| RadioIndicator | 2.5 kB (0) · 725 B CSS (0) | — | — · — · — |
| RadioList | 43.4 kB (0) · 6.9 kB CSS (0) | — | — · ✓ · — |
Package size: @astryxdesign/core 6.43 MB packed (+3.1 kB); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: cli 8 rows, core 2 rows; run pnpm api:report.
Wide change (shared styles: packages/core/src/utils/touchHitArea.stylex.ts): add ci:run-visual or ci:run-rtl to run them.
Generated by PR Enrichment workflow | View full report
![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 44 px coarse-pointer targets for CheckboxInput and RadioList, and the List touch rows, press layer and holdSelection export carried in from the stacked pull requests, rest only on the draft spec:AST-094, which no owner has approved yet. An engineering owner records that decision by commenting /approve-spec c99f34a8cd361498a635ae8dc918e42f458296c4 on this pull request (the author may do so as an engineering owner), or by saying which of these behaviours should wait for AST-094 to be approved on main. Whatever that decision, please also:
- keep the 44 px row off the CheckboxInputs that other components embed, since on a touch screen MultiSelector options grow from 36 to 56 px, compact TableSelection rows from 33 to 53 px and Markdown task items from 32 to 52 px, or document and test that growth;
- rebase onto
mainonce #8045, #8052 and #8085 land, so this pull request's diff is only the touch-target commit (it is 15,494 authored lines today, andpr-a11yfails because the branch predates a spec file thatmain's workflow runs); - fix the references to files and records that do not exist:
utils/touchPresentation.ts,sources.test.ts,packages/core/src/gestures, and AST-094's citation of an AST-090 record.
[Automated review]
| {...(isDisabled ? undefined : pressable)} | ||
| {...stylex.props( | ||
| // Under a finger the row is at least as tall as an iOS control. | ||
| touchTargetStyles.row, |
There was a problem hiding this comment.
On a touch screen this floor applies to every CheckboxInput, including the ones MultiSelector, TableSelection and List task markers embed: their rows grow from 36 to 56 px, 33 to 53 px and 32 to 52 px. Could it apply only where this row is the target (not for isLabelHidden, inert or read-only marker uses), with those composers in the touch tests?
| }); | ||
|
|
||
| /** | ||
| * The touch presentation's query (utils/touchPresentation.ts TOUCH_QUERY), |
There was a problem hiding this comment.
utils/touchPresentation.ts and TOUCH_QUERY don't exist at this head (the same reference is in List/listRow.stylex.ts:36). Point at the real source of the query or drop the reference.
| 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 pull request, so this amendment (and the one at line 235) has nothing to amend yet. Cite it once it exists, or drop the reference.
|
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.
|
There was a problem hiding this comment.
Approval needs an engineering owner to settle the governing spec: the 44 px coarse-pointer targets for CheckboxInput and RadioList, and the List touch rows, press layer and holdSelection export carried in from the stacked pull requests, rest only on the draft spec:AST-094, and whether the targets ship on their own coarse-pointer query or wait for the shared touch resolver is still open. An engineering owner records that decision by commenting /approve-spec e877f6946b2b99a33f002fada63d412e4046bb28 on this pull request (the author may do so as an engineering owner), or by naming which of these must wait for AST-094 to be approved on main. Whatever that decision, please also:
- remove
reducedMotion: 'keep'fromutils/selectionHold.ts(lines 268 and 283), because under reduced motion the deselect still runs 350 ms with a finger and with a mouse, wheredesign:reduced-motionallows at most 200 ms on touch and an immediate change otherwise; - rebase onto
mainonce #8045, #8052 and #8085 land, so the diff is only the touch-target commits (15,545 authored lines today); - conform AST-094 FR8 to
design:reduced-motionor name that amendment, and fix the references toutils/touchPresentation.ts,sources.test.ts,packages/core/src/gesturesandspec:AST-090, which do not exist.
[Automated review]
| ? {} | ||
| : {velocity: {opacity: -timing.velocity}}), | ||
| }, | ||
| {at, reducedMotion: 'keep'}, |
There was a problem hiding this comment.
With reducedMotion: 'keep' (here and at line 283) the deselect plays its full 350 ms under prefers-reduced-motion, with a finger and with a mouse alike. design:reduced-motion asks for an opacity change of 200 ms or less on a coarse pointer and an immediate change otherwise, with no opt-out in core (DR2–DR4). Could the hold drop 'keep' and resolve the deselect that way, with a browser test under reduced motion?
| on: a transition that travels becomes a cross-fade carrying the same change | ||
| of state. An entrance lands in place and fades in, an exit fades out in | ||
| place, and any other move fades through. A navigation push or pop | ||
| cross-fades over 0.5 s on the ease-in-out curve; a sheet, a full-screen |
There was a problem hiding this comment.
A 0.5 s navigation cross-fade is longer than the 200 ms design:reduced-motion DR2 allows on a touch screen, and the pending-review note under FR8 names only the draft design:motion DR5. Could FR8 follow DR2, or name the amendment to design:reduced-motion so an owner sees it?
| }); | ||
|
|
||
| /** | ||
| * The touch presentation's query (utils/touchPresentation.ts TOUCH_QUERY), |
There was a problem hiding this comment.
utils/touchPresentation.ts and TOUCH_QUERY don't exist on main or at this head. Point at the real source of the query, or drop the reference until the resolver lands.
e877f69 to
9282c28
Compare
There was a problem hiding this comment.
Approval needs an engineering owner to settle the governing spec: the 44 px coarse-pointer targets for CheckboxInput and RadioList rest only on the draft spec:AST-094, which no owner has approved for this head. An engineering owner records that decision by commenting /approve-spec 9282c287898d26192284c5f6a251282ce3ae1624 on this pull request (the author may do so as an engineering owner), or by saying the targets should wait until a 44 px touch target is approved on main. Whatever that decision, please also:
- remove the theme
platformadaptation condition from this stack, or land an approved amendment tospec:AST-012FR2 first, because FR2 closes thewhenvocabulary towidth,height,pointer,contrastandmotion; - rebase onto
mainonce the stacked pull requests land, so the diff is only the touch-target commits (33,535 authored lines today, 429 of them this pull request's); - align AST-094 FR8, FR4, DEC-1 and FR13 with
design:reduced-motionDR2 and with the code that ships, and fix the references tospec:AST-090andsources.test.ts, which do not exist.
[Automated review]
| * is everything else. Pair `ios` with `pointer: 'coarse'` for a touch | ||
| * profile, such as touch motion tuned per platform. | ||
| */ | ||
| platform?: 'ios' | 'other'; |
There was a problem hiding this comment.
spec:AST-012 FR2 closes the when vocabulary: "A when object may contain width, height, pointer, contrast, and motion … Future environment fields require explicit additions to this closed vocabulary." No record admits platform, and the theme authoring guide and template already teach it. Could this axis come out of the stack, or wait for an approved FR2 amendment?
| measures (0.55 for a scroll view and a sheet), and a release MUST pick its | ||
| rest point by projecting its velocity the way its UIKit counterpart does: | ||
| with a deceleration rate, or with a measured projection time. | ||
| - **FR4 — Physics are not themable in touch presentations.** A touch |
There was a problem hiding this comment.
FR4 (and DEC-1) say touch physics are not themable, but this stack ships theme motion roles (defineTheme({motion: {springs, durations, curves}})) and holdSelection reads its deselect clock and curve from them. Could FR4 and DEC-1 describe what the roles allow, or the roles wait, so an owner approving this record approves what ships?
9282c28 to
10c2d49
Compare
There was a problem hiding this comment.
Approval needs an engineering owner to provide or identify the governing spec for the 44 px coarse-pointer target that CheckboxInput and RadioList get here: no record states that size, and the draft spec:AST-094 this branch carries names none and has no owner approval for this head. An engineering owner records that decision by commenting /approve-spec 10c2d49a4fa0c19a3e7fbcbae4505ee855728aa3 on this pull request (the author may do so as an engineering owner), or by saying the targets should wait until a record that states a 44 px coarse-pointer target is approved on main. Whatever that decision, please also:
- keep the new row press layer off fine pointers, since its unconditional
::beforehides a caller's::beforedecoration on interactive Item, ListItem and TreeList rows with a mouse too, whichspec:AST-017FR59 does not allow for touch work; - drop the
platformaxis from the theme template (packages/cli/assets/theme.template.ts), becausedefineThemenow rejects it andspec:AST-012FR2 does not admit it; - make the failing
pr-interactioncase pass: the theme-spring proof inanimator.interaction.browser.spec.tsfails because nothing registers the springs table, so a theme'ssprings.*roles reach no animation unless something registers that table; - rebase onto
mainonce the stacked pull requests land, or retarget onto #8085, so the diff is only the touch-target commits (35,149 authored lines today, 433 of them this pull request's); - align AST-094 FR8, FR4, DEC-1 and FR13 with
design:reduced-motionDR2 and with the code that ships, and fix the references tospec:AST-090and "AST-094 OQ4", which do not exist.
[Automated review]
| isolation: 'isolate', | ||
| '::before': { | ||
| content: '""', | ||
| position: 'absolute', | ||
| inset: 0, | ||
| zIndex: -1, | ||
| borderRadius: 'inherit', | ||
| pointerEvents: 'none', | ||
| backgroundColor: colorVars['--color-overlay-pressed'], | ||
| // On at once while the press is believed, and while a selection hold | ||
| // keeps the row selected (its own animations move it from there); | ||
| // nothing transitions it. | ||
| opacity: { | ||
| default: 0, |
There was a problem hiding this comment.
isolation and this ::before (with opacity: 0 by default) apply at every pointer, so on an interactive Item, ListItem or TreeList row a caller's ::before from xstyle is hidden at rest with a mouse too. spec:AST-017 FR59 lets touch work change fine-pointer appearance only to fix a defect mouse users hit or for a named cross-device consistency change. Could the layer's content and isolation sit under @media (pointer: coarse)?
| * are primary-pointer precision, contrast preference, motion preference, and | ||
| * platform (`ios` for the browsers of iPhone and iPad, `other` for the rest). |
There was a problem hiding this comment.
The stack removed the platform condition: themeAdaptations.ts accepts only the closed axes and throws … .platform is not supported., and spec:AST-012 FR2 closes the when vocabulary. A theme author who follows this template gets a build error; could the platform clause come out?
|
|
||
| export {createAnimator} from '../animator'; | ||
| export {solveSpring, toSpring} from '../spring'; | ||
| export {springs} from '../uikit/springs'; |
There was a problem hiding this comment.
This page never calls registerSpringsTable(), and reading a table no longer registers it, so in animator.interaction.browser.spec.ts:321 the themed box plays the table's spring and pr-interaction fails (111.7 px off its themed curve). No shipped module registers the springs table either. Could the table be registered here, or spring roles resolve without an explicit registration?
| @@ -177,6 +190,13 @@ const STATIC_EXPORTS = { | |||
| types: './dist/utils/interactionOverlay.stylex.d.ts', | |||
| default: './dist/utils/interactionOverlay.stylex.js', | |||
| }, | |||
| // Proposed; pending owner review (spec:AST-094 OQ4): the Reduce Motion | |||
There was a problem hiding this comment.
AST-094 has only OQ1 and OQ2, so this new stable ./motion/reducedMotion.stylex export points at an open question that does not exist. Could it cite the record that governs it, or ship under an experimental subpath?
10c2d49 to
1f32db9
Compare
There was a problem hiding this comment.
Approval needs an engineering owner to provide or identify the governing spec for the 44 px coarse-pointer target that CheckboxInput and RadioList get here: no current record states that size, and the pull request itself marks it as proposed and pending owner review. An engineering owner records that decision by commenting /approve-spec 1f32db965954a61a6e29b435510203f160f0fba9 on this pull request (the author may do so as an engineering owner), or by saying the targets should wait until a record that states a coarse-pointer target size is approved on main. Whatever that decision, the diff also needs narrowing:
- rebase onto
mainonce the stacked pull requests land, or retarget onto #8085, so the diff carries only the touch-target commits (34,549 authored lines today, 433 of them this pull request's).
[Automated review]
|
Posted by Cindy's agent on her behalf. Cindy's decision on target size: 44 px, for touch only. Requirements:
vjeux, could you update the PR to match these? |
1f32db9 to
2a302ea
Compare
|
Thanks. Fixed at 1fc0c9e:
|
There was a problem hiding this comment.
Changes requested: the CheckboxInput and RadioList hit areas follow spec:AST-094 FR42 and the target-size decision on this pull request, keep every layout unchanged, and the token now reads px, rem or em, but this branch still carries the whole #8085 chain (21,418 of its 22,247 authored lines). Please land #8085 and the pull requests beneath it on their own, then rebase so only the 12 touch-target files remain.
- The size report on this pull request flagged CheckboxInput (+2.8 KB) and RadioListItem (+3.2 KB) past the touch-only JavaScript budget; please justify the hit-area cost in the description, as
architecture:component-style-authoringINV6 asks.
[Automated review]
There was a problem hiding this comment.
Changes requested: the touch-target work itself, the CheckboxInput and RadioList hit areas and the --size-touch-target token, matches spec:AST-094 FR42 and the target-size decision, but this branch still merges all of #8085, so 21,368 of its 22,369 authored lines belong to that pull request. Please land #8085 first, then rebase so this pull request carries only its own change; nothing else in it blocks approval.
[Automated review]
There was a problem hiding this comment.
Changes requested: the CheckboxInput and RadioList hit areas and the --size-touch-target token match spec:AST-094 FR42 and the target-size decision on this pull request, and they leave every layout and mouse interaction unchanged. This branch, though, still merges a snapshot of #8085 that makes up 30,787 of its 31,801 changed lines and that #8085 has since moved past. Please land #8085 first, then rebase so this pull request carries only its own 16 files; nothing else found blocks approval.
[Automated review]
49c8e40 to
378f996
Compare
There was a problem hiding this comment.
Changes requested. The CheckboxInput and RadioList hit areas and the --size-touch-target token follow spec:AST-094 FR42 and DEC-20 and leave every layout unchanged, but five things need fixing first.
- This branch still merges a snapshot of #8085 that makes up 30,787 of its 31,801 changed lines and that #8085 has since replaced with a rebuild on
main, and the branch now conflicts withmain, so please land #8085 and then rebase so only this pull request's 16 files remain. - On a phone-sized Chromium page, a touch tap 9 px above a labelled
CheckboxInputbox toggles it onmainbut does nothing here, because the browser's touch adjustment moves that click onto the row's edge andhandleHitAreaClick(CheckboxInput.tsx:365-378) treats the edge as inside, so please decide the forwarded click from where the press began (thepointerdownpoint) and add a browser case for it. useTouchHitAreaadds its own module-level(pointer: coarse)listener (utils/touchHitArea.ts:87-110) beside the onewatchHitAreaalready keeps inutils/hitArea.ts, so please letwatchHitAreare-read the size in its own pointer-change handler (for example through aminSizegetter) and deletelistenForPointer.- The merged snapshot sets
isolation: 'isolate'on every List under every pointer (List.tsx:154), which changes mouse stacking againstspec:AST-017FR59, while #8085's current head gates it to(pointer: coarse). - The title says a radio option reaches a 44 pt target, but at the default 8 px gap an option reaches at most 38 px and a middle one 32 px, as the changeset says, so please retitle it to say the hit area grows toward 44 px.
Two new comments could also be shortened (see the inline notes); those are suggestions, not blockers.
[Automated review]
| const rewatches = new Set<() => void>(); | ||
| let pointerQuery: MediaQueryList | null = null; | ||
|
|
||
| function onPointerChange(): void { | ||
| for (const rewatch of [...rewatches]) { | ||
| rewatch(); | ||
| } | ||
| } | ||
|
|
||
| /** Runs `rewatch` on each change of the primary pointer; returns the stop. */ | ||
| function listenForPointer(view: Window, rewatch: () => void): () => void { | ||
| rewatches.add(rewatch); | ||
| if (pointerQuery == null) { | ||
| pointerQuery = view.matchMedia?.(TOUCH_QUERY) ?? null; | ||
| pointerQuery?.addEventListener('change', onPointerChange); | ||
| } | ||
| return () => { | ||
| rewatches.delete(rewatch); | ||
| if (rewatches.size === 0) { | ||
| pointerQuery?.removeEventListener('change', onPointerChange); | ||
| pointerQuery = null; | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
This is a second module-level change listener on (pointer: coarse): watchHitArea in utils/hitArea.ts already keeps one for every watch (onPointerChange), and with both, one labelled CheckboxInput registers two. Could watchHitArea re-read the size in its own pointer-change handler (for example minSize as a getter, the way WatchHitAreaOptions is already being extended in #8085), with useTouchHitArea passing () => touchTargetSize(element)? Then rewatches, pointerQuery and listenForPointer can go, and the base module's "listens to no media query of its own" test can come back.
| // band and a swipeable row (utils/touchHitArea.stylex.ts, ListItem) order | ||
| // above the rows after them and stay below a page-level sticky header at | ||
| // the same z-index (`spec:AST-027` DEC-1, DEC-2). | ||
| isolation: 'isolate', |
There was a problem hiding this comment.
This isolates every List under every pointer, so under a mouse a row's raised content now stacks below positioned content after the list, although the touch band is the only reason for it (spec:AST-017 FR59) and the changeset says a mouse is unchanged. #8085's current head gates it: isolation: {default: null, '@media (pointer: coarse)': 'isolate'}. A rebase onto #8085 picks that up.
| // How far a control's hit area grows under a coarse primary pointer | ||
| // (`spec:AST-094` FR42): a labelled checkbox and a radio option reach it | ||
| // through an invisible hit area that stops at half the gap to each | ||
| // neighbour, so a target between close neighbours stays smaller (a middle | ||
| // RadioList option reaches 32 px at the default 8 px gap). Rows and | ||
| // controls keep their drawn size. 44 px, the platform's control size: | ||
| // Apple's Human Interface Guidelines give iOS controls a hit target of at | ||
| // least 44 × 44 pt. A larger value, in px, rem or em, raises the reach; a | ||
| // smaller one never lowers it. It is read again when the pointer or the | ||
| // theme changes. |
There was a problem hiding this comment.
Non-blocking: the second sentence runs past 40 words and repeats the changeset. Suggested: "How far a hit area grows under a coarse pointer. It stops at half the gap to a neighbour. A smaller value never lowers it below 44 px."
| // The hit layer is the container's own ::after, so a tap on it lands on | ||
| // the container outside its box, where nothing else answers: it toggles | ||
| // the checkbox, as a tap on the box does. A click inside the box (the gap | ||
| // between box and label) stays as it was. |
There was a problem hiding this comment.
Non-blocking: one 36-word sentence with several topics. Suggested: "The hit layer is the container's ::after. A tap on it lands on the container, outside its box. Forward that tap to the input."
vjeux
changed the title
vjeux
changed the title
vjeux
changed the title
378f996 to
d816209
Compare
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec d816209 |
There was a problem hiding this comment.
Changes requested. The CheckboxInput and RadioList hit bands follow spec:AST-094 FR42 and the approved record text, keep every control's size, and add nothing for a mouse, but four things need fixing first.
- On a phone-sized Chromium page, a touch tap 9 px above a labelled
CheckboxInputtoggles it onmainbut does nothing here, because the browser's touch adjustment moves the click onto the row's top edge andhandleRowClick(CheckboxInput.tsx:436-449) counts the edge as inside, so please decide from thepointerdownpoint (or treat the edge as outside) and add a browser case for an adjusted tap. CheckboxInput.tsx:426-432andRadioListItem.tsx:319-325startwatchHitAreafrom a layout effect, but the watch is work caused by the row attaching, so please start it from a memoized callback ref merged into each row's existing ref, asInfoTip.tsx:157-162does, and delete both effects.- The title says a radio option's touch target is at least 44 px, but at the default 8 px gap an outer option reaches 38 px and a middle one 32 px, so please retitle it to say the hit area grows toward 44 px.
- The new FR14 and FR13 verification rows in the
CheckboxInputandRadioListItemrecords fill only two of five cells, while the verification-map contract asks each row for its representative states and the failure that must make it fail, so please fill those cells and the requirement rows' basis.
One comment could also be shortened (see the inline note); that is a suggestion, not a blocker.
[Automated review]
| const isOnBand = | ||
| event.clientX < box.left || | ||
| event.clientX > box.right || | ||
| event.clientY < box.top || | ||
| event.clientY > box.bottom; |
There was a problem hiding this comment.
On a phone-sized Chromium page, a touch tap 9 px above the box lands on the band, and the browser's touch adjustment then moves the click onto the row's top edge (clientY equals box.top). These strict comparisons count that edge as inside, so the click is dropped and nothing toggles, while main toggles the same tap. Deciding from the pointerdown point (which the browser does not adjust), or counting the edge as outside, keeps it; a browser case for an adjusted tap would pin it.
| useIsomorphicLayoutEffect(() => { | ||
| const row = rowRef.current; | ||
| if (row == null || !hasHitArea) { | ||
| return; | ||
| } | ||
| return watchHitArea(row, insets => writeHitInsets(row, insets)); | ||
| }, [hasHitArea]); |
There was a problem hiding this comment.
This watch is work caused by the row attaching. architecture:react-component-runtime INV6 gives that to a callback ref, and the row already has one. Could it be a useCallback ref keyed on hasHitArea that returns watchHitArea(...), merged with disabledMessageTooltip.interactionRef, as InfoTip.tsx:157-162 does? Then this effect and rowRef can go.
| useIsomorphicLayoutEffect(() => { | ||
| const row = rowRef.current; | ||
| if (row == null || isDisabled) { | ||
| return; | ||
| } | ||
| return watchHitArea(row, insets => writeHitInsets(row, insets)); | ||
| }, [isDisabled]); |
There was a problem hiding this comment.
Same as in CheckboxInput: a memoized callback ref keyed on isDisabled, passed to useMergedRefs(ref, …) in place of rowRef, can own this watch, and its cleanup stops it.
| const rowRef = useRef<HTMLDivElement>(null); | ||
| const mergedRowRef = useMergedRefs(ref, rowRef); | ||
| // Syncs the band's insets with the layout around the row, through | ||
| // utils/hitArea's observers. A disabled option watches nothing. |
There was a problem hiding this comment.
Suggestion (non-blocking): "A disabled option watches nothing." restates the isDisabled guard below; the first sentence is enough.
| | FR10, AR3 | `CheckboxInput.test.tsx` focus-ring ownership suite | Built-in and prop-dropping replacement indicators; blur | A ring on the wrong element, or none, fails. | `audit:CheckboxInput/a11y` | | ||
| | FR11 | `CheckboxInput.test.tsx` pressed-state suite and its suite for an indicator that paints its own press | Enabled press; disabled; touch fade | A missing overlay, or one on a disabled box, fails. | `audit:CheckboxInput/visual` | | ||
| | FR12, AR5 | `CheckboxInput.test.tsx` coarse-pointer, RTL, and forced-colors suites | `sm`, `md`; RTL; forced colors | A misplaced hit target or a mark lost in forced colors fails. Evidence gap: no test asserts field `width`. | `audit:CheckboxInput/layout` | | ||
| | FR14 | `touchTarget.test.tsx` hit-band, band-tap and minimum cases; `TouchTargets.interaction.browser.spec.ts` checkbox cases | |
There was a problem hiding this comment.
This row fills two of the table's five cells. The verification-map contract (architecture:component-test-sufficiency) asks each row for its representative states and the failure that must make it fail, for example: labelled, label-hidden, disabled and read-only; 24 and 60 px rows; a mouse; and a band where none belongs, a row that changes size, or a band under a mouse fails. The FR14 requirement row at line 123 also leaves its basis cell empty.
| | FR8 | `RadioList.test.tsx` pressed-state and bare-row cases; `RadioListItem.press.source-build.test.mjs` | Enabled, disabled, touch | Press paint on a disabled option, or a default row background, fails. Evidence gap: no test asserts the hover tint. | `audit:RadioListItem/visual` | | ||
| | FR9, FR10, AR3, AR4 | `RadioList.test.tsx` coarse-pointer, RTL, and forced-colors cases | Coarse pointer; RTL; forced colors | Evidence gap: the forced-colors case passes on any matching rule (the checkbox mark supplies one), so it does not prove the dot; the coarse-pointer and RTL cases check only the direction-symmetric centering class, not the 24 by 24 minimum or RTL placement. | `audit:RadioListItem/accessibility` | | ||
| | FR11, FR12 | `RadioList.test.tsx` theme-target and rest-forwarding cases | Sizes; selected; disabled | A target off the painted row, or a missing state, fails. Evidence gap: no test asserts `ref`, `className`, `style`, or `xstyle` on the row. | `audit:RadioListItem/theming` | | ||
| | FR13 | `touchTarget.test.tsx` hit-band and minimum cases; `TouchTargets.interaction.browser.spec.ts` RadioList case | |
There was a problem hiding this comment.
Same here: please add the states (enabled and disabled options, both orientations, a mouse), the failure (a band on a disabled option, a row or gap that changes size, a band past half the gap), and the audit section; the FR13 row at line 129 also needs its basis.
Under a coarse pointer, a labelled CheckboxInput and each enabled RadioList option get an invisible hit band from utils/hitArea, as InfoTip does. The band grows toward 44 x 44 px, never makes the target smaller than the row's own box, and stops at half the gap to a closer neighbour. Nothing changes size, and a mouse sees no change. Red first: with main's CheckboxInput and RadioListItem, 5 of the 9 cases in Indicator/touchTarget.test.tsx fail; the 4 layout guards pass.
|
/approve-spec ff1b675 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. This pull request comes from a fork, so its build, lint and test jobs stop at "Waiting for a maintainer's ci:run label"; a maintainer adds the ci:run label so CI runs.
The CheckboxInput and RadioList hit bands follow spec:AST-094 FR42 and the record text approved for this head: under a finger, a labelled checkbox and each enabled radio option take taps in a band clamped to half the gap to each neighbour, and a mouse sees no change. The four points from the previous round are fixed, including the tap 9 px above a checkbox, which toggles it again.
- Non-blocking: on a full-width checkbox row the band also covers the strip above and below the empty space right of the label, while that space itself still ignores taps; worth a look on a device.
[Automated review]
Implements
spec:AST-094FR42 for CheckboxInput and RadioList (owner decision, 2026-11-04).User need
On a phone, a standalone CheckboxInput's target is its 24 px row, and RadioList rows are 24 px with 8 px dead gaps. iOS controls are 44 × 44 pt; fingers miss.
Evidence
With Chromium and WebKit touch (
TouchTargets): a tap just above a CheckboxInput row toggles it, also when its click lands on the row's edge, a tap in a RadioList row's half of the gap selects it, and no box changes.Problem and solution fit
Under a coarse pointer, a labelled, enabled, editable CheckboxInput and each enabled RadioList option get an invisible hit band. It grows toward 44 × 44 px, never shrinks the target below the row, and stops at half the gap to a closer neighbour. Nothing changes size; a mouse sees no change.
watchHitAreaon the row, and its cleanup stops it.utils/touchHitArea.stylex.tsis the band: the row's coarse-only::after, grown by the insets, with the row's box clipped out, so the label, its links and the input keep their taps.pointerdownpoint, so a click moved onto the row's edge counts and a click from code does not. Radio rows already hand taps to the radio.Current specification
spec:AST-094FR42,component:CheckboxInputFR14 andcomponent:RadioListItemFR13 carry the owner's sentence verbatim: "Under a coarse pointer, the touch target grows toward 44 × 44 px, up to halfway to a neighboring target, and is never smaller than the control's own box (owner decision, 2026-11-04;spec:AST-094FR42)."Public delta and caller burden
No API, token or default change. Under a finger a RadioListItem row's
::afteris the band, so decorate a row with a child. Both importutils/hitArea, already loaded by Switch and InfoTip. Under a coarse pointer a CheckboxInput row isposition: relative, so an absolutely positioned child resolves against it.Design and alternatives
utils/touchHitArea.stylex.ts, and fix(RadioList): disabled radios stay focusable, and arrow keys skip disabled options #8823 re-pads the RadioListItem record's tables; whichever lands second merges.Scope
Writing rubric results
R1 narration: none; the dated parenthetical is the owner's verbatim attribution.
R2 research: none.
R3 evidence: the new verification rows name tests.
R4 rejected alternatives: none in a record.
R5 change references: none.
R6 open questions: none changed.
Testing
Rebuilt on main 35985f2 as one commit, ff1b675 (11 files): no #8085 snapshot, no token, no List change. It differs from 178420b only in the three record lines that carry the owner's sentence; every run below was made at 178420b, whose code and tests are byte-identical. It merges cleanly into main 273031c, which has not touched these files.
CheckboxInput.tsxandRadioListItem.tsx, 5 of the 9 cases inIndicator/touchTarget.test.tsxfail: the band, its press, and "grows a 24 px checkbox row (radio option) to 44 px, and leaves a 60 px one at its own box", which drive the realwatchHitArea. The 4 layout guards pass. At this head 9/9.TouchTargetson this head's Storybook build: Chromium 7/7 and WebKit 7/7 (container--cpus=4 -e CI=1 -e LP_NUM_THREADS=4), no retries. It covers a 60 px row (insets 0; the point 2 px above is not its own) and the mouse control for CheckboxInput and RadioList.astryx-build-sha.txtstamp CI writes.test:ui, 809 files, 20,125 tests.tsc0; ESLint 0 errors on the changed files; Prettier clean; INV29 0;check:knowledge,check:repo,check:use-client,check:token-docs,check:motion-roles,check:changesets,check:i18n-runtime,lab:readiness:check,verify-exports,sync-exports --checkand CLI api-types--check;review:pre0 findings; public text clean.