Skip to content

fix(Select): guard empty results and retain modal popup scope - #752

Merged
garrity-miepub merged 5 commits into
mainfrom
fix/select-empty-modal-scope
Nov 12, 2026
Merged

garrity-miepub merged 5 commits into
mainfrom
fix/select-empty-modal-scope

Conversation

@lattln

@lattln lattln commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Addresses the shared Select defects tracked in #753 and reported in mieweb/eSheet#256:

  • Safely handle empty search results for ArrowDown, ArrowUp, Home, End, and Enter without emitting an invalid selection.
  • Keep popup DOM descendants within dialog/alertdialog focus and accessibility scope; use the browser top layer to preserve positioning in transformed dialogs.
  • Re-query modal focus boundaries on Tab so dynamically opened search controls stay contained.
  • Expose active options through aria-activedescendant on the trigger and searchable input, with matching consumer-facing documentation.
  • Set explicit popup foreground colors and correct the dark-theme browser regression's token overrides to take precedence over Storybook brand styles.

Validation

  • 37 Select unit tests passed, including no-match navigation, active-option associations, real-Modal Tab/Shift+Tab containment, and alertdialog scope.
  • pnpm typecheck and full pnpm lint passed.
  • Prettier validation and formatting passed for the three follow-up files.
  • Integrated-browser assertions passed for dark popup/option colors, focused search within the dialog, and transformed-modal alignment (approximately 0.005 px error).
  • The CLI Playwright runner could not launch Chrome in this environment; the relevant browser assertions were executed through the integrated browser instead.

Full downstream eSheet compose/upload integration and assistive-technology testing were not repeated during this follow-up. No npm release has been published by this update.

Copilot AI balanced review requested due to automatic review settings November 12, 2026 03:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The Select portal still mispositions popups in transformed dialogs, allows focus to escape the modal when no options match, and excludes alertdialog containers from its scope lookup.

3 open issues
What changed in this PR

This PR addresses shared Select issues reported in mieweb/eSheet#256: keyboard handling after empty searches and popup containment within dialogs.

Changes:

  • Guards empty-result navigation and selection against missing options.
  • Targets the containing dialog for popup portals, with a body fallback.
  • Adds keyboard and dialog-containment regressions and updates Storybook documentation.
File Description
src/​components/​Select/​Select.tsx Adds empty-result guards and dialog-scoped portal targeting.
src/​components/​Select/​Select.test.tsx Adds no-match keyboard and modal popup containment tests.
src/​components/​Select/​Select.stories.tsx Documents the revised portal behavior.

Review effort: Balanced

Comment thread src/components/Select/Select.tsx Resolved
Comment thread src/components/Select/Select.tsx Resolved
Comment thread src/components/Select/Select.tsx Resolved
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

Deploying ui with  Cloudflare Pages  Cloudflare Pages

Latest commit: 18e7e5f
Status: ✅  Deploy successful!
Preview URL: https://2eda5c02.ui-6d0.pages.dev
Branch Preview URL: https://fix-select-empty-modal-scope.ui-6d0.pages.dev

View logs

Copilot AI balanced review requested due to automatic review settings November 12, 2026 03:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The popover’s default foreground overrides the dark theme’s text color, making unselected option labels black against a dark background; the panel needs an explicit foreground color and verification without Storybook’s dark color-scheme override.

1 open issue
3 resolved since last review

Review effort: Balanced

Comment thread src/components/Select/Select.tsx Resolved
Copilot AI balanced review requested due to automatic review settings November 12, 2026 03:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new visual test’s token overrides lose to existing !important declarations, causing its color assertion to fail before the geometry regression check, and accessibility descriptions still contradict the implementation.

2 open issues
1 resolved since last review

Review effort: Balanced

Comment thread tests/visual/components.spec.ts Resolved
Comment thread src/components/Select/Select.tsx Resolved
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings November 12, 2026 19:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Native-popover modal scope and active-option announcements still require human assistive-technology validation, since DOM relationships and focus assertions cannot establish screen-reader behavior.

No open issues 🎉

2 resolved since last review

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings November 12, 2026 19:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Native top-layer popup and active-descendant changes require human validation of screen-reader announcements and downstream integration beyond the supplied DOM and geometry assertions.

No open issues 🎉

Review effort: Balanced

@garrity-miepub
garrity-miepub merged commit 04f2dd4 into main Nov 12, 2026
11 checks passed
@garrity-miepub
garrity-miepub deleted the fix/select-empty-modal-scope branch November 12, 2026 21:17
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.

3 participants