Skip to content

fix(overlays): show focus indicators only during keyboard navigation and improve focus management#31165

Open
brandyscarney wants to merge 18 commits into
nextfrom
FW-6922
Open

fix(overlays): show focus indicators only during keyboard navigation and improve focus management#31165
brandyscarney wants to merge 18 commits into
nextfrom
FW-6922

Conversation

@brandyscarney
Copy link
Copy Markdown
Member

@brandyscarney brandyscarney commented May 22, 2026

Issue number: internal


What is the current behavior?

The focus indicator is displayed on the option when opening certain interfaces by clicking on the Select component:

Modal Popover
select-modal select-popover

What is the new behavior?

  • Hide the focus indicator when the Select component is opened using pointer events
  • Display the focus indicator when the Select component is opened using keyboard navigation (Space or Enter)
  • Cycle focus through the Select elements (radios, handle, cancel buttons) in the correct order and allow Shift + Tab to go backwards in order
  • Removes the native focus from Checkbox and does not allow focusable when inside of an item (this matches Radio's behavior)
  • Adds e2e tests for focus functionality in both Select and Modal to avoid future regressions

Does this introduce a breaking change?

  • Yes
  • No

Other information

@brandyscarney brandyscarney requested a review from a team as a code owner May 22, 2026 16:22
@brandyscarney brandyscarney requested a review from thetaPC May 22, 2026 16:22
@brandyscarney brandyscarney marked this pull request as draft May 22, 2026 16:22
@vercel
Copy link
Copy Markdown

vercel Bot commented May 22, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
ionic-framework Ready Ready Preview, Comment Jun 2, 2026 5:53pm

Request Review

@@ -114,10 +114,6 @@ slot[name="end"]::slotted(*) {

// Item in Select Modal
// --------------------------------------------------
:host(.in-select-modal) {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This was added to hide the focused state.

@@ -15,11 +15,6 @@ ion-item {
--border-width: 0;
}

ion-item.ion-focused::part(native)::after {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This was added to hide the focused state.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The ionic theme wasn't added to this test so now it is generating screenshots.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The focus indicator is no longer displayed when opening with a mouse click.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The focus indicator is no longer displayed when opening with a mouse click.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The focus indicator is no longer displayed when opening with a mouse click.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The focus indicator is no longer displayed when opening with a mouse click.

@@ -8,7 +8,7 @@ import { configs, test } from '@utils/test/playwright';
* does not. The overlay rendering is already tested in the respective
* test files.
*/
configs({ directions: ['ltr'] }).forEach(({ title, config, screenshot }) => {
configs({ modes: ['ios', 'md', 'ionic-md'], directions: ['ltr'] }).forEach(({ title, config, screenshot }) => {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This now checks for the ionic theme and captures screenshots to avoid future focus behavior regressions.

'checkbox-checked': checked,
'checkbox-disabled': disabled,
'ion-focusable': true,
// Focus styling should not apply when the checkbox is in an item
'ion-focusable': !inItem,
Copy link
Copy Markdown
Member Author

@brandyscarney brandyscarney Jun 1, 2026

Choose a reason for hiding this comment

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

This matches radio behavior. Otherwise when a checkbox in an item has focus (such as inside of a select popover) the item and checkbox will both show a focus indicator.

Copy link
Copy Markdown
Member Author

@brandyscarney brandyscarney Jun 2, 2026

Choose a reason for hiding this comment

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

This was removed intentionally because we don't want to show the native focus when we are adding our own. We need to add some ion-focused styles to checkbox for ios and md to fix this. Should I make a follow-up?

test('holding Enter to open a popover should not immediately dismiss it', async ({ page, skip }, testInfo) => {
// TODO (ROU-5437)
skip.browser('webkit', 'Safari 16 only allows text fields and pop-up menus to be focused.');

Copy link
Copy Markdown
Member Author

@brandyscarney brandyscarney Jun 2, 2026

Choose a reason for hiding this comment

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

I updated the keyboard presses to this:

-await page.keyboard.press('Enter');
+await pageUtils.pressKeys('Enter');

This handles Safari tabbing so we are able to check Safari again in most of these tests.

@brandyscarney brandyscarney changed the title fix(overlays): properly focus elements in a sheet modal fix(overlays): show focus indicators only during keyboard navigation and improve focus management Jun 2, 2026
@brandyscarney brandyscarney marked this pull request as ready for review June 2, 2026 18:27
Copy link
Copy Markdown
Member

@ShaneK ShaneK left a comment

Choose a reason for hiding this comment

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

This is looking really good! I tested the functionality and the changes here seem to work fine, but I noticed some things and just wanted a bit of clarification. Good work so far, though!

* Order: option controls (radio/checkbox) → sheet handle → end-slot
* header button. Any other focusables are appended after.
*/
const sortSheetModalFocusables = (overlay: HTMLElement, elements: HTMLElement[]): HTMLElement[] => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This adds a fair bit of component-specific structure to the generic overlay trap: sortSheetModalFocusables knows the sheet layout, and elsewhere the trap matches ion-select-modal, .action-sheet-button, .modal-handle, select-interface-option, and the header Cancel button by their private class names. Before this PR the trap only knew to exclude ion-toast, so each of these selectors is now an implicit contract that breaks Tab order if those components get restyled. The roving-tabindex resolution feels general enough to live here, but would it be cleaner for the components to tag their participating elements with a shared marker (a data- attr or one documented class) the trap reads generically? Or I guess we could at least leave a note at each class definition that it's load-bearing for the focus trap?

'checkbox-disabled': disabled,
'ion-focusable': true,
// Focus styling should not apply when the checkbox is in an item
'ion-focusable': !inItem,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Bringing checkbox to parity with radio here makes sense. One edge case worth confirming: in a multi-input item hasCover() is false, so the item won't carry ion-focusable either, and the checkbox was the .ion-focusable child the item relied on. Does a checkbox sitting next to a second input still show a focus ring somewhere when you tab to it? Radio has the same shape today so I don't think it's a blocker, just want to make sure it's intentional.

* test files.
*/
configs({ directions: ['ltr'] }).forEach(({ title, config, screenshot }) => {
configs({ modes: ['ios', 'md', 'ionic-md'], directions: ['ltr'] }).forEach(({ title, config, screenshot }) => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You're widening this to ionic-md but not ionic-ios, even though the item.ionic.scss and select-modal.ionic.scss changes apply to both Ionic modes. Was leaving out ionic-ios deliberate?

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

Labels

package: core @ionic/core package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants