Skip to content

feat(ButtonGroup): a finger's slide runs the button it lifts on - #8657

Merged
cixzhang merged 2 commits into
meta:mainfrom
vjeux:uikit/c01-buttongroup-tracking
Nov 5, 2026
Merged

cixzhang merged 2 commits into
meta:mainfrom
vjeux:uikit/c01-buttongroup-tracking

Conversation

@vjeux

@vjeux vjeux commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Stack: #8657 → #8660

#9848 merged as 7b87c5c, so this pull request carries only its own commit. On main bfc1dcc, main's DropdownMenu change gives DropdownMenuPopover a pressTouchModel prop, and this commit's PressGroupContext wrap sits around it. Main's hit-area pass measures in layout px, so a group's growth reads its members through the same layout map (toLayout), and a group in an opening layer keeps its insets. Main's #10022 renames the focus-return import beside this commit's PressGroupContext import, and main's #8085 adds ownsDescendantTouches to the hit-area watch beside this commit's growth and checkAtPress. The rebase keeps both sides: the press check reads checkAtPress per watch, and main's descendant rule per element. The group's hit layer is main's shared touch hit area (#8085): useTouchHitArea takes a growth and a checkAtPress option, which the group passes, and the root applies touchHitAreaStyles.layer (architecture:react-component-runtime INV16).

Size: about 2,220 authored lines, of which about 1,600 are this change's own tests (unit and real-touch browser cases for the slide, the 44 px targets, the wide group, a disabled member, the read counts and the shared hit area's options). The code under them has no other user, so the tests can't land without it.

User need

Part of the UIKit-parity work. On an iPhone you can press one button in a row and slide to another, and the one you lift on runs. In a ButtonGroup a slide runs nothing, and its 28–32 px buttons are under the 44 px a finger needs.

Problem and solution fit

family:action-groups lists ButtonGroup as a member. Its FR1 (decided by the spec owners on 2026-11-03, DEC-1) scopes the slide to the touch presentation: a finger and a pen slide, and a mouse press acts as the platform's native button does.

  • Under the touch presentation (spec:AST-094 FR13: a coarse primary pointer), the shared tracking (useActionGroupTracking) follows a held finger or pen. The lift runs only the button under it, and a lift outside runs nothing.
  • A desktop mouse press behaves exactly as on main: only the pressed button activates, with no drag-across. The group adds no tracking marker, no touch-action marker and no hit layer where the primary pointer is fine.
  • Targets: under a finger, each button's target grows toward 44 px through a hit layer on the group, up to half the gap to a neighbour, without the buttons growing. Each side of the layer grows as far as the buttons on that edge would (groupGrowth in utils/hitArea.ts), so an end button's target reaches past the group. A grown target never reaches into a neighbour's or a disabled button's. Where the group's box is wider than its buttons (a flex child that stretches), the layer ends at the outer buttons' targets, and the group takes a press on its own box only where a button's target holds it. So a tap beside the buttons keeps its click for the page. A press on a disabled button keeps its click too, so a tooltip on it still opens its reason. A press reads no box outside the group: the tracker reads the group's own members when a press begins, and the hit layer's watch does not re-measure the page then (checkAtPress: false). A hidden member (an empty box) is no tracker item and grows nothing.
  • Focus: after a slide or a tap, focus goes where a click on the lifted button would. The lifted button takes focus before its click runs, as a native press gives it, so a click that moves focus or selects text keeps its result. This applies to AlertDialog's actions too, through the shared hook.
  • Selection: a selection that the lifted button's own click makes stays, through fix(gestures): a selection a claimed lift's own handlers make stays #9848's fix to the drag's selection hold (merged).
  • Scrolling content: a finger and a pen wait the content-touch delay and slop before lighting, so a pen can still scroll the page.
  • Members: a member that opens a menu or a listbox keeps its own press and opening; DropdownMenu opts its trigger out, whatever its aria-haspopup. A member that opens a dialog, a grid or a tree joins the slide. A Button in a member's layer, such as a DropdownMenu's, is not tracked: the group's press context is layer-scoped, as component:ButtonGroup FR10 asks.
  • The press mark: the group's press context carries the pointer types its tracker follows (GROUP_PRESS_POINTERS: a finger and a pen), and Button writes them into the press-group mark, so the press controller leaves only those presses to the tracker (fix(AlertDialog): a mouse press on the actions acts natively again #9771's rule). AlertDialog uses the same list through the same context: its actions take the mark from Button, so its own copy of the list and its explicit mark are gone.
  • One hook: the group's press glue lives in one internal hook, useGroupPressTracking. Its hit layer is the shared touch hit area: useTouchHitArea (utils/touchHitArea.ts) gains the growth and checkAtPress options for a group, and the root applies touchHitAreaStyles.layerBelowChildren: the shared band, drawn as the root's ::before, so its buttons paint over it. So the group keeps one watch, and its layer, when a re-render hands the root a new merged ref, such as a caller's inline ref. The touch presentation is read through main's useTouchPresentation, one subscription per group; fix(CheckboxListItem): a row answers a finger the same wherever it lands #8212 makes that one shared watch per query.

