Skip to content

fix(ItemAction,Button): disable loading actions; show focus and stay inert while disabled - #1470

Merged
tenphi merged 9 commits into
mainfrom
andrew/cub-5275-itemactions-isloading-shows-a-spinner-but-leaves-the-action
Oct 6, 2026
Merged

tenphi merged 9 commits into
mainfrom
andrew/cub-5275-itemactions-isloading-shows-a-spinner-but-leaves-the-action

Conversation

@tenphi

@tenphi tenphi commented Oct 5, 2026 •

Copy link
Copy Markdown
Member

Describe changes

Fixes CUB-5275. A loading ItemAction (Item.Action) showed a spinner but stayed pressable, so moving a loading Button into Item.Action lost its double-press guard. Consumers had to pair isDisabled with isLoading by hand.

Related keyboard gaps are fixed in the same layers:

  • a control that keeps focus while disabled showed no focus ring;
  • a disabled Button with a tooltip still opened its MenuTrigger menu from the keyboard;
  • Enter or Space on a focused disabled ItemButton or ItemAction clicked the element around it.

ItemAction

  • Loading disables, as on Button: isDisabled = isLoading || (isDisabledProp ?? contextIsDisabled). isDisabled={false} still overrides a disabled row, but not a loading action. A computed isDisabled={!x} can't tell "explicit false" from "not disabled", so letting it win would quietly bring the double press back. No consumer relies on the override.
  • Disabled through getDisabledElementProps, like Button and Item:
    • A loading action is aria-disabled and inert instead of natively disabled. It keeps the focus it already holds, and its focus ring, so a keyboard press doesn't drop the user's place to <body>. It also keeps its tooltip, and Tab skips it (tabIndex={-1}) until loading ends, tooltip or not. That differs from a loading Button with a tooltip, which stays a Tab stop; ItemAction keeps one rule: a loading action is never a Tab stop.
    • An icon-only action with a tooltip that disables itself is marked the same way. Keyboard users can still reach it and read the tooltip, as on Button. A disabled link action (to) is always aria-disabled and stays a Tab stop, as on Button.
    • If the host row (ItemActionProvider) is disabled too, a button action that isn't loading stays natively disabled, as a disabled fieldset would leave it.
    • While inert, the action drops its activation handlers (omitActivationEventProps, as Item does). Without that, a MenuTrigger's own onKeyDown would still open the menu from an aria-disabled trigger.
  • inherit-disabled now covers every disabled action inside a disabled row. Before, an action disabled by its own prop inside a disabled row faded twice. That was already true on main.
  • The spinner counts as an icon for padding (has-icon), like Button's hasLeftIcon, so a label-only action keeps its size while loading.

Button

While inert (disabled or loading with a tooltip, or rendered as a link), Button now drops activation handlers the same way. A MenuTrigger hands its trigger an onKeyDown that ignores the trigger's disabled state, so Enter, Space and ArrowDown used to open the menu from a disabled Button with a tooltip.

Inert elements (getDisabledElementProps)

The shared inert props now cancel Enter and Space on the element itself. Otherwise the browser turns them into a click on the focused element, and that click reaches a clickable row's or card's onClick. On main, ItemButton and ItemAction already let that click through. Button didn't, because React Aria's own key handler prevented it, and dropping that handler would have brought the problem to Button too. Keys from a focused descendant are left alone.

Focus ring on disabled controls (shared useFocus, useAction)

On main, useFocus passed isDisabled to React Aria, which detaches the focus handlers. It then reset its own state during render, and useAction hid focused while disabled. So a control that keeps focus while disabled (a disabled Button or ItemAction with a tooltip or rendered as a link, an ItemButton of a type with a ring, and an interactive InfoBadge) showed no ring. It also stayed without one after being enabled, until focus left and came back.

