Skip to content

fix: Update <FocusTrap> to handle single focusable child element#306

Merged
maximo-macchi-cb merged 4 commits intomasterfrom
max/focus-trap-support-no-focusable-children
Jan 22, 2026
Merged

fix: Update <FocusTrap> to handle single focusable child element#306
maximo-macchi-cb merged 4 commits intomasterfrom
max/focus-trap-support-no-focusable-children

Conversation

@maximo-macchi-cb
Copy link
Copy Markdown
Contributor

What changed? Why?

This PR adds a fallback check if there's only one focusable element in a <FocusTrap>. If there's no second focusable element, the <FocusTrap> continues its focus logic and does not error out.

A note has been added to the docs to warn customers that at least one child element should be focusable. This is done because adding fallback checks for the first focusable element adds a bit more complexity. It's an edge case we shouldn't have to consider because a <FocusTrap> should always have at least one focusable element.

Root cause (required for bugfixes)

There's a constant called secondElement used to handle focus logic for our dropdown components. This constant assumes there are at least two focusable children elements based on how it's queried: const secondElement = focusableElements[1]. When there's only one focusable child element, secondElement is undefined and so any reference to an attribute of secondElement such as const secondElementIsMenuItemOrOption = secondElement?.role === 'menuitem' || secondElement?.role === 'option' errors out.

UI changes

iOS Old iOS New
old screenshot new screenshot
Android Old Android New
old screenshot new screenshot
Web Old Web New
old screenshot new screenshot

Testing

How has it been tested?

  • Unit tests
  • Interaction tests
  • Pseudo State tests
  • Manual - Web
  • Manual - Android (Emulator / Device)
  • Manual - iOS (Emulator / Device)

Testing instructions

Illustrations/Icons Checklist

Required if this PR changes files under packages/illustrations/** or packages/icons/**

  • verified visreg changes with Terran (include link to visreg run/approval)
  • all illustration/icons names have been reviewed by Dom and/or Terran

Change management

type=routine
risk=low
impact=sev5

automerge=false

@maximo-macchi-cb maximo-macchi-cb self-assigned this Jan 15, 2026
@cb-heimdall
Copy link
Copy Markdown
Collaborator

cb-heimdall commented Jan 15, 2026

✅ Heimdall Review Status

Requirement Status More Info
Reviews 1/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 1
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1
CODEOWNERS ✅ See below

CODEOWNERS

Code Owner Status Calculation
ui-systems-eng-team 1/1
Denominator calculation
Additional CODEOWNERS Requirement
Show calculation
Sum 0
0
From CODEOWNERS 1
Sum 1

(activeElementIsMenuItemOrOption || secondElementIsMenuItemOrOption))
) {
secondElement.focus();
secondElement?.focus();
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.

Would it be feasible to add tests for FocusTrap for when there are 0, 1, or 2+ children?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good idea! I've rewritten the test file to test <FocusTrap> more directly rather than using <Dropdown> (which is now deprecated).

I didn't include tests for 0 children. Handling that edge case requires making more changes to the <FocusTrap> component. While not a huge lift, I don't think it's an edge case we should be considering. The purpose of the component is to trap focus on focusable elements. The note I added to the docs in this PR calls this out as well. I could add a warning when in a dev environment if that's helpful.

hcopp
hcopp previously approved these changes Jan 22, 2026
@maximo-macchi-cb maximo-macchi-cb force-pushed the max/focus-trap-support-no-focusable-children branch from 8ad6b95 to 725ea64 Compare January 22, 2026 17:16
@maximo-macchi-cb maximo-macchi-cb merged commit ddb633e into master Jan 22, 2026
28 checks passed
@maximo-macchi-cb maximo-macchi-cb deleted the max/focus-trap-support-no-focusable-children branch January 22, 2026 19:47
cb-ekuersch pushed a commit that referenced this pull request Jan 30, 2026
)

* Update FocusTrap to handle single focusable child element

* Remove deprecated Dropdown tests

* Add more basic tests that test component directly

* Update changelogs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants