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 18 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?
| '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.
| * 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?
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