-
Notifications
You must be signed in to change notification settings - Fork 13.4k
fix(overlays): show focus indicators only during keyboard navigation and improve focus management #31165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: next
Are you sure you want to change the base?
fix(overlays): show focus indicators only during keyboard navigation and improve focus management #31165
Changes from all commits
641d05e
3278a01
808c193
ceecf4d
a3377a3
17983b1
add3c34
31665fa
1aa5b7c
641419f
395987c
ea55440
38259e2
4bd07bf
7abfe13
1d0b267
29e7e8e
a11a5c9
9e200ed
272c7f7
f4a33db
7951ac3
26048b9
86edcb5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably? Just so we don't forget it in the future
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, let's create a ticket and then can you link it to the checkbox modular migration ticket? If it's not done by then, we can just add it during the migration.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually created FW-7585 already for this #31165 (comment) which requests the same thing. I linked it to the modular migration ticket. |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -259,7 +259,12 @@ export class Item implements ComponentInterface, AnchorInterface, ButtonInterfac | |||||
|
|
||||||
| private isFocusable(): boolean { | ||||||
| const focusableChild = this.el.querySelector('.ion-focusable'); | ||||||
| return this.canActivate() || focusableChild !== null; | ||||||
| // An item is focusable when it can receive keyboard focus: when it is | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the colon supposed to be a comma?
Suggested change
|
||||||
| // clickable (has a `button` or `href`), when it has a single input cover | ||||||
| // (e.g. a radio or checkbox), or when it contains a focusable child. | ||||||
| // Focusable items get the `ion-focusable` class so the `ion-focused` | ||||||
| // class is applied while tabbing through them. | ||||||
| return this.isClickable() || this.hasCover() || focusableChild !== null; | ||||||
| } | ||||||
|
|
||||||
| private hasStartEl() { | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -391,6 +391,75 @@ configs({ modes: ['ios', 'ionic-ios'], directions: ['ltr'] }).forEach(({ title, | |
|
|
||
| await expect(dragHandle).toBeFocused(); | ||
| }); | ||
|
|
||
| test('it should preserve the last arrow-focused radio when tabbing', async ({ page, pageUtils }) => { | ||
| await page.goto('/src/components/modal/test/sheet', config); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this useful since you call page.setContent right after it? I thought setContent fully replaced the page's content, but I may be mistaken and am just curious
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm under the same assumption. I'm assuming it was from a copy and paste. |
||
|
|
||
| await page.setContent( | ||
| ` | ||
| <ion-app> | ||
| <ion-button id="open-modal">Open</ion-button> | ||
| <ion-modal trigger="open-modal"> | ||
| <ion-header> | ||
| <ion-toolbar> | ||
| <ion-title>Options</ion-title> | ||
| <ion-buttons slot="end"> | ||
| <ion-button id="cancel-button">Cancel</ion-button> | ||
| </ion-buttons> | ||
| </ion-toolbar> | ||
| </ion-header> | ||
| <ion-content> | ||
| <ion-list> | ||
| <ion-radio-group value="one"> | ||
| <ion-item> | ||
| <ion-radio value="one">One</ion-radio> | ||
| </ion-item> | ||
| <ion-item> | ||
| <ion-radio value="two">Two</ion-radio> | ||
| </ion-item> | ||
| <ion-item> | ||
| <ion-radio value="three">Three</ion-radio> | ||
| </ion-item> | ||
| </ion-radio-group> | ||
| </ion-list> | ||
| </ion-content> | ||
| </ion-modal> | ||
| </ion-app> | ||
| <script> | ||
| const modal = document.querySelector('ion-modal'); | ||
| const cancelButton = document.querySelector('#cancel-button'); | ||
|
|
||
| modal.breakpoints = [0, 0.5, 1]; | ||
| modal.initialBreakpoint = 0.5; | ||
| modal.handleBehavior = 'cycle'; | ||
|
|
||
| cancelButton.addEventListener('click', () => { | ||
| modal.dismiss(); | ||
| }); | ||
| </script> | ||
| `, | ||
| config | ||
| ); | ||
|
|
||
| const ionModalDidPresent = await page.spyOnEvent('ionModalDidPresent'); | ||
|
|
||
| await page.click('#open-modal'); | ||
| await ionModalDidPresent.next(); | ||
|
|
||
| const modal = page.locator('ion-modal'); | ||
| const firstRadio = modal.locator('ion-radio').nth(0); | ||
| const secondRadio = modal.locator('ion-radio').nth(1); | ||
| const handle = modal.locator('.modal-handle'); | ||
|
|
||
| await firstRadio.focus(); | ||
| await expect(firstRadio).toBeFocused(); | ||
|
|
||
| await pageUtils.pressKeys('ArrowDown'); | ||
| await expect(secondRadio).toBeFocused(); | ||
|
|
||
| await pageUtils.pressKeys('Tab'); | ||
| await expect(handle).toBeFocused(); | ||
| }); | ||
| }); | ||
|
|
||
| test.describe(title('sheet modal: drag events'), () => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,18 +23,6 @@ ion-item { | |
| --border-radius: #{globals.$ion-border-radius-400}; | ||
| } | ||
|
|
||
| // TODO(): Remove this when the focus styles are added back to the interface | ||
| ion-item.ion-focused::part(native)::after { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was added to hide the focused state. |
||
| // Your styles for the ::after pseudo element when ion-item is focused | ||
| outline: none; | ||
| } | ||
|
|
||
| ion-item.ion-focused.item-checkbox-checked, | ||
| ion-item.ion-focused.item-radio-checked { | ||
| --background-focused: #{globals.$ion-bg-primary-subtle-default}; | ||
| --background-focused-opacity: 1; | ||
| } | ||
|
|
||
| // Toolbar | ||
| // ---------------------------------------------------------------- | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.