Skip to content

CODAP-614: trap keyboard focus in user entry modal#2435

Merged
kswenson merged 5 commits intomainfrom
CODAP-614-trap-focus-in-modals
Feb 27, 2026
Merged

CODAP-614: trap keyboard focus in user entry modal#2435
kswenson merged 5 commits intomainfrom
CODAP-614-trap-focus-in-modals

Conversation

@kswenson
Copy link
Member

@kswenson kswenson commented Feb 25, 2026

Summary

  • Convert UserEntryModal from a plain div to Chakra Modal for proper focus trapping, overlay, and escape handling
  • Add initialFocusRef to focus the default "Open Document" button on open
  • Add :focus-visible indicators to modal buttons for clear keyboard navigation
  • Match header border-radius to Chakra ModalContent corners
  • Add aria-labelledby so screen readers announce the dialog title
  • Render modal portal inside the drop overlay container to preserve file drag-and-drop functionality

Fixes CODAP-614

Test plan

  • Open CODAP with no document -- modal appears
  • Verify Tab cycles focus between the two buttons (does not escape to toolbar)
  • Verify Shift+Tab cycles in reverse
  • Verify Escape closes the modal
  • Verify focus ring is clearly visible on both buttons
  • Verify "Open Document" button is focused by default on open
  • Verify drag-and-drop of a .codap file onto the user entry screen opens the document
  • Verify screen reader announces "What would you like to do?" when modal opens

Generated with Claude Code

Convert UserEntryModal from a plain div to Chakra Modal for proper
focus trapping, overlay, and escape handling. Add focus-visible
indicators to modal buttons.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR improves accessibility by converting the UserEntryModal from a custom div-based modal to using Chakra UI's Modal component. The change provides proper keyboard focus trapping, escape key handling, and better overlay management—replacing manual implementations with a well-tested library component.

Changes:

  • Replaced custom div-based modal with Chakra Modal, ModalOverlay, and ModalContent components
  • Removed manual keyboard event handling (Escape and Enter keys) in favor of Chakra's built-in behavior
  • Added initialFocusRef to automatically focus the "Open Document" button when the modal opens
  • Removed manual positioning CSS (position, top, left, transform, z-index) as Chakra handles this
  • Added :focus-visible styling for clear keyboard navigation indicators
  • Added border-radius to modal header to match Chakra's ModalContent rounded corners

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
v3/src/components/menu-bar/user-entry-modal.tsx Converted to Chakra Modal with focus management; removed custom keyboard handlers and React import; added ref for default button focus
v3/src/components/menu-bar/user-entry-modal.scss Removed positioning styles delegated to Chakra; added header border-radius and :focus-visible indicator styles

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@codecov
Copy link

codecov bot commented Feb 25, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.51%. Comparing base (305d5d4) to head (5b4ebb6).
⚠️ Report is 34 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2435   +/-   ##
=======================================
  Coverage   85.51%   85.51%           
=======================================
  Files         758      758           
  Lines       42086    42081    -5     
  Branches    10412    10420    +8     
=======================================
- Hits        35990    35986    -4     
+ Misses       6084     6080    -4     
- Partials       12       15    +3     
Flag Coverage Δ
cypress 69.16% <100.00%> (-0.02%) ⬇️
jest 57.06% <88.88%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@cypress
Copy link

cypress bot commented Feb 25, 2026

codap-v3    Run #10462

Run Properties:  status check passed Passed #10462  •  git commit 43fa0539ec: Increment the build number
Project codap-v3
Branch Review main
Run status status check passed Passed #10462
Run duration 02m 19s
Commit git commit 43fa0539ec: Increment the build number
Committer github-actions[bot]
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 4
View all changes introduced in this branch ↗︎

kswenson and others added 2 commits February 24, 2026 23:31
Add aria-labelledby to associate the dialog with its title, so screen
readers announce "What would you like to do?" when the modal opens.
Issue identified by accesslint accessibility audit.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nd-drop

Chakra Modal portals to document.body by default, which covered the
user-entry-drop-overlay and broke file drag-and-drop on the user entry
screen. Use containerRef to render the portal inside the overlay so
drag events bubble to the drop handler correctly.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@kswenson
Copy link
Member Author

Accessibility Audit (accesslint)

An accessibility audit was run on the user entry modal. The migration to Chakra Modal is a net positive for accessibility, providing focus trapping, aria-modal, Escape handling, scroll locking, and focus restoration. Two issues were identified and addressed in this PR:

Fixed in this PR:

  • Dialog accessible name (WCAG 4.1.2) — Added aria-labelledby to associate the dialog with its title so screen readers announce "What would you like to do?"
  • Drag-and-drop regression — Chakra's portal rendering broke file drag-and-drop on the user entry screen; fixed by rendering the portal inside the drop overlay container

Pre-existing issues (not addressed in this PR):

Issue WCAG Severity
Button text contrast 2.1:1 (white on #72bfca, needs 4.5:1) 1.4.3 Critical
Button boundary contrast 2.1:1 against white bg (needs 3:1) 1.4.11 High
Default button outline low contrast against button bg (1.78:1) 1.4.11 Medium
Fixed pixel dimensions may overflow at 200% zoom 1.4.4/1.4.10 Medium
No accessibility unit test coverage Best practice Medium

These pre-existing contrast and layout issues could be addressed in a follow-up story.

@kswenson kswenson requested a review from emcelroy February 25, 2026 07:56
Copy link
Contributor

@emcelroy emcelroy left a comment

Choose a reason for hiding this comment

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

Looks good 👍 I'm approving but there is a small issue I think you should address before merging. (See comment on user-entry-modal.tsx.)

…d add tests

Replace raw divs with Chakra ModalHeader and ModalBody so that
aria-labelledby and aria-describedby are automatically wired up on the
dialog element. Disable closeOnOverlayClick to match V2 behavior.
Add unit tests for accessibility attributes, focus trapping, and
modal interactions.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Contributor

@emcelroy emcelroy left a comment

Choose a reason for hiding this comment

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

Looks good! (I was going to comment on the use of eslint-disable testing-library/no-node-access in the test file, but tried removing it myself and quickly realized why you opted to use it.)

@kswenson kswenson merged commit 9c8144b into main Feb 27, 2026
27 checks passed
@kswenson kswenson deleted the CODAP-614-trap-focus-in-modals branch February 27, 2026 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants