fix(overlays): show focus indicators only during keyboard navigation and improve focus management#31165
fix(overlays): show focus indicators only during keyboard navigation and improve focus management#31165brandyscarney wants to merge 24 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| @@ -114,10 +114,6 @@ slot[name="end"]::slotted(*) { | |||
|
|
|||
| // Item in Select Modal | |||
| // -------------------------------------------------- | |||
| :host(.in-select-modal) { | |||
There was a problem hiding this comment.
This was added to hide the focused state.
| @@ -15,11 +15,6 @@ ion-item { | |||
| --border-width: 0; | |||
| } | |||
|
|
|||
| ion-item.ion-focused::part(native)::after { | |||
There was a problem hiding this comment.
This was added to hide the focused state.
There was a problem hiding this comment.
The ionic theme wasn't added to this test so now it is generating screenshots.
There was a problem hiding this comment.
The focus indicator is no longer displayed when opening with a mouse click.
There was a problem hiding this comment.
The focus indicator is no longer displayed when opening with a mouse click.
There was a problem hiding this comment.
The focus indicator is no longer displayed when opening with a mouse click.
There was a problem hiding this comment.
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 }) => { | |||
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.'); | ||
|
|
There was a problem hiding this comment.
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.
ShaneK
left a comment
There was a problem hiding this comment.
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[] => { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
| 'checkbox-disabled': disabled, | ||
| 'ion-focusable': true, | ||
| // Focus styling should not apply when the checkbox is in an item | ||
| 'ion-focusable': !inItem, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This is indeed an issue but it is a bigger issue than I think this ticket should handle. After my change, an item with two checkboxes shows no focus state on the item or the individual checkboxes, but before this change it didn't show a focus state for the individual checkboxes either. Radio seems to have the same pre-existing issue.
The bigger problem here seems to be Checkbox lacks focus styles entirely for ios and md. Radio has them with the ion-focused class, see:
But Checkbox doesn't have any at all:
As a result I have created two tickets to follow up on this:
- Add checkbox focus indicator:
FW-7585 - Multiple inputs in item lack individual focus:
FW-7586
| * test files. | ||
| */ | ||
| configs({ directions: ['ltr'] }).forEach(({ title, config, screenshot }) => { | ||
| configs({ modes: ['ios', 'md', 'ionic-md'], directions: ['ltr'] }).forEach(({ title, config, screenshot }) => { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yes, I think adding ionic-ios would add a lot of screenshots with no benefit. The visual difference between the ios and md modes is nonexistent because the ionic theme applies the same structure and styles regardless of the mode.
Here is the same screenshot for the ionic theme in each mode:
ionic-md |
ionic-ios |
|---|---|
![]() |
![]() |
I also confirmed there are no behavioral differences between the two modes across all four select interfaces:
- Identical focusable elements rendered in both modes.
- The focus sequence through each interface is the same, as the focus-trap and roving focus logic are not mode-dependent.
So ionic-ios coverage here would only duplicate ionic-md. If we add different behavior in the future based on the mode I could see a reason to add it.


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:
What is the new behavior?
Shift + Tabto go backwards in orderDoes this introduce a breaking change?
Other information