Skip to content

Conversation

Sidnioulz
Copy link
Member

@Sidnioulz Sidnioulz commented Sep 3, 2025

Caution

Must base onto next once the IconButton branch is merged! ⚠️⚠️⚠️

What I did

This PR is part of the work done on WithTooltip replacement. It specifically addresses Select's use of WithTooltip.

Select's menu is not a tooltip, it's an interactive popover with a listbox role. We already have KB nav and aria markup working, all we were missing was:

  • A popover placement logic that doesn't depend on Popper.js
  • A hook to close the popover when clicking outside of its content

The PR uses low-level react-aria hooks to achieve this.

The miminalist popover inside Select.tsx will likely not be reusable. We have higher-level primitives for most use cases, but here I needed something very low-level to keep the existing KB nav logic in place.

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. Run local instance
  2. Go to http://localhost:6006/?path=/story/select-component--mouse-selection-multi
  3. Play around with the stories

Documentation

N/A

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 canary-release-pr.yml --field pr=<PR_NUMBER>

Greptile Summary

Updated On: 2025-09-03 17:34:52 UTC

This PR migrates the Select component from using WithTooltipPure (based on Popper.js) to react-aria overlay hooks for better semantic correctness and accessibility. The Select component's dropdown menu was previously implemented as a tooltip, but semantically it should be an interactive popover with a listbox role.

The main changes include:

  1. New dependency: Adds react-aria-components@^1.12.1 to the core package to access overlay positioning and interaction hooks

  2. Custom MinimalistPopover component: Replaces WithTooltipPure with a new component that uses three react-aria hooks:

    • useOverlayPosition for popover placement without Popper.js
    • useInteractOutside for handling clicks outside the popover
    • useOverlay for focus management and dismissal behavior
  3. Improved ARIA semantics: Updates role assignments so single-select uses button role and multi-select uses combobox role, following proper accessibility guidelines

  4. Enhanced keyboard navigation: Maintains existing keyboard shortcuts while adding Tab key handling to close the popover

  5. Visual consistency: Adds borderRadius: 2 to focused select options for consistent styling

The migration preserves all existing functionality including complex multi-select logic, keyboard navigation, and ARIA markup while moving to a more semantically correct and accessible implementation. The new MinimalistPopover is intentionally low-level and non-reusable to maintain the existing keyboard navigation behavior.

Confidence score: 4/5

  • This PR introduces significant architectural changes but follows established patterns and maintains existing functionality
  • Score reflects the complexity of the migration and potential edge cases with the new react-aria dependency
  • Pay close attention to the Select component implementation and test the popover positioning thoroughly

@Sidnioulz Sidnioulz requested a review from ghengeveld September 3, 2025 17:33
@Sidnioulz Sidnioulz self-assigned this Sep 3, 2025
@Sidnioulz Sidnioulz added ui maintenance User-facing maintenance tasks accessibility ci:normal a11y: keyboard Accessibility issues related to keyboard navigation or shortcuts labels Sep 3, 2025
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

4 files reviewed, 1 comment

Edit Code Review Bot Settings | Greptile

Copy link

nx-cloud bot commented Sep 3, 2025

View your CI Pipeline Execution ↗ for commit 7b31b8c

Command Status Duration Result
nx run-many -t check -c production --parallel=7 ✅ Succeeded 1m 16s View ↗
nx run-many -t build -c production --parallel=3 ✅ Succeeded 3m 38s View ↗

☁️ Nx Cloud last updated this comment at 2025-09-05 12:56:04 UTC

@Sidnioulz Sidnioulz force-pushed the sidnioulz/issue-32249-tooltip-select-migration branch from ee2c22d to b0fa742 Compare September 3, 2025 21:48
Copy link
Member

@ghengeveld ghengeveld left a comment

Choose a reason for hiding this comment

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

Not sure why CI didn't run. Code looks good, but I'd like to see how the Chromatic snapshots turn out. @MichaelArestad should probably take this for a spin as well.

@Sidnioulz Sidnioulz force-pushed the sidnioulz/issue-32249-tooltip-select-migration branch from a44079b to 8eb0e22 Compare September 4, 2025 09:38
@Sidnioulz
Copy link
Member Author

Not sure why CI didn't run. Code looks good, but I'd like to see how the Chromatic snapshots turn out. @MichaelArestad should probably take this for a spin as well.

I was hoping you wouldn't review this one so quickly, it's based on the IconButton/Select branch and I hadn't finished fixing all the E2E and unit tests on that. It should be green soon enough :)

@storybook-pr-benchmarking
Copy link

storybook-pr-benchmarking bot commented Sep 4, 2025

Package Benchmarks

Commit: 1e18255, ran on 4 September 2025 at 14:06:02 UTC

The following packages have significant changes to their size or dependencies:

storybook

Before After Difference
Dependency count 48 48 0
Self size 30.78 MB 32.57 MB 🚨 +1.79 MB 🚨
Dependency size 17.61 MB 17.61 MB 0 B
Bundle Size Analyzer Link Link

@storybook/cli

Before After Difference
Dependency count 204 204 0
Self size 879 KB 879 KB 0 B
Dependency size 81.74 MB 83.53 MB 🚨 +1.79 MB 🚨
Bundle Size Analyzer Link Link

@storybook/codemod

Before After Difference
Dependency count 173 173 0
Self size 35 KB 35 KB 🚨 +42 B 🚨
Dependency size 76.81 MB 78.60 MB 🚨 +1.79 MB 🚨
Bundle Size Analyzer Link Link

create-storybook

Before After Difference
Dependency count 49 49 0
Self size 1.52 MB 1.52 MB 0 B
Dependency size 48.39 MB 50.18 MB 🚨 +1.79 MB 🚨
Bundle Size Analyzer node node

@Sidnioulz Sidnioulz force-pushed the sidnioulz/issue-31261 branch from 268805e to 3a794d4 Compare September 4, 2025 13:07
@Sidnioulz Sidnioulz force-pushed the sidnioulz/issue-32249-tooltip-select-migration branch from eee554c to 1e18255 Compare September 4, 2025 13:51
@Sidnioulz Sidnioulz force-pushed the sidnioulz/issue-32249-tooltip-select-migration branch from 1e18255 to 7b31b8c Compare September 5, 2025 12:48
@Sidnioulz Sidnioulz changed the base branch from sidnioulz/issue-31261 to a11y-consolidation September 5, 2025 12:48
@Sidnioulz Sidnioulz merged commit 08d081b into a11y-consolidation Sep 5, 2025
4 of 7 checks passed
@Sidnioulz Sidnioulz deleted the sidnioulz/issue-32249-tooltip-select-migration branch September 5, 2025 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y: keyboard Accessibility issues related to keyboard navigation or shortcuts accessibility ci:normal maintenance User-facing maintenance tasks ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants