Don't focus Autocomplete menu items while menu is closed, fix SR announcement bug, and some UI fixes to stories - #8343
Conversation
… of focus staying on the input when entering the menu, which is the expected behavior and already was working correctly except in cases with overlay
🦋 Changeset detectedLatest commit: 803a0c4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Uh oh! @llastflowers, at least one image you shared is missing helpful alt text. Check your pull request body to fix the following violations:
Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.
|
|
github-actions
Bot
added
the
integration-tests: recommended
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
 |
packages/react/src/Autocomplete/Autocomplete.test.tsx — import type React from 'react' is unused in this test file and will likely fail linting (unused… |
 |
.changeset/big-corners-type.md — The changeset summary includes story-only UI tweaks, which aren’t consumer-facing and makes the… |
What changed in this PR
This PR improves Autocomplete keyboard + screen reader behavior by preventing roving focus/active-descendant updates while the menu is closed, and aligns the related Storybook feature story UI with the intended appearance.
Changes:
- Disable the menu’s
useFocusZonebehavior whileshowMenuis false to avoid focusing menu items before the menu is open. - Prevent default handling of
ArrowUp/ArrowDownon the input (capture phase) when the menu is closed to avoid external/ancestor key handlers moving focus into menu options prematurely. - Add a regression test for the “menu rendered without overlay” case, plus small CSS tweaks for the feature stories, and a patch changeset.
| File | Description |
|---|---|
| packages/react/src/Autocomplete/AutocompleteMenu.tsx | Disables focus zone when the menu is closed to prevent premature menu-item focusing/active descendant updates. |
| packages/react/src/Autocomplete/AutocompleteInput.tsx | Adds keydown capture prevention for arrow keys when closed to avoid focus being stolen by outside handlers before the menu opens. |
| packages/react/src/Autocomplete/Autocomplete.test.tsx | Adds coverage for active-descendant behavior when Autocomplete.Menu is rendered without an overlay. |
| packages/react/src/Autocomplete/Autocomplete.features.stories.module.css | Adjusts styling used by the Autocomplete feature stories (overlay input container). |
| .changeset/big-corners-type.md | Adds a patch changeset describing the user-facing behavior fix. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
🤖 Lint issues have been automatically fixed and committed to this PR. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
packages/react/src/Autocomplete/Autocomplete.features.stories.module.css — border-color/border-style have no effect here because border-width is set to 0, and the raw… |
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
.changeset/big-corners-type.md — The changeset summary includes story-only UI tweaks, which aren’t consumer-facing and makes the… View resolved comment |
|
packages/react/src/Autocomplete/Autocomplete.test.tsx — import type React from 'react' is unused in this test file and will likely fail linting (unused… View resolved comment |
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
.changeset/big-corners-type.md:5
- The changeset entry is a bit long and doesn’t follow the usual “Component: …” format, which makes the public changelog harder to scan. Consider making it a terse, consumer-facing summary starting with
Autocomplete:.
Don't focus Autocomplete menu items while menu is closed: fix SR announcement bug and menu item skipping bug
packages/react/src/Autocomplete/Autocomplete.features.stories.module.css:51
- This
border-radiuscalc hard-codes1px. Using the border width token keeps the visual intent but avoids baking in a specific pixel value.
/* stylelint-disable-next-line primer/borders */
border-radius: calc(var(--borderRadius-large) - 1px);
padding-top: var(--base-size-4);
llastflowers
added
update snapshots
github-actions
Bot
removed
the
update snapshots
llastflowers
added
the
update snapshots
github-actions
Bot
removed
the
update snapshots
|
Integration test results from github/github-ui PR:
|
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: None
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
packages/react/src/Autocomplete/Autocomplete.features.stories.module.css — border-color/border-style have no effect here because border-width is set to 0, and the raw… View resolved comment |
Suppressed comments (2)
packages/react/src/Autocomplete/Autocomplete.features.stories.module.css:40
- Avoid hard-coded
1pxhere; the same file already usesvar(--borderWidth-default)(e.g..AnchorContainer). Using the border-width token makes the intent clearer and keeps this aligned if border widths change.
/* stylelint-disable-next-line primer/spacing */
padding: 1px;
packages/react/src/Autocomplete/Autocomplete.features.stories.module.css:50
- Avoid hard-coded
1pxin the radius calculation; prefer subtracting the border width token so the rounding stays consistent with theme/token changes.
/* stylelint-disable-next-line primer/borders */
border-radius: calc(var(--borderRadius-large) - 1px);
Closes https://github.com/github/primer/issues/6428
Changelog
Prevent Autocomplete menu items from being focused before the menu is open, which resolves a screen reader and functional bug visible in
InOverlayWithCustomScrollContainerRefandRenderingTheMenuOutsideAnOverlaystoriesBefore:
Focusing input announces first menu item, despite the menu being closed. It also doesn't announce the input label or the presence of the input element or its status:
Pressing down-arrow opens the menu and skips to the second menu item, since the first one was already selected before opening the menu. The screen reader announces the first menu item, and that it is unselected. To select the first menu item and have it announced correctly, the user has to press up-arrow from here, which is confusing:
After:
Focusing input correctly announces only the input label and input element and its status:
Pressing down-arrow opens the menu and focuses the first menu item, and correctly announces its label and appropriate menu context:
Another couple small fixes in
InOverlayWithCustomScrollContainerRefwhile I'm here:Before:
After:
Before (zoomed):
After (zoomed):
Before (padding/cursor):
After (padding/cursor):
Rollout strategy
Testing & Reviewing
In the 2 stories mentioned above, use these repro steps to test: