Repository navigation
fix(MultiSelector): a search status the next keystroke replaces is withdrawn, not joined - #8640
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:spec-owner-review
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.
This pull request intentionally changes spec:AST-086 FR9, which today joins every message written before the previous one is exposed. Approval waits on an engineering owner deciding whether an owner's superseded, unspoken status may be withdrawn instead, and whether that also covers the empty-panel case and FR14's in-modal containers. Any engineering owner, the author included, records that decision by commenting /approve-spec <full head SHA> on this pull request. Separately from that decision, please address:
- Inside a scrimless BottomSheet or Drawer nested in an open Dialog, the withdrawal looks in the sheet instead of the Dialog the status was written to, so superseded statuses still join there, unlike a picker placed directly in the Dialog (
useStatusAnnouncement.ts:118). - Inside an open modal, a status that was already exposed can withdraw another owner's later identical message, because the modal withdrawal is keyed by text.
- If the amendment is approved, FR9's "none is dropped", FR14's own join sentence, the FR9 and FR14 verification rows and the current-state impact need to match it.
[Automated review]
| joined, | ||
| // Not on the shared channel: the route wrote it inside the open | ||
| // Astryx modal enclosing the caller, or holds it for that modal. | ||
| dialog: joined == null ? (modal?.dialogRef.current ?? null) : null, |
There was a problem hiding this comment.
Inside a scrimless BottomSheet or Drawer nested in an open Dialog, useAnnounce's route writes to the Dialog (the first open modal up the provider chain, enclosingOpenModal in modalAnnounce.ts), but this records the nearest provider's dialog, the non-modal sheet. The next withdrawFromModalPending finds no region there, so the superseded status still joins: a MultiSelector placed there still announces 1 result Create "Urg" Create "Urge" Create "Urgent" after a sub-frame burst. A picker placed directly in the Dialog does get the withdrawal, so the two placements disagree. If the rule is meant to apply inside modals, record the dialog the route actually wrote to (resolve it the way the route does, or have the route hand it back) and add the nested case to the hook tests; otherwise drop the in-modal withdrawal.
| withdrawPendingAnnouncement(previous.joined, previous.politeness); | ||
| } | ||
| if (previous.dialog != null) { | ||
| withdrawFromModalPending( |
There was a problem hiding this comment.
The previous status is never forgotten once it has been exposed, and the modal withdrawal is keyed by text. So inside an open modal, when another owner writes the same words in the frame where this status changes, the other owner's message is withdrawn: the region reads 2 results where the shared channel keeps 1 result 2 results. Forget the previous status once it has been exposed, or key the withdrawal to the write rather than its text.
| identical message repeated within the same frame is one message (FR6). An | ||
| empty message clears the region, including messages joined but not yet | ||
| exposed. | ||
| one is exposed are joined in order into one announcement; none is dropped. A |
There was a problem hiding this comment.
This sentence still says "none is dropped" just before the new rule withdraws a status. If the amendment stands, FR14's restated join rule (line 179), the FR9 and FR14 verification rows (lines 314 and 316) and Current-state impact need matching changes, and the empty-panel withdrawal (another announcement route) needs stating.
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsMultiSelector (@astryxdesign/core) · View in Storybook
Component Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: The accessibility audit did not run on this pull request. It runs after merges to Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
…oined (#9070) ## Exact decision needed Whether `spec:AST-086` FR9's shared channel joins a component's superseded message about its current state with its next one, and the optional public `useAnnounce` option that asks for replacement. Today every message written to a region before the previous one is exposed is joined, so a component that states its current state on every input, such as a search's result count, has all its stale states spoken together when input outpaces a frame (a barcode scanner, dictation, automation): `1 result Create "Urg" Create "Urge" Create "Urgen" Create "Urgent"`. ## Existing authority searched - `spec:AST-086` FR9 (the join), FR6 (an identical message repeated in a frame is one message), FR8 (a busy start its outcome overtakes is dropped), FR13 and FR14 (own-content messages and modal containers). - `architecture:interaction-modality` INV8: superseded work must not change the current status or announcement, with supersession left to component contracts. - `spec:AST-075` DEC-2 and FR7: Astryx's comboboxes announce their result count with `useAnnounce`, the same hook products use. Tokenizer's record: its sheet's count and empty-state text go through `useAnnounce`. MultiSelector's record (draft): its result count and create row label go through the polite channel. - `spec:AST-002` FR1, FR2, FR5, FR7, FR16 and FR20 for the new public option; `has*` booleans such as `hasClear` and `hasPaging` for its name. - Open pull requests that mention AST-086 (#7871, #7902, #7910, #8175, #8211, #8583, #8001): none edits FR9. #8640 implements the replacement for MultiSelector and waits for this decision. ## Decision A `useAnnounce` instance created with `hasReplace` states one current state of its owner, such as a search's result count: each message it writes withdraws that instance's previous message while it is not yet exposed, whatever their urgencies, and is spoken in the region of its own urgency; a message already exposed has been heard and stays. Messages from other instances, and from instances without `hasReplace`, still join. `useAnnounce` takes an optional options object with one field, `hasReplace` (default `false`), and the `announce(message, politeness)` it returns is unchanged. FR14's modal containers withdraw the same way, and a picker's own empty-panel message comes from its search's `hasReplace` instance (FR13). Astryx's search counts and MultiSelector's create row use a separate `useAnnounce({hasReplace: true})` instance (`spec:AST-075` FR7, Tokenizer's and MultiSelector's records); selection messages keep their own plain instance. DEC-4 records it; its decider is the approving engineering owner, with INV8 and FR8 as its reasons. Public option (`spec:AST-002` FR5, FR20): two otherwise identical messages, `"7 results"` written twice in one frame, need different outcomes. As a search's count it replaces the earlier one; as two distinct notices both are spoken. Only the caller knows which it sends, and the channel cannot derive it. Before: `const announce = useAnnounce(); announce(t('resultCount', {count}));`. After: `const announceCount = useAnnounce({hasReplace: true}); announceCount(t('resultCount', {count}));`. Existing calls are unchanged. ## Rejected alternatives - Each owner debouncing its own messages: every announcement waits, and every owner repeats the rule. - An exception for every `useAnnounce` message: Astryx's comboboxes and Tokenizer announce through `useAnnounce`, so their counts would keep joining. - A string key on each message (`announce(message, politeness, {currentState})`): a third argument whose strings every call must keep consistent, where one instance already states one state. ## Claim scope and non-goals FR9's replacement within one `hasReplace` instance, the option that asks for it, and the AST-075, Tokenizer and MultiSelector sentences that describe their counts' announcement. Which messages a component sends through a `hasReplace` instance stays with its own record. ## Evidence and implementation relationship - On main, the MultiSelector CreateRow accessibility spec, which types into the create row at about four keystrokes per frame, fails 9 of 10 runs alone and 26 of 48 under load, its polite region reading the joined statuses. - #8640 implements the replacement for MultiSelector. With it, the spec passes 10 of 10 alone and 48 of 48 under load. It lands after this record changes, adopting `useAnnounce({hasReplace: true})`. ## Writing rubric results - R1 narration: none, in AST-086, AST-075, Tokenizer's or MultiSelector's record. - R2 research in the record: none. - R3 evidence naming a change: none. - R4 rejected alternatives over one line or outside a decision: one, inside DEC-4, one line; nothing added to the other records. - R5 pull request or issue references remaining: none. - R6 ruled open questions still listing options: none. ## Scope - [x] This PR contains specification records only. - [x] It records observable behavior and preserves equivalent internal implementations. - [x] Any named internal mechanism is intentionally public, names its dependent caller/system, and explains why an equivalent implementation would not satisfy the contract. - [x] Verification states the evidence layer and failure signal without prescribing CI job/workflow topology. - [x] It records one intentional durable decision, not a rescue for a separable tagalong. - [x] The canonical knowledge owner, affected current claims, and overlapping open work are named; any ownership collision is resolved or called out for human decision. - [x] Adjacent API, visual, accessibility, composition, and implementation facts remain outside scope unless this exact decision depends on them. - [x] Public text and artifacts contain no internal Meta context. ## Validation Local gates at 2b84901: `pnpm check:knowledge` (records and templates aligned), Prettier, `git diff --check`, `pnpm check:repo`.
a38f5fe to
3b3c1b4
Compare
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Requesting changes: the new hasReplace option and MultiSelector's adoption match spec:AST-086 FR9, FR13 and FR14, and the new tests show the stale statuses gone, but inside an open modal the withdraw handle can drop another announcer's message. The handle returned at modalAnnounce.ts:308 is keyed by text and outlives its message's exposure, so after an instance's 1 result has been spoken, another announcer's 1 result written in the frame of that instance's next message is withdrawn: the Dialog's region reads 2 results where the shared channel keeps 1 result 2 results. Scoping the handle to the pending window it wrote into, giving held words a holder count, and adding that sequence as a test should resolve it.
[Automated review]
| deliverBeyond(dialog, joined, politeness); | ||
| }, | ||
| }); | ||
| return () => withdrawFromModalPendingRouted(dialog, message, politeness); |
There was a problem hiding this comment.
This handle never expires and is keyed by text, while the region's holder count starts fresh once its words are exposed. So after a hasReplace instance's 1 result has been spoken, another announcer's 1 result written in the frame of the instance's next message is withdrawn: the Dialog's region reads 2 results, where the shared channel keeps 1 result 2 results. FR14 lets the instance withdraw only its own previous message not yet exposed. Scoping the handle to the pending window it wrote into (for example a token that writeModalRegion keeps while the words are unexposed) would fix it. Held words (hold and withdrawFromModalPendingRouted) need a holder count for the same reason.
There was a problem hiding this comment.
Fixed in 1597836: a modal region write now returns the span it joined, and its handle takes back only words still waiting in that span, so after the count's 1 result is spoken, another announcer's 1 result written with the count's next message stays (1 result 2 results, as on the shared channel). Held words count their announcers and a handle reaches only its own entry, which is gone once delivered. Tests for the region sequence, a held share during a sheet's exit, and words held after an earlier delivery fail before the change.
3b3c1b4 to
1597836
Compare
There was a problem hiding this comment.
Requesting changes: useAnnounce({hasReplace}) and MultiSelector's adoption match spec:AST-086 FR9, FR13 and FR14, and the new tests show the stale statuses gone, including the earlier in-modal equal-words case. One path is still missing: when a modal hands off or releases an instance's unexposed status (handOffUnexposed, releaseHeld and the write-frame redirect in modalAnnounce.ts), the handle the new route returns is dropped, so the instance's next message cannot withdraw it and both are spoken (1 result 2 results in a Dialog that leaves and re-enters the accessibility tree, or across a BottomSheet's exit). Handing off each waiting message separately, letting the instance's handle follow the handle the new route returns, and adding those sequences as tests should resolve it; separately, two tests the description lists as failing on main (the in-modal equal-words and held-share guards) pass there and guard an earlier revision instead.
[Automated review]
| redirect: joined => { | ||
| deliverBeyond(dialog, joined, politeness); | ||
| }, | ||
| }); | ||
| // Only words of the span this write joined: once they are exposed, or | ||
| // redirected joined with others in the write frame, the handle has | ||
| // nothing of its own left to take. | ||
| return () => withdrawFromModalRegion(dialog, message, politeness, span); |
There was a problem hiding this comment.
This handle is bound to the span, and deliverBeyond's handle is discarded here, in handOffUnexposed and in releaseHeld. Once a modal hands off or releases the words (it closes, runs its exit, or a step is left before they are exposed), the instance's next message withdraws nothing, so 1 result 2 results is spoken where FR9 and FR14 expect 2 results (Dialog set aria-hidden and back; BottomSheet exit then close). Handing off each waiting message separately and letting this handle follow the new route's handle would close it; the three sequences make good tests.
There was a problem hiding this comment.
Fixed in 4e89f16: each write keeps a receipt per announcer beside the words, and when a modal hands them on (handOffUnexposed, releaseHeld, the write-frame redirect) they move one delivery per announcer and each receipt follows its words, so the instance's next message takes its own back there. Tests for the Dialog leaving the accessibility tree and coming back, a sheet's exit and close, the Dialog closing, a step left, and the write-frame redirect fail before the change. The description now lists the in-modal equal-words and held-share tests as guards that pass on main.
…thdrawn, not joined `spec:AST-086` FR9 joins messages written to a region before the previous one is exposed, so MultiSelector, which states its search's status on every keystroke, said every superseded status when input outpaced a frame, as a barcode scanner, dictation or automation types: `1 result Create "Urg" Create "Urge" ...`. meta#9070 records the fix. `useAnnounce` takes an optional `{hasReplace}`: an instance created with it withdraws its previous message while it is not yet exposed, whatever their urgencies, and the newer is spoken in the region of its own urgency; messages from other instances, and from instances without it, still join. Each write gives its announcer a receipt kept beside the words: in the shared region, a modal's container, words held for an exiting modal, or a message waiting for its modal to be presented. When a modal hands its waiting words on (it leaves the accessibility tree, exits, closes, a step is left, or its write frame finds it unexposed), they move one delivery per announcer, and each receipt follows its words. A receipt takes back only its own share and only while the words wait, so it never reaches equal words another announcer wrote after its own were exposed. The busy-announcement hook takes its start back through the same receipt. `useAnnounceRenderedText` can speak through an owner's announcer. MultiSelector sends its result count, its create row and its empty-panel message (FR13) through one `hasReplace` instance; selection changes keep their own. The CreateRow spec reads each search status whole again instead of accepting the joined text. Red first: on main, four statuses in one frame were all said (now the last); a count then an empty panel, and an empty panel then a count, said both (now the newer), on the page and inside a Dialog; a `hasReplace` instance's messages joined in one frame, polite then assertive, inside a modal and in the commit that presents one, and when its modal handed its first message on before it was exposed; and the tightened CreateRow spec failed 6 of 10 runs alone and 35 of 48 at 8 workers.
1597836 to
4e89f16
Compare
cixzhang
added
the
ci:run
There was a problem hiding this comment.
Recommending approval: useAnnounce({hasReplace}) and MultiSelector's adoption match spec:AST-086 FR8, FR9, FR13 and FR14, and the modal hand-off, release and write-frame paths now carry each announcer's receipt, so a status a modal hands on is taken back by the instance's next message. The new unit tests and the tightened CreateRow Chromium spec fail on main and pass with this change. What remains is for CI's remaining test shards on this head to finish green.
[Automated review]
Why
When input outpaces a frame (a barcode scanner, dictation, automation), a screen-reader user hears MultiSelector's stale statuses joined together:
1 result Create "Urg" Create "Urge" Create "Urgen" Create "Urgent".What led to it
Found while checking the text-entry components' accessibility tests (part of bringing Astryx's touch behavior to parity with iOS's UIKit, text entry first). #8244 made the announcer join every message written before the previous one is read out (
spec:AST-086FR9), so two components speaking in the same frame are both heard. MultiSelector states its search's status on every keystroke, so fast typing joins statuses that no longer apply. Its CreateRow accessibility spec failed on that, and now accepts the joined text.The record
#9070, approved by the spec owner, amends
spec:AST-086FR9, FR13 and FR14: auseAnnounceinstance created withhasReplacewithdraws its own previous message while it is not yet exposed, whatever their urgencies, and the newer is spoken in the region of its own urgency; messages from other instances still join. This pull request implements it and lands after #9070, so its public option is settled by that record, not decided here (spec:AST-002FR19).Options and tradeoffs
Why this choice
It is the shape #9070 records.
useAnnounce({hasReplace: true})is one optional field; theannounce(message, politeness)it returns is unchanged. Each write gives its announcer a receipt kept beside the words, wherever the route put them: the shared region, a modal's container, words held for an exiting modal, or a message waiting for its modal to be presented. When a modal hands its waiting words on (it leaves the accessibility tree, exits, closes, a step is left, or its write frame finds it unexposed), they move one delivery per announcer and each receipt follows its words. A receipt takes back only its own share and only while the words wait, so it never reaches equal words another announcer wrote after its own were spoken. The busy-announcement hook takes its start back through the same receipt. MultiSelector sends its result count, its create row and its empty-panel message through one such instance; selection changes keep their own. Selector, Typeahead and the others adopt it separately.How to check it
MultiSelector/CreateRow.a11y.chromium.spec.tsin Chromium. It types into the create row at about four keystrokes per frame. It now reads the two search statuses exactly, in place of thenewestMessageregex; the creation message, which comes from another announcer and can still join, keeps the regex. Expected: on main it fails on the joined statuses; with this change the polite region reads1 result, thenCreate "Urgent".Evidence
Create "Urge" Create "Urgen" Create "Urgent"; with this change it passes 10 of 10 alone and 48 of 48 at 8 workers.hasReplaceinstance's messages in one frame, polite then assertive, inside a modal and in the commit that presents one; and its first message, handed on by its modal before it was exposed (the Dialog leaving the accessibility tree and coming back, a sheet's exit and close, the Dialog closing, a step left for the next one, or its write frame finding it unexposed), is taken back by its next. Guards that pass on main and with the change: other instances and plain instances still join, a shared share stays, an exposed message stays, a selection change and a count in one frame are both said, and inside a modal an equal message another announcer writes after the count's was spoken, and another announcer's share of words held while a sheet exits, both stay.cache.test.mjslock tests that fail on this machine without the change too (fix(cli): a crashed run's release install is recognized by the Node it ran on, whatever its name #8520).