From 32bc681192b1833f1c897e692d2d36ba90c6af53 Mon Sep 17 00:00:00 2001
From: Maria Hutt <thetaPC@users.noreply.github.com>
Date: Wed, 15 May 2024 08:02:57 -0700
Subject: [PATCH] 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 <sean.perkins@outsystems.com>
Co-authored-by: Brandy Carney <brandyscarney@gmail.com>
---
 .../picker-column/picker-column.tsx           | 25 ++++++++++++++++---
 .../picker/test/basic/picker.e2e.ts           |  6 ++---
 .../picker/test/keyboard-entry/picker.e2e.ts  |  6 ++---
 3 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/core/src/components/picker-column/picker-column.tsx b/core/src/components/picker-column/picker-column.tsx
index 16f7cbaeb4..0b4f57dd8a 100644
--- a/core/src/components/picker-column/picker-column.tsx
+++ b/core/src/components/picker-column/picker-column.tsx
@@ -212,8 +212,8 @@ export class PickerColumn implements ComponentInterface {
    */
   @Method()
   async setFocus() {
-    if (this.scrollEl) {
-      this.scrollEl.focus();
+    if (this.assistiveFocusable) {
+      this.assistiveFocusable.focus();
     }
   }
 
@@ -619,7 +619,7 @@ export class PickerColumn implements ComponentInterface {
     }
 
     if (newOption !== null) {
-      this.value = newOption.value;
+      this.setValue(newOption.value);
 
       // This stops any default browser behavior such as scrolling
       ev.preventDefault();
@@ -687,6 +687,25 @@ export class PickerColumn implements ComponentInterface {
           ref={(el) => {
             this.scrollEl = el;
           }}
+          /**
+           * When an element has an overlay scroll style and
+           * a fixed height, Firefox will focus the scrollable
+           * container if the content exceeds the container's
+           * dimensions.
+           *
+           * This causes keyboard navigation to focus to this
+           * element instead of going to the next element in
+           * the tab order.
+           *
+           * The desired behavior is for the user to be able to
+           * focus the assistive focusable element and tab to
+           * the next element in the tab order. Instead of tabbing
+           * to this element.
+           *
+           * To prevent this, we set the tabIndex to -1. This
+           * will match the behavior of the other browsers.
+           */
+          tabIndex={-1}
         >
           <div class="picker-item-empty" aria-hidden="true">
             &nbsp;
diff --git a/core/src/components/picker/test/basic/picker.e2e.ts b/core/src/components/picker/test/basic/picker.e2e.ts
index 0c51664a3e..5c4a65caf7 100644
--- a/core/src/components/picker/test/basic/picker.e2e.ts
+++ b/core/src/components/picker/test/basic/picker.e2e.ts
@@ -105,8 +105,7 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) =>
       );
     });
 
-    // TODO(FW-6232): remove this skip when tabbing is working properly
-    test.skip('tabbing should correctly move focus between columns', async ({ page }) => {
+    test('tabbing should correctly move focus between columns', async ({ page }) => {
       const firstColumn = page.locator('ion-picker-column#first');
       const secondColumn = page.locator('ion-picker-column#second');
 
@@ -121,8 +120,7 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) =>
       await expect(secondColumn).toBeFocused();
     });
 
-    // TODO(FW-6232): remove this skip when tabbing is working properly
-    test.skip('tabbing should correctly move focus back', async ({ page }) => {
+    test('tabbing should correctly move focus back', async ({ page }) => {
       const firstColumn = page.locator('ion-picker-column#first');
       const secondColumn = page.locator('ion-picker-column#second');
 
diff --git a/core/src/components/picker/test/keyboard-entry/picker.e2e.ts b/core/src/components/picker/test/keyboard-entry/picker.e2e.ts
index 3aff0cc767..64d4a1cce1 100644
--- a/core/src/components/picker/test/keyboard-entry/picker.e2e.ts
+++ b/core/src/components/picker/test/keyboard-entry/picker.e2e.ts
@@ -7,8 +7,7 @@ import type { E2ELocator } from '@utils/test/playwright/page/utils/locator';
  */
 configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => {
   test.describe(title('picker: keyboard entry'), () => {
-    // TODO(FW-6232): remove this skip when keyboard entry is working properly
-    test.skip('should scroll to and update the value prop for a single column', async ({ page }) => {
+    test('should scroll to and update the value prop for a single column', async ({ page }) => {
       await page.setContent(
         `
         <ion-picker>
@@ -123,8 +122,7 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) =>
       await expect(secondColumn).toHaveJSProperty('value', 24);
     });
 
-    // TODO(FW-6232): remove this skip when keyboard entry is working properly
-    test.skip('should select 00', async ({ page }) => {
+    test('should select 00', async ({ page }) => {
       await page.setContent(
         `
         <ion-picker>