Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions core/src/components/select/select.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -329,7 +329,7 @@ export class Select implements ComponentInterface {
);

if (selectedItem) {
focusElement(selectedItem);
focusVisibleElement(selectedItem);

/**
* Browsers such as Firefox do not
Expand All @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion core/src/utils/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();

/**
Expand Down
93 changes: 72 additions & 21 deletions core/src/utils/overlays.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -131,38 +137,55 @@ export const createOverlay = <T extends HTMLIonOverlayElement>(
*/
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 shadowRoot = firstInput?.shadowRoot;
if (shadowRoot) {
// If there are no inner focusable elements, just focus the host element.
firstInput = shadowRoot.querySelector(focusableQueryString) || firstInput;
}
const firstInput = ref.querySelector(focusableQueryString) as HTMLElement | null;

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');
Copy link
Contributor Author

@liamdebeasi liamdebeasi Feb 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there should be no behavior change to focusFirstDescendant and focusLastDescendant. All I did here was abstract the focusing logic to focusElementInOverlay and had those functions call it instead. I needed to use focusElementInOverlay to implement the fix, so I figured I should consolidate otherwise we're writing the same code 3x.


/**
* 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 <button>
* element inside of ion-button's shadow root, not
* the host element itself.
*/
const focusElementInOverlay = (hostToFocus: HTMLElement | null | undefined, overlay: HTMLIonOverlayElement) => {
let elementToFocus = hostToFocus;

const shadowRoot = lastInput?.shadowRoot;
const shadowRoot = hostToFocus?.shadowRoot;
if (shadowRoot) {
// If there are no inner focusable elements, just focus the host element.
lastInput = shadowRoot.querySelector(focusableQueryString) || lastInput;
elementToFocus = shadowRoot.querySelector<HTMLElement>(focusableQueryString) || hostToFocus;
}

if (lastInput) {
lastInput.focus();
if (elementToFocus) {
focusVisibleElement(elementToFocus);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I noticed was that focusLastDescendant called .focus() whereas focusFirstDescendant calls focusVisibleElement. I believe this was an oversight on my end when I originally implemented this since I was only concerned about moving focus to the first element in the select popover: 8be9ae1

However, focusVisibleElement also calls .focus so there should be no behavior change.

} else {
// Focus overlay instead of letting focus escape
overlay.focus();
Expand Down Expand Up @@ -219,6 +242,20 @@ const trapKeyboardFocus = (ev: Event, doc: Document) => {
*/
if (lastOverlay === target) {
lastOverlay.lastFocus = undefined;
/**
* Toasts can be presented from an overlay.
* However, focus should still be returned to
* the overlay when clicking a toast. Normally,
* focus would be returned to the last focusable
* descendant in the overlay which may not always be
* the button that the toast was presented from. In this case,
* the focus may be returned to an unexpected element.
* To account for this, we make sure to return focus to the
* last focused element in the overlay if focus is
* moved to the toast.
*/
} else if (target.tagName === 'ION-TOAST') {
focusElementInOverlay(lastOverlay.lastFocus, lastOverlay);

/**
* Otherwise, we must be focusing an element
Expand Down Expand Up @@ -295,6 +332,20 @@ const trapKeyboardFocus = (ev: Event, doc: Document) => {
*/
if (lastOverlay.contains(target)) {
lastOverlay.lastFocus = target;
/**
* Toasts can be presented from an overlay.
* However, focus should still be returned to
* the overlay when clicking a toast. Normally,
* focus would be returned to the last focusable
* descendant in the overlay which may not always be
* the button that the toast was presented from. In this case,
* the focus may be returned to an unexpected element.
* To account for this, we make sure to return focus to the
* last focused element in the overlay if focus is
* moved to the toast.
*/
} else if (target.tagName === 'ION-TOAST') {
focusElementInOverlay(lastOverlay.lastFocus, lastOverlay);
} else {
/**
* Otherwise, we are about to have focus
Expand Down
120 changes: 120 additions & 0 deletions core/src/utils/test/overlays/overlays.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,126 @@ 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 }) => {
test.info().annotations.push({
type: 'issue',
description: 'https://github.com/ionic-team/ionic-framework/issues/28261',
});

/**
* 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(
`
<ion-modal>
<ion-content>
<input id="show-toast">Button A</input>
<button>Button B</button>
<ion-toast trigger="show-toast"></ion-toast>
</ion-content>
</ion-modal>

<script>
const toast = document.querySelector('ion-toast');
toast.buttons = ['Ok'];
</script>
`,
config
);

const modal = page.locator('ion-modal');
const showToastTrigger = 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 trigger to open toast
await showToastTrigger.click();

// Wait for toast to be presented
await ionToastDidPresent.next();

// 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 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 ({
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(
`
<ion-action-sheet></ion-action-sheet>
<ion-toast></ion-toast>

<script>
const actionSheet = document.querySelector('ion-action-sheet');
actionSheet.buttons = [
'Other Button',
{
text: 'Button',
id: 'show-toast',
handler: () => {
document.querySelector('ion-toast').present();
return false;
}
}
];

const toast = document.querySelector('ion-toast');
toast.buttons = ['Ok'];
</script>
`,
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();
});
test('should not return focus to another element if focus already manually returned', async ({
page,
skip,
Expand Down