Skip to content

Commit 32bc681

Browse files
thetaPCsean-perkinsbrandyscarney
authored
fix(picker): update keyboard navigation (#29497)
Issue number: N/A --------- <!-- Please do not submit updates to dependencies unless it fixes an issue. --> <!-- Please try to limit your pull request to one type (bugfix, feature, etc). Submit multiple pull requests if needed. --> ## What is the current behavior? <!-- Please describe the current behavior that you are modifying. --> The tests for picker were failing because the focus was not set to the appropriate element and because Firefox would focus on an element it shouldn't be. ## What is the new behavior? <!-- Please describe the behavior or changes that are being added by this PR. --> - Adjusted the `setFocus()` - Skipped the `picker-opts` in the tab order - Removed `skip()` from the tests ## Does this introduce a breaking change? - [ ] Yes - [x] No <!-- If this introduces a breaking change: 1. Describe the impact and migration path for existing applications below. 2. Update the BREAKING.md file with the breaking change. 3. Add "BREAKING CHANGE: [...]" to the commit description when merging. See https://github.com/ionic-team/ionic-framework/blob/main/docs/CONTRIBUTING.md#footer for more information. --> ## Other information <!-- Any other information that is important to this PR such as screenshots of how the component looks before and after the change. --> N/A --------- Co-authored-by: Sean Perkins <[email protected]> Co-authored-by: Brandy Carney <[email protected]>
1 parent c63d07b commit 32bc681

File tree

3 files changed

+26
-11
lines changed

3 files changed

+26
-11
lines changed

core/src/components/picker-column/picker-column.tsx

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -212,8 +212,8 @@ export class PickerColumn implements ComponentInterface {
212212
*/
213213
@Method()
214214
async setFocus() {
215-
if (this.scrollEl) {
216-
this.scrollEl.focus();
215+
if (this.assistiveFocusable) {
216+
this.assistiveFocusable.focus();
217217
}
218218
}
219219

@@ -619,7 +619,7 @@ export class PickerColumn implements ComponentInterface {
619619
}
620620

621621
if (newOption !== null) {
622-
this.value = newOption.value;
622+
this.setValue(newOption.value);
623623

624624
// This stops any default browser behavior such as scrolling
625625
ev.preventDefault();
@@ -687,6 +687,25 @@ export class PickerColumn implements ComponentInterface {
687687
ref={(el) => {
688688
this.scrollEl = el;
689689
}}
690+
/**
691+
* When an element has an overlay scroll style and
692+
* a fixed height, Firefox will focus the scrollable
693+
* container if the content exceeds the container's
694+
* dimensions.
695+
*
696+
* This causes keyboard navigation to focus to this
697+
* element instead of going to the next element in
698+
* the tab order.
699+
*
700+
* The desired behavior is for the user to be able to
701+
* focus the assistive focusable element and tab to
702+
* the next element in the tab order. Instead of tabbing
703+
* to this element.
704+
*
705+
* To prevent this, we set the tabIndex to -1. This
706+
* will match the behavior of the other browsers.
707+
*/
708+
tabIndex={-1}
690709
>
691710
<div class="picker-item-empty" aria-hidden="true">
692711
&nbsp;

core/src/components/picker/test/basic/picker.e2e.ts

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -105,8 +105,7 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) =>
105105
);
106106
});
107107

108-
// TODO(FW-6232): remove this skip when tabbing is working properly
109-
test.skip('tabbing should correctly move focus between columns', async ({ page }) => {
108+
test('tabbing should correctly move focus between columns', async ({ page }) => {
110109
const firstColumn = page.locator('ion-picker-column#first');
111110
const secondColumn = page.locator('ion-picker-column#second');
112111

@@ -121,8 +120,7 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) =>
121120
await expect(secondColumn).toBeFocused();
122121
});
123122

124-
// TODO(FW-6232): remove this skip when tabbing is working properly
125-
test.skip('tabbing should correctly move focus back', async ({ page }) => {
123+
test('tabbing should correctly move focus back', async ({ page }) => {
126124
const firstColumn = page.locator('ion-picker-column#first');
127125
const secondColumn = page.locator('ion-picker-column#second');
128126

core/src/components/picker/test/keyboard-entry/picker.e2e.ts

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,7 @@ import type { E2ELocator } from '@utils/test/playwright/page/utils/locator';
77
*/
88
configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => {
99
test.describe(title('picker: keyboard entry'), () => {
10-
// TODO(FW-6232): remove this skip when keyboard entry is working properly
11-
test.skip('should scroll to and update the value prop for a single column', async ({ page }) => {
10+
test('should scroll to and update the value prop for a single column', async ({ page }) => {
1211
await page.setContent(
1312
`
1413
<ion-picker>
@@ -123,8 +122,7 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) =>
123122
await expect(secondColumn).toHaveJSProperty('value', 24);
124123
});
125124

126-
// TODO(FW-6232): remove this skip when keyboard entry is working properly
127-
test.skip('should select 00', async ({ page }) => {
125+
test('should select 00', async ({ page }) => {
128126
await page.setContent(
129127
`
130128
<ion-picker>

0 commit comments

Comments
 (0)