Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
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: No accessibility violations detected. Visual Regression55 of 502 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 35 more. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks — this is a focused, well-evidenced addition, and no code changes are being requested here.
Approval is waiting on a maintainer spec decision. MultiSelector's component record is currently a draft, and this change adds the footer as proposed FR11/DEC-3 pending owner review. A maintainer needs to adopt that footer contract as the governing current spec — caller content under the list and outside the listbox/grid in both presentations, Tab continuing into its controls with popover Tab-past-last and Escape closing, and the untargeted Panel footer — or identify another governing current record for it.
For context, the behavior itself checks out: the new footer unit tests, the full MultiSelector and consumer-doc suites, and real-browser walks of Tab, Shift+Tab, Escape, Tab-past-last, and clicking the footer control all pass, the no-footer default is unchanged, and no concrete defect was found. The remaining step is that maintainer decision.
[Automated review]
414729b to
02ce4db
Compare
There was a problem hiding this comment.
Approval is waiting on a maintainer. MultiSelector's component record is still a draft, so a maintainer needs to adopt the proposed footer contract (FR11/DEC-3, the untargeted Panel footer, and FR10's narrowing to "no control within grid rows is a tab stop") as current, or point to another governing spec. They also need to confirm whether AST-058's "no Tab stop inside the popup" wording covers only option actions. Separately, two keyboard issues need fixes and regression tests:
- One Escape pressed inside a menu opened from a footer control closes both the menu and the panel, because the footer's own Escape handling runs before the shared dismissal stack, which already closes the panel from footer focus on its own.
- Without
hasSearch, Shift+Tab from the footer's first control stays in the footer instead of returning to the trigger, because the popover's focus trap wraps it, so keyboard users must close the panel to get back to the options.
[Automated review]
cixzhang
left a comment
cixzhang
left a comment
There was a problem hiding this comment.
Owner ruling, cixzhang, 2026-10-13: with a footer, the panel holds a control that is not an option, so it has to be a dialog, and Tab has to wrap. The footer's placement outside the listbox is right; the popup around it still claims to be a plain listbox.
Today the trigger says aria-haspopup="listbox" (or grid) and its aria-controls names the listbox. A screen-reader user is promised a list of options, then Tab lands on "Manage labels". The same rule landed for DropdownMenu in component:DropdownMenu DEC-10 and AR5.
What the spec and code should say, in component:MultiSelector FR11 and DEC-3:
- Dialog with a footer: the pointer popover is a
role="dialog"witharia-modal="true", named by the component's label, holding the search row, thelistboxorgrid, and the footer. The listbox keeps only options. - Trigger:
aria-haspopup="dialog", andaria-controlsnames the popover that holds the dialog. With search,aria-controlson the search field keeps naming the listbox. - Tab wraps: Tab and Shift+Tab cycle through the dialog's stops (search field, its clear button, the listbox when it is a stop, and the footer's controls) and wrap at either end. Do not close the panel on Tab past the last footer control. Escape and a press outside close the panel and return focus to the trigger. Hidden controls are not stops.
- No search plus footer: the trigger is a select-only combobox using
aria-activedescendant. A dialog means focus moves into the popup, so with a footer the panel switches to that model: focus moves into the dialog on open (to the listbox, which takes a Tab stop), and the trigger stops owning the arrow keys while the panel is open. - No footer: nothing changes. The bottom sheet is already a dialog; the footer goes inside it, with no second dialog.
Please also record the APG Dialog (Modal) parts it adopts in the accessibility contract, under spec:AST-020/FR15, as DropdownMenu AR5 does.
02ce4db to
d67f1de
Compare
|
Thanks. Applied in d67f1de, on top of a rebase onto main (80e8be2).
Tests: I rewrote the footer suite, and 5 of its 9 cases fail on the previous head. The Chromium spec now walks the dialog with a second story for the no-search case, and 3 of its 4 tests fail on a build of the previous head. 1276 related unit tests, the MultiSelector Chromium specs, typecheck, |
There was a problem hiding this comment.
Thanks for applying the dialog ruling. Approval now waits on a maintainer to record the footer contract as current (or point to another current spec that governs it), because MultiSelector's spec record is still a draft and a public addition is accepted only against current authority; that decision should also settle the two points you left open (Tab wrapping inside the bottom sheet, and the no-search trigger keeping its combobox role). Separately, two issues need fixes:
- One Escape inside a menu opened from a footer control closes both the menu and the panel, because the footer's own Escape handler runs before the shared dismissal stack; removing that branch fixes it, and Escape from a plain footer control still closes the panel.
- Without search, the listbox that now receives focus is announced by the trigger's visible text (such as "Select…") instead of the field's label; naming it with
label, as therenderTriggerpath already does, fixes it.
[Automated review]
d67f1de to
ff46fd3
Compare
There was a problem hiding this comment.
Approval is waiting on a maintainer: component:MultiSelector is still a draft record, so the footer contract this PR writes into it (FR11, AR1, DEC-3 and the narrowed FR10) needs to be recorded as current, or another current spec named that governs it, including whether Tab wraps inside the bottom sheet and whether the no-search trigger keeps its combobox role. Separately, two keyboard and screen-reader issues need fixes, each with a regression test:
- One Escape inside a menu opened from a footer control closes both the menu and the panel, because the footer's own Escape branch in
handleFooterKeyDownacts on a key the menu already handled; deleting it leaves Escape to the shared dismissal stack, and Escape from a plain footer button still closes the panel. - Without search, the listbox that receives focus is named by the trigger's text ("Select…") instead of the label; naming it with
aria-label={label}whenever it owns the keyboard, as therenderTriggerpath already does, fixes it.
[Automated review]
| if (event.key === 'Escape') { | ||
| onKeyDown(event); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This branch also receives every Escape that bubbles from a layer a footer control opens (React events bubble through portals), so one Escape inside a footer DropdownMenu closes the menu and the panel; overlay-dismissal FR2 says one request affects one surface. With these four lines removed, the first Escape closes only the menu, Escape from a plain footer button still closes the panel through the shared stack, and the footer suite still passes. Consider deleting the branch and adding a regression with a DropdownMenu in the footer.
There was a problem hiding this comment.
Removed in 0f9d691: Escape is left to the shared dismissal stack, which still closes the panel from a plain footer control. New case "an Escape inside a menu a footer control opens closes only that menu" (a DropdownMenu in the footer) fails at the previous head.
| @@ -2345,13 +2470,30 @@ export function MultiSelector<T extends MultiSelectorOptionType>({ | |||
| : listboxLabelProps)} | |||
There was a problem hiding this comment.
With a footer and no search, this listbox now takes focus (listboxOwnsKeyboard), but here it keeps aria-labelledby on the trigger, and Chromium names it from the trigger’s text: “Select…”, then “1 selected”, while the dialog is “Labels”. Naming it with aria-label={label} whenever listboxOwnsKeyboard is true, as the renderTrigger path and the no-search sheet do, and asserting getByRole('listbox', {name: 'Labels'}) in PanelFooter.a11y.chromium.spec.ts would fix it.
There was a problem hiding this comment.
Fixed in 0f9d691: whenever the listbox takes focus itself, it carries aria-label={label}. The no-search unit case and PanelFooter.a11y.chromium.spec.ts assert the name "Labels", before and after a pick changes the trigger's text.
ff46fd3 to
0f9d691
Compare
There was a problem hiding this comment.
Approval is waiting on a maintainer: component:MultiSelector is still a draft record, so the footer contract this PR writes into it (FR11, AR1, DEC-3 and the narrowed FR10) needs to be recorded as current, or another current spec named that governs it, including whether Tab wraps inside the bottom sheet and whether the no-search trigger keeps its combobox role. Separately, one keyboard issue needs a fix and a regression test: the footer's Tab handler also takes Tab presses from a layer a footer control opens, so Tab from the last control of a Popover opened from the footer moves focus to the picker's search field and leaves that popover open; returning early when the key comes from inside a popover or dialog nested in the footer, as the Escape fix did for menus, would resolve it.
[Automated review]
| | FR9 | With `hasSearch` and `hasCreate`, once options have loaded, a `Create "<query>"` row leads the list when the trimmed query equals no option label under the search's own case-insensitive matching. Picking it, or Enter with nothing highlighted, calls `onChange` with the query appended to the value and a `{type: 'create', query}` descriptor, clears the search, and announces the creation; every other change passes no descriptor. The row has no checkbox, is never selected, is excluded from select-all and from `renderOption`, and is announced instead of the empty message when it is the only result. While `isLoading`, no row is offered and Enter commits nothing. `hasCreate` without `hasSearch` warns in development and offers nothing. | `spec:AST-056` FR5 / DEC-3, `component:Tokenizer` `hasCreate`, docs, and tests | Proposed; awaiting owner approval | | ||
| | option action | `action` absent, present | The option's secondary control as one node; the popup is a grid of rows pairing option and action | Pointer popup and Touch sheet | absent | Caller | additive | `null` is none; a control without a name warns in development | | ||
| | FR10 | Once any declared option — at any depth of sections, before any query filters it — carries an `action`, the popup is a `role="grid"` and stays one while mounted; the trigger advertises `aria-haspopup="grid"` (the bottom sheet, and a popover with a footer (FR11), keep `dialog`). Each option, the select-all row included, is a `role="row"` carrying the option's id, `aria-selected` and `aria-disabled`, with exactly two `gridcell` children: the option's content, then its action or an empty cell. A section is a `rowgroup` named by its title. Pressing the option cell toggles; pressing the action fires the caller's control and nothing else. Up/Down move rows onto the option cell; the inline-end arrow moves to the action cell and the inline-start arrow back, following visual direction under RTL; Enter on the action cell activates the control. `aria-activedescendant` names the row or the action cell. No control within grid rows is a tab stop. | `spec:AST-058` FR1–FR8, AR1–AR4 | Proposed; awaiting owner approval | | ||
| | FR11 | With `footer`, the shared panel content ends with a divider and the footer, outside the listbox or grid and outside the list's scroll container, in both presentations. In the Pointer popup the popover is then a `role="dialog"` with `aria-modal="true"`, named by `label`, holding the search row, the listbox or grid, and the footer; the listbox or grid holds only options. The trigger, the caller's control included, says `aria-haspopup="dialog"` and its `aria-controls` names the popover that holds the dialog; with search, the search field's `aria-controls` still names the listbox. Focus moves into the panel as it opens: to the search field with search; otherwise to the listbox, which takes a Tab stop and owns the arrow, Enter, Escape, and typeahead keys, while the trigger keeps its combobox role but no longer carries `aria-activedescendant`. In both presentations Tab and Shift+Tab cycle through the panel's stops (the search field, its clear button while it shows, the listbox when it takes a stop, then the footer's controls) and wrap at either end; Tab never closes the panel, a control that is hidden or under a hidden ancestor within the panel is not a stop, and with no other stop focus stays where it is. Escape from anywhere in the panel and a press outside close it, and focus returns to the trigger. A press inside the footer is not a light dismiss. The bottom sheet, already a dialog, holds the footer with no second dialog. Without `footer`, nothing changes: no dialog, a `listbox` or `grid` popup, and Tab closes the panel. | DEC-3; `spec:AST-020` FR15; docs and tests | Proposed; awaiting owner approval | |
There was a problem hiding this comment.
FR11 gives the list that takes focus without search a Tab stop and the keys, but never says what names it. The code names it with aria-label={label} and both test suites assert "Labels"; saying so here keeps a later change from falling back to the trigger's text ("Select…").
There was a problem hiding this comment.
Done in 9811324: FR11 says the listbox that takes focus without search is "named by label".
There was a problem hiding this comment.
Approval is waiting on a maintainer spec decision: component:MultiSelector is still a draft record, so a maintainer needs to record the footer contract this PR writes into it (FR12, AR1, DEC-4 and the FR10 edit) as current, or point to another current spec that governs it. That decision should also settle whether Tab wraps inside the bottom sheet, whether the no-search trigger stays a combobox, and whether spec:AST-058 FR2 and FR5 need a footer exception for pickers whose options carry actions. No code changes are requested: the footer behaves as described in real Chromium, the touched test suites and CI pass, and cixzhang's changes-requested review stays open until they review again.
[Automated review]
4180d79 to
110d0ca
Compare
There was a problem hiding this comment.
Approval is waiting on a spec decision: the footer contract exists only as proposed FR12, AR1, DEC-4 and an FR10 edit in the draft component:MultiSelector record, so an engineering owner needs to approve it by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner) or record it as current, including whether Tab wraps inside the bottom sheet and whether the no-search trigger stays a combobox. For pickers whose options carry actions, a footer also departs from spec:AST-058 FR2 (a grid popup) and FR5 (Tab keeps its meaning), so that record needs a matching amendment for a picker with a footer. No code changes are requested: the footer behaves as described in real Chromium and the touched suites pass, and cixzhang's changes-requested review stays open until they review again.
[Automated review]
| | FR8 | With `renderTrigger`, the caller-rendered control receives `ref`, `id`, `onClick`, `onKeyDown`, `onFocus`, `aria-haspopup`, `aria-expanded`, `aria-controls`, and `aria-busy`; the panel is anchored to it, focus returns to it on close, the listbox is named by `label`, and the listbox takes focus and owns the keyboard on open. No Field, status, clear action, spinner, or indicator renders. `handleRef` exposes `open`, `close`, `toggle`, `isOpen`; `onOpenChange` fires on every open and close. | DEC-1, docs, and tests | Proposed; awaiting owner approval | | ||
| | FR9 | With `hasSearch` and `hasCreate`, once options have loaded, a `Create "<query>"` row leads the list when the trimmed query equals no option label under the search's own case-insensitive matching. Picking it, or Enter with nothing highlighted, calls `onChange` with the query appended to the value and a `{type: 'create', query}` descriptor, clears the search, and announces the creation; every other change passes no descriptor. The row has no checkbox, is never selected, is excluded from select-all and from `renderOption`, and is announced instead of the empty message when it is the only result. While `isLoading`, no row is offered and Enter commits nothing. `hasCreate` without `hasSearch` warns in development and offers nothing. | `spec:AST-056` FR5 / DEC-3, `component:Tokenizer` `hasCreate`, docs, and tests | Proposed; awaiting owner approval | | ||
| | option action | `action` absent, present | The option's secondary control as one node; the popup is a grid of rows pairing option and action | Pointer popup and Touch sheet | absent | Caller | additive | `null` is none; a control without a name warns in development | | ||
| | FR10 | Once any declared option — at any depth of sections, before any query filters it — carries an `action`, the popup is a `role="grid"` and stays one while mounted; the trigger advertises `aria-haspopup="grid"` (the bottom sheet, and a popover with a footer (FR12), keep `dialog`). Each option, the select-all row included, is a `role="row"` carrying the option's id, `aria-selected` and `aria-disabled`, with exactly two `gridcell` children: the option's content, then its action or an empty cell. A section is a `rowgroup` named by its title. Pressing the option cell toggles; pressing the action fires the caller's control and nothing else. Up/Down move rows onto the option cell; the inline-end arrow moves to the action cell and the inline-start arrow back, following visual direction under RTL; Enter on the action cell activates the control. `aria-activedescendant` names the row or the action cell. No control within grid rows is a tab stop. | `spec:AST-058` FR1–FR8, AR1–AR4 | Proposed; awaiting owner approval | |
There was a problem hiding this comment.
With a footer this row makes a grid picker's trigger say aria-haspopup="dialog", and FR12 adds Tab stops inside the panel. Current spec:AST-058 FR2 says the trigger advertises grid (only the bottom sheet keeps dialog) and FR5 keeps Tab's current meaning, with a verification row that fails "a sequential tab stop inside the panel". That record needs a matching amendment for a picker with a footer, decided with this contract.
110d0ca to
03023b5
Compare
There was a problem hiding this comment.
Approval is waiting on a spec decision: the footer contract exists only as proposed FR12, AR1, DEC-4 and an FR10 edit in the draft component:MultiSelector record, so an engineering owner needs to approve that text by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), including whether Tab wraps inside the bottom sheet and whether the no-search trigger stays a combobox. For pickers whose options carry actions, a footer also departs from spec:AST-058 FR2 (a grid popup) and FR5 (Tab keeps its meaning), so that record needs a matching amendment. Separately, one fix is needed: moveDialogTab reads the page's document.activeElement (MultiSelector.tsx:1928), so in a picker rendered into an iframe or a Picture-in-Picture window forward Tab never leaves the search field; reading panel.ownerDocument.activeElement, with a second-document regression test, fixes it. cixzhang's changes-requested review stays open until they review again.
[Automated review]
| if (stops.length === 0) { | ||
| return; | ||
| } | ||
| const active = document.activeElement; |
There was a problem hiding this comment.
This reads the page's document, so when the picker renders in a second document (a same-origin iframe or a Picture-in-Picture window, which usePopover and useFocusTrap now support) active is the <iframe> element and index is always -1: Tab from Search lands on Search again and Shift+Tab always jumps to the last stop. panel.ownerDocument.activeElement keeps the cycle in the panel's own document; a regression case beside Popover's second-document tests would pin it.
The panel was the search row, a divider and the list, so a picker could not end with a control of its own, such as a door to where its options are managed. footer renders caller content after the list behind a divider, outside the listbox or grid and the list's scroll container, in both presentations. Tab from the panel's keyboard focus moves into the footer's controls and keeps the panel open; in the popover, Tab past the last control and Escape inside it close the panel. Proposed FR11 and DEC-3 of component:MultiSelector record it; FR10's tab-stop sentence now names the grid's rows.
…wraps Owner ruling on this pull request: a footer puts a control that is not an option in the panel, so the popover is a role="dialog" with aria-modal="true", named by `label`, holding the search row, the listbox or grid, and the footer. The trigger (the caller's control included) says aria-haspopup="dialog" and its aria-controls names the popover; the search field keeps naming the listbox. Without search, focus moves into the listbox on open, which takes a Tab stop and the keys, and the trigger drops aria-activedescendant. Tab and Shift+Tab cycle through the panel's stops (search field, its clear button, the listbox when it is a stop, the footer's controls), wrap at both ends, skip hidden controls and never close; Escape and a press outside close and return focus to the trigger. The bottom sheet, already a dialog, holds the footer with no second one and wraps Tab the same way. Without a footer nothing changes. Spec: FR11 rewritten, AR1 records the APG Dialog (Modal) parts under spec:AST-020/FR15, DEC-3 carries the ruling, FR10 names the dialog popup. Tests: the footer suite rewritten (5 of 9 fail on the previous head), the Chromium spec walks the dialog (3 of 4 fail on the previous head), and a PanelFooterWithoutSearch story.
…that menu; a focused listbox is named by the label The footer's own Escape branch also received every Escape that React bubbles through the portal of a layer a footer control opens, so one Escape closed that menu and the panel. Escape is now left to the shared dismissal stack, which still closes the panel from a plain footer control. Without search, the listbox that takes focus is named by aria-label from the label, as the bottom sheet's is, instead of the trigger's text.
… with that layer The footer's Tab handler also took Tab presses from a popover a footer control opened (inside the footer's DOM, or bubbled through a portal), moving focus to the search field and leaving the popover open. It now returns early for a key from such a layer. FR11 says the no-search list is named by label.
…s inside it The outer trap's document listener was installed first and ran first, so Tab from the last control of a popover opened from a footer control (also the panel's last stop) wrapped to the panel's search field and left the popover open. A trap now leaves Tab to an active trap nested in its container that holds focus. Unit cases cover both directions and the outer trap once the inner deactivates; a Chromium case walks past the footer popover's last stop on a new story.
…ption actions Two regression cases for combinations the footer must keep: a renderTrigger picker (the caller's control says aria-haspopup="dialog" and names the popover, focus moves into the listbox, Tab wraps through the footer, Escape returns focus to the control), and options with actions (the grid sits inside the dialog, its action controls take no Tab stop, and the arrows still reach and activate them).
…mes the popover that holds the dialog FR11, the code and the owner's ruling already say the trigger's aria-controls names the popover that holds the dialog; AR1, DEC-3 and the renderTrigger paragraph said it names the dialog. Record wording only.
In the bottom sheet the trigger's aria-controls names the listbox and it keeps aria-activedescendant, since both key on the footer dialog, which the sheet is not; FR11 now scopes those two clauses to the Pointer popup. isHiddenWithin moves above the matcher's comment, so that comment sits on normalizeLabel again.
…e Pointer popup The bottom sheet is a dialog with or without a footer, so "Without footer, nothing changes: no dialog" held for the popover only; FR12 now says the Pointer popup is unchanged. Record wording only; the cell keeps its width.
03023b5 to
55738a3
Compare
There was a problem hiding this comment.
Approval is waiting on a spec decision: the footer contract exists only as proposed FR12, AR1, DEC-4 and an FR10 edit in the draft component:MultiSelector record, so an engineering owner needs to approve that text by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), including whether Tab wraps inside the bottom sheet and whether the no-search trigger stays a combobox. For pickers whose options carry actions, a footer also departs from spec:AST-058 FR2 (a grid popup) and FR5 (Tab keeps its meaning), so that record needs a matching amendment. Separately, one fix is still needed: moveDialogTab reads the page's document.activeElement (MultiSelector.tsx:1932), so in a picker rendered into an iframe or a Picture-in-Picture window forward Tab never leaves the search field; reading panel.ownerDocument.activeElement, with a second-document regression test, fixes it. cixzhang's changes-requested review stays open until they review again.
[Automated review]
Main's meta#8611 (component:Popover DEC-9) made useFocusTrap leave a Tab that starts outside its container to the page. This branch's nested-trap check runs after it: a Tab inside a trap nested in this one's container is still inside this container, so the outer trap still leaves it to the inner one. The doc keeps both sentences.
There was a problem hiding this comment.
Approval is waiting on a spec decision: the footer contract exists only as proposed FR12, AR1, DEC-4 and an FR10 edit in the draft component:MultiSelector record, so an engineering owner needs to approve that text by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), including whether Tab wraps inside the bottom sheet and whether the no-search trigger stays a combobox. For pickers whose options carry actions, a footer also departs from spec:AST-058 FR2 (a grid popup) and FR5 (Tab keeps its meaning), so that record needs a matching amendment. Separately, one fix is still needed: in a picker rendered into an iframe or a Picture-in-Picture window, forward Tab never leaves the search field, because moveDialogTab reads the page's document.activeElement (MultiSelector.tsx:1932) and the footer's Tab handler checks instanceof Element against the page (:1967); reading panel.ownerDocument.activeElement and checking the node type instead, with a second-document regression test beside Popover's, fixes it. cixzhang's changes-requested review stays open until they review again.
[Automated review]
| return; | ||
| } | ||
| const footer = footerRef.current; | ||
| const target = event.target instanceof Element ? event.target : null; |
There was a problem hiding this comment.
In a picker rendered into an iframe or a Picture-in-Picture window, event.target is an Element of that document's realm, so this instanceof Element is false for every footer control and the footer's Tab is never handled; together with document.activeElement at line 1932, forward Tab from Search never leaves the field there. Checking the node type here (nodeType === 1) and reading panel.ownerDocument.activeElement at line 1932 makes a jsdom iframe probe walk Search, Manage labels, Help, Search, as in the page.
Main's meta#8711 appended a 'MultiSelector caller aria-labelledby' suite to MultiSelector.test.tsx where this branch appends its own; the file keeps main's suite as is, then this branch's cases.
There was a problem hiding this comment.
Approval is waiting on a spec decision: the footer contract exists only as proposed FR12, AR1, DEC-4 and an FR10 edit in the draft component:MultiSelector record, so an engineering owner needs to approve that text by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), including whether Tab wraps inside the bottom sheet and whether the no-search trigger stays a combobox. For pickers whose options carry actions, a footer also departs from spec:AST-058 FR2 (a grid popup) and FR5 (Tab keeps its meaning), so that record needs a matching amendment. Separately, one fix is still needed: in a picker rendered into an iframe or a Picture-in-Picture window, forward Tab never leaves the search field, because moveDialogTab reads the page's document.activeElement (MultiSelector.tsx:1941) and the footer's Tab handler checks instanceof Element against the page (:1976); reading the panel's ownerDocument.activeElement and checking the node type instead, with a second-document regression test beside Popover's, fixes it. cixzhang's changes-requested review stays open until they review again.
[Automated review]
|
I'm pausing this work for now to reduce contention on the repo. Thank you for the reviews and the spec guidance. |
User need
What led to it: this comes from moving every menu and picker in our app onto Astryx, which began with a press, slide and lift bug in its phone menus. Our label picker needs a "Manage labels" button at the bottom of its panel, the way pickers usually end with a link to where the items are managed, but MultiSelector's panel ends at its list.
Why: without a place under the list, an app has to put that button outside the picker, away from the labels it manages.
Problem and solution fit
A new optional
footerrenders under the list, behind a divider and outside its scroll area, in both the popover and the bottom sheet. With a footer, the popover becomes a modal dialog (the owner's ruling): focus moves in on open, Tab cycles through the search field and the footer controls and wraps, and Escape or a press outside closes and returns focus. Closing the panel when a footer control acts is out of scope.Current specification
component:MultiSelector(draft): DEC-4 records the footer and the owner's dialog ruling; FR12 states the contract; AR1 names the APG Dialog (Modal) parts (spec:AST-020/FR15); FR10 notes thedialogpopup. Other proposals: #7272 FR11, #7274 FR13.Public delta and caller burden
footer; without it nothing changes.useFocusTrap: a trap nested inside another now owns Tab while focus is in it, so a popover opened from a footer button keeps its own Tab order.Design and alternatives
action. Works for one row, not for the panel.emptyText. Shows only when nothing matches.footerprop, as onCommandPaletteandSideNav(chosen). It keeps the button out of the list. The cost: with a footer the panel is a dialog that traps Tab, asDropdownMenu's header and footer already do.Evidence
Thirteen cases in
MultiSelector.test.tsxcover the dialog wiring, the Tab cycle, the sheet, Escape (also from a footer menu) and the unchanged no-footer path; they fail onmain. A Chromium test checks the dialog semantics, Tab wrapping and Escape in three stories.Writing rubric results
R1 narration: none in FR12, AR1 or DEC-4.
R2 research: none.
R3 evidence: FR12 cites DEC-4,
spec:AST-020FR15 and tests.R4 rejected alternatives: four, in DEC-4.
R5 change references: none.
R6 open questions: none.
Scope
Testing
In Storybook, open Core/MultiSelector → Panel Footer and open the picker.