useFocus now reports whether its element holds focus, disabled or not, and useAction passes that through. Its isDisabled option is gone, which simplifies all 13 call sites. No blur reaches React when, in a render of the control:

  • the focused element is replaced, as when a tooltip wrapper is added or dropped (tooltip={off ? reason : undefined});
  • its fieldset is disabled;
  • it loses its tabIndex.

So the hook drops a focus its element no longer holds, and never sets one: after each render of the control and, for a loss the control doesn't re-render for, on the next key or pointer press React Aria reports, through the focus-visible listener every useFocus already registers. Keys typed in a text field don't count, and neither does a focus moved by code, so a stale ring can last until the next press. The second case covers ItemButton's auto tooltip mounting on overflow while the row is focused, and a stateful parent disabling the fieldset around a control. So a Tab no longer leaves two rings. It reads the active element the way React Aria does, so a shadow root is covered too. That also fixes stale rings main shows in all these cases, for enabled controls too.

The ring is the standard one. ItemButton's default item type gets none: its rows show focus with their fill, and a ring would get in the way of menu navigation and selection, so a disabled row shows no focus, as before. disabled still wins for fill and color in every theme; current primary lacked that guard for its focus overlay and now has it. ActiveZone, which isn't an aria-disabled control, keeps its own mask.

Entropy change

  • useFocus: Slightly Decreased. One option and a render-time reset are removed, along with the flag every caller passed. One invariant is added: the hook reports focus only while its element holds it, checked after the control renders and from the focus-visible listener it already had, with no new subscription. It lives in the layer that owns the bug instead of in its 13 callers. No public API changes (useFocus isn't exported).
  • useAction focused mod: Slightly Decreased. A second disabled mask goes, and focus on a disabled Tab stop becomes visible.
  • Button: Neutral. It adopts the inert-handler line that Item and ItemAction already use, instead of being the one aria-disabled trigger that still acts.
  • getDisabledElementProps: Neutral. One key handler in the shared inert props makes "an inert element does nothing on Enter or Space" true for every component that uses it, instead of each one guarding it.
  • ItemAction: Moderately Increased for maintainers, and justified. The precedence rule, two derived flags, a two-term keepEvents and tabIndex={-1} while loading give a disabled action four forms (native; aria-disabled Tab stop; aria-disabled, skipped by Tab; native under a disabled host). Each maps to an acceptance criterion and reuses the getDisabledElementProps contract Button and Item already have.
  • Consumers: Moderately Decreased. isLoading alone is enough; no hand-written isDisabled={isLoading}, and no API added.
  • Keyboard UX: Moderately Decreased. Focus on disabled Tab stops is visible, a disabled trigger no longer opens its menu, the ring comes back after re-enabling and no stale ring stays behind. Nothing new to learn: focusable-but-inert controls already existed on Button and Item.

The ring and menu fixes belong here: this PR makes a disabled icon-only action with a tooltip a Tab stop, so without the ring it would itself add focus keyboard users can't see. The menu fix is the same inert-handler line this PR adds to ItemAction.

Chromatic: expect diffs only in the ItemAction "States" story's Loading row and the InsideItem story's "With Loading States": loading actions fade like disabled ones, and the label-only spinner gets icon padding. No story focuses a disabled or loading control, so the ring changes no snapshot.

Verified

  • Unit tests:
    • ItemAction.test.tsx (new), interactions.test.tsx, button.test.tsx, and ring tests in ItemButton.test.tsx and InfoBadge.test.tsx.
    • Enter and Space on a disabled Button or ItemButton with a tooltip don't click the element around it.
    • A focused, disabled current primary chip keeps its resting fill (temporary Chromium check: fails before, passes after).
    • interactions.browser.test.tsx (Chromium; jsdom can't show these):
      • a focused control whose fieldset is disabled or whose tabIndex is removed drops its focus;
      • a stateful parent's fieldset around {children}: one ring after Tab, not two;
      • a child restructuring itself: the ring goes on the next key;
      • a memoized row moved by a key press keeps focus and its ring.
    • Temporary Chromium check of the real ItemButton auto tooltip mounting on overflow: the stale ring clears on the next key.
    • Every new behaviour test fails on the code before it. On main, the invariants pass.
  • Real Chromium (temporary browser spec, not committed):
    • A disabled Tab stop with a tooltip (Button, ItemAction) shows a 1px ring while disabled and after enabling, on that element only. Pointer focus shows none.
    • A conditional tooltip toggled with isDisabled (Button, ItemButton) leaves no stale ring in either direction.
    • A native disable while focused (Button, Checkbox, fieldset) or a removed tabIndex leaves no ring, during or after.
    • A loading ItemAction keeps its ring throughout.
    • A Button menu stays closed on ArrowDown, Enter and Space, whether disabled or loading.
  • Storybook (Chromium): loading actions fade like disabled ones and have cursor: default; the tooltip opens on hover.
  • Gates: lint and diagnostics:compiler pass (334 compiled functions, 158 diagnostics, as on main), as does audit-docs for ItemAction and InfoBadge. The ItemButton skipActionsWidthTransition audit note is the same on main. tsc shows the same 22 errors as main. The full jsdom suite passes (3281 tests). Under heavy machine load a few unrelated tests hit the 5 s timeout and pass when rerun alone.

Follow-up after release: bump in cubejs-enterprise. Drop isDisabled={isAuthenticating} (MCP auth Connect) and || isCheckingAccess (report delivery refresh), and switch ToolMessage.browser.test.tsx's toBeDisabled() to an aria-disabled assertion.

Checklist
  • Pipeline is passed
  • Tests are added (including unit tests and stories in the storybook)
  • Tests are passed successfully
  • Changeset(s) is(are) added
  • Commit message follows commit guidelines

Closes: CUB-5275

🤖 Generated with Claude Code

tenphi and others added 8 commits October 5, 2026 19:39
A loading ItemAction showed a spinner but stayed pressable, so a second
press re-ran the action. Loading now always disables it, matching Button;
isDisabled={false} still overrides a disabled row but not loading.

A disabled or loading action with a tooltip is now marked aria-disabled
and kept inert instead of natively disabled, so its tooltip still opens
(getDisabledElementProps, as Button and Item use). The spinner on a
label-only action gets the icon padding.

Refs CUB-5275

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A loading action now stays focusable (aria-disabled) so a keyboard
press keeps its place. An action disabled by its host row or field
keeps the native attribute, so it stays out of the tab order as in a
disabled fieldset. On the aria-disabled path, handlers a parent passed
in (MenuTrigger's onKeyDown) are dropped, so a disabled trigger can't
open its menu. A loading action with isDisabled={false} in a disabled
row no longer fades twice. Docs gain a loading example.

Refs CUB-5275

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ring

A loading action keeps focus it already holds but gets `tabIndex={-1}`, so
Tab skips it as it skips a loading `Button`. An action disabled by its own
prop inside a disabled row now counts as inherited: it stays natively
disabled and fades once.

The shared `useFocus` now reports focus again when a control is re-enabled
while it still holds focus. React Aria fires no focus event in that case, so
an `aria-disabled` control (a loading ItemAction, a Button with a tooltip)
lost its focus ring until focus moved away and back.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The re-enable check only knew elements focused while enabled, so a control
that took focus while aria-disabled (a disabled tooltip Tab stop) got no ring
once enabled. Keep React Aria's focus handlers attached while disabled and
hide the ring until enabled. React Aria reports the blur itself when an
element is natively disabled, so no stale ring is left behind; the
render-time reset is no longer needed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…enus

`useFocus` reports whether its element holds focus, disabled or not, and
drops a focus the element lost without a blur React sees: the element
replaced (a tooltip wrapper added or dropped), disabled through a fieldset,
or stripped of its tabIndex. Its unused `isDisabled` option goes, and
`useAction` no longer hides `focused` while disabled, so a disabled Button,
ItemButton or ItemAction that keeps focus (aria-disabled with a tooltip, or
a link) shows its focus ring.

Button drops activation handlers while inert, as Item and ItemAction do, so
a disabled or loading Button with a tooltip no longer opens its MenuTrigger
menu from the keyboard.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…keyboard rules

Round-5 review follow-ups:
- Chromium test for the useFocus guard on a fieldset disable and a removed
  tabIndex, which jsdom cannot reproduce; it fails without the guard.
- Ring tests for a disabled ItemButton with a tooltip, a disabled interactive
  InfoBadge, a disabled ItemAction link and a re-enabled ItemAction Tab stop;
  loading rows for Button's menu keys.
- ItemAction docs gain an Accessibility section for the Tab, focus ring,
  menu key and aria-disabled rules, and the prop bullets shrink to what each
  prop means. InfoBadge docs mention the ring.
- The guard comment and focus changeset say the guard runs when the control
  re-renders; the isInert JSDoc points at omitActivationEventProps.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…cking its container

Dropping a Button's activation handlers while inert also dropped React Aria's
keydown preventDefault, so Enter or Space on a focused aria-disabled Button
made the browser click it and the click reached an ancestor's onClick.
ItemButton and ItemAction already did this through the same inert props.
INERT_PROPS now cancels both keys on the element itself, which covers all
three; keys from a focused descendant are left alone.

The useFocus guard reads the active element the way React Aria does, so it
also looks inside a shadow root.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Now that a disabled control can show focus, `current.primary` was the one
flavour whose focus overlay had no disabled guard, so a focused disabled chip
darkened. The overlay now skips disabled, like every other theme's fill.

The focus changeset names the ItemButton types that have a ring: the default
`item` type shows focus with its fill and gets no ring, by design, so the
ItemButton ring test uses `type="outline"`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@vercel

vercel Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
cube-ui-kit Ready Ready Preview Oct 5, 2026 7:08pm UTC

Request Review

@changeset-bot

changeset-bot Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: d127716

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

This PR includes changesets to release 1 package
Name Type
@cube-dev/ui-kit 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

@github-actions

github-actions Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

📦 NPM canary release

Deployed canary version 0.0.0-canary-35a6e75.

@github-actions

github-actions Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

🧪 Storybook is successfully deployed!

@github-actions

github-actions Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

🏋️ Size limit report

Name Size Passed?
All 598.9 KB (-0.01% 🔽👏) Yes 🎉
Tree shaking (just a Button) 129.86 KB (+0.09% 🔺) Yes 🎉

Compared against main at 05f03e8 — run 37345053487, 2026-10-05T16:59:36Z.

To see which modules changed, download the size-limit-statoscope-report artifact from this run and open report.html.

… not re-render

The guard that drops a focus its element no longer holds only ran after the
control's own render. A focus lost to a change elsewhere stayed: a stateful
parent disabling the `fieldset` around a control, or a child restructuring
itself (ItemButton's auto tooltip mounting once its label overflows). No blur
reaches React there, so the stale ring stayed until focus came back, and after
a Tab two rings showed at once.

The same check now also runs from React Aria's focus-visible listener, which
every `useFocus` already registers, on each key or pointer press it reports
(keys typed in a text field don't count, nor does a focus moved by code). The
element ref is cleared on blur, so an unfocused control does no more than a
null check. Chromium tests cover both cases and a memoized row moved by a key
press, which React refocuses after the commit and keeps its ring.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@tenphi
tenphi merged commit e7bf7d2 into main Oct 6, 2026
18 checks passed
@tenphi
tenphi deleted the andrew/cub-5275-itemactions-isloading-shows-a-spinner-but-leaves-the-action branch October 6, 2026 09:07
@tenphi tenphi mentioned this pull request Oct 6, 2026

This branch was successfully deployed

2 active deployments
Preview — d127716d Deployed Oct 5, 2026 by vercel[bot]
Chromatic staging — d127716d Deployed Oct 5, 2026 by tenphi via Prepare Storybook for review & tests #3982
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant