From ad3675bcbf9c61827fe4c3e5f330147cb8e683d4 Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Wed, 31 Jan 2024 16:21:50 -0500 Subject: [PATCH 1/7] refactor: clean up implementation, rename function for clarity --- core/src/components/select/select.tsx | 6 +-- core/src/utils/helpers.ts | 2 +- core/src/utils/overlays.ts | 70 +++++++++++++++++++-------- 3 files changed, 53 insertions(+), 25 deletions(-) diff --git a/core/src/components/select/select.tsx b/core/src/components/select/select.tsx index 9fe26349bb1..e15ae27e653 100644 --- a/core/src/components/select/select.tsx +++ b/core/src/components/select/select.tsx @@ -2,7 +2,7 @@ import type { ComponentInterface, EventEmitter } from '@stencil/core'; import { Component, Element, Event, Host, Method, Prop, State, Watch, h, forceUpdate } from '@stencil/core'; import type { LegacyFormController, NotchController } from '@utils/forms'; import { compareOptions, createLegacyFormController, createNotchController, isOptionSelected } from '@utils/forms'; -import { findItemLabel, focusElement, getAriaLabel, renderHiddenInput, inheritAttributes } from '@utils/helpers'; +import { findItemLabel, focusVisibleElement, getAriaLabel, renderHiddenInput, inheritAttributes } from '@utils/helpers'; import type { Attributes } from '@utils/helpers'; import { printIonWarning } from '@utils/logging'; import { actionSheetController, alertController, popoverController } from '@utils/overlays'; @@ -329,7 +329,7 @@ export class Select implements ComponentInterface { ); if (selectedItem) { - focusElement(selectedItem); + focusVisibleElement(selectedItem); /** * Browsers such as Firefox do not @@ -355,7 +355,7 @@ export class Select implements ComponentInterface { 'ion-radio:not(.radio-disabled), ion-checkbox:not(.checkbox-disabled)' ); if (firstEnabledOption) { - focusElement(firstEnabledOption.closest('ion-item')!); + focusVisibleElement(firstEnabledOption.closest('ion-item')!); /** * Focus the option for the same reason as we do above. diff --git a/core/src/utils/helpers.ts b/core/src/utils/helpers.ts index 3dff5a5e657..81ffb4efa92 100644 --- a/core/src/utils/helpers.ts +++ b/core/src/utils/helpers.ts @@ -262,7 +262,7 @@ export const findItemLabel = (componentEl: HTMLElement): HTMLIonLabelElement | n return null; }; -export const focusElement = (el: HTMLElement) => { +export const focusVisibleElement = (el: HTMLElement) => { el.focus(); /** diff --git a/core/src/utils/overlays.ts b/core/src/utils/overlays.ts index f4eaad1e4cf..82023b79136 100644 --- a/core/src/utils/overlays.ts +++ b/core/src/utils/overlays.ts @@ -22,7 +22,13 @@ import type { import { CoreDelegate } from './framework-delegate'; import { OVERLAY_BACK_BUTTON_PRIORITY } from './hardware-back-button'; -import { addEventListener, componentOnReady, focusElement, getElementRoot, removeEventListener } from './helpers'; +import { + addEventListener, + componentOnReady, + focusVisibleElement, + getElementRoot, + removeEventListener, +} from './helpers'; import { printIonWarning } from './logging'; let lastOverlayIndex = 0; @@ -131,38 +137,55 @@ export const createOverlay = ( */ const focusableQueryString = '[tabindex]:not([tabindex^="-"]):not([hidden]):not([disabled]), input:not([type=hidden]):not([tabindex^="-"]):not([hidden]):not([disabled]), textarea:not([tabindex^="-"]):not([hidden]):not([disabled]), button:not([tabindex^="-"]):not([hidden]):not([disabled]), select:not([tabindex^="-"]):not([hidden]):not([disabled]), .ion-focusable:not([tabindex^="-"]):not([hidden]):not([disabled]), .ion-focusable[disabled="false"]:not([tabindex^="-"]):not([hidden])'; +const isOverlayHidden = (overlay: Element) => overlay.classList.contains('overlay-hidden'); +/** + * Focuses the first descendant in an overlay + * that can receive focus. If none exists, + * the entire overlay will be focused. + */ export const focusFirstDescendant = (ref: Element, overlay: HTMLIonOverlayElement) => { - let firstInput = ref.querySelector(focusableQueryString) as HTMLElement | null; + const firstInput = ref.querySelector(focusableQueryString) as HTMLElement | null; - const shadowRoot = firstInput?.shadowRoot; - if (shadowRoot) { - // If there are no inner focusable elements, just focus the host element. - firstInput = shadowRoot.querySelector(focusableQueryString) || firstInput; - } - - if (firstInput) { - focusElement(firstInput); - } else { - // Focus overlay instead of letting focus escape - overlay.focus(); - } + focusElementInOverlay(firstInput, overlay); }; -const isOverlayHidden = (overlay: Element) => overlay.classList.contains('overlay-hidden'); - +/** + * Focuses the last descendant in an overlay + * that can receive focus. If none exists, + * the entire overlay will be focused. + */ const focusLastDescendant = (ref: Element, overlay: HTMLIonOverlayElement) => { const inputs = Array.from(ref.querySelectorAll(focusableQueryString)) as HTMLElement[]; - let lastInput = inputs.length > 0 ? inputs[inputs.length - 1] : null; + const lastInput = inputs.length > 0 ? inputs[inputs.length - 1] : null; + + focusElementInOverlay(lastInput, overlay); +}; + +/** + * Focuses a particular element in an overlay. If the element + * doesn't have anything focusable associated with it then + * the overlay itself will be focused. + * This should be used instead of the focus() method + * on most elements because the focusable element + * may not be the host element. + * + * For example, if an ion-button should be focused + * then we should actually focus the native + + + + + + + `, + config + ); + + const modal = page.locator('ion-modal'); + const showToastButton = page.locator('#show-toast'); + + const toast = page.locator('ion-toast'); + const toastButton = toast.locator('button'); + + const ionToastDidPresent = await page.spyOnEvent('ionToastDidPresent'); + + // Show overlay + await modal.evaluate((el: HTMLIonModalElement) => el.present()); + + // Click button to open toast + await showToastButton.click(); + + // Wait for toast to be presented + await ionToastDidPresent.next(); + + // Verify button in overlay is focused + await expect(showToastButton).toBeFocused(); + + // Click a button in the toast and therefore attempt to move focus + await toastButton.click(); + + // Verify button in overlay is still focused + await expect(showToastButton).toBeFocused(); + }); + + test.only('focusing toast from a scoped overlay should return focus to the last focused element', async ({ + page, + skip, + }) => { + test.info().annotations.push({ + type: 'issue', + description: 'https://github.com/ionic-team/ionic-framework/issues/28261', + }); + skip.browser('webkit', 'WebKit does not consider buttons to be focusable'); + + await page.setContent( + ` + + + + + `, + config + ); + + const actionSheet = page.locator('ion-action-sheet'); + const showToastButton = page.locator('#show-toast'); + + const toast = page.locator('ion-toast'); + const toastButton = toast.locator('button'); + + const ionToastDidPresent = await page.spyOnEvent('ionToastDidPresent'); + + // Show overlay + await actionSheet.evaluate((el: HTMLIonActionSheetElement) => el.present()); + + // Click button to open toast + await showToastButton.click(); + + // Wait for toast to be presented + await ionToastDidPresent.next(); + + // Verify button in overlay is focused + await expect(showToastButton).toBeFocused(); + + // Click a button in the toast and therefore attempt to move focus + await toastButton.click(); + + await page.pause(); + + // Verify button in overlay is still focused + await expect(showToastButton).toBeFocused(); + }); }); }); From 86eea70ff773fa38b93f91a750a7918dcb1290e8 Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Tue, 6 Feb 2024 16:03:01 -0500 Subject: [PATCH 6/7] remove .only --- core/src/utils/test/overlays/overlays.e2e.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/utils/test/overlays/overlays.e2e.ts b/core/src/utils/test/overlays/overlays.e2e.ts index 9d1784836b3..4fd5870ddf1 100644 --- a/core/src/utils/test/overlays/overlays.e2e.ts +++ b/core/src/utils/test/overlays/overlays.e2e.ts @@ -255,7 +255,7 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => await expect(modalInputOne).toBeFocused(); }); - test.only('focusing toast from a shadow overlay should return focus to the last focused element', async ({ + test('focusing toast from a shadow overlay should return focus to the last focused element', async ({ page, skip, }) => { @@ -310,7 +310,7 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => await expect(showToastButton).toBeFocused(); }); - test.only('focusing toast from a scoped overlay should return focus to the last focused element', async ({ + test('focusing toast from a scoped overlay should return focus to the last focused element', async ({ page, skip, }) => { From 5d0121bd9eda801f9d618c30df58ba5ae2c5297c Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Wed, 7 Feb 2024 12:12:25 -0500 Subject: [PATCH 7/7] test: update test so we can run on webkit --- core/src/utils/test/overlays/overlays.e2e.ts | 27 ++++++++++---------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/core/src/utils/test/overlays/overlays.e2e.ts b/core/src/utils/test/overlays/overlays.e2e.ts index 4fd5870ddf1..92d368fd7a4 100644 --- a/core/src/utils/test/overlays/overlays.e2e.ts +++ b/core/src/utils/test/overlays/overlays.e2e.ts @@ -255,21 +255,22 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => await expect(modalInputOne).toBeFocused(); }); - test('focusing toast from a shadow overlay should return focus to the last focused element', async ({ - page, - skip, - }) => { + test('focusing toast from a shadow overlay should return focus to the last focused element', async ({ page }) => { test.info().annotations.push({ type: 'issue', description: 'https://github.com/ionic-team/ionic-framework/issues/28261', }); - skip.browser('webkit', 'WebKit does not consider buttons to be focusable'); + /** + * Triggers for an overlay are typically buttons. However in this case, + * buttons are not considered keyboard focusable by WebKit. Inputs are, + * so we use an input here so we can still test on WebKit. + */ await page.setContent( ` - + Button A @@ -284,7 +285,7 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => ); const modal = page.locator('ion-modal'); - const showToastButton = page.locator('#show-toast'); + const showToastTrigger = page.locator('#show-toast'); const toast = page.locator('ion-toast'); const toastButton = toast.locator('button'); @@ -294,20 +295,20 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => // Show overlay await modal.evaluate((el: HTMLIonModalElement) => el.present()); - // Click button to open toast - await showToastButton.click(); + // Click trigger to open toast + await showToastTrigger.click(); // Wait for toast to be presented await ionToastDidPresent.next(); - // Verify button in overlay is focused - await expect(showToastButton).toBeFocused(); + // Verify trigger in overlay is focused + await expect(showToastTrigger).toBeFocused(); // Click a button in the toast and therefore attempt to move focus await toastButton.click(); - // Verify button in overlay is still focused - await expect(showToastButton).toBeFocused(); + // Verify trigger in overlay is still focused + await expect(showToastTrigger).toBeFocused(); }); test('focusing toast from a scoped overlay should return focus to the last focused element', async ({