Accessibility improvements, Keyboard navigation in Safari - #600
Open
eschmar wants to merge 13 commits into
Open
Conversation
|
Thanks 🙏 |
Base UI's useButton applies tabIndex 0 to everything it renders and ours leaned on the native default, which Safari does not honour: with its default keyboard setting it walks text fields and links and skips a <button> that carries no explicit tabindex. Every trigger in the library was unreachable by Tab there, while the Base UI pendants beside them were fine. The switch and the checkbox already worked, because a span with a role has to declare one. button.Button covers the triggers composed from it, and the accordion and select triggers render their own button. Each keeps a caller's tabindex when Attributes carries one, the way data-slot already does. The anchor branch of button.Button is left alone: links are in Safari's tab order already.
Opening moved focus into the popup and the arrow keys took it from there, except in Safari, where focus stayed on the trigger and the arrows then did nothing — the menu and the select opened and were immediately dead to the keyboard. The popover had the same gap: its panel never took focus there. Safari refuses focus() for a short and variable window after it activates a button or shows a popover. floating-ui-react's enqueueFocus queues a single animation frame and that suffices for the reference, because it focuses from a React effect, already a task later than the handler that opened the popup. Ported here it is a race: a microtask plus a frame, and a task plus a frame, both land inside the window often enough to strand focus on the trigger. So enqueueFocus keeps the reference's shape — one call site per popup, the shouldFocus guard that stops a popup which closed meanwhile from pulling focus back — and retries across a few frames instead of once, stopping the moment it takes. That is immune to how long the window is. Chromium succeeds on the first attempt. Enter and Space stay on the detail-0 click a native button synthesises, the way useClick and useListNavigation split it in the reference; only the arrows are taken on keydown.
…ase ui The list rendered three plain buttons over three plain divs, so a screen reader announced unrelated controls followed by loose text. Tabs.List, Tabs.Tab and Tabs.Panel bring the tablist, tab and tabpanel roles, aria-selected, the panel each tab controls and the roving tabindex; tabs.js moves the state attributes with the visual ones. The aria-controls pair derives from the tabs id and the tab's value rather than a generated id, so an htmx swap of one half keeps it.
useTabsList binds them and ours bound nothing, so reaching the last tab meant tabbing through every one before it. The arrows follow the orientation and the writing direction, Home and End jump to the ends, disabled tabs are skipped and the movement wraps. Moving focus does not activate. That is Base UI's activateOnFocus=false default, and the right one here: a panel is free to load its content when it becomes active, so selecting on every keystroke would cost a request per arrow press. Enter and Space activate, through the click a native button already fires, and the roving tabindex follows the focus so tabbing away and back returns to where the user was. ListProps.ActivateOnFocus ports the opt-in for the other behaviour.
aria-expanded was there but nothing tied the button to the content, so the state change was announced with no way to reach what it revealed. Accordion.Trigger brings aria-controls and Accordion.Panel the region role labelled by its trigger; the item resolves an id once and the two halves derive the pair from it, the way collapsible shares its id.
The popup carried no accessible name, so it was announced as a bare menu. The menu-button pattern names it after its trigger, which had no id to point at; the id derives from the one Root, Trigger and Content already share.
It only had one when the caller passed TriggerProps.ID to point a label at it, which left the control opening the listbox unaddressable everywhere else. The fallback derives from the id the root already shares, so a caller-supplied id still wins and keeps its label. The listbox popup stays unlabelled: naming it after the trigger would mean the content resolving an id only Trigger can know, and base ui leaves it unnamed too.
It was the only disclosure trigger in the set without one, so a caller wiring their own aria-describedby had nothing to point at. Derived from the id Root, Trigger and Content already share, like dropdownmenu.
Arrowing the list moved data-highlighted and scrolled the option into view but announced nothing. Focus stays on the input in this pattern, so aria-activedescendant is the only channel there is and the arrows were silent without it. Items derive an id from the combobox id and their value, and setHighlight points the input at it — there rather than in the key handler, so hover, auto-highlight and open-with-selection get it too. Select keeps moving real focus onto the option instead, the other pattern the authoring practices allow and the one base ui uses.
The menu-button pattern opens on ArrowDown with the first item focused and on ArrowUp with the last, which is how you reach the end of a long menu in one keystroke; ours only answered click, Enter and Space and let the arrows scroll the page. open's focusFirst flag becomes focusOn, and itemsIn already filters disabled items so ArrowUp lands on the last enabled one. Opening consumes the key: the popup is open by the time the navigation handler runs in the same dispatch, so without stopImmediatePropagation it walks the highlight a second time and ArrowDown lands on the second item.
ArrowDown on the last item stopped dead and ArrowUp on the first did the same, so reaching the other end of a long menu meant arrowing all the way back. Menu.Root defaults loopFocus to true in the reference, and our own tabs list already wraps, so the menu was the odd one out twice over. The context menu carried an identical copy of moveFocus and the identical gap. Disabled items stay out of the walk. The reference marks them aria-disabled and lets the arrows land on them; ours are natively disabled buttons, which cannot take focus at all, and changing that is a wider decision than this commit.
The same gap the dropdown menu, the select and the popover had: opening moved focus into the popup and the arrow keys took it from there, except in Safari, where focus stayed on the trigger and the menu was dead to the keyboard from the moment it appeared. It was missed the first time because the context menu is not one of the components the review harness renders, so nothing exercised it. Same enqueueFocus port as the other three. The combobox was checked for this too and needs nothing: it keeps focus on the input and moves aria-activedescendant instead, so it never asks Safari for the focus it would refuse.
The submenu arrived with this release carrying role=menu and nothing to name it, the same gap the top level menu had: a screen reader announces "menu" with no indication of which item opened it, which is the one thing that tells you where you are once you are two levels deep. Sub now generates the id its two halves share, the way DropdownMenu already does for the root, SubTrigger renders it and SubContent points aria-labelledby at it. Callers who pass their own SubTriggerProps.ID keep it.
eschmar
force-pushed
the
a11y-conformance
branch
from
August 15, 2026 10:42
3ac1b1d to
e7d11b9
Compare
Author
|
Rebased and included the new submenu |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First off, really like your approach here, so nice that this project exists. Thanks!
Accessibility fixes for a number of components from the following agent loop:
Changes:
<button>carrying no explicittabindex. Base UI'suseButtonappliestabIndex 0to everything it renders, while shadcn-templ relied on the native default (none).tabs.ListProps.ActivateOnFocus(ports Base UI's activateOnFocus, default false).Deliberate differences from Base UI left as-is:
aria-disabledand lets the arrows land on them; shadcn-templ's are nativelydisabledbuttons that cannot take focusTriggercan know, which would take JavaScript, and the reference leaves it unnamed too.Menu.RootdefaultsloopFocusto true,Select.Roothas no such prop.Tests before/after
Ten of the eleven Chromium failures are checks shadcn/ui on Base UI fails too. The exception is one React Aria helper that asserts the combobox listbox leaves the DOM when it closes, server-rendered components hide instead. With these changes, Safari and Chromium should behave identically. The 3 tests failing additionally in Safari are due to the test tooling, playwright chromium vs. webkit.
Full disclosure, code by Claude Opus 5, review/verification by me. Hope this is helpful!