[HadrielWonda]

Closes #8633

The filter input in FilteredActionList always set aria-expanded="true" and aria-controls={listId}, even while the body shows a loader or a message and the list is not rendered. The combobox then referenced a missing element (axe aria-valid-attr-value).

FilteredActionList now passes listId to the input only while the list is rendered. The input sets aria-expanded from that, so while the body shows a loader or a message the input has aria-expanded="false" and no aria-controls. When items are shown, nothing changes.

Changelog

New

• Tests covering the combobox's list reference when items, a message, or a body loader are shown

Changed

• FilteredActionList: the filter input only references the list while it is rendered
• FilteredActionListInput: listId is optional, and aria-expanded reflects whether it is set

Removed

None

Rollout strategy

• No changes needed in github/github-ui
• Needs changes in github/github-ui
• Ships with feature flag(s)
• Not sure
• None

Patch changeset included.

Testing & Reviewing

• New tests fail on main (message and body-loader cases) and pass with this change.
• FilteredActionList, SelectPanel, experimental/SelectPanel2, ActionList and FormControl tests: 388 passed. tsc --noEmit, ESLint and Prettier are clean.
• To check by hand, open the loading-with-body-spinner or with-no-results-message story: the input now has aria-expanded="false" and no aria-controls, and axe no longer reports aria-valid-attr-value.

[changeset-bot]

🦋 Changeset detected

Latest commit: 60a7e03

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Patch

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

[Muguett-DBY]

I found a callback-ref regression when the default primer_react_merged_forwarded_refs flag is off:

<FilteredActionList
  items={[]}
  onFilterChange={() => {}}
  scrollContainerRef={() => {}}
  loading
/>

scrollContainerRef accepts a callback, but in this path useProvidedRefOrCreate returns that function and its .current is undefined. The new !== null check treats that as present, so reading .clientHeight throws. Current main doesn't throw here: its previous truthiness guard skips the body loader (although that also means the loader isn't shown for this ref).

Could the new guard treat both null and undefined as absent, or use the merged internal ref for this path? I compared the exact PR component and current main with React 18 server rendering and stubbed unrelated imports; I haven't run the full browser or project suite. I used OpenAI Codex for the analysis and focused reproduction.

[HadrielWonda]

@Muguett-DBY thanks, good catch. I reproduced it: with the flag off, rerender of your example throws Cannot read properties of undefined (reading 'clientHeight').

Why the types didn't catch it: with the flag off, providedScrollContainerRef is cast to React.RefObject<HTMLDivElement> before it reaches useProvidedRefOrCreate. A callback ref then comes back as the function itself, so readScrollContainerRef.current is typed HTMLDivElement | null but is undefined at runtime. My !== null check trusted the type.

Fix (60a7e03): I went with your first suggestion. The loader height is now read with optional chaining, and the body loader only shows when a height was actually read:

const bodyLoaderHeight =
  loading && loadingType.appearsInBody ? readScrollContainerRef.current?.clientHeight : undefined
const showBodyLoader = bodyLoaderHeight !== undefined

This keeps main's behaviour for both flag states and reads current once instead of twice. I didn't switch the flag-off path to the merged internal ref. With the flag off, that ref isn't the one attached to the scroll container, so it would mean changing the ref wiring the flag scaffolding keeps stable. It goes away at graduation, when readScrollContainerRef becomes the internal ref.

Regression test: handles a scroll container callback ref while loading runs in the existing forwarded-refs block with the flag on and off. It checks that a re-render doesn't throw, that the spinner shows only when the flag is on (as on main), and that aria-controls is either absent or points at an element in the document. It fails on the previous commit (flag off) and passes now.

Checks: with the component change, the FilteredActionList, SelectPanel, experimental/SelectPanel2, ActionList and FormControl tests passed (390). After the final test-only tweak, the FilteredActionList tests passed again (29). tsc --noEmit, ESLint and Prettier are clean.