Current specification

family:action-groups FR1–FR5 and DEC-1; component:ButtonGroup FR10; spec:AST-094 FR11, FR13, FR26–FR28, FR43.

Public delta and caller burden

No API or prop-default change. Released default preserved: yes for a mouse; under the touch presentation a press gains the slide. Under the touch presentation, an enabled group's root is also position: relative, as the containing block of its hit layer. The layer is the root's ::before (touchHitAreaStyles.layerBelowChildren), which cuts out the root's own box. The buttons paint after it, so each button keeps every pixel of its own box, edges included. So the layer needs no z-index, and the root no isolation. With a fine primary pointer the root keeps its stacking: no position, no layer.

Record change at this head, for the re-stamp: component:ButtonGroup's Compatibility class now reads "…the gesture host's data-astryx-touch-action marker on the group root and the hit layer's inset properties, and an enabled group's root is also position: relative, so absolutely positioned descendants resolve against it there". It read "…the hit layer's inset properties; while the hit layer grows a member's touch target, an enabled group's root is also position: relative and isolation: isolate, so absolutely positioned and z-indexed descendants resolve against it there". The changeset says, in the spec owners' words: "An enabled ButtonGroup root is position: relative under a coarse pointer, so absolutely positioned children anchor to the group."

Census: absolutely positioned children inside a group. Under a coarse pointer an enabled group's root is position: relative, so an absolutely positioned child anchors to the group there. The repository's every ButtonGroup and ToggleButtonGroup use was read: 124 groups in 29 files (Core's components, tests and docs, Lab, the Storybook stories, the docs site, the sandbox, and the CLI's templates and docs). Their children are Button (162), ToggleButton (75), IconButton (10), DropdownMenu (8), a nested ButtonGroup (2), Tooltip, HoverCard, Drawer, OverflowList, a plain button and a test helper (1 each), with icons inside the members.

  • No group puts an absolutely or fixed positioned element of its own directly inside it, except OverflowList's hidden measurement row in the library page template's "Filter by category" ToggleButtonGroup. It has no inset, so it stays at its static position. Probed in Chromium and WebKit with the element around it positioned (local probe, not part of the diff): its offset parent switches, but its width, each item's width and the visible items are identical.
  • A member's own positioned parts, such as Button's status region and press overlay, sit inside the member, which is position: relative on main too, so they don't change.
  • Tooltip, HoverCard, DropdownMenu and Drawer render their layers in the top layer, which a group's position doesn't reach.
  • With a fine primary pointer the root keeps main's stacking: no position, no layer. A browser case checks each pointer.

The ButtonGroup record is owner-gated: it needs an engineering owner's /approve-spec <head sha>.

Design and alternatives

  • Per-button handlers never see a press that began on a neighbour, and fixed margins overlap.
  • Chosen: the shared tracker with utils/hitArea.ts's nearest-target rule, as AlertDialog uses, so the groups can't drift apart.
  • The layer is the root's ::before, where a single control's band is its ::after. Drawn after the buttons, the clip path's edge gave the exact pixel on each button's top edge, and the group's start column, to the group, in Chromium and WebKit. Drawn before them, each button owns every pixel of its box.
  • Tried: anchoring the hit layer through CSS anchor positioning, so the root keeps main's containing block under a finger too. The layer's containing block is then an ancestor outside any scroll container, so the scroll container no longer clips it: a tap just past a scroller's edge lands on the group, in Chromium and in WebKit, and in Chromium the layer lags a scroll (local probe, not part of the diff). So the root is positioned under a coarse pointer, and never under a fine one.

Testing

To check by hand, in the ButtonGroup "Slide across" story:

  1. On an iPhone (or the iOS simulator), press Cut, slide to Paste, and lift: the log reads "Pressed: Paste". Lifting outside the group adds nothing.
  2. On an iPhone, tap just below a button: it runs.
  3. On a desktop, press Cut with the mouse, move to Paste, and release: nothing runs, and only Cut looks pressed while held, as on main.

