Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
github-actions
Bot
added
the
needs:spec-owner-review
meta-cla
Bot
added
the
CLA Signed
|
Posted by Cindy's agent on her behalf. Thanks for the action-group work. Two review points:
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. |
…e's tap path WebKit synthesises a click only from its own tap gesture, not from a raw touch sequence (the harness's touch.ts states it), and the trigger opens on that click; the case played a raw touch script and so opened nothing in WebKit, with the group untracked too. It now taps through the driver's tap(), which reaches the click in both engines.
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This needs an engineering owner's decision on its governing spec before it can be approved, and it also needs changes. The ToggleButtonGroup tracking is governed only by records this branch proposes rather than ones already on main (family:action-groups FR1–FR5, architecture:interaction-modality INV13, and spec:AST-094 DEC-17 with its FR11, FR19 and FR24 amendments); any engineering owner, the author included if they are one, can record that decision by commenting /approve-spec <full head SHA> on this pull request, or the records can land through #8622 first. Whatever that decision, these still need changes:
- Against main this branch carries #8657's whole stack (32,863 authored changed lines, 449 of them this change), so please land the stack first and update the branch until the diff is the five ToggleButtonGroup files.
- Under a mouse, the toggle a press starts on keeps Button's
:active0.98 scale while the tracker lights another toggle, because a ToggleButton keepsstyles.pressableand the tracked look overrides onlybackground-image; the browser test checks only the overlay. ToggleButtonGroup.spec.mdstill says each member owns its press paint, and thespec:AST-094record conflicts with main.
[Automated review]
| sizeStyles[size], | ||
| isIconOnly && styles.iconOnly, | ||
| interactionOverlayStyles.backgroundImage, | ||
| isGroupTracked && trackedStyles.overlay, |
There was a problem hiding this comment.
This tracked look replaces only background-image. A ToggleButton has no ButtonGroup context, so it still gets styles.pressable (line 790), whose :active arm keeps scale(0.98) on the toggle a mouse press started on while the tracker lights another; in Chromium :active stays on that toggle until the lift. A transform arm in trackedStyles that mirrors the overlay arm (no scale for a pointer :active, scale for [data-astryx-press="on"]) would leave one pressed-looking toggle.
| const looks = await group.getByRole('button').evaluateAll(toggles => | ||
| toggles.map(toggle => ({ | ||
| press: toggle.getAttribute('data-astryx-press'), | ||
| image: getComputedStyle(toggle).backgroundImage, | ||
| })), | ||
| ); | ||
| expect(looks[0]).toEqual({press: null, image: 'none'}); | ||
| expect(looks[2].press).toBe('on'); |
There was a problem hiding this comment.
These assertions read data-astryx-press and backgroundImage only. Mid-slide, the press-start toggle's computed transform is still matrix(0.98, 0, 0, 0.98, 0, 0) from Button's :active arm, so this passes while two toggles look pressed; asserting transform on looks[0] would catch it.
… watcher test's tail fix) into the AlertDialog tracking base
…xposes no ontouchstart is taken once Chromium fixes ontouchstart when the page is created, so turning touch emulation on later sends Touch Events without it: the host reads Pointer Events for the finger and must not read its Touch Events too. A one-finger long press fails here, the finger tracked twice.
…'s tracked actions to its tracker) onto F2's fZ (meta#8360 23ade34) Conflicts, doc text only, in utils/pressFeedback.ts and its test: the header keeps the line's selection-lock and touchcancel paragraphs, then the group-item paragraph, and lists the group-item exports; the test imports both sides' names.
…from their pointer events install() reads a finger from its pointer events when the window has no ontouchstart, but the four touch handlers still took the Touch Events a browser sends anyway (Chromium after touch emulation turns on), tracking the finger twice, so a one-finger long press never recognized. Each touch handler now returns while usesTouchEvents is false, after noting the fingers down, as install()'s comment promised.
…ter events) into meta#8320
…ter events) into meta#8293
…ter events) into meta#8356
…ter events) into meta#8358
…'s selection An action group's press is a long press with no minimum duration, so it claims at the press's pointerdown, before any movement. The drag selection hold then cleared the page's selection, so a plain click on a group's button left its Copy or Cut with nothing to act on. The case fails on this head: the selection is gone after the pointerdown.
…only the toggle under the mouse Fails in Chromium and WebKit (family:action-groups FR2): with Bold keyboard-focused, a mouse press on Bold held over Underline leaves Bold with the pressed overlay (and the press scale) beside Underline's paint, as their :active:focus-visible arms still match Bold.
…pointer press The press scale's :active:focus-visible arm on a button its group tracks now skips the button a pointer press began on (data-astryx-pointer-press, from meta#8657's hook), as the tracked overlay's arms do: a mouse press that starts on a keyboard-focused toggle no longer leaves it at 0.98 beside the toggle under the pointer (family:action-groups FR2). A held Space still scales it.
…age's selection The drag selection hold cleared the page's selection when any mouse or pen contact was claimed, but an action group's press claims at its pointerdown, before it has moved or selected anything, so a plain click on a group's button left its Copy or Cut with nothing to act on. A claim before any movement now keeps the page's selection: the hold still refuses a new one, and puts the page's back over a selection that appears while it runs. A claim after movement clears what its drag selected, as before (spec:AST-094 FR41). The case before the new one now waits for its hold to end a task after the lift, so no hold outlives that test.
…t every pointer does - GestureHost's header, the gestures README and coarsePointer.ts no longer describe the coarse-only host: a finger or a pen takes the touch-down lock under any primary pointer, a kept press's finger is kept during a fine pointer's press too, and recognizers attach for every pointer while the page listeners last a press (spec:AST-094 FR19, FR24, FR41, FR43). - The fine-pointer selection case asserted the replaced rule and passed only because it read iOS WebKit's inline lock without iOS WebKit: it now asserts the lock a fine pointer's finger and pen take, and that a mouse takes none. - FR41's sentence and its verification row say that a press claimed at its own pointerdown, before it moves, keeps the page's selection, while a drag that moved before its claim loses what it selected.
…ion, in the present tense The sentence said a drag that moved before its claim loses the selection it started, but the hold clears the page's selection then, whatever made it; and an unclaimed mouse drag kept "today's" behavior, a phrase that ages. It now says a claim after movement clears the page's selection, a press claimed at its own pointerdown keeps it, and an unclaimed mouse drag keeps its released behavior.
…lection Fails in Chromium and WebKit (spec:AST-094 FR41): with text selected on the page, a mouse click on Cut runs Cut but clears the selection, as the group's press claims at its pointerdown and the gesture host's drag hold cleared the page's selection there; a Copy or Cut action in a group then has nothing to act on. meta#8665's fix keeps a selection that was there before a press claimed at its own pointerdown.
…lection) into ButtonGroup press tracking
…ion has run out Under a loaded machine WebKit read Bold at matrix(0.998567, ...): the press had just left Bold for Underline and Bold was still easing back from the press scale, so a single read landed part-way through the transition. Both reads of Bold's scale now wait until the toggle has no running transition and assert its settled look; a toggle left at 0.98 still fails.
There was a problem hiding this comment.
This can't be approved yet: at this head ToggleButtonGroup.spec.md rewrites PR1 to let the group read its toggles' boxes at each press, listen to the shared coarse-pointer query and watch its hit layer, and spec:AST-094 FR41 gains a rule for the text selection of a claimed mouse or pen drag, while the records on main allow neither. An engineering owner decides both by commenting /approve-spec <full head SHA> on this pull request (the author may do so if they are an engineering owner), and these changes are needed either way:
- Land #8657's stack first, so this diff shrinks from 32,852 authored lines to this change's 847.
- Make the root-entry case of
scripts/touch-support-bundle.test.mjspass, as #8671 proposes; the root entry now bundles the gesture modules. - Mark only toggles that have a
value(ToggleButton.tsx:372), or amend the record's AV1, which says a valueless member behaves standalone, and add a case for it.
[Automated review]
| clickAction={clickAction} | ||
| // A toggle its group tracks carries the group item marker: the group's | ||
| // tracker paints its press and runs its click (family:action-groups). | ||
| {...(group?.tracksPress === true ? pressGroupItemProps : undefined)} |
There was a problem hiding this comment.
This marks every member of a tracking group, including a ToggleButton without value. The record's AV1 says such a member "reads only the group's size and disabled state and otherwise behaves standalone", but with the marker the group takes its press: a press that starts on it and slides onto Bold toggles Bold. Marking only valued members (group?.tracksPress === true && value != null) keeps AV1; otherwise AV1 needs amending in the same record change, with a test either way.
There was a problem hiding this comment.
Thanks, fixed: only toggles with a value carry the group marker now, so a valueless toggle keeps its own press, as AV1 says. A unit case that failed first covers the slide from it onto Bold and its own tap.
|
|
||
| - **PR1 — Group listeners only for press tracking.** The group adds no | ||
| observer or measurement, and no event listener beyond the press tracking | ||
| - **PR1 — Layout reads and listeners only for press tracking.** Each press |
There was a problem hiding this comment.
This PR1 rewrite allows press-time box reads, the shared (pointer: coarse) query and a coarse-pointer hit-layer watch, where the current record says the group adds no observer or measurement. It is a normative change, so it waits on an engineering owner's /approve-spec at this head.
| */ | ||
|
|
||
| import {useEffect, useRef, useState, type RefObject} from 'react'; | ||
| import {isActive} from '../gestures/GestureRecognizer'; |
There was a problem hiding this comment.
These value imports put twelve gesture modules (54,082 minified bytes, GestureHost.ts 29,005 of them) in the root entry's bundle through ToggleButtonGroup, ButtonGroup and AlertDialog, so the unchanged root-entry case of scripts/touch-support-bundle.test.mjs fails on this head. The runtime architecture allows the static import; the pin needs updating, as #8671 proposes.
… a tracking group Fails: the group marks every member, so a ToggleButton without a value carries the group item marker and the group takes its press; a finger that starts on it and slides onto Bold toggles Bold. The record's AV1 says such a member behaves standalone.
…item marker A ToggleButton without a value reads only the group's size and disabled state and otherwise behaves standalone (the record's AV1), so its press stays its own: the group no longer takes a press that starts on it, and a slide from it onto Bold toggles nothing.
There was a problem hiding this comment.
This can't be approved yet: ToggleButtonGroup.spec.md rewrites PR1 to let the group read its toggles' boxes at each press, listen to the shared coarse-pointer query and watch its hit layer, and spec:AST-094 FR41 gains a rule for the text selection of a claimed mouse or pen drag, while the records on main allow neither. An engineering owner decides both by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), and these changes are needed either way:
- Land #8657's stack first, so this diff shrinks from 32,902 authored lines to this change's 897.
- Make the root-entry case of
scripts/touch-support-bundle.test.mjspass, as #8671 proposes; the root entry now bundles the gesture modules. - Count members the group does not track as neighbours in the finger resolution (
hooks/useActionGroupTracking.ts:148-178): with 32 px toggles a finger lifted 2 px inside a valueless toggle toggles the valued one beside it, againstspec:AST-094FR42, so add a case for it too.
[Automated review]
| {...(group?.tracksPress === true && value != null | ||
| ? pressGroupItemProps | ||
| : undefined)} |
There was a problem hiding this comment.
With this, a toggle without value is no longer a neighbour in the finger resolution: resolveNearestAction (hooks/useActionGroupTracking.ts:148-160) weighs only marked toggles, so with 32 px icon toggles and the 4 px gap a finger that starts on Bold and lifts up to 2 px inside a valueless toggle beside it toggles Bold (4 px at 28 px). That runs against spec:AST-094 FR42 ("A hit-area expansion never overlaps a neighboring interactive element's own target"). Counting the group's untracked buttons as neighbours, as the disabled toggles are, keeps AV1 and FR42; a 32 px case with a lift 1 px inside the valueless toggle would cover it.
There was a problem hiding this comment.
Thanks, fixed in #8657's finger resolution: members the group doesn't track (a toggle without a value; in a ButtonGroup, a menu's trigger) now count as neighbours, like disabled ones, so no grown target reaches into one (FR42). 32 px cases with a lift 1 px inside such a member failed first in both groups.
…ger runs nothing Fails: the finger resolution weighs only the tracked buttons and the disabled ones, so Cut's target, grown toward 44 px, reaches 2 px into a 32 px menu trigger 4 px away; a finger slid from Cut and lifted 1 px inside the trigger keeps Cut lit and runs it, against spec:AST-094 FR42 (a grown target never overlaps a neighbour's own).
…s neighbours A member the group does not track (a menu's trigger in a ButtonGroup, a toggle without a value in a ToggleButtonGroup) keeps its own target: the finger resolution now measures the group's interactive members it does not track beside its disabled actions, so no tracked action's grown target reaches into one (spec:AST-094 FR42), and a finger lifted inside one runs nothing.
… a value toggles nothing Fails: a toggle without a value carries no group marker, and the finger resolution weighs only the marked toggles and the disabled ones, so Bold's target, grown toward 44 px, reaches 2 px into a 32 px valueless toggle 4 px away; a finger slid from Bold and lifted 1 px inside it keeps Bold lit, against spec:AST-094 FR42.
There was a problem hiding this comment.
This can't be approved yet: ToggleButtonGroup.spec.md rewrites PR1 to let the group read its toggles' boxes at each press, listen to the shared coarse-pointer query and watch its hit layer, and spec:AST-094 FR41 gains a rule for the text selection of a claimed mouse or pen drag, while the records on main allow neither. An engineering owner decides both by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), and these changes are needed either way:
- Land #8657's stack first, so this diff shrinks from 33,004 authored lines to this change's 934.
- Make the root-entry case of
scripts/touch-support-bundle.test.mjspass, as #8671 proposes; the root entry now bundles the gesture modules. - Name in PR1 and its verification row the boxes of untracked members (a toggle without
value) that a finger's press now reads, since the finger resolution counts them as neighbours.
The finger target beside a valueless toggle is fixed, with a case in each group.
[Automated review]
| observer or measurement, and no event listener beyond the press tracking | ||
| - **PR1 — Layout reads and listeners only for press tracking.** Each press | ||
| reads the group's box and each enabled toggle's box once, when it begins; | ||
| a finger's press also reads the disabled toggles' boxes then. Nothing is |
There was a problem hiding this comment.
This list now misses a read: under a finger, measureNeighbours (hooks/useActionGroupTracking.ts:178-201) also reads the box of every member the group does not track, such as a toggle without value, once per press. Naming those boxes here and in the PR1 verification row keeps the record an owner approves in step with the code.
Stack: on #8657. Review this PR alone: vjeux/astryx@uikit/c01-buttongroup-tracking...uikit/c01-togglegroup-tracking
User need
Part of the UIKit-parity work. In a toggle group (Bold, Italic, Underline) you can press one toggle and slide to another; only the one you lift on should change. Today a slide changes nothing, and small toggles are under the 44 px a finger needs.
Problem and solution fit
What led to it: #8622, now merged, makes ToggleButtonGroup a member of
family:action-groups. The group uses the same tracker and touch targets as ButtonGroup (#8657); the lift toggles only that toggle, once, in single and multiple mode.Fixed in review:
ToggleButtonGroup.doc.mjsdescribes the slide.Current specification
family:action-groupsFR1–FR5;spec:AST-020DEC-5 (a lift outside the group changes nothing).Public delta and caller burden
No API change.
onChangefires once, for the lifted toggle.Design and alternatives
Sharing ButtonGroup's tracker and finger resolver keeps the groups from drifting apart. The scale fix lives in Button, so an alert's tracked buttons get it too.
Testing
To check by hand, in the ToggleButton "Group multiple" story:
What was run: unit and real-browser tests in Chromium and WebKit; the slide, the 44 px target and the mouse look fail without this change, and each review fix has a case that fails without it. Not from this PR: the touch-bundle root-entry check (#8671), and two node tests that fail the same way on main locally (
story-tree.test.ts,release/cache.test.mjs's lock cases). These are local runs: GitHub Actions doesn't run fork pull requests right now.Evidence
Size (gzip): ToggleButton JS 46.1 → 66.6 KB (+20.5 KB), CSS +152 B; Button JS +37 B, CSS +28 B.
Scope
Writing rubric results
component:ToggleButtonGroup: PR1 names the press-time reads, the shared coarse-pointer query and the hit layer's watch; the ownership boundary names the press across members; the compatibility lines name the DOM change;verified_byand the map name the tracking tests. R1–R6 pass: present tense, no research, references, rejected alternatives or open questions.