Repository navigation
fix(AlertDialog): a press that slides between the actions runs the one it lifts on - #8083
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 of this change waits on an engineering owner's spec decision, and the diff needs to shrink to the AlertDialog fix. The touch behavior for AlertDialog (Cancel and the action tracked as one group under a finger, with AlertDialog painting its buttons' touch press) and the draft spec:AST-094 it relies on have no approved spec yet, so an engineering owner needs to approve or amend them by commenting /approve-spec <full head SHA> on this pull request; the author may record that decision if they are an engineering owner. As written, AST-094's FR11 (two press profiles) and FR13 (no component-owned media query) contradict the AlertDialog implementation, so that decision has to settle which side changes. Independently of the spec decision:
- Rebase onto
mainonce the stacked pull requests land, so this one carries only its AlertDialog commit (today it is 23,663 authored lines). - Run the interaction proofs inside the existing core test lane, or land an AST-030 amendment that admits
pr-interactionfirst (AST-030 FR10/FR11). - Add
## Writing rubric resultsfor spec:AST-094 to the description, and drop or fix its citation of spec:AST-090, which is not in the repository. - Add a pen case for the action-group tracker, or limit it to touch.
[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.
pr-interaction routes itself (its own scope step below) to run Playwright proofs for core components. spec:AST-030 FR11 says new coverage inside an existing surface "MUST join those owners and MUST NOT create a separately routed lane or required context", and FR10 says a new test type "is not a new surface by itself". Run these proofs inside the existing core test owner, or land an owner-approved AST-030 amendment that admits the lane first.
| name: 'ActionGroupTracking', | ||
| minimumPressDuration: options.highlightOnsetMs, | ||
| allowableMovement: Number.POSITIVE_INFINITY, | ||
| allowedPointerTypes: options.pointerTypes ?? ['touch', 'pen'], |
There was a problem hiding this comment.
Pens are tracked by default here, so AlertDialog changes pen behavior too, while its docs and changeset only describe touch screens and no test drives a pen. architecture:interaction-modality asks pointer-dependent changes to verify "mouse, touch, and pen as applicable". Add a pen slide case to useActionGroupTracking.test.tsx, or pass pointerTypes: ['touch'] from AlertDialog.
There was a problem hiding this comment.
Fixed in 4a23248: pens stay tracked, as UIKit tracks Apple Pencil on an alert's actions like a finger, and two pen cases now drive the group: a slide from Delete to Cancel runs Cancel, and a lift off both actions runs nothing. The docs and changeset say a finger or a pen.
| element.setAttribute(PRESSED_ATTRIBUTE, 'fading'); | ||
| fadesRef.current.set( | ||
| element, | ||
| setTimeout(() => { |
There was a problem hiding this comment.
This fade timer is never cleared when tracking is disabled or the component unmounts, so it can write to a closed or detached button. architecture:react-component-runtime INV5 wants component-owned timers released on disable and unmount; a cleanup that clears fadesRef and removes the attribute the timer would have removed covers it.
There was a problem hiding this comment.
Fixed in 4a23248: turning tracking off and unmounting now end every pending release fade (its timer, and the 'fading' paint it would remove). The new case 'leaves no fade behind when tracking turns off or the group unmounts' failed before the fix.
| options: ActionGroupTrackingOptions, | ||
| ): LongPressGestureRecognizer { | ||
| const optionsRef = useRef(options); | ||
| useIsomorphicLayoutEffect(() => { |
There was a problem hiding this comment.
architecture:react-component-runtime INV3: "A new or changed Effect MUST name the external system it synchronizes." A one-line comment naming the recognizer delegate that reads optionsRef outside render would satisfy it.
There was a problem hiding this comment.
Fixed in 4a23248: the effect now says it keeps the recognizer's delegate and listener, which run outside render, reading the latest options; the new fade effect says what it ends and when.
| follows. | ||
| - **FR7 — Exits animate as UIKit's do.** A touch presentation MUST play the | ||
| exit its UIKit counterpart plays. **Proposed; pending owner review:** this | ||
| departs from the draft `spec:AST-090` FR1 (exits off by default) and FR10 |
There was a problem hiding this comment.
spec:AST-090 is not in the repository at this head or on main (it is still an open proposal), so a reader following this amendment finds nothing. Drop the AST-090 amendment here (and in Current-state impact) until that record lands, or describe it in the pull request instead.
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| lab/CommentThread | 🔴 250.0 kB (+22.9 kB) · 2.4 kB CSS (0) | — | — · — · — |
| lab/Comments | 🔴 255.3 kB (+22.8 kB) · 2.5 kB CSS (0) | — | — · — · — |
| AlertDialog · Storybook | 🔴 79.0 kB (+22.6 kB) · 12.8 kB CSS (+42 B) | 210 (+22) | — · — · — |
182 more components
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| lab/MobilePowerSearch | 214.5 kB (+859 B) · 1.2 kB CSS (0) | — | — · — · — |
| PowerSearch | 184.6 kB (+504 B) · 20.7 kB CSS (+15 B) | — | — · — · — |
| Tokenizer | 70.4 kB (+208 B) · 17.1 kB CSS (+28 B) | — | — · — · — |
| ContextMenuItem | 52.8 kB (+188 B) · 9.9 kB CSS (+19 B) | — | — · — · — |
| Markdown | 145.3 kB (+179 B) · 18.3 kB CSS (+20 B) | — | — · — · — |
| DateTimeInput | 158.7 kB (+182 B) · 17.0 kB CSS (+15 B) | — | — · — · — |
| charts/Chart | 63.1 kB (+185 B) · 20 B CSS (0) | — | — · — · — |
| CodeBlock | 59.2 kB (+161 B) · 11.1 kB CSS (+17 B) | — | — · — · — |
| richtext/RichTextEditor | 277.8 kB (+171 B) · 2.6 kB CSS (0) | — | — · — · — |
| DialogHeader | 48.5 kB (+145 B) · 10.2 kB CSS (+25 B) | — | — · — · — |
| TableHeaderCell | 76.0 kB (+145 B) · 15.6 kB CSS (+25 B) | — | — · — · — |
| Banner | 47.4 kB (+148 B) · 10.3 kB CSS (+20 B) | — | — · — · — |
| ComplexSelector | 55.7 kB (+150 B) · 11.1 kB CSS (+18 B) | — | — · — · — |
| Thumbnail | 49.7 kB (+144 B) · 11.3 kB CSS (+21 B) | — | — · — · — |
| TableSelectionToolbar | 52.4 kB (+142 B) · 11.5 kB CSS (+20 B) | — | — · — · — |
| AppShell | 68.7 kB (+143 B) · 12.2 kB CSS (+18 B) | — | — · — · — |
| DateRangeInput | 70.0 kB (+141 B) · 11.9 kB CSS (+20 B) | — | — · — · — |
| SideNavHeading | 59.0 kB (+139 B) · 10.6 kB CSS (+21 B) | — | — · — · — |
| BaseTypeahead | 55.3 kB (+140 B) · 10.3 kB CSS (+19 B) | — | — · — · — |
| Lightbox | 50.6 kB (+139 B) · 10.2 kB CSS (+19 B) | — | — · — · — |
| ChatDictationButton | 46.1 kB (+139 B) · 9.8 kB CSS (+18 B) | — | — · — · — |
| LayerProvider | 62.9 kB (+139 B) · 10.5 kB CSS (+18 B) | — | — · — · — |
| BreadcrumbItem | 67.3 kB (+135 B) · 11.5 kB CSS (+21 B) | — | — · — · — |
| DropdownMenu | 73.2 kB (+139 B) · 15.5 kB CSS (+16 B) | — | — · — · — |
| NumberInput | 55.1 kB (+135 B) · 11.2 kB CSS (+20 B) | — | — · — · — |
| Stepper | 47.1 kB (+136 B) · 9.9 kB CSS (+19 B) | — | — · — · — |
| TopNavHeading | 58.4 kB (+132 B) · 10.4 kB CSS (+21 B) | — | — · — · — |
| TopNavMegaMenu | 54.5 kB (+134 B) · 11.2 kB CSS (+19 B) | — | — · — · — |
| Typeahead | 63.0 kB (+130 B) · 11.8 kB CSS (+23 B) | — | — · — · — |
| ChatSendButton | 45.3 kB (+134 B) · 9.5 kB CSS (+18 B) | — | — · — · — |
| TableCell | 76.4 kB (+135 B) · 15.8 kB CSS (+16 B) | — | — · — · — |
| SideNavCollapseButton | 46.2 kB (+132 B) · 9.8 kB CSS (+18 B) | — | — · — · — |
| ContextMenu | 75.2 kB (+126 B) · 15.4 kB CSS (+23 B) | — | — · — · — |
| Pagination | 86.1 kB (+127 B) · 16.0 kB CSS (+22 B) | — | — · — · — |
| Selector | 77.1 kB (+127 B) · 15.7 kB CSS (+21 B) | — | — · — · — |
| MoreMenu | 73.5 kB (+131 B) · 15.5 kB CSS (+16 B) | — | — · — · — |
| Popover | 50.2 kB (+130 B) · 12.4 kB CSS (+17 B) | — | — · — · — |
| TextInput | 53.0 kB (+126 B) · 11.0 kB CSS (+21 B) | — | — · — · — |
| TopNav | 51.9 kB (+125 B) · 10.7 kB CSS (+21 B) | — | — · — · — |
| Toast | 59.6 kB (+126 B) · 10.1 kB CSS (+19 B) | — | — · — · — |
| DateInput | 103.4 kB (+126 B) · 15.3 kB CSS (+17 B) | — | — · — · — |
| MultiSelector | 72.8 kB (+120 B) · 15.2 kB CSS (+23 B) | — | — · — · — |
| SideNav | 65.8 kB (+123 B) · 10.9 kB CSS (+20 B) | — | — · — · — |
| TimeInput | 90.2 kB (+122 B) · 14.3 kB CSS (+21 B) | — | — · — · — |
| ChatComposer | 61.1 kB (+124 B) · 11.2 kB CSS (+18 B) | — | — · — · — |
| ChatLayoutScrollButton | 45.9 kB (+123 B) · 9.9 kB CSS (+19 B) | — | — · — · — |
| MobileNavToggle | 45.9 kB (+122 B) · 9.7 kB CSS (+19 B) | — | — · — · — |
| Button · Storybook | 42.5 kB (+122 B) · 9.5 kB CSS (+18 B) | 605 (+14) | — · — · — |
| lab/CommentComposer | 102.5 kB (+139 B) · 1.9 kB CSS (0) | — | — · — · — |
| ChatLayout | 48.6 kB (+120 B) · 10.3 kB CSS (+18 B) | — | — · — · — |
| Table | 84.5 kB (+114 B) · 16.0 kB CSS (+23 B) | — | — · — · — |
| lab/ListInput | 81.6 kB (+137 B) · 1.6 kB CSS (0) | — | — · — · — |
| lab/Schedule | 61.5 kB (+137 B) · 3.9 kB CSS (0) | — | — · — · — |
| Carousel | 50.8 kB (+119 B) · 10.5 kB CSS (+17 B) | — | — · — · — |
| DrawerHeader | 48.6 kB (+116 B) · 11.6 kB CSS (+19 B) | — | — · — · — |
| TopNavMenu | 58.5 kB (+117 B) · 10.5 kB CSS (+18 B) | — | — · — · — |
| ChatComposerTokenElement | 56.3 kB (+117 B) · 10.4 kB CSS (+17 B) | — | — · — · — |
| ToggleButton | 43.4 kB (+115 B) · 9.6 kB CSS (+19 B) | — | — · — · — |
| ChatComposerInput | 56.3 kB (+116 B) · 10.4 kB CSS (+17 B) | — | — · — · — |
| MobileNav | 50.3 kB (+112 B) · 10.4 kB CSS (+21 B) | — | — · — · — |
| lab/EmojiPicker | 70.3 kB (+133 B) · 722 B CSS (0) | — | — · — · — |
| lab/TourStep | 55.6 kB (+133 B) · 590 B CSS (0) | — | — · — · — |
| TabMenu | 55.5 kB (+118 B) · 10.4 kB CSS (+14 B) | — | — · — · — |
| lab/DiffViewer | 70.9 kB (+132 B) · 2.1 kB CSS (0) | — | — · — · — |
| Calendar | 54.4 kB (+114 B) · 10.3 kB CSS (+17 B) | — | — · — · — |
| lab/TransferList | 74.5 kB (+131 B) · 1.7 kB CSS (0) | — | — · — · — |
| richtext/RichTextEditorToolbar | 222.1 kB (+130 B) · 454 B CSS (0) | — | — · — · — |
| IconButton | 42.5 kB (+109 B) · 9.5 kB CSS (+18 B) | — | — · — · — |
| FileInput | 53.8 kB (+104 B) · 10.7 kB CSS (+21 B) | — | — · — · — |
| lab/TransferListSelector | 80.9 kB (+124 B) · 1.9 kB CSS (0) | — | — · — · — |
| lab/MobileTokenizer | 101.2 kB (+123 B) · 880 B CSS (0) | — | — · — · — |
| SideNavItem | 54.8 kB (+102 B) · 10.5 kB CSS (+20 B) | — | — · — · — |
| lab/ChatEmojiPicker | 65.2 kB (+118 B) · 773 B CSS (0) | — | — · — · — |
| lab/ChatReactionBar | 65.7 kB (+103 B) · 1.1 kB CSS (0) | — | — · — · — |
| richtext/RichTextView | 258.3 kB (+93 B) · 2.3 kB CSS (0) | — | — · — · — |
| TreeList | 38.0 kB (+83 B) · 5.0 kB CSS (0) | — | — · — · — |
| Switch | 86.9 kB (+76 B) · 10.5 kB CSS (0) | — | — · — · — |
| Item | 13.3 kB (+71 B) · 4.9 kB CSS (0) | — | — · — · — |
| lab/CommentPin | 47.2 kB (+69 B) · 1.2 kB CSS (0) | — | — · — · — |
| CommandPalette | 48.9 kB (+68 B) · 12.2 kB CSS (0) | — | — · — · — |
| CheckboxListItem | 53.4 kB (+67 B) · 11.0 kB CSS (0) | — | — · — · — |
| SegmentedControlItem | 10.4 kB (+67 B) · 4.0 kB CSS (0) | — | — · — · — |
| DropdownMenuItem | 17.5 kB (+66 B) · 5.5 kB CSS (0) | — | — · — · — |
| DropdownMenuRadioItem | 18.3 kB (+66 B) · 5.9 kB CSS (0) | — | — · — · — |
| DropdownMenuCheckboxItem | 18.2 kB (+65 B) · 5.9 kB CSS (0) | — | — · — · — |
| Collapsible | 22.5 kB (+63 B) · 4.5 kB CSS (0) | — | — · — · — |
| Outline | 24.3 kB (+62 B) · 4.2 kB CSS (0) | — | — · — · — |
| CommandPaletteItem | 10.0 kB (+59 B) · 3.6 kB CSS (0) | — | — · — · — |
| ClickableCard | 14.6 kB (+58 B) · 7.1 kB CSS (0) | — | — · — · — |
| SelectorOption | 16.6 kB (+57 B) · 5.4 kB CSS (0) | — | — · — · — |
| SelectableCard | 14.6 kB (+56 B) · 7.3 kB CSS (0) | — | — · — · — |
| Step | 27.9 kB (+56 B) · 5.6 kB CSS (0) | — | — · — · — |
| Tab | 11.3 kB (+56 B) · 4.2 kB CSS (0) | — | — · — · — |
| Token | 25.1 kB (+56 B) · 4.8 kB CSS (0) | — | — · — · — |
| CheckboxInput | 48.8 kB (+55 B) · 9.7 kB CSS (0) | — | — · — · — |
| Link | 42.9 kB (+54 B) · 7.9 kB CSS (0) | — | — · — · — |
| TopNavItem | 10.6 kB (+54 B) · 4.0 kB CSS (0) | — | — · — · — |
| TopNavMegaMenuItem | 10.7 kB (+54 B) · 4.2 kB CSS (0) | — | — · — · — |
| DropdownMenuSubMenu | 56.1 kB (+53 B) · 9.9 kB CSS (0) | — | — · — · — |
| ListItem | 52.8 kB (+50 B) · 11.0 kB CSS (0) | — | — · — · — |
| SegmentedControl | 81.7 kB (+45 B) · 7.8 kB CSS (0) | — | — · — · — |
| AvatarGroupOverflow | 39.0 kB (+39 B) · 6.5 kB CSS (0) | — | — · — · — |
| RadioListItem | 51.3 kB (+38 B) · 10.1 kB CSS (0) | — | — · — · — |
| Timestamp | 21.3 kB (−1 B) · 9.9 kB CSS (+19 B) | — | — · — · — |
| charts/ChartAxis | 6.7 kB (0) · 112 B CSS (0) | — | — · — · — |
| AspectRatio | 3.8 kB (0) · 3.6 kB CSS (0) | — | — · — · — |
| Avatar | 35.0 kB (0) · 6.5 kB CSS (0) | — | — · — · — |
| AvatarGroup | 37.7 kB (0) · 6.6 kB CSS (0) | — | — · — · — |
| AvatarStatusDot | 7.2 kB (0) · 3.6 kB CSS (0) | — | — · — · — |
| Badge | 4.0 kB (0) · 4.0 kB CSS (0) | — | — · — · — |
| Blockquote | 3.6 kB (0) · 3.5 kB CSS (0) | — | — · — · — |
| BottomSheetSwitcher | 15.0 kB (0) · 3.4 kB CSS (0) | — | — · — · — |
| Breadcrumbs | 16.0 kB (0) · 3.4 kB CSS (0) | — | — · — · — |
| ButtonGroup | 9.0 kB (0) · 3.4 kB CSS (0) | — | — · — · — |
| Card | 6.5 kB (0) · 6.5 kB CSS (0) | — | — · — · — |
| Center | 4.2 kB (0) · 4.7 kB CSS (0) | — | — · — · — |
| ChatComposerDrawer | 17.5 kB (0) · 4.7 kB CSS (0) | — | — · — · — |
| ChatMessage | 16.7 kB (0) · 3.7 kB CSS (0) | — | — · — · — |
| ChatMessageBubble | 7.1 kB (0) · 3.7 kB CSS (0) | — | — · — · — |
| ChatMessageList | 22.8 kB (0) · 7.9 kB CSS (0) | — | — · — · — |
| ChatMessageMetadata | 19.6 kB (0) · 4.1 kB CSS (0) | — | — · — · — |
| ChatSystemMessage | 6.8 kB (0) · 3.8 kB CSS (0) | — | — · — · — |
| ChatTokenizedText | 7.1 kB (0) · 4.0 kB CSS (0) | — | — · — · — |
| ChatToolCalls | 25.8 kB (0) · 9.3 kB CSS (0) | — | — · — · — |
| CheckboxList | 43.3 kB (0) · 8.6 kB CSS (0) | — | — · — · — |
| Citation | 19.9 kB (0) · 4.4 kB CSS (0) | — | — · — · — |
| Code | 3.6 kB (0) · 3.5 kB CSS (0) | — | — · — · — |
| CollapsibleGroup | 6.5 kB (0) · 3.3 kB CSS (0) | — | — · — · — |
| CommandPaletteEmpty | 6.2 kB (0) · 3.5 kB CSS (0) | — | — · — · — |
| CommandPaletteFooter | 17.0 kB (0) · 3.7 kB CSS (0) | — | — · — · — |
| CommandPaletteGroup | 3.5 kB (0) · 3.5 kB CSS (0) | — | — · — · — |
| CommandPaletteInput | 26.8 kB (0) · 8.5 kB CSS (0) | — | — · — · — |
| CommandPaletteList | 16.0 kB (0) · 3.4 kB CSS (0) | — | — · — · — |
| Drawer | 36.9 kB (0) · 8.2 kB CSS (0) | — | — · — · — |
| DropdownMenuGroup | 6.2 kB (0) · 3.4 kB CSS (0) | — | — · — · — |
| DropdownMenuRadioGroup | 6.1 kB (0) · 3.3 kB CSS (0) | — | — · — · — |
| Field | 41.4 kB (0) · 8.3 kB CSS (0) | — | — · — · — |
| FormLayout | 6.4 kB (0) · 3.5 kB CSS (0) | — | — · — · — |
| Grid | 4.4 kB (0) · 3.8 kB CSS (0) | — | — · — · — |
| GridSpan | 3.5 kB (0) · 3.4 kB CSS (0) | — | — · — · — |
| InputGroup | 42.0 kB (0) · 8.4 kB CSS (0) | — | — · — · — |
| InputGroupText | 6.4 kB (0) · 3.9 kB CSS (0) | — | — · — · — |
| Kbd | 16.7 kB (0) · 3.6 kB CSS (0) | — | — · — · — |
| LayoutPanel | 7.6 kB (0) · 5.0 kB CSS (0) | — | — · — · — |
| MetadataList | 16.7 kB (0) · 3.8 kB CSS (0) | — | — · — · — |
| MetadataListItem | 6.6 kB (0) · 3.5 kB CSS (0) | — | — · — · — |
| NavHeadingMenu | 14.5 kB (0) · 3.4 kB CSS (0) | — | — · — · — |
| NavIcon | 3.5 kB (0) · 3.4 kB CSS (0) | — | — · — · — |
| OverflowList | 8.6 kB (0) · 3.6 kB CSS (0) | — | — · — · — |
| Overlay | 19.8 kB (0) · 4.0 kB CSS (0) | — | — · — · — |
| ProgressBar | 10.8 kB (0) · 7.3 kB CSS (0) | — | — · — · — |
| RadioList | 42.9 kB (0) · 8.3 kB CSS (0) | — | — · — · — |
| ResizeHandle | 18.7 kB (0) · 4.0 kB CSS (0) | — | — · — · — |
| Section | 7.6 kB (0) · 6.3 kB CSS (0) | — | — · — · — |
| SideNavSection | 6.6 kB (0) · 3.7 kB CSS (0) | — | — · — · — |
| Skeleton | 3.8 kB (0) · 3.8 kB CSS (0) | — | — · — · — |
| Slider | 88.6 kB (0) · 9.4 kB CSS (0) | — | — · — · — |
| StatusDot | 23.2 kB (0) · 6.3 kB CSS (0) | — | — · — · — |
| TableBody | 3.4 kB (0) · 3.3 kB CSS (0) | — | — · — · — |
| TableFooter | 3.4 kB (0) · 3.3 kB CSS (0) | — | — · — · — |
| TableHeader | 3.4 kB (0) · 3.3 kB CSS (0) | — | — · — · — |
| TableRow | 6.5 kB (0) · 4.0 kB CSS (0) | — | — · — · — |
| TabList | 35.4 kB (0) · 7.5 kB CSS (0) | — | — · — · — |
| TextArea | 47.2 kB (0) · 9.8 kB CSS (0) | — | — · — · — |
| Timer | 9.6 kB (0) · 7.0 kB CSS (0) | — | — · — · — |
| ToggleButtonGroup | 6.4 kB (0) · 3.4 kB CSS (0) | — | — · — · — |
| Toolbar | 34.7 kB (0) · 7.9 kB CSS (0) | — | — · — · — |
| TopNavMegaMenuFeaturedCard | 6.9 kB (0) · 3.6 kB CSS (0) | — | — · — · — |
| TypeaheadItem | 6.3 kB (0) · 3.6 kB CSS (0) | — | — · — · — |
| lab/ChartDotGL | 7.4 kB (0) · 20 B CSS (0) | — | — · — · — |
| lab/ChartDotGLInteractive | 8.3 kB (0) · 20 B CSS (0) | — | — · — · — |
| lab/ChartHeatmapGL | 8.5 kB (0) · 20 B CSS (0) | — | — · — · — |
| lab/ChartStreamGL | 7.5 kB (0) · 20 B CSS (0) | — | — · — · — |
| lab/ChatReasoning | 7.0 kB (0) · 967 B CSS (0) | — | — · — · — |
| lab/ChatTypingIndicator | 24.4 kB (0) · 612 B CSS (0) | — | — · — · — |
| lab/ChatUnreadDivider | 3.6 kB (0) · 348 B CSS (0) | — | — · — · — |
| lab/CircularProgress | 8.5 kB (0) · 1.1 kB CSS (0) | — | — · — · — |
| lab/CodeEditor | 14.1 kB (0) · 743 B CSS (0) | — | — · — · — |
| lab/LogStream | 8.0 kB (0) · 1.6 kB CSS (0) | — | — · — · — |
| lab/Stat | 4.2 kB (0) · 646 B CSS (0) | — | — · — · — |
| lab/SVGIcon | 7.6 kB (0) · 764 B CSS (0) | — | — · — · — |
| lab/ThreeDScatterGL | 7.6 kB (0) · 20 B CSS (0) | — | — · — · — |
Package size: @astryxdesign/core 6.10 MB packed (+10.2 kB); no new dependencies.
Wide change (shared styles: packages/core/src/utils/interactionOverlay.stylex.ts): add ci:run-visual, ci:run-a11y or ci:run-rtl to run them.
Generated by PR Enrichment workflow | View full report
24a7782 to
37860b7
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about the new direction here: AlertDialog tracking Cancel and its action as one group under touch and pen, taking its actions' touch press over from Button, and the draft spec:AST-094 requirements this branch adds, including where FR11 and FR13 meet the alert's behavior. 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. Independently of that decision, these need changes:
- Land the dependency stack separately and rebase so this pull request carries only the AlertDialog commit (24,788 authored lines today).
- Run the interaction proofs in the existing Core test lane, or admit the new
pr-interactionlane through an AST-030 amendment first (FR10, FR11). - Add a pen test or drop pen from the hook's default, clear pending fade timers when tracking stops, and comment what each new effect synchronizes.
- Report the Writing rubric results for the new record and fix its references to AST-090,
utils/touchPresentation.tsand the missing motion files.
[Automated review]
| name: 'ActionGroupTracking', | ||
| minimumPressDuration: options.highlightOnsetMs, | ||
| allowableMovement: Number.POSITIVE_INFINITY, | ||
| allowedPointerTypes: options.pointerTypes ?? ['touch', 'pen'], |
There was a problem hiding this comment.
Pen is tracked by default here, but no test drives a pen through the group tracking. architecture:interaction-modality asks pointer-dependent behavior to verify mouse, touch and pen as applicable: add a pen case (a slide and a lift off both), or default to ['touch'].
There was a problem hiding this comment.
Fixed in 4a23248: pens stay tracked, as UIKit tracks Apple Pencil on an alert's actions like a finger, and two pen cases now drive the group: a slide from Delete to Cancel runs Cancel, and a lift off both actions runs nothing. The docs and changeset say a finger or a pen.
| const release = (element: HTMLElement) => { | ||
| clearFade(element); | ||
| element.setAttribute(PRESSED_ATTRIBUTE, 'fading'); | ||
| fadesRef.current.set( | ||
| element, | ||
| setTimeout(() => { | ||
| fadesRef.current.delete(element); | ||
| if (element.getAttribute(PRESSED_ATTRIBUTE) === 'fading') { | ||
| element.removeAttribute(PRESSED_ATTRIBUTE); | ||
| } | ||
| }, PRESS_FADE_MS), | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Each release starts a fade timer that is cleared only when the same action is painted, cut or released again. The lift that runs an action usually closes the alert, so the timer outlives tracking; architecture:react-component-runtime INV5 wants node-owned timers released when disabled or unmounted. Clear every pending fade on disable and unmount.
There was a problem hiding this comment.
Fixed in 4a23248: turning tracking off and unmounting now end every pending release fade (its timer, and the 'fading' paint it would remove). The new case 'leaves no fade behind when tracking turns off or the group unmounts' failed before the fix.
| useIsomorphicLayoutEffect(() => { | ||
| optionsRef.current = options; | ||
| }); |
There was a problem hiding this comment.
architecture:react-component-runtime INV3: a new Effect names the external system it synchronizes. One comment saying this keeps the recognizer's delegate reading the latest options would satisfy it.
There was a problem hiding this comment.
Fixed in 4a23248: the effect now says it keeps the recognizer's delegate and listener, which run outside render, reading the latest options; the new fade effect says what it ends and when.
| # 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.
spec:AST-030 FR11: new coverage inside an existing surface joins that surface's lane and does not create a separately routed lane; FR10 admits a new surface only through an owner-approved amendment first. This job routes itself with its own scope check.
| 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.
No spec:AST-090 exists in the repository, here or on main, so FR7's proposed departure points at nothing an owner can read. Cite the record that exists, or drop the reference.
37860b7 to
0d3aae6
Compare
There was a problem hiding this comment.
This needs an owner decision before it can be approved: the AlertDialog touch group tracking has no current spec behind it, so approval needs an engineering owner to approve the governing spec, the proposed component:AlertDialog requirement and the draft spec:AST-094 (FR1–FR23, DEC-1/DEC-2), and to settle whether AST-094's FR11 and FR13 or the alert's own press profile and coarse-pointer media query should change. An engineering owner records that decision by commenting /approve-spec <full head SHA>, and the author may do so if they are an engineering owner. Independently of that decision, please:
- land the gesture, motion and harness dependencies through their own pull requests and rebase this one onto them;
- run the interaction proofs in the existing Core test lane instead of a separately routed
pr-interactionjob, or amend AST-030 first; - clear pending press-fade timers when tracking turns off or the dialog unmounts, and add a pen test or limit tracking to touch;
- add AST-094's Writing rubric results to the description and fix the references to AST-090,
utils/touchPresentation.ts,uikit/switch.ts,uikit/contextMenu.tsandsources.test.ts, none of which exist.
[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.
pr-interaction routes itself with its own scope check, which spec:AST-030 FR11 rules out: new coverage inside an existing surface joins that surface's test lane and must not create a separately routed lane or required context. Run these proofs inside the Core test lane, or land an AST-030 amendment admitting a new surface first (FR10).
| element.setAttribute(PRESSED_ATTRIBUTE, 'fading'); | ||
| fadesRef.current.set( | ||
| element, | ||
| setTimeout(() => { |
There was a problem hiding this comment.
This fade timer is cleared only when the same action is painted, cut or released again. The lift that runs an action usually closes the alert, so the timer outlives tracking, while architecture:react-component-runtime INV5 asks for component-owned timers to be released when disabled or unmounted. Clearing every pending fade, and removing the attribute it would have removed, when tracking turns off and on unmount would fix it.
There was a problem hiding this comment.
Fixed in 4a23248: turning tracking off and unmounting now end every pending release fade (its timer, and the 'fading' paint it would remove). The new case 'leaves no fade behind when tracking turns off or the group unmounts' failed before the fix.
| name: 'ActionGroupTracking', | ||
| minimumPressDuration: options.highlightOnsetMs, | ||
| allowableMovement: Number.POSITIVE_INFINITY, | ||
| allowedPointerTypes: options.pointerTypes ?? ['touch', 'pen'], |
There was a problem hiding this comment.
Pens are tracked by default here, but no test drives a pen and the docs and changeset mention only touch screens. architecture:interaction-modality asks pointer-dependent changes to verify mouse, touch and pen as applicable: a pen slide and a pen lift-off-both case would cover it, or AlertDialog could pass pointerTypes: ['touch'].
There was a problem hiding this comment.
Fixed in 4a23248: pens stay tracked, as UIKit tracks Apple Pencil on an alert's actions like a finger, and two pen cases now drive the group: a slide from Delete to Cancel runs Cancel, and a lift off both actions runs nothing. The docs and changeset say a finger or a pen.
| options: ActionGroupTrackingOptions, | ||
| ): LongPressGestureRecognizer { | ||
| const optionsRef = useRef(options); | ||
| useIsomorphicLayoutEffect(() => { |
There was a problem hiding this comment.
architecture:react-component-runtime INV3 asks every new Effect to name the external system it synchronizes; a one-line comment saying what this keeps current would satisfy it.
There was a problem hiding this comment.
Fixed in 4a23248: the effect now says it keeps the recognizer's delegate and listener, which run outside render, reading the latest options; the new fade effect says what it ends and when.
| 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 spec:AST-090 at this revision or on main, so this departure (and the amendment at line 235) points at nothing. Cite the record that holds these rules, or drop the reference.
| ## Touch presentation | ||
|
|
||
| Whether a component presents itself the touch (UIKit) way is answered once, | ||
| in `../utils/touchPresentation.ts`: |
There was a problem hiding this comment.
../utils/touchPresentation.ts does not exist, and neither do TOUCH_QUERY, COMPACT_TOUCH_QUERY or useTouchPresentation; point component authors at what exists, or remove this section until it lands.
There was a problem hiding this comment.
0d3aae6 to
a79b8e8
Compare
There was a problem hiding this comment.
This needs an engineering owner's spec decision before it can be approved: the AlertDialog touch group tracking has no current spec behind it, so an owner has to approve or reject the proposed component:AlertDialog requirement, including whether callers can turn it off under the current touch-default rule, and the draft spec:AST-094 (FR1–FR23, DEC-1/DEC-2), and say which side changes where AST-094's FR4, FR11 and FR13 disagree with this branch. An engineering owner records that decision by commenting /approve-spec <full head SHA>, and the author may do so if they are an engineering owner. Independently of that decision, please:
- land the gesture, motion, theme-role, resolver and harness dependencies through their own pull requests and rebase this one onto them, settling the theme roles and the
pr-interactionjob there againstdesign:reduced-motion,spec:AST-012FR2 andspec:AST-030FR11; - keep the compact-touch line at width below
md(768 px) or height below 500, asspec:AST-012FR9 now requires, instead of 744 px; - load AlertDialog's touch tracking lazily or justify its 17 KB gzip cost (
architecture:component-style-authoringINV6); - clear the press-fade timers on disable and unmount, add a pen test or limit tracking to touch, add AST-094's Writing rubric results, and fix the references to AST-090,
uikit/switch.ts,uikit/contextMenu.tsandsources.test.ts.
[Automated review]
| export const TOUCH_QUERY = '(pointer: coarse)'; | ||
|
|
||
| /** The viewport is compact: layout behaviours. */ | ||
| export const COMPACT_QUERY = '(width < 744px), (height < 500px)'; |
There was a problem hiding this comment.
spec:AST-012 FR9 (current on main) defines compact as width below the effective md point (768 by default) or height below 500, and says no resolver keeps size numbers of its own; DEC-6 rejects matching platform size classes. At 744 px a coarse-pointer screen 744–767 px wide gets the popover where the definition gives the sheet. Use (width < 768px), (height < 500px) until the theme exports the definition.
| fadesRef.current.set( | ||
| element, | ||
| setTimeout(() => { | ||
| fadesRef.current.delete(element); | ||
| if (element.getAttribute(PRESSED_ATTRIBUTE) === 'fading') { | ||
| element.removeAttribute(PRESSED_ATTRIBUTE); | ||
| } | ||
| }, PRESS_FADE_MS), | ||
| ); |
There was a problem hiding this comment.
This timer is cleared only when the same action is painted, cut or released again. The lift that runs an action usually closes the alert, so the timer outlives tracking; architecture:react-component-runtime INV5 asks component-owned timers to be released when disabled or unmounted. Clear fadesRef (and the attribute it would remove) when tracking turns off and on unmount.
| name: 'ActionGroupTracking', | ||
| minimumPressDuration: options.highlightOnsetMs, | ||
| allowableMovement: Number.POSITIVE_INFINITY, | ||
| allowedPointerTypes: options.pointerTypes ?? ['touch', 'pen'], |
There was a problem hiding this comment.
Pens are tracked by default and AlertDialog passes no pointerTypes, but no test drives a pen and the docs and changeset mention only touch screens. architecture:interaction-modality asks pointer-dependent changes to verify mouse, touch and pen as applicable: add a pen slide and a pen lift-off-both case, or pass ['touch'].
| import {themeProps} from '../utils/themeProps'; | ||
| import {useTranslator} from '../i18n'; | ||
| import {useMediaQuery} from '../hooks'; | ||
| import {useActionGroupTracking} from '../hooks/useActionGroupTracking'; |
There was a problem hiding this comment.
This import brings the gesture host, the arena, the pan, tap and hover recognizers, the velocity tracker and the whole UIKIT_GESTURES table into every AlertDialog import: 30,347 to 47,413 bytes gzip (core modules, packages external). architecture:component-style-authoring INV6 asks touch-only JavaScript over 2 KB to load lazily on a coarse pointer or be justified, and shared tables to ship only the rows a component uses.
| # 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.
pr-interaction routes itself with its own scope check, which spec:AST-030 FR11 rules out: new coverage inside an existing surface joins that surface's test lane and must not create a separately routed lane or required context. Run these proofs inside the Core test lane, or land an FR10 amendment first.
| * 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 keeps when a closed vocabulary (width, height, pointer, contrast, motion) and says future environment fields require explicit additions to it; platform needs that amendment before it ships as a stable theme option.
| * Cross-Fade Transitions on (opacity CABasicAnimation 0.5 s, | ||
| * (0.42, 0, 0.58, 1)), Oct 23 2026 | ||
| */ | ||
| reducedMotionCrossFadeMs: 500, |
There was a problem hiding this comment.
design:reduced-motion DR2 (current) caps a touch cross-fade at 200 ms, and DR4 says core theme values offer no way to a longer reduced-motion animation. This 500 ms default, and the theme role that can retune it (the docs set it to 300), would give touch presentations a longer fade.
| 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 spec:AST-090 at this revision or on main, so this departure (and the amendment at line 235) points at nothing an owner can read. Cite the record that holds these rules, or drop the reference.
a79b8e8 to
4a23248
Compare
|
Addressed at 4a23248:
Waiting in the reviewers' queue: the spec decision (component:AlertDialog FR6 and the draft AST-094), the |
|
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. |
There was a problem hiding this comment.
This needs an engineering owner's spec decision before it can be approved: AlertDialog's touch and pen group tracking has no current spec behind it, so an owner has to approve or reject the proposed component:AlertDialog requirement, including whether callers can turn it off under the current touch-default rule, and the draft spec:AST-094 (FR1–FR23, DEC-1/DEC-2), and say which side changes where AST-094's FR4, FR11 and FR13 disagree with this branch. 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. Independently of that decision, please:
- land the gesture, motion, theme-role, resolver and harness dependencies through their own pull requests and rebase this one onto them, settling the theme roles and the
pr-interactionjob there againstdesign:reduced-motion,spec:AST-012FR2 andspec:AST-030FR11; - keep the compact-touch line at width below
md(768 px) or height below 500, asspec:AST-012FR9 requires, instead of 744 px; - report AlertDialog's size, load its touch tracking lazily or justify its 16 KB gzip cost (
architecture:component-style-authoringINV6), and attach it only while the alert is open; - prove pens in the AlertDialog browser cases or track touch only, add AST-094's Writing rubric results, and fix the references to AST-090,
uikit/switch.ts,uikit/contextMenu.tsandsources.test.ts.
[Automated review]
| if (element == null) { | ||
| return undefined; | ||
| } | ||
| return getGestureHost(element.ownerDocument).attach(element, recognizer, { |
There was a problem hiding this comment.
The recognizer attaches here whatever isEnabled says, and attaching installs the gesture host's twelve document and two window listeners (GestureHost.ts:463-490). Through useActionGroupTracking, every mounted AlertDialog keeps them while closed. architecture:react-component-runtime INV5 asks optional behavior to acquire local resources only while enabled; keying this attach on isEnabled (or attaching from AlertDialog only while open) would cover it.
| name: 'ActionGroupTracking', | ||
| minimumPressDuration: options.highlightOnsetMs, | ||
| allowableMovement: Number.POSITIVE_INFINITY, | ||
| allowedPointerTypes: options.pointerTypes ?? ['touch', 'pen'], |
There was a problem hiding this comment.
Pens are tracked by default, and a pen lift relies on the host swallowing the browser's own trusted click after lifted.click() (GestureHost.ts:1018-1036, :1147-1163). The two pen cases run in jsdom, where no browser click follows, so they cannot catch a double activation. architecture:interaction-modality asks for real-browser evidence when behavior depends on pen synthesis and hit testing: a pen tap and a pen slide in AlertDialog.interaction.browser.spec.ts, or ['touch'] here, would settle it.
|
Agreed: one implementation. The local |
4a23248 to
65889f0
Compare
…d tracker, every pointer) into meta#8084 meta#8083's actions now track every pointer through the shared tracker, only the actions are the group, and `presentation` leaves meta#8083. Resolved in AlertDialog's files: - `presentation` (its type, prop, docs and story) is this pull request's own: `'dialog'` keeps the theme motion and Dialog's 400 width; the actions track every pointer either way. - The touch layer (AlertDialogTouch.ts) keeps only the motion; AlertDialog runs the group tracking on the actions' row, tracked while the alert is presented (its fade included) and inert during either transition (`isInert`, which the adapter maps to the tracker's inert press). - The tests keep this pull request's motion, width and leave cases; the tracking cases are meta#8083's, plus one: with `presentation="dialog"` a slide from the action to Cancel cancels.
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision: tracking AlertDialog's two actions as one group under every pointer departs from spec:AST-094 FR10, FR11, FR19 and FR41 and the buttons family's press-feedback owner as they stand on main, and the family:action-groups record it relies on is still an open spec pull request (#8622). An engineering owner settles that by approving, for this head, the fine-pointer listener, activation through the action's own click(), the tracker painting the press, the always-on label selection lock and the 200 ms release fade, by commenting /approve-spec <full head SHA> (the author may do so as an engineering owner), or by landing that amendment on main. Whatever the decision, these changes are still needed:
- Land the stacked gesture, motion and harness pull requests separately and rebase, so this one carries only its AlertDialog change instead of about 29,600 authored lines, and drop the unused
useIsScrollable. - With a mouse, sliding from Delete onto Cancel leaves Delete pressed by
:activewhile Cancel is lit and about to run; let only the tracker paint a tracked action's press, and assert it in the mouse browser case. AlertDialog.doc.mjsstill tells builders a mouse keeps each button's click; describe tracking under every pointer.- Add real-device iOS Safari and Android Chrome evidence for the changeset's iOS Safari claim, or narrow the claim.
[Automated review]
| // actions keep their default styles otherwise. | ||
| manageStyle: false, | ||
| }); | ||
| const actionTrackingProps = isTracking ? pressGroupItemProps : undefined; |
There was a problem hiding this comment.
With a mouse, a press on Delete slid onto Cancel leaves Delete pressed by :active (its box stays at 0.98 scale, 66.12 × 31.36 against 67.47 × 32 at rest) while the tracker paints Cancel data-astryx-press="on", so both actions look pressed and the release runs Cancel. Under a coarse pointer :active is off, which is why the touch path shows one highlight. Letting only the tracker paint a tracked item's press (no :active press arm while data-astryx-press-group-item is set) would keep one highlight for every pointer; the mouse browser case could assert it.
| usage: { | ||
| description: | ||
| 'AlertDialog asks the user to confirm a destructive or irreversible action before it happens. Use it for things like deleting content, revoking access, or discarding unsaved changes.\n\nIt implements the WAI-ARIA APG [Alert Dialog pattern](https://www.w3.org/WAI/ARIA/apg/patterns/alertdialog/): `role="alertdialog"`, a title linked by `aria-labelledby`, a consequence description linked by `aria-describedby`, focus moved into the dialog on open and returned to the trigger on close, and no dismissal by clicking outside. Escape cancels.\n\nAlertDialog passes its requested width through to Dialog, which clamps the surface to the container and dynamic viewport with token gutters. Generic Dialog footers should wrap, but Dialog does not own action semantics or order; consumer composition controls that. AlertDialog owns its confirmation semantics: above 640px, actions render horizontally and may move onto another row; at 640px and below, the destructive action appears above Cancel and both buttons fill the footer width. Button labels retain their standard single-line behavior. The breakpoint follows available width, not pointer or hover capability. The body scrolls when block space is constrained.\n\nFor cases where you want to show an alert without managing open state, use the `useImperativeAlertDialog` hook: call `alert.show(options)` and render `alert.element` in your tree.', | ||
| 'AlertDialog asks the user to confirm a destructive or irreversible action before it happens. Use it for things like deleting content, revoking access, or discarding unsaved changes.\n\nIt implements the WAI-ARIA APG [Alert Dialog pattern](https://www.w3.org/WAI/ARIA/apg/patterns/alertdialog/): `role="alertdialog"`, a title linked by `aria-labelledby`, a consequence description linked by `aria-describedby`, focus moved into the dialog on open and returned to the trigger on close, and no dismissal by clicking outside. Escape cancels.\n\nAlertDialog passes its requested width through to Dialog, which clamps the surface to the container and dynamic viewport with token gutters. Generic Dialog footers should wrap, but Dialog does not own action semantics or order; consumer composition controls that. AlertDialog owns its confirmation semantics: above 640px, actions render horizontally and may move onto another row; at 640px and below, the destructive action appears above Cancel and both buttons fill the footer width. Button labels retain their standard single-line behavior. The breakpoint follows available width, not pointer or hover capability. The body scrolls when block space is constrained.\n\nOn a touch screen the two actions are tracked as one group under a finger or a pen, the way iOS tracks the actions of an alert: the action under the finger highlights as soon as it is touched, the highlight follows the finger from one action to the other, and lifting the finger runs the action under it. Lifting it off both actions runs nothing. A mouse, the keyboard and assistive technology keep the click of each button.\n\nFor cases where you want to show an alert without managing open state, use the `useImperativeAlertDialog` hook: call `alert.show(options)` and render `alert.element` in your tree.', |
There was a problem hiding this comment.
This says the actions are tracked "On a touch screen … under a finger or a pen" and that "A mouse, the keyboard and assistive technology keep the click of each button", but the head tracks a mouse too (useActionGroupTracking.ts:82, :148), as the changeset says. The dense description at line 199 has the same wording.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] AlertDialog: its two actions are tracked as one group under every pointer, a finger, a pen or a mouse, the way iOS tracks an alert's actions. A press that lands on one action and slides to the other now runs the action it lifts on. Before, iOS Safari ran the action the finger first touched (Delete, when the finger slid to Cancel), and other browsers ran nothing and left the alert open. The pressed action highlights as soon as it is pressed instead of 150 ms later, the highlight follows the pointer between the actions, and lifting off both still runs nothing; a second finger or the window losing focus ends the press with nothing run. Only the actions are tracked: a press on the title or the message is not, and their text stays selectable with a mouse. The keyboard and assistive technology keep each button's own activation. |
There was a problem hiding this comment.
"Before, iOS Safari ran the action the finger first touched" is a claim about iOS Safari behavior; spec:AST-094 Platform support asks for real-device evidence for those (spec:AST-013 FR10), and the description's evidence is the iOS 26.2 simulator and an Android emulator.
| } from './scrollGeometry'; | ||
| import {useIsomorphicLayoutEffect} from './useIsomorphicLayoutEffect'; | ||
|
|
||
| export function useIsScrollable( |
There was a problem hiding this comment.
Nothing imports this hook at this head (AlertDialog does not use it, despite the header), so it and its test add 118 unused lines to the change; it could land with the component that needs it.
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision: tracking AlertDialog's two actions as one group under every pointer departs from spec:AST-094 FR10, FR11, FR19 and FR41 and the buttons family's press-feedback owner as they stand on main, and the family:action-groups record it relies on is still the open spec pull request #8622. An engineering owner can settle that by landing the amendment on main, or by approving this head's behavior (the fine-pointer listener, activation through the action's own click(), the tracker painting the press, the always-on label selection lock and the 200 ms release fade) with /approve-spec <full head SHA>; the author may do so as an engineering owner. Whatever the decision, these changes are still needed:
- Land the stacked gesture, motion and harness pull requests separately and rebase, so this one carries only its AlertDialog change instead of 29,933 authored lines, and drop the unused
useIsScrollable. - With a mouse, sliding from Delete onto Cancel leaves Delete pressed by
:activewhile Cancel is lit and about to run; let only the tracker paint a tracked action's press, and assert it in the mouse browser case. AlertDialog.doc.mjsstill tells builders that a mouse keeps each button's click; describe tracking under every pointer.- Add real-device iOS Safari and Android Chrome evidence for the changeset's iOS Safari claim, or narrow it, and a browser case for the default pen tracking.
[Automated review]
| const DETACHED: RefObject<Element | null> = {current: null}; | ||
|
|
||
| /** Every pointer a group tracks (`family:action-groups` FR5). */ | ||
| const EVERY_POINTER: ReadonlyArray<PointerKind> = ['touch', 'pen', 'mouse']; |
There was a problem hiding this comment.
Pens are tracked by default here, and a pen's single activation depends on the browser's click being swallowed after the group's click(), but only jsdom drives a pen (useActionGroupTracking.test.tsx:443, :471), where no browser click follows. architecture:interaction-modality asks pointer-dependent behavior to verify "mouse, touch, and pen as applicable" through browser paths (INV4); a pen slide and a pen tap in AlertDialog.interaction.browser.spec.ts, one activation each, would cover it.
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision: tracking AlertDialog's two actions as one group under every pointer departs from spec:AST-094 FR10, FR11, FR19 and FR41 and the buttons family's press-feedback owner as they stand on main, and the family:action-groups record it relies on is still the open spec pull request #8622. An engineering owner can settle that by landing that amendment on main, or by approving this behavior (the fine-pointer listener, activation through the action's own click(), the tracker painting the press, the always-on label selection lock and the 200 ms release fade) with /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner. Whatever the decision, these changes are still needed:
- Land the stacked gesture, motion, harness and build pull requests separately and rebase, so this one carries only its AlertDialog change instead of 30,117 authored lines, and drop the unused
useIsScrollable. - With a mouse, sliding from Delete onto Cancel leaves Delete pressed by
:activewhile Cancel is lit and about to run; let only the tracker paint a tracked action's press, and assert it in the mouse browser case. AlertDialog.doc.mjsstill tells builders that a mouse keeps each button's click; describe tracking under every pointer.- Add real-device iOS Safari and Android Chrome evidence for the changeset's iOS Safari claim, or narrow it, and a browser case for the default pen tracking.
[Automated review]
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision: tracking AlertDialog's two actions as one group under every pointer departs from spec:AST-094 FR10, FR11, FR19 and FR41 and the buttons family's press-feedback owner as they stand on main, and the family:action-groups record it relies on is still the open spec pull request #8622. An engineering owner can settle that by landing that record on main, or by approving this behavior (the fine-pointer listener, activation through the action's own click(), the tracker painting the press, the always-on label selection lock and the 200 ms release fade) with /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner. Whatever the decision, these changes are still needed:
- Land the stacked gesture, motion, harness and build pull requests separately, and #8671 before this one since
main's bundle test fails on the root entry without it, then rebase so this pull request carries only its AlertDialog change instead of 30,268 authored lines, without the unuseduseIsScrollable. - With a mouse, sliding from Delete onto Cancel leaves Delete pressed by
:activewhile Cancel is lit and about to run; let only the tracker paint a tracked action's press, and assert it in the mouse browser case. AlertDialog.doc.mjsstill tells builders that a mouse keeps each button's click; describe the behavior that is approved.- Add real-device iOS Safari and Android Chrome evidence for the changeset's iOS Safari claim, or narrow it, and a real-browser pen case.
[Automated review]
| usage: { | ||
| description: | ||
| 'AlertDialog asks the user to confirm a destructive or irreversible action before it happens. Use it for things like deleting content, revoking access, or discarding unsaved changes.\n\nIt implements the WAI-ARIA APG [Alert Dialog pattern](https://www.w3.org/WAI/ARIA/apg/patterns/alertdialog/): `role="alertdialog"`, a title linked by `aria-labelledby`, a consequence description linked by `aria-describedby`, focus moved into the dialog on open and returned to the trigger on close, and no dismissal by clicking outside. Escape cancels.\n\nAlertDialog passes its requested width through to Dialog, which clamps the surface to the container and dynamic viewport with token gutters. Generic Dialog footers should wrap, but Dialog does not own action semantics or order; consumer composition controls that. AlertDialog owns its confirmation semantics: above 640px, actions render horizontally and may move onto another row; at 640px and below, the destructive action appears above Cancel and both buttons fill the footer width. Button labels retain their standard single-line behavior. The breakpoint follows available width, not pointer or hover capability. The body scrolls when block space is constrained.\n\nFor cases where you want to show an alert without managing open state, use the `useImperativeAlertDialog` hook: call `alert.show(options)` and render `alert.element` in your tree.', | ||
| 'AlertDialog asks the user to confirm a destructive or irreversible action before it happens. Use it for things like deleting content, revoking access, or discarding unsaved changes.\n\nIt implements the WAI-ARIA APG [Alert Dialog pattern](https://www.w3.org/WAI/ARIA/apg/patterns/alertdialog/): `role="alertdialog"`, a title linked by `aria-labelledby`, a consequence description linked by `aria-describedby`, focus moved into the dialog on open and returned to the trigger on close, and no dismissal by clicking outside. Escape cancels.\n\nAlertDialog passes its requested width through to Dialog, which clamps the surface to the container and dynamic viewport with token gutters. Generic Dialog footers should wrap, but Dialog does not own action semantics or order; consumer composition controls that. AlertDialog owns its confirmation semantics: above 640px, actions render horizontally and may move onto another row; at 640px and below, the destructive action appears above Cancel and both buttons fill the footer width. Button labels retain their standard single-line behavior. The breakpoint follows available width, not pointer or hover capability. The body scrolls when block space is constrained.\n\nOn a touch screen the two actions are tracked as one group under a finger or a pen, the way iOS tracks the actions of an alert: the action under the finger highlights as soon as it is touched, the highlight follows the finger from one action to the other, and lifting the finger runs the action under it. Lifting it off both actions runs nothing. A mouse, the keyboard and assistive technology keep the click of each button.\n\nFor cases where you want to show an alert without managing open state, use the `useImperativeAlertDialog` hook: call `alert.show(options)` and render `alert.element` in your tree.', |
There was a problem hiding this comment.
This says the actions are tracked "On a touch screen … under a finger or a pen" and that "A mouse, the keyboard and assistive technology keep the click of each button", but at this revision a mouse is tracked too: AlertDialog passes no pointerTypes, so useActionGroupTracking tracks ['touch', 'pen', 'mouse'] (useActionGroupTracking.ts:85), as the changeset and the component header say. Please describe the behavior an owner approves here and in the dense description (:199).
| // actions keep their default styles otherwise. | ||
| manageStyle: false, | ||
| }); | ||
| const actionTrackingProps = isTracking ? pressGroupItemProps : undefined; |
There was a problem hiding this comment.
With a mouse under a fine pointer, a press on Delete slid onto Cancel keeps Delete's :active press (Button.tsx:128-131, scale 0.98), because the group's fine-pointer pointerdown listener is passive (GestureHost.ts:916), while the tracker paints Cancel data-astryx-press="on" (Button.tsx:132). At the release both actions look pressed and Cancel runs. If mouse tracking stays, keep :active off a tracked group item (for example :active:where(:not([data-astryx-press-group-item]))) so the tracker is the one painter.
| } from './scrollGeometry'; | ||
| import {useIsomorphicLayoutEffect} from './useIsomorphicLayoutEffect'; | ||
|
|
||
| export function useIsScrollable( |
There was a problem hiding this comment.
Nothing imports this hook at this revision; only its own test does, although the header names AlertDialog as a consumer (:10-12). Please drop it and its test from this pull request.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] AlertDialog: its two actions are tracked as one group under every pointer, a finger, a pen or a mouse, the way iOS tracks an alert's actions. A press that lands on one action and slides to the other now runs the action it lifts on. Before, iOS Safari ran the action the finger first touched (Delete, when the finger slid to Cancel), and other browsers ran nothing and left the alert open. The pressed action highlights as soon as it is pressed instead of 150 ms later, the highlight follows the pointer between the actions, and lifting off both still runs nothing; a second finger or the window losing focus ends the press with nothing run. Only the actions are tracked: a press on the title or the message is not, and their text stays selectable with a mouse. The keyboard and assistive technology keep each button's own activation. |
There was a problem hiding this comment.
"Before, iOS Safari ran the action the finger first touched" is a claim about iOS Safari behavior, and spec:AST-094 Platform support asks for real-device evidence for every iOS Safari and Android Chrome claim (spec:AST-013 FR10); the description has simulator and emulator recordings and calls a real-device check the release gate. Please attach real-device recordings or narrow the wording to what the simulator shows.
| test.describe('with a mouse', () => { | ||
| test.use({hasTouch: false, isMobile: false}); | ||
|
|
||
| test('a press on Delete slid to Cancel runs Cancel once, and a click on Delete runs Delete once (family:action-groups FR5)', async ({ |
There was a problem hiding this comment.
This mouse case asserts only which click ran, so the double press during the slide (Delete still :active while Cancel is lit) goes unobserved; reading both buttons' transform or press state over Cancel would cover it. Pens are tracked by default too but have no real-browser case here, only jsdom (architecture:interaction-modality asks for mouse, touch and pen where behavior depends on the pointer).
There was a problem hiding this comment.
Approval is on hold until an engineering owner decides whether AlertDialog's actions become one tracked group under every pointer: no current spec on main gives an alert's buttons the menu's lift-decides model (spec:AST-094 FR26–FR28 cover menus, and FR11 keeps a control's activation with the browser's click), and the always-on text-selection lock on the action row and the 200 ms release fade need that decision too. An engineering owner records it by commenting /approve-spec b2c6bd383d54678a9ed5df38255ea21a9dba314b on this pull request, and the author may do so as an engineering owner. Whatever the decision, please address these:
- Land the stacked pull requests and rebase, so this one carries only its AlertDialog files (it now changes 30,568 authored lines), and drop the unused
useIsScrollablehook. - A mouse pressed on Delete and moved onto Cancel shows both buttons pressed: Button's
:activeplus the group's paint. userEvent.clickon an open alert's Delete runs Cancel in jsdom, so consumer tests that click alert actions this way break.scripts/touch-support-bundle.test.mjs"is absent from the root entry" fails on this head, and the AlertDialog docs still say a mouse keeps each button's click.
[Automated review]
There was a problem hiding this comment.
AlertDialog's slide-to-the-lifted-action fix works under a finger and a mouse, but approval waits on an engineering owner deciding whether the lifted action's highlight keeps the shared 200 ms press fade or takes the 100 ms measured for UIKit's alert actions (spec:AST-094 FR1/FR2). An engineering owner records that decision by commenting /approve-spec 458e2d9e88dc59e321db7703e20c831db74f056b on this pull request, and the author may do so as an engineering owner. Whatever that decision, these need changes:
- Rebase onto the landed stack so this pull request carries only its 1,840-line AlertDialog change; it is 28,849 authored lines now.
- In a jsdom test,
userEvent.clickon the action now runs Cancel instead ofonAction, which would break consumers' tests. - Under a mouse, Delete keeps its
:activepress while the group lights Cancel, so two actions look pressed at once (family:action-groupsFR2). - The actions row's always-on
user-select: nonegoes pastspec:AST-094FR41's per-press suppression and a mouse's released behavior. - The root-entry case of
scripts/touch-support-bundle.test.mjsfails, opening the alert on a desktop logs a false "stays inactive" warning, the docs say a mouse keeps each button's click,useIsScrollableis unused, and the changeset's iOS Safari claim needs real-device evidence.
[Automated review]
| trackingRow: { | ||
| touchAction: 'pinch-zoom', | ||
| userSelect: 'none', | ||
| WebkitUserSelect: 'none', | ||
| }, |
There was a problem hiding this comment.
This keeps the actions row unselectable for every pointer for as long as the alert is open. spec:AST-094 FR41 suppresses selection for the press only and keeps a mouse's released behavior, and FR43 rules out added stylesheet bytes. The recognizer already locks selection per press for a finger or a pen (useActionGroupTracking.ts:166), so userSelect and WebkitUserSelect can go, with touch-action left to the gesture host.
There was a problem hiding this comment.
Fixed at 8462850. The row's user-select is gone. The gesture host's per-press lock covers a finger or a pen, and a mouse keeps its selection; a browser case checks both. The row keeps touch-action: pinch-zoom, which the host keeps, because Chrome on Android cancels a pointer it pans. That atomic class already ships for six other components, so it adds no stylesheet bytes.
| // actions keep their default styles otherwise. | ||
| manageStyle: false, | ||
| }); | ||
| const actionTrackingProps = isTracking ? pressGroupItemProps : undefined; |
There was a problem hiding this comment.
With a mouse, a press on Delete slid onto Cancel keeps Delete's own :active press (Button.tsx:128-131, scale 0.98) while the group paints Cancel data-astryx-press="on", so both actions look pressed at once, against family:action-groups FR2 ("at most one at a time"). The group's paint should be the only press on a tracked item.
There was a problem hiding this comment.
Fixed at 8462850. A Button inside a tracked group takes no :active paint, so only the group paints the press. A mouse press slid from Delete onto Cancel now lights only Cancel. A browser case fails without the change and passes with it.
| const DETACHED: RefObject<Element | null> = {current: null}; | ||
|
|
||
| /** Every pointer a group tracks (`family:action-groups` FR5). */ | ||
| const EVERY_POINTER: ReadonlyArray<PointerKind> = ['touch', 'pen', 'mouse']; |
There was a problem hiding this comment.
Tracking the mouse changes what a consumer's jsdom test sees: userEvent.click on Delete now runs Cancel (onOpenChange(false), no onAction), because jsdom lays nothing out and the tracker's hit test (actionGroupTracker.ts:309-324) matches the first member's empty box at the origin; on the base the same test calls onAction. Leaving the press to the browser's click when no member has a laid-out box would keep those tests working.
There was a problem hiding this comment.
Fixed at 8462850. Where several actions hold the point (no layout, as in jsdom), the press resolves to the action it landed on. userEvent.click on the action runs onAction again, and a unit case pins it.
| usage: { | ||
| description: | ||
| 'AlertDialog asks the user to confirm a destructive or irreversible action before it happens. Use it for things like deleting content, revoking access, or discarding unsaved changes.\n\nIt implements the WAI-ARIA APG [Alert Dialog pattern](https://www.w3.org/WAI/ARIA/apg/patterns/alertdialog/): `role="alertdialog"`, a title linked by `aria-labelledby`, a consequence description linked by `aria-describedby`, focus moved into the dialog on open and returned to the trigger on close, and no dismissal by clicking outside. Escape cancels.\n\nAlertDialog passes its requested width through to Dialog, which clamps the surface to the container and dynamic viewport with token gutters. Generic Dialog footers should wrap, but Dialog does not own action semantics or order; consumer composition controls that. AlertDialog owns its confirmation semantics: above 640px, actions render horizontally and may move onto another row; at 640px and below, the destructive action appears above Cancel and both buttons fill the footer width. Button labels retain their standard single-line behavior. The breakpoint follows available width, not pointer or hover capability. The body scrolls when block space is constrained.\n\nFor cases where you want to show an alert without managing open state, use the `useImperativeAlertDialog` hook: call `alert.show(options)` and render `alert.element` in your tree.', | ||
| 'AlertDialog asks the user to confirm a destructive or irreversible action before it happens. Use it for things like deleting content, revoking access, or discarding unsaved changes.\n\nIt implements the WAI-ARIA APG [Alert Dialog pattern](https://www.w3.org/WAI/ARIA/apg/patterns/alertdialog/): `role="alertdialog"`, a title linked by `aria-labelledby`, a consequence description linked by `aria-describedby`, focus moved into the dialog on open and returned to the trigger on close, and no dismissal by clicking outside. Escape cancels.\n\nAlertDialog passes its requested width through to Dialog, which clamps the surface to the container and dynamic viewport with token gutters. Generic Dialog footers should wrap, but Dialog does not own action semantics or order; consumer composition controls that. AlertDialog owns its confirmation semantics: above 640px, actions render horizontally and may move onto another row; at 640px and below, the destructive action appears above Cancel and both buttons fill the footer width. Button labels retain their standard single-line behavior. The breakpoint follows available width, not pointer or hover capability. The body scrolls when block space is constrained.\n\nOn a touch screen the two actions are tracked as one group under a finger or a pen, the way iOS tracks the actions of an alert: the action under the finger highlights as soon as it is touched, the highlight follows the finger from one action to the other, and lifting the finger runs the action under it. Lifting it off both actions runs nothing. A mouse, the keyboard and assistive technology keep the click of each button.\n\nFor cases where you want to show an alert without managing open state, use the `useImperativeAlertDialog` hook: call `alert.show(options)` and render `alert.element` in your tree.', |
There was a problem hiding this comment.
"A mouse, the keyboard and assistive technology keep the click of each button" (and line 199's "mouse/keyboard/AT keep each button click") no longer holds: the group tracks 'mouse' too (useActionGroupTracking.ts:85), as the changeset says.
There was a problem hiding this comment.
Fixed at 8462850. The docs now say every pointer is tracked, and only the keyboard and assistive technology keep each button's click.
| } else if (!this.isCoarse) { | ||
| this.warnFinePointer(); |
There was a problem hiding this comment.
An action-group attach takes this branch too, so opening an AlertDialog under a fine pointer logs "It stays inactive until the pointer is coarse" although the host serves the group there (family:action-groups FR5). Skip the warning for options.actionGroup, as for perEventPointer.
There was a problem hiding this comment.
This warning is not on main any more. A unit case opens the alert under a fine pointer and checks for no console warning.
| * @position Internal hook for a touch-tracked surface whose content scroller | ||
| * keeps the browser's pan only where it overflows (AlertDialog, the action | ||
| * sheet card) |
There was a problem hiding this comment.
Nothing imports this hook, though the header names AlertDialog and an action sheet as consumers. Please drop it and its test from this pull request.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] AlertDialog: its two actions are tracked as one group under every pointer, a finger, a pen or a mouse, the way iOS tracks an alert's actions. A press that lands on one action and slides to the other now runs the action it lifts on. Before, iOS Safari ran the action the finger first touched (Delete, when the finger slid to Cancel), and other browsers ran nothing and left the alert open. The pressed action highlights as soon as it is pressed instead of 150 ms later, the highlight follows the pointer between the actions, and lifting off both still runs nothing; a second finger or the window losing focus ends the press with nothing run. Only the actions are tracked: a press on the title or the message is not, and their text stays selectable with a mouse. The keyboard and assistive technology keep each button's own activation. |
There was a problem hiding this comment.
"Before, iOS Safari ran the action the finger first touched" is a claim about iOS Safari behavior, and spec:AST-094 Platform support asks for real-device evidence for every such claim. Attach real-device evidence or drop the sentence.
There was a problem hiding this comment.
Removed at 8462850. The changeset makes no iOS Safari claim now.
|
/approve-spec 8462850 |
There was a problem hiding this comment.
AlertDialog's slide between the actions now runs the action the pointer lifts on for a finger, a pen and a mouse, and the spec side is approved for this head; three fixes remain before approval:
- After a slide onto the action, focus stays on Cancel instead of moving to the lifted action as a click on it would (
family:action-groupsFR1), and under touch a plain tap no longer focuses the tapped action either. useActionGroupTracking'sresolveTargetre-implementsactionGroupTracker's hit test to add the landed-on tie-break; move that rule into the tracker so it stays the one copy of the tracking (interaction-modalityINV13).- The AlertDialog record now says FR8 ships, but its FR8 verification row still says no test exists and FR8 is not shipped, and
verified_byand Compatibility were not updated.
Two comments in the new hook restate names and could go; details are in the inline notes.
[Automated review]
| pointerRef.current === 'touch' | ||
| ? optionsRef.current.highlightExpansion | ||
| : 0, | ||
| onActivate: optionsRef.current.onActivate, |
There was a problem hiding this comment.
The lift runs the button's own click(), which moves no focus, and the browser's click is swallowed. In the Async story (Chromium), a mouse click on Revoke focuses Revoke, but a press on Cancel slid onto Revoke runs Revoke and leaves focus on Cancel; under touch even a tap now leaves focus on Cancel (the base focused Revoke). family:action-groups FR1: "After a slide, focus does what a click on the lifted action does." Giving the lifted action that focus at the lift (for example item.focus({preventScroll: true}) before its click when the press moved focus to the action it began on, or a finger lifted) plus a slide case and a tap case that check document.activeElement would close this.
There was a problem hiding this comment.
Fixed at 6aa706c. After a slide, focus does what a click on the lifted action does. A mouse or a pen moves it there when the press focused the action it began on. A finger does unless the engine is iOS WebKit. A lift outside the actions moves no focus. Unit cases cover the slide, the tap and the lift outside; two of them fail without the change.
| resolveTarget: (x, y, targets) => { | ||
| const margin = | ||
| pointerRef.current === 'touch' | ||
| ? optionsRef.current.highlightExpansion | ||
| : 0; | ||
| const holders = targets.filter( | ||
| target => | ||
| x >= target.left - margin && | ||
| x <= target.right + margin && | ||
| y >= target.top - margin && | ||
| y <= target.bottom + margin, | ||
| ); | ||
| const landed = landedRef.current; | ||
| const pressed = holders.find( | ||
| target => landed != null && target.element.contains(landed), | ||
| ); | ||
| return (pressed ?? holders[0])?.element ?? null; |
There was a problem hiding this comment.
This copies actionGroupTracker's default hit test (hooks/actionGroupTracker.ts:239-255, the rect grown by the margin) to add one rule: the action the press landed on wins where several hold the point. interaction-modality INV13 keeps the tracking in that one module, so a later fix to its hit test would miss AlertDialog. Moving the tie-break into the tracker's default (begin can take the press's target) would let this option go, with the pointer-dependent margin kept in highlightExpansion only.
There was a problem hiding this comment.
Fixed at 6aa706c. The tie-break now lives in actionGroupTracker: begin takes the node the press landed on, and the default hit test prefers the action holding it where several hold the point. The hook passes that node and sets no resolveTarget. A tracker case fails without the rule.
| | End-user impact | A keyboard or screen-reader user is told a decision is needed (the title and description are announced), lands on Cancel, and cannot lose the dialog by clicking past it. On a touch screen the alert scales in and fades out as iOS's does and ignores taps while it enters (FR6, FR7), and on a phone it is 320 px wide (FR9); under every pointer, mouse included, a press that slides from Cancel to the action runs the action it lifts on (FR8). | | ||
| | Builder impact | None. | | ||
| | Compatibility/readiness | FR1–FR5 and AR1 record shipped behavior, apart from AR1's Tab wrap. FR6–FR9 are requirements not shipped yet. Each gap is listed under Current-state impact. | | ||
| | Compatibility/readiness | FR1–FR5, FR8 and AR1 record shipped behavior, apart from AR1's Tab wrap. FR6, FR7 and FR9 are requirements not shipped yet. Each gap is listed under Current-state impact. | |
There was a problem hiding this comment.
With FR8 now listed as shipped, the FR8 verification row (line 228) still ends "No test exists yet; FR8 is not shipped", verified_by names none of the new suites, and Compatibility (lines 68-71) still says the released default holds until FR6–FR9 ship. Naming AlertDialog.test.tsx ("AlertDialog action tracking"), useActionGroupTracking.test.tsx and AlertDialog.interaction.browser.spec.ts there would keep the record consistent.
There was a problem hiding this comment.
Fixed at 6aa706c. FR8's verification row names AlertDialog.test.tsx ("AlertDialog action tracking"), useActionGroupTracking.test.tsx and AlertDialog.interaction.browser.spec.ts. verified_by lists the two new suites, and Compatibility says FR8 changes action activation under a slide.
| /** The ref handed to the recognizer while tracking is off: no element. */ | ||
| const DETACHED: RefObject<Element | null> = {current: null}; | ||
|
|
||
| /** A group tracks every pointer. */ |
There was a problem hiding this comment.
Non-blocking (INV29): this restates EVERY_POINTER, as line 92's comment restates landedRef; both can go.
| // A held pointer of any kind is tracked across the two actions, and the lift | ||
| // runs the action under it. Each tracked action carries the press-group | ||
| // marker from its first render, so the tracker is its one painter. |
There was a problem hiding this comment.
Non-blocking (INV29): the first sentence narrates the hook call. Suggested: "Each action carries the press-group marker from its first render, so the tracker is its only painter."
|
/approve-spec 6aa706c |
There was a problem hiding this comment.
Requesting changes: a press that slides between AlertDialog's actions now runs the action it lifts on under a mouse, a pen and a finger, and the spec side is approved for this head, with one fix left:
Button.tsx(trackedOverlay, lines 156-173) re-declares the shared interaction overlay's hover and press arms without:active, so a later change toutils/interactionOverlay.stylex.tswould miss AlertDialog's two tracked actions; keep that arm set in the shared overlay module, or say in the description why it cannot live there and list Button in that file's SYNC header.
Two optional tidy-ups: the AlertDialog record's PR1 still says the tracking arrives "once FR8 ships", and the two useImperativeAlertDialog cases could return to user.click now that the tracker resolves a press to the action it landed on.
[Automated review]
| // A tracked action's overlay: the shared one without its `:active` arm, so | ||
| // the press attribute is its only press paint. | ||
| trackedOverlay: { | ||
| backgroundImage: { | ||
| default: null, | ||
| [ENABLED]: { | ||
| default: null, | ||
| [HOVER_HOVER]: { | ||
| default: null, | ||
| ':hover:where(:not(:disabled,[aria-disabled="true"]))': HOVER_IMAGE, | ||
| [PRESSED_ON]: PRESS_IMAGE, | ||
| [PRESSED_FADING]: PRESS_IMAGE, | ||
| }, | ||
| [PRESSED_ON]: PRESS_IMAGE, | ||
| [PRESSED_FADING]: PRESS_IMAGE, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
This re-declares interactionOverlayStyles.backgroundImage (utils/interactionOverlay.stylex.ts:245-266) minus its :active arm: the enabled guard, the hover media query, the hover gradient and both press arms (lines 85-90 copy the module's constants). That module is the one place a change to the press is made, and architecture:react-component-runtime INV16 asks that a shared concern be extended in its owner. A later change to the overlay's hover or arm selectors would reach every Button but these two tracked actions. Keeping the tracked arm set in the overlay module (as the previous revision's backgroundImageTracked did) would fix it; if the module must not grow its public export, please say so in the description and add Button to its SYNC header.
There was a problem hiding this comment.
Fixed at 3576bcb. Button no longer copies the arms: a tracked Button keeps the shared interactionOverlayStyles.backgroundImage and only sizes its own :active image to zero unless the tracker lights it. The overlay module is a public entry (./utils/interactionOverlay.stylex in the package exports), so it gains no export. Its SYNC header names Button.
…e it lifts on AlertDialog's two actions are an action group under every pointer, the way UIKit tracks an alert's actions (component:AlertDialog FR8, family:action-groups). A new internal hook, useActionGroupTracking, drives the shared action-group tracker with a long press: the action under the pointer highlights at once, the highlight follows the pointer between the actions, and the lift runs the action under it through its own click. A lift outside both, a cancelled pointer, a second finger or the window losing focus runs nothing. After a slide, focus does what a click on the lifted action does (family:action-groups FR1). Where several actions hold the point (no layout, as in a test renderer), the shared tracker resolves the press to the action it landed on, so a user-event click on an action runs that action. One painter per press. The press controller leaves a tracked action's pointer press to the group's tracker, through the private press-group marker (privateAttributes/pressGroupItem.stylex.ts); a key's press there still paints. A Button inside a tracked group (an internal context) takes no :active paint, so a mouse press slid from one action to the other lights one action at a time (family:action-groups FR2). The gesture host writes the actions' touch-action (pinch-zoom only) and refuses a text selection only while a finger or a pen holds them (spec:AST-094 FR41). AlertDialog, and the public imports that reach it (Tokenizer, PowerSearch, Table), are declared gesture users in the bundle test. AlertDialog's record now says FR8 ships. Supersedes meta#8637 (the press controller's skip for group-tracked actions), whose change is folded in here, with the focus-after-lift from meta#8657.
|
/approve-spec 3576bcb |
There was a problem hiding this comment.
Recommending approval. AlertDialog's Cancel and action now act as one group for a finger, a pen and a mouse: a press that slides between them runs the action it lifts on, and a lift off both runs nothing, as the AlertDialog record's FR8 and the action-groups family require. Button now keeps the shared interaction overlay and only hides its own :active image inside a tracked group, which resolves the earlier duplication, and a Space-key press on the action still looks the same. Base and head checks in Chromium, the unit suites, typecheck and lint back this up.
[Automated review]
User impact
In an AlertDialog, a press that slides from one action to the other runs nothing on lift. UIKit runs the action under the lift.
Problem and solution fit
family:action-groupsmakes every group of sibling actions behave one way.AlertDialog tracks a held pointer of any kind across its actions through a new internal hook over the shared tracker. The lift runs the action under the pointer through its own click; a lift outside both runs nothing. Focus then does what a click on the lifted action does (FR1), with the focus logic from #8657.
The press controller leaves a tracked action's pointer press to the tracker, its one painter (folded in from #8637, which this supersedes). A tracked action takes no
:activepaint, so a mouse slide lights one action; a key's press still paints.Expected behavior and authority
component:AlertDialogFR8 (current);family:action-groupsFR1–FR5;spec:AST-094FR41. The record's FR8 gap note goes, and its verification row,verified_byand compatibility bullet now say FR8 ships: text-only edits that need an eng-owner/approve-specon this head.The lifted action fades on the shared 200 ms press fade, as the spec owners decided: a tap is not a gesture.
Smallest restoration
hooks/useActionGroupTracking.ts: the hook.hooks/actionGroupTracker.ts: where several actions hold the point (no layout), the press resolves to the action it landed on.utils/pressFeedback.ts,privateAttributes/pressGroupItem.stylex.ts: the marker and its skip.hooks/pressGroupContext.ts,Button.tsx: the tracker alone paints a tracked press. A tracked Button sizes its own:activeimage to zero unless the tracker lights it. The overlay module, a public entry, gains no export; its SYNC header names Button.scripts/touch-support-bundle.test.mjs: AlertDialog and its importers are gesture users.touch-action: pinch-zoom: Chrome on Android cancels a pointer it pans, which would end the press.Evidence
Scope
Testing
Local gates at 3576bcb:
pnpm check:repo, strict ESLint and Prettier on the changed files, the private-attribute baseline check, and the touched unit suites (1,492, with every tracker user) pass.pnpm test:ui: 19,027 passed. Node project after a fresh core build: 16,055 passed, 2 expected failures; 7 CLI cases timed out at 30 s under machine load (load average 55). Rerun on their own, the 6 files' 761 cases pass (one file only when run alone).Writing rubric results
component:AlertDialog: FR8's gap note, the readiness row, the Current-state impact bullet, the compatibility bullets, FR8's verification row andverified_bynow say FR8 ships.