facebook / facebook/astryx

Button uses native disabled for busy state, which drops focus

Open
#4,871 1 comment 0 reactions 0 assignees View on GitHub
accessibility bug
Dominant language
TypeScript
Stars
13.1k
Forks
1.1k
Avg merge
1d 15h
Merged PRs (30d)
687

Description

## What happens

While a `clickAction` is pending, `Button` sets the native `disabled` attribute on the ``. A natively disabled element cannot hold focus, so the browser moves focus to `` the moment the action starts, and it is not restored when the action settles.

In `packages/core/src/Button/Button.tsx`:

```ts
const isLoadingState = isLoading || isPending;
const buttonDisabled =
isDisabled || groupDisabled || (isLoadingState && !isInterruptible);
...
disabled={useAriaDisabled ? undefined : buttonDisabled}
```

`useAriaDisabled` is only true when a `tooltip` is set, so in the common case the busy button is natively disabled.

## Why it's a problem

A keyboard user who activates "Save" with Enter loses their place: focus falls to ``, so the next Tab restarts from the top of the document, and screen readers announce nothing when the action completes. This is the classic reason busy state should never be expressed as `disabled`.

It also contradicts the documented rule in the API Conventions wiki page ("Disabled vs Busy"), which says busy is visual-only — `aria-busy` plus a spinner — and that only `isDisabled` uses the native attribute. Input components already follow that rule: they render `aria-busy` and keep the control focusable, guarding re-entry in the change handler.

## Why the native disable looks redundant

`Button` already guards re-entry in the click handler with a ref:

```ts
const actionInFlightRef = useRef(false);
if (buttonDisabled || (actionInFlightRef.current && !isInterruptible)) {
e.preventDefault();
return;
}
```

That guard is what actually makes a fire-once action fire once — it dedupes even a same-tick double click, which `disabled` does not reliably do. The native attribute adds the focus loss without adding the safety.

## Suggested fix

While busy (and not otherwise disabled):

- keep the button focusable — no native `disabled`
- keep `aria-busy="true"` and the spinner
- set `aria-disabled="true"` so AT announces the state
- suppress Enter/Space activation via the existing handler path, the way the `aria-disabled` + tooltip branch already does
- leave `isDisabled` behavior unchanged: that stays native `disabled`

`isInterruptible` can stay as-is; it would simply become the variant that also lets activation through.

## Notes

Changing this affects every consumer that relies on a busy button being unclickable, so the re-entry guard needs to cover the keyboard path too before the attribute comes off. Worth a changeset and a note in the release notes.

Contributor guide

Open the contributing guide

Research direction

Start in packages/core/src/Button/Button.tsx, tracing isLoadingState, buttonDisabled, the click handler, and the existing aria-disabled path. Compare the behavior with the input components and the API Conventions wiki page’s “Disabled vs Busy” rule. Done means busy buttons retain focus, expose aria-busy and aria-disabled, block unintended keyboard re-entry, and preserve native disabled behavior for isDisabled.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
accessibility, frontend
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
70/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.