What was run:

  • At this head: Storybook built at the stack's tip (a8f8340, whose code the tip 215e9f3 keeps: the tip changes one record sentence), in Chromium and in WebKit (container, CI settings).
    • Chromium: ButtonGroup 19/19. Among them, "the pixels on a button's own edges are the button's" fails on c53's band, where each button's top-edge pixel resolves to the group, and passes here. The finger-stacking case passes too. ToggleButtonGroup 12/12, main's list touch-target cases 16/16, the hit-area cases 2/2, DropdownMenu 5/5 and 2/2, AlertDialog 11/11, presentedHitTesting 6/6.
    • WebKit: the same 8 files, 71 passed and 2 skipped by engine gates.
    • Accessibility contracts, Chromium: Button 20/20, ToggleButton 14/14, MenuPress 13/13, MenuButton 13/13, InfoTipTouch 4/4.
    • Unit: touchHitArea and the ButtonGroup tracking cases 47/47. This commit alone: typecheck; test:ui 21,141/21,141. At the stack's tip: test:ui 21,158/21,158; test:node 16,638 passed, 2 expected failures, 71 skipped, and 1 failed: main's own import-hint case below, which passes alone at the tip, twice.
    • Static: typecheck, CI=true pnpm lint on the changed files, strict ESLint, Prettier, check:knowledge, changeset coverage and review:pre pass.

