Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/big-corners-type.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@primer/react": patch
---

Don't focus Autocomplete menu items while menu is closed: fix SR announcement bug and menu item skipping bug

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these ActionMenu snapshot updates expected? 👀

@llastflowers llastflowers Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re the ArrowUp thing: Good call-out, I hadn't noticed that! I think this was probably unintentional behavior before, because none of the other Autocomplete stories do that before or after these changes. Now they are all the same, which seems preferable imo!

Re the snapshots: They are flaky, and the version in this PR is actually the correct ones. I think they've been switched back and forth on main multiple times 😂 I'm actually trying to fix that in another PR as we speak!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah that makes sense. Thanks for clarifying! Having consistent behavior across all the stories seems preferable to me too.

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
Expand Up @@ -36,17 +36,18 @@
}

.OverlayInputBar {
/* stylelint-disable-next-line primer/spacing */
padding: 1px;
border-width: 0;
border-bottom-width: var(--borderWidth-default);
border-color: var(--borderColor-default);
border-style: solid;
}
Comment thread
llastflowers marked this conversation as resolved.

.OverlayInput {
display: flex;
border: 0;
padding-left: var(--base-size-16);
padding-right: var(--base-size-16);
/* stylelint-disable-next-line primer/borders */
border-radius: calc(var(--borderRadius-large) - 1px);
padding-top: var(--base-size-4);
padding-bottom: var(--base-size-4);
box-shadow: none;
Expand Down
45 changes: 44 additions & 1 deletion packages/react/src/Autocomplete/Autocomplete.test.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {render, fireEvent, screen, waitFor} from '@testing-library/react'
import userEvent from '@testing-library/user-event'
import React from 'react'
import {createRef} from 'react'
import {describe, expect, it, vi} from 'vitest'
import type {AutocompleteInputProps} from '../Autocomplete'
import Autocomplete from '../Autocomplete'
Expand Down Expand Up @@ -269,6 +269,49 @@ describe('Autocomplete', () => {
})

describe('Autocomplete.Menu', () => {
it('only sets an active descendant while a menu rendered without an overlay is open', async () => {
const user = userEvent.setup()
const ancestorRef = createRef<HTMLDivElement>()
const handleAncestorKeyDown = (event: KeyboardEvent) => {
if (!event.defaultPrevented && event.key === 'ArrowDown') {
ancestorRef.current?.querySelector<HTMLElement>('[role="option"]')?.focus()
}
}

render(
<BaseStyles>
<div ref={ancestorRef}>
<label htmlFor="autocompleteInput" id="autocompleteLabel">
Autocomplete field
</label>
<Autocomplete id="autocompleteId">
<Autocomplete.Input id="autocompleteInput" />
<Autocomplete.Menu items={mockItems} selectedItemIds={[]} aria-labelledby="autocompleteLabel" />
</Autocomplete>
</div>
</BaseStyles>,
)
const inputNode = screen.getByRole('combobox', {name: AUTOCOMPLETE_LABEL})
ancestorRef.current?.addEventListener('keydown', handleAncestorKeyDown)

expect(inputNode).toHaveAttribute('aria-expanded', 'false')
expect(screen.getByRole('listbox', {hidden: true}).closest('[hidden]')).toBeInTheDocument()

await user.click(inputNode)
expect(inputNode).not.toHaveAttribute('aria-activedescendant')

await user.keyboard('{ArrowDown}')
await waitFor(() => {
expect(inputNode).toHaveFocus()
expect(inputNode).toHaveAttribute('aria-activedescendant', mockItems[0].id)
expect(screen.getByRole('option', {name: mockItems[0].text})).not.toHaveFocus()
})

// eslint-disable-next-line github/no-blur
fireEvent.blur(inputNode)
await waitFor(() => expect(inputNode).not.toHaveAttribute('aria-activedescendant'))
})

it('calls a custom filter function', async () => {
const user = userEvent.setup()
const filterFnMock = vi.fn()
Expand Down
10 changes: 10 additions & 0 deletions packages/react/src/Autocomplete/AutocompleteInput.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const AutocompleteInput = React.forwardRef(
onBlur,
onChange,
onKeyDown,
onKeyDownCapture,
onKeyUp,
onKeyPress,
value,
Expand Down Expand Up @@ -111,6 +112,14 @@ const AutocompleteInput = React.forwardRef(
[inputRef, setInputValue, setHighlightRemainingText, onKeyDown, showMenu, setShowMenu],
)

const handleInputKeyDownCapture: KeyboardEventHandler<HTMLInputElement> = event => {
onKeyDownCapture?.(event)

if (!showMenu && ARROW_KEYS_NAV.has(event.key) && !event.altKey) {
event.preventDefault()
}
}

const handleInputKeyUp: KeyboardEventHandler<HTMLInputElement> = useCallback(
event => {
onKeyUp?.(event)
Expand Down Expand Up @@ -174,6 +183,7 @@ const AutocompleteInput = React.forwardRef(
onBlur={handleInputBlur}
onChange={handleInputChange}
onKeyDown={handleInputKeyDown}
onKeyDownCapture={handleInputKeyDownCapture}
onKeyPress={onInputKeyPress}
onKeyUp={handleInputKeyUp}
ref={mergedRef}
Expand Down
3 changes: 2 additions & 1 deletion packages/react/src/Autocomplete/AutocompleteMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,7 @@ function AutocompleteMenu<T extends AutocompleteItemProps>(props: AutocompleteMe
useFocusZone(
{
containerRef: listContainerRef,
disabled: !showMenu,
focusOutBehavior: 'wrap',
focusableElementFilter: element => {
return !(element instanceof HTMLInputElement)
Expand Down Expand Up @@ -354,7 +355,7 @@ function AutocompleteMenu<T extends AutocompleteItemProps>(props: AutocompleteMe
}

return (
<VisuallyHidden isVisible={showMenu}>
<VisuallyHidden isVisible={showMenu} hidden={!showMenu}>
{loading ? (
<div className={classes.SpinnerWrapper}>
<Spinner />
Expand Down
Loading