Repository navigation
feat(List): a row's touch target grows toward 44 px where its neighbors leave room - #8085
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
needs:design-review
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
![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 about List row height on touch screens: no current spec governs it, so an owner needs to approve, amend (for example, rows with an action only) or reject the 52 / 62 / 44 px floors under (pointer: coarse), which also reach static and Markdown lists, and name the record that holds them. 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. Two changes are needed either way:
- On a touch screen, bullets and numbers now sit 6 px above their text in marker lists and Markdown lists, because the row grows while the marker stays at its top; please keep the marker on the label's first line and cover it with a touch-context test.
- The diff carries the whole stack (14,152 authored lines); once the earlier pull requests land, rebase so this one holds only its own commit.
[Automated review]
| density === 'compact' | ||
| ? listRowStyles.touchCompact | ||
| : isRenderable(description) | ||
| ? listRowStyles.touchTwoLine | ||
| : listRowStyles.touchOneLine, |
There was a problem hiding this comment.
On a coarse pointer this floor makes the row taller than its content: Item centers the label, but the marker container keeps align-self: baseline and stays at the row's top, so bullets and numbers sit 6 px above their text (measured in core-list--bulleted-list, 40 to 52 px rows, and in Markdown, 32 to 44 px items). Keep the marker on the label's first line when the row grows, for example with the first-line slot treatment Item uses for align="first-line", and add a touch-context check that the two line up.
| /** | ||
| * A row's height on a touch screen (primary pointer coarse): UIKit's table | ||
| * rows (`list.metrics`), at least that tall, never shorter than the | ||
| * platform's control size. A theme's `min-height` on `list-item` replaces it. | ||
| */ |
There was a problem hiding this comment.
This docblock describes the touch row heights, but it sits on embeddedStyles (the divider radius reset). listRowStyles in listRow.stylex.ts already carries the same text, so this copy can go.
| }); | ||
|
|
||
| /** | ||
| * The touch presentation's query (utils/touchPresentation.ts TOUCH_QUERY), |
There was a problem hiding this comment.
utils/touchPresentation.ts does not exist at this head, so this pointer to TOUCH_QUERY leads nowhere; name the query's actual source or drop the reference.
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| Table | 91.8 kB (+867 B) · 16.8 kB CSS (+98 B) | — | — · — · — |
| Markdown | 152.9 kB (+852 B) · 19.0 kB CSS (+105 B) | — | — · — · — |
| CheckboxListItem | 59.0 kB (+847 B) · 10.6 kB CSS (+83 B) | — | — · — · — |
123 more components
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| ListItem | 58.2 kB (+839 B) · 10.6 kB CSS (+83 B) | — | — · — · — |
| TableCell | 83.6 kB (+820 B) · 16.6 kB CSS (+100 B) | — | — · — · — |
| ContextMenu | 82.4 kB (+814 B) · 16.1 kB CSS (+105 B) | — | — · — · — |
| TableHeaderCell | 83.2 kB (+817 B) · 16.3 kB CSS (+101 B) | — | — · — · — |
| lab/CommentThread | 265.0 kB (+847 B) · 2.4 kB CSS (0) | — | — · — · — |
| charts/ChartTooltip | 143.0 kB (+828 B) · 423 B CSS (0) | — | — · — · — |
| lab/Comments | 270.3 kB (+825 B) · 2.5 kB CSS (0) | — | — · — · — |
| charts/Chart | 165.7 kB (+802 B) · 494 B CSS (0) | — | — · — · — |
| RadioListItem | 56.4 kB (+635 B) · 9.5 kB CSS (+27 B) | — | — · — · — |
| InfoTip · Storybook | 38.0 kB (+450 B) · 5.1 kB CSS (+117 B) | 148 (−12) | — · — · — |
| CheckboxInput · Storybook | 53.3 kB (+422 B) · 8.8 kB CSS (+34 B) | 416 (−8) | — · — · — |
| MultiSelector | 79.7 kB (+433 B) · 15.5 kB CSS (+22 B) | — | — · — · — |
| richtext/RichTextEditor | 283.1 kB (+436 B) · 2.6 kB CSS (0) | — | — · — · — |
| richtext/RichTextView | 263.5 kB (+420 B) · 2.3 kB CSS (0) | — | — · — · — |
| DropdownMenu | 77.4 kB (+275 B) · 16.2 kB CSS (+97 B) | — | — · — · — |
| PowerSearch | 246.6 kB (+254 B) · 22.4 kB CSS (+109 B) | — | — · — · — |
| MoreMenu | 77.6 kB (+265 B) · 16.2 kB CSS (+97 B) | — | — · — · — |
| lab/TransferList | 79.3 kB (+297 B) · 1.7 kB CSS (0) | — | — · — · — |
| Selector | 82.4 kB (+276 B) · 16.1 kB CSS (+14 B) | — | — · — · — |
| lab/TransferListSelector | 86.4 kB (+281 B) · 1.9 kB CSS (0) | — | — · — · — |
| ContextMenuItem | 24.1 kB (+279 B) · 6.0 kB CSS (+1 B) | — | — · — · — |
| Pagination | 91.2 kB (+247 B) · 16.5 kB CSS (+11 B) | — | — · — · — |
| richtext/RichTextEditorToolbar | 233.0 kB (+254 B) · 454 B CSS (0) | — | — · — · — |
| BreadcrumbItem | 71.0 kB (+246 B) · 11.2 kB CSS (+1 B) | — | — · — · — |
| DropdownMenuCheckboxItem | 20.9 kB (+234 B) · 6.3 kB CSS (+1 B) | — | — · — · — |
| SelectorOption | 19.1 kB (+221 B) · 5.7 kB CSS (+2 B) | — | — · — · — |
| DropdownMenuRadioItem | 20.9 kB (+216 B) · 6.3 kB CSS (+1 B) | — | — · — · — |
| Item · Storybook | 15.7 kB (+215 B) · 5.2 kB CSS (+1 B) | 1,682 (+51) | — · — · — |
| DropdownMenuSubMenu | 59.3 kB (+211 B) · 9.2 kB CSS (+1 B) | — | — · — · — |
| DropdownMenuItem | 20.1 kB (+209 B) · 5.9 kB CSS (+2 B) | — | — · — · — |
| Switch | 89.8 kB (+94 B) · 9.5 kB CSS (+3 B) | — | — · — · — |
| List | 5.4 kB (+13 B) · 469 B CSS (+34 B) | — | — · — · — |
| CheckboxList | 45.4 kB (+26 B) · 7.5 kB CSS (+12 B) | — | — · — · — |
| DateTimeInput | 175.0 kB (+11 B) · 17.2 kB CSS (+19 B) | — | — · — · — |
| SelectableCard | 16.7 kB (+21 B) · 7.2 kB CSS (+3 B) | — | — · — · — |
| Overlay | 21.5 kB (+20 B) · 3.9 kB CSS (+3 B) | — | — · — · — |
| ClickableCard | 16.5 kB (+20 B) · 6.9 kB CSS (+2 B) | — | — · — · — |
| lab/Schedule | 63.3 kB (+21 B) · 3.9 kB CSS (0) | — | — · — · — |
| Typeahead | 64.8 kB (+18 B) · 11.2 kB CSS (+1 B) | — | — · — · — |
| ComplexSelector | 57.3 kB (+17 B) · 10.4 kB CSS (+1 B) | — | — · — · — |
| Field | 41.7 kB (+18 B) · 6.3 kB CSS (0) | — | — · — · — |
| NumberInput | 56.5 kB (+17 B) · 10.1 kB CSS (+1 B) | — | — · — · — |
| Token | 27.2 kB (+19 B) · 4.7 kB CSS (−1 B) | — | — · — · — |
| TreeList | 40.8 kB (+16 B) · 5.3 kB CSS (+2 B) | — | — · — · — |
| lab/CommentComposer | 108.4 kB (+18 B) · 1.9 kB CSS (0) | — | — · — · — |
| DateInput | 153.0 kB (+11 B) · 15.5 kB CSS (+6 B) | — | — · — · — |
| Slider | 92.2 kB (+14 B) · 8.3 kB CSS (+3 B) | — | — · — · — |
| Tokenizer | 73.6 kB (+10 B) · 17.1 kB CSS (+7 B) | — | — · — · — |
| FieldLabel | 37.8 kB (+15 B) · 4.8 kB CSS (0) | — | — · — · — |
| InputGroup | 43.9 kB (+16 B) · 7.3 kB CSS (−1 B) | — | — · — · — |
| TextInput | 54.3 kB (+15 B) · 9.9 kB CSS (0) | — | — · — · — |
| TimeInput | 139.8 kB (+8 B) · 14.5 kB CSS (+7 B) | — | — · — · — |
| FileInput | 55.1 kB (+14 B) · 9.6 kB CSS (0) | — | — · — · — |
| RadioList | 43.1 kB (+14 B) · 6.4 kB CSS (0) | — | — · — · — |
| lab/EmojiPicker | 72.1 kB (+14 B) · 722 B CSS (0) | — | — · — · — |
| lab/SelectableCardGroup | 51.7 kB (+14 B) · 162 B CSS (0) | — | — · — · — |
| TextArea | 49.0 kB (+12 B) · 8.8 kB CSS (0) | — | — · — · — |
| lab/ListInput | 84.8 kB (+12 B) · 1.6 kB CSS (0) | — | — · — · — |
| BaseTypeahead | 56.9 kB (0) · 9.6 kB CSS (+11 B) | — | — · — · — |
| DateRangeInput | 74.2 kB (+9 B) · 11.2 kB CSS (+1 B) | — | — · — · — |
| lab/ChatEmojiPicker | 68.6 kB (+10 B) · 773 B CSS (0) | — | — · — · — |
| lab/DiffViewer | 72.7 kB (+10 B) · 2.1 kB CSS (0) | — | — · — · — |
| SegmentedControl | 83.6 kB (0) · 6.7 kB CSS (+8 B) | — | — · — · — |
| lab/ChatReactionBar | 69.2 kB (+8 B) · 1.1 kB CSS (0) | — | — · — · — |
| AppShell | 75.0 kB (0) · 12.2 kB CSS (+4 B) | — | — · — · — |
| SideNav | 75.5 kB (0) · 10.1 kB CSS (+4 B) | — | — · — · — |
| CodeBlock | 60.5 kB (0) · 10.0 kB CSS (+3 B) | — | — · — · — |
| Calendar | 55.7 kB (0) · 9.3 kB CSS (+2 B) | — | — · — · — |
| LayerProvider | 64.1 kB (+1 B) · 9.4 kB CSS (+1 B) | — | — · — · — |
| MobileNav | 56.4 kB (0) · 9.3 kB CSS (+2 B) | — | — · — · — |
| Thumbnail | 50.9 kB (0) · 10.2 kB CSS (+2 B) | — | — · — · — |
| TopNav | 58.0 kB (0) · 9.6 kB CSS (+2 B) | — | — · — · — |
| TopNavMegaMenu | 55.8 kB (0) · 10.6 kB CSS (+2 B) | — | — · — · — |
| AlertDialog | 89.7 kB (0) · 13.1 kB CSS (+1 B) | — | — · — · — |
| Avatar | 34.8 kB (0) · 5.4 kB CSS (+1 B) | — | — · — · — |
| AvatarGroup | 37.5 kB (0) · 5.5 kB CSS (+1 B) | — | — · — · — |
| Carousel | 52.4 kB (+1 B) · 9.5 kB CSS (0) | — | — · — · — |
| ChatLayout | 49.7 kB (0) · 9.2 kB CSS (+1 B) | — | — · — · — |
| ChatMessageList | 22.9 kB (0) · 6.8 kB CSS (+1 B) | — | — · — · — |
| ChatToolCalls | 26.0 kB (0) · 8.2 kB CSS (+1 B) | — | — · — · — |
| CommandPalette | 58.2 kB (0) · 12.3 kB CSS (+1 B) | — | — · — · — |
| Popover | 54.9 kB (−3 B) · 12.8 kB CSS (+4 B) | — | — · — · — |
| ProgressBar | 10.9 kB (+1 B) · 6.2 kB CSS (0) | — | — · — · — |
| SideNavItem | 56.4 kB (0) · 9.8 kB CSS (+1 B) | — | — · — · — |
| StatusDot | 22.9 kB (0) · 5.2 kB CSS (+1 B) | — | — · — · — |
| Timer | 9.6 kB (0) · 5.9 kB CSS (+1 B) | — | — · — · — |
| TopNavHeading | 60.7 kB (0) · 9.8 kB CSS (+1 B) | — | — · — · — |
| TopNavMenu | 60.6 kB (0) · 9.9 kB CSS (+1 B) | — | — · — · — |
| lab/TourStep | 60.8 kB (+1 B) · 590 B CSS (0) | — | — · — · — |
| AvatarGroupOverflow | 40.1 kB (0) · 5.4 kB CSS (0) | — | — · — · — |
| Banner | 48.5 kB (0) · 9.2 kB CSS (0) | — | — · — · — |
| ChatComposer | 63.1 kB (0) · 10.5 kB CSS (0) | — | — · — · — |
| ChatComposerInput | 61.1 kB (0) · 10.0 kB CSS (0) | — | — · — · — |
| ChatComposerTokenElement | 61.1 kB (0) · 10.0 kB CSS (0) | — | — · — · — |
| ChatDictationButton | 47.1 kB (0) · 8.8 kB CSS (0) | — | — · — · — |
| ChatLayoutScrollButton | 47.1 kB (0) · 8.9 kB CSS (0) | — | — · — · — |
| ChatSendButton | 47.0 kB (0) · 8.7 kB CSS (0) | — | — · — · — |
| CheckboxIndicator | 2.7 kB (0) · 813 B CSS (0) | — | — · — · — |
| CheckIndicator | 8.0 kB (0) · 681 B CSS (0) | — | — · — · — |
| CommandPaletteInput | 26.9 kB (0) · 7.4 kB CSS (0) | — | — · — · — |
| DialogHeader | 49.4 kB (0) · 10.2 kB CSS (0) | — | — · — · — |
| DrawerHeader | 49.6 kB (0) · 11.6 kB CSS (0) | — | — · — · — |
| Heading | 7.9 kB (0) · 4.9 kB CSS (0) | — | — · — · — |
| HoverCard | 19.3 kB (0) · 3.9 kB CSS (0) | — | — · — · — |
| Lightbox | 54.6 kB (0) · 9.1 kB CSS (0) | — | — · — · — |
| RadioIndicator | 2.5 kB (0) · 725 B CSS (0) | — | — · — · — |
| SideNavCollapseButton | 47.4 kB (0) · 8.7 kB CSS (0) | — | — · — · — |
| SideNavHeading | 61.3 kB (0) · 10.0 kB CSS (0) | — | — · — · — |
| Spinner | 18.5 kB (0) · 5.7 kB CSS (0) | — | — · — · — |
| TabMenu | 58.0 kB (0) · 9.8 kB CSS (0) | — | — · — · — |
| Text | 7.4 kB (0) · 4.9 kB CSS (0) | — | — · — · — |
| Timestamp | 21.4 kB (0) · 8.9 kB CSS (0) | — | — · — · — |
| Toast | 60.3 kB (0) · 9.0 kB CSS (0) | — | — · — · — |
| Tooltip | 20.6 kB (0) · 3.9 kB CSS (0) | — | — · — · — |
| lab/CommentPin | 48.3 kB (0) · 1.2 kB CSS (0) | — | — · — · — |
| Button | 46.5 kB (0) · 8.7 kB CSS (−1 B) | — | — · — · — |
| IconButton | 46.6 kB (0) · 8.7 kB CSS (−1 B) | — | — · — · — |
| Link | 44.1 kB (0) · 6.8 kB CSS (−1 B) | — | — · — · — |
| MobileNavToggle | 46.9 kB (0) · 8.7 kB CSS (−1 B) | — | — · — · — |
| Stepper | 48.2 kB (0) · 8.8 kB CSS (−1 B) | — | — · — · — |
| TableSelectionToolbar | 56.1 kB (0) · 11.8 kB CSS (−1 B) | — | — · — · — |
| ToggleButton | 47.4 kB (0) · 8.8 kB CSS (−1 B) | — | — · — · — |
| charts/ChartLegend | 35.5 kB (−2 B) · 224 B CSS (0) | — | — · — · — |
Package size: @astryxdesign/core 6.47 MB packed (+10.5 kB); no new dependencies.
API changes: core +0 · ~1 · −0
Committed API report behind this build: cli 9 rows, core 5 rows; run pnpm api:report.
1 API change (advisory)
| Package | Where | Signature | Usage | |
|---|---|---|---|---|
| ~ | core |
. · Hooks · hook useClickableContainer |
hook useClickableContainer: ({…}: UseClickableContainerOptions) => ClickableContainerResult → hook useClickableContainer: (options: UseClickableContainerOptions) => ClickableContainerResult |
import {useClickableContainer} from '@astryxdesign/core'; useClickableContainer(…) |
Wide change (shared styles: packages/core/src/theme/derivedVarRegistry.test.ts, packages/core/src/utils/touchHitArea.stylex.ts): add ci:run-visual, ci:run-a11y or ci:run-rtl to run them.
Generated by PR Enrichment workflow | View full report
081b45a to
e1fefc4
Compare
e1fefc4 to
ea5685d
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about List row height on touch screens: no current spec governs it, so an owner needs to approve, amend (for example, rows with an action only) or reject the 52 / 62 / 44 px floors under (pointer: coarse), which also reach static and Markdown lists, and name the record that holds them. 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. Two changes are needed either way:
- On a touch screen, list markers still sit 6 px above their text in marker lists and Markdown lists, because the row grows while the marker stays at its top; please keep the marker on the label's first line and cover it with a touch-context test.
- The diff carries the whole stack (15,116 authored lines); once the earlier pull requests land, rebase so this one holds only its own commit (the earlier line notes on
ListItem.tsxandlistRow.stylex.tsstill apply).
[Automated review]
ea5685d to
6caf11a
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about List row height on touch screens: no current spec governs the 52 / 62 / 44 px floors under (pointer: coarse), which also reach static and Markdown lists, so an owner needs to approve, amend (for example, rows with an action only) or reject them and name the record that holds them. 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. Needed either way:
- Keep list markers on the label's first line: on a touch screen they sit 6 px above their text in marker and Markdown lists; a touch-context test should hold it.
- Once the earlier stacked pull requests land, rebase so this one carries only its own commit (the earlier line notes on
ListItem.tsxandlistRow.stylex.tsstill apply).
[Automated review]
6caf11a to
34fb8e7
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about two pieces of direction no current spec covers: the 52 / 62 / 44 px List row floors under (pointer: coarse), which also reach static and Markdown lists (approve, amend or reject them and name the record that holds them), and the draft spec:AST-094 requirements FR1–FR23. 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. Needed either way:
- Keep list markers on the label's first line: on a touch screen they sit 6 px above their text in marker and Markdown lists, and a touch-context test should hold it.
- Make CI's typecheck pass: the new "Back, slowly" path in
List.stories.tsx:429handsanimatea{durationMs}timing thatHoldTimingdoes not accept (TS2345). - Once the earlier stacked pull requests land, rebase so this one carries only its own commit (the earlier line notes on
ListItem.tsxandlistRow.stylex.tsstill apply).
[Automated review]
| }; | ||
| const settle = async (to: 0 | 1, timing?: {durationMs: number}) => { | ||
| const current = hold.current; | ||
| const arrived = await current?.animate(to, timing); |
There was a problem hiding this comment.
CI's typecheck fails here (TS2345): timing is typed {durationMs: number}, which matches no member of HoldTiming, since a duration needs a curve (HoldCurveTiming). Pass {durationMs: 3000, curve: [0, 0, 1, 1]} or type the parameter as HoldTiming; if a bare duration should be supported, as the runtime already allows, make curve optional in HoldCurveTiming instead.
There was a problem hiding this comment.
Changes requested: one blocking item. Under a finger, interactive List rows now grow their tap target toward 44 px without reaching a neighbour's, as component:List FR10 asks, and mouse behaviour is unchanged.
- Miss:
InfoTipstill keeps its own callback ref overwatchHitAreaand its own::afterhit layer (packages/core/src/InfoTip/InfoTip.tsx, lines 109-123 and 153-161) beside the new shareduseTouchHitAreaandtouchHitAreaStyles.layer, so please move InfoTip onto the shared hook and layer and delete its copy, or say in the description why the shared layer cannot serve it. - Optional: once this lands, the readiness row of
docs/specs/components/List/List.spec.mdstill says the FR10 touch target is "specified, not implemented yet", and a small follow-up keeps the record accurate.
CI has not run on this head yet: it skips drafts, and because the branch lives in a fork, a maintainer adds the ci:run label once it is ready for review.
[Automated review]
| * <li ref={hitAreaRef} {...stylex.props(isInteractive && touchHitAreaStyles.layer)} /> | ||
| * ``` | ||
| */ | ||
| export function useTouchHitArea(enabled: boolean): RefCallback<HTMLElement> { |
There was a problem hiding this comment.
Miss (not raised in an earlier round): InfoTip still runs its own copy of this pattern, a callback ref over watchHitArea (InfoTip/InfoTip.tsx lines 153-161) and its own ::after reading the same inset properties (lines 109-123). Moving InfoTip onto useTouchHitArea(true) and touchHitAreaStyles.layer, and deleting its copy, keeps one path, so the stop-time clearing, the motion wait and the border offset here also reach InfoTip's users. If the shared layer cannot serve InfoTip, please say why in the description.
There was a problem hiding this comment.
Done at 5ab35de: InfoTip's button uses useTouchHitArea(true) and touchHitAreaStyles.layer, and its own watch and ::after are gone. A new test checks that a stopped watch clears its insets.
There was a problem hiding this comment.
Per the design owner, InfoTip's move is now its own pull request, #9916, stacked on this one. This pull request keeps InfoTip as it is on main.
There was a problem hiding this comment.
Update: folded back into this pull request at f33aedc, per the design owner.
|
/approve-spec 647cf8e |
|
/approve-spec aca580d |
There was a problem hiding this comment.
Changes requested: one blocking item. Under a finger, interactive List rows now grow their tap target toward 44 px without reaching a neighbour's, as component:List FR10 asks, and the browser proofs pass on this head.
InfoTipstill keeps its own callback ref overwatchHitAreaand its own::afterhit layer (InfoTip/InfoTip.tsxlines 109-123 and 153-161) beside the shareduseTouchHitAreaandtouchHitAreaStyles.layerthis pull request adds, and the copies already differ (only the shared one clears its insets when it stops and waits for nearby motion to settle), so move InfoTip onto the shared hook and layer here, as the previous revision did, rather than in #9916.
Non-blocking: the changeset says "a mouse is unchanged", but the delegate-marker change fixes a mouse-visible ClickableCard defect that the new test shows on main, so one changeset sentence would tell consumers. The stack line names #9894, which no longer contains this head.
[Automated review]
| * <li ref={hitAreaRef} {...stylex.props(isInteractive && touchHitAreaStyles.layer)} /> | ||
| * ``` | ||
| */ | ||
| export function useTouchHitArea(enabled: boolean): RefCallback<HTMLElement> { |
There was a problem hiding this comment.
Blocking (one shared path): InfoTip still runs its own copy of this hook and layer, a callback ref over watchHitArea (InfoTip/InfoTip.tsx lines 153-161) and an ::after reading the same inset properties (lines 109-123). Its watch neither clears its insets on stop nor waits for nearby motion, as this hook does. Moving InfoTip onto useTouchHitArea(true) and touchHitAreaStyles.layer in this pull request, and deleting its copy, leaves one path.
There was a problem hiding this comment.
The design owner asked for one solution per pull request. This one keeps the List touch target, and InfoTip's move is #9916, stacked on it, which deletes the copy and adds a test for the stop-time clearing. Until #9916 merges, InfoTip keeps its copy exactly as on main, so this pull request changes nothing for InfoTip.
There was a problem hiding this comment.
Fair point — you're right on the scope. This PR doesn't touch InfoTip, so its copy is exactly the pre-existing one from main, not a new duplication this PR introduces, and forcing the move in here would expand the scope the design owner asked to keep to one solution per PR.
The "blocking" framing was too strong if #9916, stacked on this one, deletes the copy and adds the stop-time clearing test. The remaining risk is only sequencing: if #9916 stalls or is dropped, the duplication persists. I can't change the verdict here, but the next full review should re-evaluate it with #9916 in view.
There was a problem hiding this comment.
Update: folded into this pull request at f33aedc, per the design owner.
|
/approve-spec 3d0e824 |
There was a problem hiding this comment.
Changes requested: the List touch target matches component:List FR10 and passes its browser proofs, but one shared-path issue remains before this can land.
Blocking
- InfoTip still runs its own copy of the hook and layer this pull request adds (a callback ref over
watchHitAreaatInfoTip.tsx:153-161and its own::afterat:109-123); please move InfoTip ontouseTouchHitArea(true)andtouchHitAreaStyles.layerhere and delete the copy, as #9916 does, so a fix to the shared hit area reaches InfoTip users too.
Non-blocking
- Miss: a
ListItemwith an inlinerefrestarts its hit-area watch and clears its band on every parent render (ListItem.tsx:422,touchHitArea.ts:67-70), and PowerSearch's filter rows pass one. - The
Stack:line does not form a stack: neither #9916 nor #9894 contains this head.
[Automated review]
| * <li ref={hitAreaRef} {...stylex.props(isInteractive && touchHitAreaStyles.layer)} /> | ||
| * ``` | ||
| */ | ||
| export function useTouchHitArea(enabled: boolean): RefCallback<HTMLElement> { |
There was a problem hiding this comment.
Blocking (one shared path): InfoTip still runs its own copy of this hook and layer: a callback ref over watchHitArea (InfoTip/InfoTip.tsx lines 153-161) and an ::after that reads the same inset properties (lines 109-123). Its copy does not clear its insets on stop or wait for nearby motion as this hook does. Please move InfoTip onto useTouchHitArea(true) and touchHitAreaStyles.layer in this pull request and delete the copy (the change #9916 makes).
There was a problem hiding this comment.
Folded at f33aedc, per the design owner: InfoTip's button uses useTouchHitArea(true) and touchHitAreaStyles.layer, and its copy is deleted.
| const isTouchTarget = | ||
| (onClick != null || href != null || interactiveRef != null) && !isDisabled; | ||
| const hitAreaRef = useTouchHitArea(isTouchTarget); | ||
| const mergedRef = useMergedRefs(ref, hitAreaRef); |
There was a problem hiding this comment.
Miss (non-blocking): this merged ref changes whenever the caller's ref does. With an inline ref (PowerSearch's filter rows at PowerSearchBottomSheet.tsx:2121 and :2339), every render runs the old cleanup, whose stop() writes zero insets (utils/touchHitArea.ts:67-70), and starts a new watch. So the band disappears until the next measurement, and the row gets --astryx-hit-inset-*: 0px written even under a mouse. Keying the watch to the element, or skipping the zero write when nothing was written, avoids the churn.
There was a problem hiding this comment.
Fixed at f33aedc: the watch is held per element, so a ref that changes within one commit keeps it and its insets, and a watch that wrote nothing writes nothing. Tests cover an inline-ref re-render and a watch that never measured.
| // Under a coarse primary pointer, a List row's whole touch target | ||
| // follows its link. That covers a tap in the row's padding, outside the | ||
| // anchor, and a tap in the band past its box. The click goes to the | ||
| // row's anchor with its modifier keys, so the link component and the | ||
| // browser pick the destination. The primary pointer decides, as it | ||
| // decides the band, not the pointer that made the click. With a fine | ||
| // primary pointer a click in the padding does nothing. A click with no | ||
| // pointer behind it does nothing either. |
There was a problem hiding this comment.
Miss (optional, comments rule): the last two sentences restate the if below. Suggested comment: "Under a coarse primary pointer, a tap in the row's padding or band follows its link. The primary pointer decides, not the click's pointer type, since WebKit reports a finger's click as mouse."
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet, because the pull request is a draft from a fork. Once it is marked ready, a maintainer adds the ci:run label so CI runs.
The List touch target matches component:List FR10 and spec:AST-094 FR42 and passes its browser proofs, InfoTip now shares the same hook and layer, and the pressable-marker fix restores component:ClickableCard FR2 with a test that fails on main.
Non-blocking:
ListItem.tsx:415: the comment restatesuseTouchHitArea(isTouchTarget)on the next lines; delete it or keep only the rule the code does not show.component:List's readiness row still calls FR10 unimplemented; a follow-up record edit can mark it shipped.- The
Stack:line in the description does not form a stack: #9894 declares a different line and does not contain this head.
[Automated review]
| // sticky header. Outside a List nothing bounds a raise, so a row raises | ||
| // nothing there. | ||
| const isInList = ctx != null; | ||
| // Under a finger, the row's hit target grows where its neighbors leave room. |
There was a problem hiding this comment.
Non-blocking (INV29): this restates useTouchHitArea(isTouchTarget) just below. Delete it, or keep only the rule the code does not show, for example: "A disabled row offers no touch target."
|
/approve-spec f33aedc |
There was a problem hiding this comment.
This pull request is reviewed as part of its stack; see the review on #9894.
There was a problem hiding this comment.
Changes requested.
The touch target itself matches component:List FR10: rows keep their size and paint, the band stops at half the gap to each neighbour, and a mouse sees no change. One blocking finding remains:
- Regression: under a coarse pointer an interactive
ListItem's::afterbecomes the hit layer, whose clip path hides any::afterdecoration a caller adds, whilecomponent:ListFR11 tells callers to decorate such a row with::afteror a child, so either amend FR11 in this pull request with an engineering owner's/approve-spec(and the matching note inprivateAttributes/press.stylex.ts) or move the layer off the row's::after.
The declared stack with #9894 no longer holds, because #9894 is based on an earlier head of this pull request, so this pull request was reviewed on its own.
[Automated review]
| hasMarkers && styles.withCounter, | ||
| hasDividers && styles.withDivider, | ||
| hasDividers && embeddedStyles.noRadius, | ||
| isTouchTarget && touchHitAreaStyles.layer, |
There was a problem hiding this comment.
Under a coarse pointer this makes the row's ::after the hit layer. component:List FR11 tells callers to decorate an interactive row with ::after or a child, but a caller's ::after here keeps the layer's even-odd clip path and shows only 2 px and 1 px slivers, and its top/bottom/left/right replace the band's offsets. Amend FR11 in this pull request (an engineering owner approves with /approve-spec) and the note in privateAttributes/press.stylex.ts, or host the layer somewhere other than the row's ::after.
There was a problem hiding this comment.
Amended component:List FR11 in this pull request: an interactive ListItem row inside a List also uses its ::after as its touch hit area (FR10), so decorate such a row with a child. The record's compatibility section says the same, and so does the note in privateAttributes/press.stylex.ts.
|
/approve-spec 2f054d2 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet, because the pull request is a draft from a fork. Once it is marked ready, a maintainer adds the ci:run label so CI runs.
The List touch target matches component:List FR10 and spec:AST-094 FR42: rows keep their size and paint, the band stops at half the gap to each neighbour, and a mouse sees no change. The FR11 amendment that gives an interactive row's ::after to the hit layer has an engineering owner's approval for this head, which resolves the earlier ::after finding.
Non-blocking follow-ups:
- The new FR11 sentence says "inside a List", but the layer is on every interactive
ListItem, and DEC-6 still sends decoration to::after; dropping "inside a List" and aligning DEC-6 would keep the record consistent with the code. - The description's "Current specification" section still says no record changes, although this head amends FR11.
[Automated review]
| | FR9 | The list element containing the rows MUST clip in the inline axis with `overflow-inline: clip`, never `hidden`, except for static Markdown document lists that expose no swipe actions (DEC-5). Those Markdown lists, ordered or unordered, MAY use visible inline overflow so their own or descendant number markers remain whole while retaining AST-061 FR11's text starts and nested insets. General Lists and any collection with swipe-capable rows MUST retain the clip. Under `outset` and `bleed` its inline edges MUST reach the host's padding edge, so the clip never cuts resting row paint and a swiped row paints no further than that edge. | `spec:AST-057/FR9`; `component:List/DEC-5`; `spec:AST-061/FR11` | Verified by `List.test.tsx` and real-browser geometry | | ||
| | FR10 | Under a coarse primary pointer (`spec:AST-094` FR13), every ListItem row MUST offer a hit target at least 44 px tall, through its padding or an invisible hit area, with no visual change, where its neighbors leave room: the hit area never overlaps a neighboring row's or control's target, and between close neighbors it is clamped to half the gap; row height and geometry stay as they are at every density. A fine pointer keeps today's hit target. | Owner decision, 2026-10-26: no row-height change, a 44 px touch target | Accepted contract | | ||
| | FR11 | Under a coarse primary pointer, an interactive row's press MUST follow the cell profile (`spec:AST-094` FR11): it paints the `--color-overlay-pressed` token above the row's background and below its content without adding an element, and its release fades over the press model's release duration and curve on the compositor (`spec:AST-094` FR5). The row's dividers stay drawn while it is pressed, in every theme. At rest the row's paint and stacking are as they were. Under the touch presentation, an interactive Item, ListItem or TreeList row uses its `::before` as the press layer, and a pressed row's divider keeps its resting colour. A caller's own `::before` on such a row merges into the press layer only while it is pressed; use `::after` or a child for decoration. | `spec:AST-094` FR5, FR11; `component:List/DEC-6` | Accepted contract | | ||
| | FR11 | Under a coarse primary pointer, an interactive row's press MUST follow the cell profile (`spec:AST-094` FR11): it paints the `--color-overlay-pressed` token above the row's background and below its content without adding an element, and its release fades over the press model's release duration and curve on the compositor (`spec:AST-094` FR5). The row's dividers stay drawn while it is pressed, in every theme. At rest the row's paint and stacking are as they were. Under the touch presentation, an interactive Item, ListItem or TreeList row uses its `::before` as the press layer, and a pressed row's divider keeps its resting colour. A caller's own `::before` on such a row merges into the press layer only while it is pressed; use `::after` or a child for decoration. An interactive ListItem row inside a List also uses its `::after` as its touch hit area (FR10), so decorate such a row with a child. | `spec:AST-094` FR5, FR11; `component:List/DEC-6` | Accepted contract | |
There was a problem hiding this comment.
Non-blocking: this sentence covers rows "inside a List", but ListItem.tsx composes the hit layer on every interactive ListItem (and List.test.tsx asserts a row outside a List keeps its band). DEC-6 below still says decoration that must stay through a press "goes on ::after or a child". Suggest "An interactive ListItem row also uses its ::after as its touch hit area (FR10), so decorate such a row with a child", and the same carve-out at the end of DEC-6.
|
/approve-spec 288433f |
There was a problem hiding this comment.
Approved. CI has finished green on this head.
The List touch target matches component:List FR10 and spec:AST-094 FR42: rows keep their size and paint, the band stops at half the gap to each neighbour, and a mouse sees no change. The component:List FR11 amendment that gives an interactive row's ::after to the hit layer has an engineering owner's approval for this head.
Non-blocking follow-ups:
- The new FR11 sentence says "inside a List", but the layer is on every interactive
ListItem, and DEC-6 still sends decoration to::after; dropping "inside a List" and aligning DEC-6 would keep the record consistent with the code. - The description's "Current specification" section still says no record changes, although this head amends FR11.
[Automated review]
|
/approve-spec 22fb4cd |
There was a problem hiding this comment.
Changes requested: one blocking item.
The List touch target still matches component:List FR10 and spec:AST-094 FR42: rows keep their size and paint, the band stops at half the gap to each neighbour, and a mouse sees no change. The FR11 amendment has an engineering owner's approval for this head.
Blocking
- Regression: since the rebase,
CheckboxInput(CheckboxInput.tsx:425-433) andRadioListItem(RadioListItem.tsx:315-323) host the sametouchHitAreaStyles.layerbut keep it live with their ownwatchHitAreacallback refs instead of the newuseTouchHitArea, so the hook's fixes already miss them: when a checkbox or radio row's watch stops and restarts (disabled, then enabled), its old insets stay on the row, while aListItemrow's are cleared; please move both ontouseTouchHitArea, as InfoTip now is, or say in the description why the hook cannot serve them.
Non-blocking
- FR11's new sentence says "inside a List", but the band is on every interactive
ListItem, and DEC-6 still sends decoration to::after. - The description's Current specification section still says no record changes, although this head amends FR11.
[Automated review]
| * <li ref={hitAreaRef} {...stylex.props(isInteractive && touchHitAreaStyles.layer)} /> | ||
| * ``` | ||
| */ | ||
| export function useTouchHitArea(enabled: boolean): RefCallback<HTMLElement> { |
There was a problem hiding this comment.
Regression: CheckboxInput.tsx:425-433 and RadioListItem.tsx:315-323 compose the same touchHitAreaStyles.layer but still call watchHitArea(row, insets => writeHitInsets(row, insets)) from their own callback refs, so the clear-on-stop, at-rest and ref-churn handling here never reaches them. Moving both onto useTouchHitArea(hasHitArea) / useTouchHitArea(!isDisabled), as InfoTip now is, leaves one path.
There was a problem hiding this comment.
Moved CheckboxInput and RadioListItem onto useTouchHitArea, so all four hosts of the band share one path. A checkbox row and a radio option now clear their insets when they stop taking taps, with a unit test for each.
…rs leave room Under a coarse primary pointer, an interactive List row gets an invisible hit layer on its ::after. The shared touch hit area measures each row and grows the layer toward 44 px, stopping at half the gap to the nearest neighboring row or control. Rows look and lay out as before, and a mouse is unchanged. A List link row's padding and band follow its link under a finger, through the shared clickable-container guards: a control inside the row and the click that ends a text selection keep their own behavior. A standalone Item keeps its anchor as its only link target. Under a finger a List isolates its rows, so a raised band or swipeable row stays below a page-level sticky header. A row's watch of the shared hit area owns the touches on its content, so a tap on a swipe panel's entry beside an open row stays the entry's. Other watches, such as Switch's grown input, keep the nearer-neighbour rule. A stopped watch clears the insets it wrote, and a watch that wrote nothing writes nothing. The shared press listener leaves the document it went on, so a watch whose window closed first stops cleanly. A re-render that hands a row a new merged ref, such as a caller's inline ref, keeps its watch and its insets. A divided row declares its divider's width on its own layer, so content inside the row never inherits it. The layer is the shared touch band that checkboxes and radio options use; it adds the border width a host declares on each side, 0 by default. InfoTip's button moves onto the same hook and layer, so one path serves both, and its own copy is deleted. CheckboxInput and RadioListItem keep their band live through the same hook, so their insets clear when the control stops taking taps. A delegate row renders its pressable-container marker itself, so a row that stops delegating drops it. The List record's FR11 tells callers to decorate an interactive List row with a child, since under a finger its `::after` is the touch hit area. The record marks the touch target (FR10) implemented and lists the files that verify it. Its DEC-6 sends decoration that must stay through a press to a child, or to a free `::after`.
The With end controls story's drill-in chevrons take rtlStyles.mirror, so they point to the row's end under right-to-left. Story only.
|
/approve-spec a60c052 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet, because the pull request is a draft from a fork. Once it is marked ready, a maintainer adds the ci:run label so CI runs.
Under a finger, interactive List rows now grow their tap target toward 44 px without reaching a neighbour's half of the gap, and rows keep their size and paint; with a mouse nothing changes. This matches component:List FR10, and the reworded FR11 and DEC-6 carry an engineering owner's approval for this head.
CheckboxInputandRadioListItemnow use the shareduseTouchHitAreahook, so all four hosts of the touch band share one path, and the new tests that clear their insets fail on the base and pass here.- The changed unit suites, the core typecheck and the build-css test pass; base and head frames show identical mouse rendering and unchanged checkbox and radio bands.
[Automated review]
Stack: #8085 → #9894
Batch: one intent at 2,603 authored lines (2,487 added, 116 removed), over the 2,000 cap: one touch target for List rows, InfoTip, checkboxes and radio options, on the same touch hook and layer, folded into one pull request at the design owner's ruling, since diverging copies of the touch target are one problem. Tests are 1,913 of those lines (unit 1,171, browser 742), product code 549, stories 106, and docs and records 35. Both stories are fixtures for this pull request's browser proofs: List › With end controls for the two neighbour-control cases in
ListTouchHitArea.interaction.browser.spec.ts, and InfoTip › In A Divided List for the divided-row case inInfoTipTouch.a11y.chromium.spec.ts. The story that draws each row's touch target is #9894, stacked on this one.size exemption: Specs & Decisions (fold required by review)
Rebuilt on main with only this change; it no longer depends on another pull request.
User need
Under a finger, a list row is 40 px tall, short of the 44 px target a finger needs (
spec:AST-094FR42,component:ListFR10). A tap that lands just above or below a row misses it.Problem and solution fit
What led to it: in an iOS table the whole cell takes the tap, so a finger rarely misses. In Astryx the hit area stopped at the row's 40 px box, and taps between rows did nothing.
Under a coarse primary pointer, each interactive row's invisible hit area now grows toward 44 px. It stops halfway to the next row or control, so no two targets overlap. Nothing moves and nothing paints. A tap in that band, or in a List link row's own padding, follows the row's link through the shared clickable-container guards, so a control inside the row and the click that ends a text selection keep their own behavior. A standalone
Itemkeeps its anchor as its only link target. A mouse sees no change (spec:AST-017FR59). Under a finger a List isolates its rows, so a raised band stays below a page-level sticky header (spec:AST-027DEC-1, DEC-2). With a mouse the list keeps its stacking. A row's watch of the shared hit area owns the touches on its content, so a tap on a swipe panel's entry beside an open row stays the entry's: before, a nearer neighbor row swallowed that tap. Other watches, such as Switch's grown input, keep the nearer-neighbour rule, and a stopped watch clears the insets it wrote. A divided row declares its divider's width on its own layer, so content inside the row, such as an InfoTip, never inherits it. A re-render that hands a row a new merged ref, such as a caller's inline ref, keeps its watch and its insets, and a watch that wrote nothing writes nothing. InfoTip's button, which grew the same way through its own copy, now uses the same hook and layer, and its copy is deleted.Item renders the pressable-container marker only on delegate rows. That is how this pull request keeps a link row unmarked, so a mouse click in its padding still reaches a card around it. It also fixes the same marker on main: a row that stopped delegating kept it, and a card around that row ignored a mouse click in its padding. A finger press that starts on another control inside a delegate row still paints no press on the row, as on main.
List rows, InfoTip, checkboxes and radio options share one touch band,
utils/touchHitArea.stylex.ts, and keep its insets live throughuseTouchHitArea. So every host clears its insets when it stops taking taps, which a checkbox's or radio option's own watch did not do. This pull request adds the border width a host declares on each side, 0 by default, so a divided row's band starts past its divider; a checkbox's or radio option's band computes as before.The List record (
component:List) changes with it. FR11 says an interactive ListItem row also uses its::afteras its touch hit area (FR10), so a caller decorates such a row with a child. DEC-6 sends decoration that must stay through a press to a child, or to::afterwhere it is free. Its compatibility section states that under a finger a caller's::afteron such a row is replaced by the touch hit area. The readiness row marks the touch target (FR10) implemented, andverified_bylists the two files that verify it. The note inprivateAttributes/press.stylex.tssays the same.Current specification
spec:AST-094FR42 andcomponent:ListFR10, both current. This pull request amendscomponent:List: FR11's decoration sentence, DEC-6, its compatibility section, its readiness row andverified_by, as described below.Public delta and caller burden
No API change. Under a finger a row's
::afteris the hit layer, so a caller decorates a row with a child instead. The changeset says so.Design and alternatives
The size is a plain 44 px. No design token defines another size yet.
Evidence
Browser proofs under a finger in Chromium and WebKit, listed under Testing. Rows that neighbors keep under 44 px: one-line rows reach 42 px (43 px first or last), and compact rows with dividers reach 33 px.
Size (gzip, base → head, the repository's size record, measured before the latest rebase onto main): ListItem +3,143 B JS and +291 B CSS, and the components built on it (Table, ContextMenu, Markdown, CheckboxListItem) about the same. Item +233 B JS, and List +13 B JS and +34 B CSS. The JS is the shared touch hit area (
utils/hitArea.ts) that Switch and InfoTip already use, so a page with either pays it once. InfoTip grows 490 B JS and 165 B CSS for the shared hook and its coarse-only layer, and Switch 72 B JS for the opt-in press rule above.Scope
Testing
What was run at this head (on main d0cf29a): review:pre and its checklist, check:repo, check:knowledge, changeset coverage (v0.7.7), tsc,
CI=true pnpm lint(0 errors), the public text check, the root and Storybook builds, and the build-CSS test (16/16). ListTouchHitArea: 16/16 in Chromium (4 workers, pinned cores) and 16/16 in WebKit under the CI profile. The checkbox and radio touch targets (TouchTargets) pass 7/7 in both engines. ListRowPressPaths passes in both engines, and ListRowPress passes in Chromium. Its WebKit video case fails the same way on main d0cf29a in the same container (3 of 4 runs on main, 4 of 4 here, alternating), so this pull request does not change it. The a11y contracts of the touched components pass, ItemSwipeActions and InfoTipTouch (4/4, with the divided-row case) included. test:ui: 815 files, 20,245 tests, and test:node: 901 files, 16,501 tests, both clean with no unhandled errors. In Chromium, the browser's touch adjustment can move a small tap within a few px of the edge two close rows share onto the farther row. The shared hit area then drops it, so it fires nothing, never the wrong row.Writing rubric results
R1 narration: none; the FR11 sentence, the compatibility line and the readiness row state present behavior.
R2 research: none in the record.
R3 evidence: none changed;
verified_byadds the two files that verify FR10.R4 rejected alternatives: DEC-6 keeps its one Rejected line, unchanged.
R5 change references: none.
R6 open questions: none.