At the previous head, c53 (this commit a0f9257, the stack's tip 414d6d6). This head changes the hit layer's pseudo-element and its tests, the changeset sentence and one comment; the runs above cover them. On main bfc1dcc, fresh pnpm install --frozen-lockfile (exit 0):

  • This commit alone: typecheck; test:ui 21,140/21,140.
  • The shared hit area (red first): with c52's group-owned hit-area ref and style and main's useTouchHitArea without options, two of this commit's cases fail (NOT-FOR-LANDING commit 3b865ec). "Keeps its hit layer, and reads the page no more, when a parent's re-render hands it a new inline ref" fails on the root's classes, which lose the hit layer after the re-render. "Passes an owner's growth, with the element, and its press check to the watch" fails on a missing press check. Both pass here. The group measures its members with the shared tracker's measureActions, which has its own unit case (document order, both filters, one read per box).
  • Red first:
    • In a root wider than its buttons, with the three changed source files at this branch's previous head, the unit case and the hit-area case fail. Each change alone is caught: taking every press on the root fails on the cancelled click, and the old growth fails on the layer's width. The browser case fails on the previous head's build in Chromium and WebKit: the band below the root's empty part was the group's.
    • With ButtonGroup.tsx at main's, 14 of the 31 group cases fail.
    • The hidden-member browser case fails on a build whose hit layer counts a hidden member's empty box ("Expected: Above, Received: Underline"; a local probe, not part of the diff), and passes here.
    • The kept-selection browser case fails in WebKit when the lift focuses after the click ("Received: ''"), and passes here.
  • Browser, Storybook built at this stack's tip, in Chromium and in WebKit (container, CI settings): ButtonGroup 17/17; AlertDialog 11/11; SegmentedControl, the shared tracker's other user, 46/46 in Chromium and 43 in WebKit (3 skipped by that file's engine gate); the gesture system's own gestures.interaction (66 passed in Chromium, 58 in WebKit, the rest that file's engine gates) and presentedHitTesting 6/6. Main's list touch-target cases (ListTouchHitArea), which use the watch's new descendant rule, pass 16/16 in Chromium and in WebKit. Button's a11y contract passes 20/20 in Chromium, and InfoTip's touch contract 4/4.
  • Full suites at the stack's tip (this commit plus feat(ToggleButtonGroup): a finger's slide toggles the one it lifts on #8660): test:ui 21,157/21,157; test:node 16,633 passed, 2 expected failures, 76 skipped, and 1 failed: main's own import-hint case "builds its fixture where a scan of the repo's docs running at the same time never finds it", which times out at 30 s because it imports every doc (its fix is test(cli): the docs-scan case reads the scan's walk, not every doc #10060; alone it passes at this head and at main), the bundle test included.
  • Hit area in an opening layer: main's hit-area browser cases pass, 2/2 in Chromium and in WebKit. The unit "reads a group's growth from its members in its layout px, so a group in an opening layer keeps its insets" passes, and fails when the group's growth skips the layout map (local probe, not part of the diff).
  • DropdownMenu, whose trigger keeps its own press inside a group: the drag-highlight and touch-handoff cases pass, 5/5 and 2/2 in Chromium and 3/3 (2 skipped by that file's engine gate) and 2/2 in WebKit. Its MenuPress and MenuButton accessibility contracts pass, 13/13 each, in Chromium.
  • Static: typecheck, CI=true pnpm lint on the changed files, strict ESLint and Prettier on the changed TypeScript files, check:knowledge, review:pre and changeset coverage pass.

Evidence

Desktop, real Chrome, before (main) and after (this head), a mouse press on the first button released on the third:

main (bb3838b) this PR
A mouse press on Copy, moved over Paste and held main, held PR, held
After the release over Paste main, after PR, after

Measured on both builds (the "Horizontal" story, a fine primary pointer): only Copy is :active while held, no button carries the press paint or the group's marker, and nothing runs at the release.

Writing rubric results

component:ButtonGroup: the compatibility lines name the DOM change under the touch presentation (the action-group item marker on each tracked button, the gesture host's touch-action marker on the group root, and the hit layer's inset properties). "Released default preserved" is yes for a mouse, with the touch presentation's slide named. R1–R6 pass: present tense, with no research, references, rejected alternatives or open questions.

Scope

  • One capability is added.
  • Unrelated fixes, cleanup, and policy changes were removed or split.
  • Consumer docs, focused tests, and migration evidence ship with the feature.
  • Public text and artifacts contain no internal Meta context.

@vercel

vercel Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
astryx Ready Ready Preview Nov 5, 2026 2:57pm UTC

Request Review

@astracat-bot

astracat-bot Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

AI review status for this pull request.

Review status Updated
🟢 Reviewed (for maintainers only) Nov 5, 2026, 3:08 PM UTC

@github-actions github-actions Bot added needs:spec-owner-review Current knowledge records await owner approval needs:design-review Affects visuals — Design should review labels Oct 27, 2026
vjeux added a commit to vjeux/astryx that referenced this pull request Oct 27, 2026
…'s current heads) into the toggle group's tracking
vjeux added a commit to vjeux/astryx that referenced this pull request Oct 27, 2026
@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Oct 27, 2026

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs an engineering owner's decision before it can be approved: ButtonGroup's slide-and-lift tracking relies on the family:action-groups record and the spec:AST-094 DEC-17 amendments carried from #8622, which are not on main yet, and the current AST-094 FR11 keeps a touch control's activation on the browser's click. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or by landing #8622 first; the decision should also settle whether a mouse tracks, since the AST-094 menus section still says a mouse keeps today's behavior, and whether popup buttons inside a group are tracked. Independently of that decision, these need changes:

  • Against main this diff is about 32,400 changed lines, of which the ButtonGroup change is 811; please rebase once #8083, #8635 and #8622 land.
  • A click-opened popup button inside a ButtonGroup, such as DropdownMenu's bottom-sheet trigger, loses its pointer click because the group's press accepts every target (ButtonGroup.tsx:227); limit shouldTrack to tracked buttons and the hit layer, and add a test.
  • ButtonGroupContextValue is exported, so its new required tracksPress is a public type change; keep it internal or document it.

[Automated review]

highlightOnsetMs: 0,
highlightExpansion: 0,
resolveTarget: resolveGroupHitTarget,
shouldTrack: () => isGroupLaidOut(groupRef.current),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This accepts a press on any target in the group, including a button the group leaves untracked (one with aria-haspopup). Once the group's long press begins (at once outside a scrolling container, after 150 ms inside one), the arena claims the press and suppresses its click (gestures/GestureArena.ts:597-603, :865-872), and the tracker finds no marked button, so a click-opened popup button in the group, such as DropdownMenu's bottom-sheet trigger, loses its pointer click. Accepting only presses on a tracked button or on the group's own hit layer keeps the 44 px target working:

shouldTrack: target =>
  isGroupLaidOut(groupRef.current) &&
  (target === groupRef.current ||
    (target instanceof Element && target.closest(TRACKED_BUTTON) != null)),

A case where a click on an aria-haspopup button in a group still runs its handler would cover it.

* (`family:action-groups`): each button it tracks carries the group item
* marker (`utils/pressFeedback.ts` PRESS_GROUP_ITEM_ATTRIBUTE).
*/
tracksPress: boolean;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ButtonGroupContextValue is exported from @astryxdesign/core/ButtonGroup, so this required field is a public type change (a caller-typed value of it stops compiling), while the changeset says there is no API change. Button is its only reader; could it stay off the exported type?

@cixzhang

Copy link
Copy Markdown
Contributor

Posted by Cindy's agent on her behalf.

Thanks for the action-group work. Two review points:

  1. One hit-area helper: this PR adds its own hooks/actionGroupHitArea.ts. Cindy's touch-target rule (44 px on touch only, never overlapping a neighbor's target, clamped to half the gap) has one shared helper, utils/hitArea.ts with resolveHitTarget (chore(core): a shared touch hit area that grows toward 44 px and stops at half the gap to each neighbour #8635). Please use it instead of a second implementation, so the clamp behaves the same everywhere.
  2. A missing test: touching buttons must take no area from each other. For example, with 28 px buttons at x = 0, 80 and 160 px under a touch pointer, a finger at x = 82 runs only the middle button, and a finger 8 px below a button still counts for that button.

The same points apply to the ToggleButtonGroup PR (#8660). If this stays idle, we'll make the change after about 30 minutes, keeping your authorship.

vjeux added a commit to vjeux/astryx that referenced this pull request Oct 27, 2026
@vjeux

vjeux commented Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

Both points are done in this PR and in #8660: the groups call utils/hitArea.ts directly (resolveHitTarget for a finger's press, watchHitArea with writeHitInsets for the group's hit layer), and hooks/actionGroupHitArea.ts is gone. Your touching-buttons case is a unit test in each (28 px buttons: x = 82 runs only the middle one; 8 px below counts, 9 px doesn't). Also fixed: a menu trigger inside a group keeps its own click.

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs an engineering owner's decision before it can be approved: ButtonGroup's slide-and-lift tracking relies on the family:action-groups record and the spec:AST-094 DEC-17 amendments carried from #8622, which are not on main yet, and the current AST-094 FR11 keeps a touch control's activation on the browser's click. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or by landing #8622 first; the decision should also settle whether a mouse tracks, since the AST-094 menus section still says a mouse keeps today's behavior, and whether every popup button in a group stays untracked. Independently of that decision, these need changes:

  • Against main this diff is about 32,500 changed lines, of which the ButtonGroup change is 863; please rebase once #8083 and #8622 land.
  • A Ctrl-, Cmd- or Shift-click on a link member (<Button href>) in a group navigates the current page and its handler sees no modifier, because the lift runs a synthetic click(); leave modified presses to the browser and add a test.
  • The head now conflicts with main in the AST-094 record, where #8515 added DEC-19 at the place this branch adds DEC-17.
  • In development, a desktop page with a ButtonGroup logs a warning that touch work stays inactive under a fine pointer although the group tracks a mouse, and the changeset should name the old behavior and the way back.

[Automated review]

Comment thread packages/core/src/Button/Button.tsx Outdated
Comment on lines +658 to +659
const isGroupTracked =
(groupTracksPress && !opensPopup) || PRESS_GROUP_ITEM_ATTRIBUTE in props;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This marks link members (<Button href>) too. The group press begins for any left-button mouse press whatever its modifiers, and the lift runs the link through a synthetic click() (hooks/actionGroupTracker.ts, clickAndReport) while the browser's own click is swallowed, so a Ctrl-, Cmd- or Shift-click on a link member navigates the current page and onClick sees no modifier. In Chromium a Ctrl-click on a #docs link member sets this page's hash on this branch and leaves it alone on main. Leaving a press that carries a modifier key to the browser (as the host already does for a non-primary button) would keep new-tab and new-window clicks; a case for a modified click on a link member would cover it.

if (options.perEventPointer === true) {
this.perEventRecognizers.add(recognizer);
} else if (!this.isCoarse) {
this.warnFinePointer();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An action group attaches here with actionGroup: true but without perEventPointer, so in development the first enabled ButtonGroup on a desktop page logs "It stays inactive until the pointer is coarse" although the group tracks a mouse under a fine pointer; ButtonGroupTracking.test.tsx's own mouse case prints it. Could the warning skip an action-group attachment?


Rejected: the 150 ms delay on every menu row.

### DEC-17 — Every group of sibling actions tracks a held pointer the same way

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

main now has DEC-19 (#8515) inserted at this same place, and OQ9 resolved, so this record no longer merges cleanly; re-folding #8622 onto current main with DEC-19 kept beside DEC-17 would clear it.

vjeux added a commit to vjeux/astryx that referenced this pull request Oct 27, 2026
vjeux added a commit to vjeux/astryx that referenced this pull request Oct 27, 2026
vjeux added a commit to vjeux/astryx that referenced this pull request Oct 27, 2026
vjeux added a commit to vjeux/astryx that referenced this pull request Oct 27, 2026
vjeux added a commit to vjeux/astryx that referenced this pull request Oct 27, 2026
…taken only on tracked buttons) into the toggle group's tracking

The toggle group takes the same two changes in this merge, since the
module it used is gone: it calls utils/hitArea.ts directly
(resolveHitTarget for a finger's press, watchHitArea with writeHitInsets
for its hit layer), and its tracking takes a press only on a toggle or on
the group itself, so any other member keeps its own click.
vjeux added a commit to vjeux/astryx that referenced this pull request Oct 27, 2026
…n the engine's tap path) into the toggle group's tracking
vjeux added a commit to vjeux/astryx that referenced this pull request Oct 27, 2026
…n the engine's tap path) into the toggle group's tracking

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested: under the touch presentation, ButtonGroup's slide-and-lift, its 44 px finger targets and the unchanged mouse press match the action-groups family, the record's compatibility lines are approved at this head, and the duplicated pointer list from the last review is gone. One fix remains:

  • When the group's root is wider than its buttons (a flex child in a column, as in the SlideAcross story), takesPress in hooks/useGroupPressTracking.ts takes every touch press on the root and the hit layer covers the root's whole width plus its top and bottom band, so a tap beside the buttons runs nothing and its click no longer reaches the page or a clickable container; please take a press on the root only when a member's grown target holds it, keep the layer within the members' extent, and add a test for a root wider than its members.

[Automated review]

if (box.width <= 0 || box.height <= 0) {
return false;
}
return target === group || isTrackedByGroup(target as Element);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This takes every press whose target is the root, not only one on the hit layer next to a member. When the root is wider than its buttons (a flex child in a column, as in the SlideAcross story: root 358 px, buttons 180 px), a tap on the empty part of the root or in the layer's band beside it is claimed, resolveNearestAction finds no member, nothing runs, and the browser's click is cancelled. In Chromium with real touch, a click listener on the story's wrapper heard both taps on the base and neither here. family:action-groups FR5 makes the 44 px target each member's hit expansion. Suggest accepting a root press only when resolveNearestAction over the tracked members holds the point, and letting groupGrowth shrink the layer on a side where the root reaches past its edge member.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in a667721: the group takes a press on its own box only where a button's target holds it, and the layer ends at the outer buttons' targets. A new test covers a root wider than its buttons.

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested. The touch-only slide and lift, the 44 px finger targets and the unchanged desktop press hold up in the tests and in a real-touch Chromium run, and taps beside a stretched group now reach the page. Two things block landing:

  • Under the touch presentation, a tap on a disabled button that has a tooltip no longer opens its reason, because takesPress (hooks/useGroupPressTracking.ts:93-95) takes every press on a marked member and Button marks disabled members too, so the click the tooltip opens on is cancelled (component:Tooltip FR5); let that press keep its click and add a test that the reason opens.
  • At 2,078 authored lines the pull request is over the 2,000-line limit while carrying a change that stands on its own, so please move the gesture host's kept-lift selection (gestures/GestureHost.ts, utils/selectionLock.ts and its tests) into its own pull request that lands first.

Two small cleanups, not blocking: drop "The ref's cleanup stops the watch." at hooks/useGroupPressTracking.ts:150, and call trackedTargets directly instead of the growingMembers wrapper.

[Automated review]

return false;
}
if (target !== group) {
return isTrackedByGroup(target as Element);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A press on a disabled member is taken here too: Button marks disabled members (Button/Button.tsx:663-666), so a tap on a disabled button that has a tooltip is claimed, the tracker runs nothing, and the browser's click that opens the tooltip's reason is cancelled (component:Tooltip FR5). In Chromium with real touch the base opens "Nothing to copy" and this head shows nothing. Letting a press on a disabled member keep its click (decline it here, or write no mark on a disabled Button) fixes it; a case that the lift is not cancelled would cover it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in d57466c: a press on a disabled member keeps its click, so its tooltip opens its reason. A new test covers it.

// A finger's target grows toward 44 px through the hit layer, not the
// members. The watch shares the page's one `(pointer: coarse)` listener
// and grows a target only under a coarse primary pointer, so the layer
// exists only then. The ref's cleanup stops the watch.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: "The ref's cleanup stops the watch." narrates the cleanup below; the comment reads complete without it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped in d57466c.

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested. Under the touch presentation, ButtonGroup's slide and lift, its 44 px finger targets and the unchanged desktop press match the action-groups contract in the tests and in a real-touch Chromium run, and a tap on a disabled member with a tooltip opens its reason again. One thing blocks landing:

  • At 2,260 authored lines the pull request is over the 2,000-line limit without the stack exemption, because #8660 carries a different Stack: line and its head does not contain this one, and the diff still includes #9848's kept-selection commit while this pull request's own part is 2,093 lines; please either make the stack declaration hold (the identical Stack: #9848 → #8657 → #8660 line in #8660, with #8660 rebased onto this head) or land #9848 first and move one more separable part, such as the groupGrowth and checkAtPress hit-area extension with its tests, into its own pull request.

[Automated review]

@cixzhang

cixzhang commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

/approve-spec d57466c

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pull request is reviewed as part of its stack; see the review on #8660.

@cixzhang

cixzhang commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

/approve-spec 5deb446

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested. The touch-only slide, the 44 px finger targets and the unchanged mouse press follow the action-groups family record, and the ButtonGroup record edit is owner-approved at this head. Two fixes remain:

  • At 2,100 authored lines the change is over the 2,000-line limit, and the stack exemption does not apply because #8660 still declares Stack: #9848 → #8657 → #8660 and is built on this pull request's previous head, so either update #8660's Stack line and rebase it onto this head, or move the 44 px hit layer into its own pull request.
  • Miss: trackedTargets in hooks/useGroupPressTracking.ts repeats the shared tracker's measure loop in hooks/actionGroupTracker.ts, so the group's root-press and hit-layer decisions can drift from what the tracker tracks; export one measuring function from the tracker, use it in both places and delete the copy.

Non-blocking: the Chinese description in ButtonGroup.doc.mjs says the slide works on touchscreens, but it applies only under the touch presentation (a coarse primary pointer).

[Automated review]

}

/** The tracked, enabled members with a box, each box read once. */
function trackedTargets(group: HTMLElement): ActionGroupTarget[] {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Miss: this repeats the shared tracker's measure loop (hooks/actionGroupTracker.ts, the same query, filter and rect read over the same members), so takesPress and the hit layer's growth can drift from what the tracker tracks. Export one measuring function from actionGroupTracker.ts, call it here and in the tracker, and delete this copy.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 99190b1: actionGroupTracker.ts exports measureActions, which the tracker's measure and the group's trackedTargets both call; the copy is gone.

name: 'ButtonGroup',
displayName: 'Button Group',
description: '将多个按钮组合在一起,带有连接样式:共享边框、正确的圆角处理(仅外边缘),以及水平或垂直方向。',
description: '将多个按钮组合在一起,带有连接样式:共享边框、正确的圆角处理(仅外边缘),以及水平或垂直方向。在触摸屏上,按下时按钮组像菜单一样跟踪按压:按下一个按钮并滑过其他按钮,高亮随之移动。松开时所在的按钮执行,在组外松开则什么都不执行。鼠标按下与原生按钮相同。使用手指时,每个按钮的触控目标向 44 px 扩展,最多到与相邻按钮间距的一半,按钮本身不变大。组内的菜单触发器保留自己的打开方式。',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: 在触摸屏上 says touchscreens, but the slide applies only under the touch presentation (a coarse primary pointer), so a touchscreen laptop gets none. Match the English "Under the touch presentation".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 99190b1: it now reads 在触摸呈现下(主要指针为粗指针时), here and in ToggleButtonGroup's docs.

@cixzhang

cixzhang commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

/approve-spec 99190b1

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested. Under the touch presentation, ButtonGroup's slide-and-lift, its 44 px finger targets and the unchanged mouse press follow the action-groups family record, an engineering owner approved the ButtonGroup record edit at this head, and the earlier shared-measuring and Chinese-wording points are fixed.

  • The change is 2,169 authored lines, over the 2,000-line limit, and the stack exemption does not apply because #8660's Stack line is not identical to this one and #8660 is built on this pull request's previous head; rebase #8660 onto this head with the identical Stack: #8657 → #8660 line, or move the 44 px hit layer into its own pull request.

CI skipped every job on this draft, so its unit, browser and lint lanes have not run at this head.

[Automated review]

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pull request is reviewed as part of its stack; see the review on #8660.

@cixzhang

cixzhang commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

/approve-spec a5704de

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested. Under the touch presentation, ButtonGroup's slide-and-lift, its 44 px finger targets and the unchanged mouse press follow the action-groups family record, and the ButtonGroup record edit is owner-approved at this head. One item blocks:

  • At 2,175 authored lines this pull request is over the 2,000-line limit, and the Stack: #8657 → #8660 declaration no longer holds because #8660 is built on this pull request's previous commit; rebase #8660 onto this head, or move the 44 px hit layer (groupGrowth, checkAtPress, groupHitArea.stylex.ts and their tests) into its own pull request.

[Automated review]

@astracat-bot astracat-bot Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pull request is reviewed as part of its stack; see the review on #8660.

@astracat-bot astracat-bot Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested at this head when the review began: ButtonGroup's touch slide-and-lift and its 44 px finger targets follow the action-groups family rules, and the unit tests prove them; one scope item remained.

  • The pull request is 2,213 authored lines, over the 2,000-line limit, and the stack exemption did not apply because #8660 was built on this pull request's previous commit; rebase #8660 onto this head, or move the shared hit-area and action-group tracker extensions into their own pull request.

#8660 has since been rebased onto this head, which restores the stack: this pull request is now judged in the stack review on #8660, and it lands only once that review approves it. CI has not run on this head because the pull request is a draft.

[Automated review]

@cixzhang

cixzhang commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

/approve-spec 34e0781

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pull request is reviewed as part of its stack; see the review on #8660.

@cixzhang

cixzhang commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

/approve-spec f738b8d

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pull request is reviewed as part of its stack; see the review on #8660.

@cixzhang

cixzhang commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

/approve-spec 2387032

@astracat-bot astracat-bot Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pull request is reviewed as part of its stack; see the review on #8660.

@astracat-bot astracat-bot Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested. Under the touch presentation, ButtonGroup's slide-and-lift, its 44 px finger targets and the unchanged mouse press follow the action-groups family record, and the ButtonGroup record edit is owner-approved at this head; one item blocks:

  • Regression: the group's own hit-area callback ref (packages/core/src/hooks/useGroupPressTracking.ts:140-173) and packages/core/src/utils/groupHitArea.stylex.ts duplicate main's useTouchHitArea and touchHitAreaStyles.layer, which landed after this code was written (architecture:react-component-runtime INV16), and the copy already drifts: under a coarse pointer a group given an inline callback ref drops its hit layer, position: relative and isolation: isolate for a frame on every parent re-render, so please give useTouchHitArea the growth and checkAtPress options and call it once, or say in the description why it cannot serve the group.

The size is not a blocker while the #8657 → #8660 stack holds, and CI's build and test jobs have not run on this draft head yet.

[Automated review]

// and grows a target only under a coarse primary pointer, so the layer
// exists only then.
const [isGrowing, setIsGrowing] = useState(false);
const hitAreaRef = useCallback(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Miss: this callback ref and utils/groupHitArea.stylex.ts repeat the shared useTouchHitArea (utils/touchHitArea.ts) and touchHitAreaStyles.layer (architecture:react-component-runtime INV16). They have already drifted: the shared hook keeps one watch when a re-render hands the element a new merged ref, so its band never drops. Here a caller's inline ref makes the cleanup run setIsGrowing(false), so the root loses the band, position: relative and isolation: isolate until the next measure frame, and the page is re-read. Please extend useTouchHitArea with the growth and checkAtPress options watchHitArea now takes and call it once here, or say why it cannot serve the group.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in a0f9257: useTouchHitArea takes growth and checkAtPress options, the group calls it once, and groupHitArea.stylex.ts is gone. A new case covers the inline-ref re-render.

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approval waits on one engineering-owner decision. The touch-only slide, the 44 px finger targets and the unchanged mouse press otherwise hold up, and the earlier duplicate hit-area finding is fixed: the group now calls the shared touch hit area once.

What needs to happen next:

  • An engineering owner approves or rejects the reworded component:ButtonGroup compatibility lines at this head. They are the only spec authority for the group root becoming position: relative under the touch presentation, which changes where an absolutely positioned child of the group places for touch users. The earlier approval covered the previous wording. To approve, an engineering owner (the author is one) comments /approve-spec <full head SHA> on this pull request.

No blocking code finding remains. One non-blocking comment suggestion is inline, on a line an earlier review missed. #8660 does not contain this head yet, so this pull request was reviewed on its own.

[Automated review]

): GroupPressTracking {
useActionGroupTracking(ref, {
itemSelector: MEMBER_ELEMENTS,
// A disabled member is the tracker's neighbour, measured once per press.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Miss (non-blocking, comment style): this line sits above an isItem that also drops a hidden member by reading its box, and each tracked member's box is read again by measureActions, so "measured once per press" does not describe the code below. Suggested: // A disabled member is the tracker's neighbour. A hidden member has no box, so it is no item.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taken word for word in 46240e9.

Under the touch presentation, ButtonGroup's buttons are an action group
for a finger and a pen. The button under the pointer highlights at once,
the highlight follows it, and the lift runs the button under it; a lift
outside the group runs nothing. A finger picks the nearest button whose
44 px target holds it, through a hit layer on the group, so small
buttons don't grow. A mouse press acts as a native button's, as before.
A menu trigger keeps its own press and opening, and the keyboard is
unchanged. After a slide, focus does what a click on the lifted button
does.
@cixzhang

cixzhang commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

/approve-spec 46240e9

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved pending CI: CI has not run on this head yet.

Under the touch presentation, ButtonGroup's slide-and-lift, its 44 px finger targets and the unchanged mouse press hold up, and the hit layer now sits below the buttons, so each button keeps the pixels on its own edges. An engineering owner approved the component:ButtonGroup record edit for this head, which settles the touch-only position: relative root.

Next steps:

  • Mark the pull request ready for review; because the branch lives in a fork, a maintainer then adds the ci:run label so CI runs.
  • Rebase #8660 onto this head: it is still built on the previous commit, so this pull request was reviewed on its own.

No blocking finding remains.

[Automated review]

@cixzhang

cixzhang commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

/approve-spec 46240e9

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. CI needs a rerun before merging: one WebKit interaction shard failed on a motion animator test this pull request does not touch.

Under the touch presentation, ButtonGroup's slide-and-lift, its 44 px finger targets and the unchanged mouse press hold up. An engineering owner's spec approval covers the component:ButtonGroup record edit at this head, which settles the touch-only position: relative root. The latest commit only merges main with no conflict, and the touched tests and Core's typecheck pass on the merged tree.

Next steps:

  • Rerun the failed WebKit interaction shard. It failed in packages/core/src/motion/__tests__/animator.interaction.browser.spec.ts (the busy-spring proof), and every ButtonGroup browser case passed in WebKit and Chromium.
  • #8660 is still built on this pull request's previous commit, so the Stack: line does not hold yet. Rebase #8660 onto this head.

No blocking finding remains.

[Automated review]

This branch was successfully deployed

1 active deployment
Preview — 9f9d25d1 Deployed Nov 5, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:run A maintainer lets this outside contributor's pull request run CI (AST-030 FR17) CLA Signed This label is managed by the Meta Open Source bot. needs:design-review Affects visuals — Design should review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants