Skip to content

Conversation

@Sidnioulz
Copy link
Member

@Sidnioulz Sidnioulz commented Jan 6, 2026

Closes #33467

What I did

Fixed a nasty regression on keyboard handling in the toolbar (and all other Selects) introduced by #33186.

The inert attribute that prevents other components from installing focus traps whilst a Select is opened caused a race condition with the refocusing of the Select trigger when closing Select.

I've changed the close handler to ensure that the intermediary popover that sets up the hook with the inert attribute is unmounted (and its hook's cleanup function called) before actually attempting to refocus the trigger.

I've also reinforced stories to have a NRT for this use case.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

  1. Kb nav to toolbar background button
  2. Press ArrowDown to enter
  3. Press Esc to exit
  4. Press Tab, notice you land on the next toolbar item

Documentation

ø

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>

Summary by CodeRabbit

  • Bug Fixes

    • Improved focus handling in the Select so the trigger reliably regains focus after the dropdown closes.
  • Tests

    • Added interactive stories that exercise keyboard and mouse navigation for the Select, including sibling focus transitions.
  • New Features

    • Added a new interactive story demonstrating the Select inside a toolbar with enhanced interaction coverage.

✏️ Tip: You can customize this high-level summary in your review settings.

@Sidnioulz Sidnioulz requested a review from ndelangen January 6, 2026 10:20
@Sidnioulz Sidnioulz added bug accessibility patch:yes Bugfix & documentation PR that need to be picked to main branch ci:normal labels Jan 6, 2026
@nx-cloud
Copy link

nx-cloud bot commented Jan 6, 2026

View your CI Pipeline Execution ↗ for commit d92c3ce

Command Status Duration Result
nx run-many -t compile,check,knip,test,pretty-d... ✅ Succeeded 9m 57s View ↗

☁️ Nx Cloud last updated this comment at 2026-01-06 10:43:23 UTC

1 similar comment
@nx-cloud
Copy link

nx-cloud bot commented Jan 6, 2026

View your CI Pipeline Execution ↗ for commit d92c3ce

Command Status Duration Result
nx run-many -t compile,check,knip,test,pretty-d... ✅ Succeeded 9m 57s View ↗

☁️ Nx Cloud last updated this comment at 2026-01-06 10:43:23 UTC

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 6, 2026

📝 Walkthrough

Walkthrough

Select received delayed refocus logic to ensure the trigger regains focus after the listbox fully closes. Stories were extended: existing WithSiblings gained an automated play function, and a new WithSiblingsInToolbar story places Select inside a Toolbar and includes an interaction play function.

Changes

Cohort / File(s) Summary
Select Stories Enhancement
code/core/src/components/components/Select/Select.stories.tsx
Import updated to include Toolbar. WithSiblings story now includes a play function automating open/select/tab/escape flows. New WithSiblingsInToolbar story renders Select inside a Toolbar and adds a comprehensive play that exercises keyboard and mouse interactions and sibling focus transitions.
Select Focus Management
code/core/src/components/components/Select/Select.tsx
Added shouldRefocusTrigger boolean state. Close handler sets the flag instead of immediately focusing the trigger. A useEffect watches for the listbox closing and shouldRefocusTrigger to perform delayed focus on the trigger and reset the flag (accounts for inert removal timing).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d1bd61b and d92c3ce.

📒 Files selected for processing (2)
  • code/core/src/components/components/Select/Select.stories.tsx
  • code/core/src/components/components/Select/Select.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • code/core/src/components/components/Select/Select.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Format code using Prettier with yarn prettier --write <file>

Files:

  • code/core/src/components/components/Select/Select.stories.tsx
**/*.{js,jsx,json,html,ts,tsx,mjs}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Run ESLint checks using yarn lint:js:cmd <file> or the full command cross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directives to fix linting errors before committing

Files:

  • code/core/src/components/components/Select/Select.stories.tsx
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Enable TypeScript strict mode across all packages

Files:

  • code/core/src/components/components/Select/Select.stories.tsx
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size

Files:

  • code/core/src/components/components/Select/Select.stories.tsx
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use logger from storybook/internal/node-logger for server-side logging in Node.js code

Files:

  • code/core/src/components/components/Select/Select.stories.tsx
🧠 Learnings (4)
📓 Common learnings
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.

Applied to files:

  • code/core/src/components/components/Select/Select.stories.tsx
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.

Applied to files:

  • code/core/src/components/components/Select/Select.stories.tsx
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.

Applied to files:

  • code/core/src/components/components/Select/Select.stories.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: normal
  • GitHub Check: nx
  • GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (3)
code/core/src/components/components/Select/Select.stories.tsx (3)

3-3: LGTM! Import addition supports the new toolbar story.

The Toolbar import is used appropriately in the new WithSiblingsInToolbar story.


202-245: Excellent non-regression test for the focus behavior fix.

This play function thoroughly tests the focus management sequence that was broken in the regression:

  • Verifies focus returns to the Select trigger after option selection
  • Tests that Tab navigation works correctly from the Select to siblings
  • Confirms Escape returns focus to the Select trigger (the core regression fix)

This provides automated coverage for the keyboard navigation and focus behavior described in issue #33467.


247-309: Perfect test coverage for the toolbar scenario from the bug report.

This story directly reproduces and tests the scenario from issue #33467:

  • Places Select within a Toolbar component (matching the reported context)
  • Uses toolbar-specific arrow key navigation (ArrowRight/ArrowLeft)
  • Verifies the critical behavior: after selecting an option and pressing Escape, focus returns to the Select trigger rather than jumping to the start of the document

The test sequence matches the reproduction steps from the issue, providing strong automated coverage that the regression is fixed.


Comment @coderabbitai help to get the list of available commands and usage tips.

@Sidnioulz Sidnioulz force-pushed the sidnioulz/issue-33467 branch from d1bd61b to d92c3ce Compare January 6, 2026 10:32
@ndelangen ndelangen moved this to Empathy Backlog in Core Team Projects Jan 6, 2026
@ndelangen ndelangen moved this from Empathy Backlog to Empathy Queue (prioritized) in Core Team Projects Jan 6, 2026
@ndelangen ndelangen merged commit 1e1d177 into next Jan 9, 2026
69 checks passed
@ndelangen ndelangen deleted the sidnioulz/issue-33467 branch January 9, 2026 10:55
@github-project-automation github-project-automation bot moved this from Empathy Queue (prioritized) to Done in Core Team Projects Jan 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

accessibility bug ci:normal patch:yes Bugfix & documentation PR that need to be picked to main branch

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[Bug]: After selecting something in select, keyboard focus jumps to start of page

3 participants