Skip to content

Commit

Permalink
fix(overlays): focus is returned to last focus element when focusing …
Browse files Browse the repository at this point in the history
…toast (#28950)

Issue number: resolves #28261

---------

<!-- 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. -->

When moving focus from a focus-trapped overlay to a toast, focus is
moved back to the overlay. This is the correct behavior as focus should
never leave a focus-trapped overlay (unless the overlay is dismissed or
focus is moved to a _new_ top-most overlay). However, the way we return
focus is a bit unexpected because it always returns focus to the last
focusable element in the overlay.

This means that if you were focused on the first focusable element,
presented the toast, and then focused the toast, focus might not be
moved back to that first focusable element. In the case of the linked
issue, this was causing an unexpected scroll so that the last focused
element could be in view.

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- This fix adds an exception for `ion-toast` (as it is the only overlay
that is **not** focus trapped) that ensures that focus is moved back to
the last focus element.

## 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/.github/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. -->


Dev build: `7.7.1-dev.11707253408.186eea70`

Note: We don't recommend this pattern in general because it would be
impossible for a screen reader user to focus the toast. However, we can
at least improve the experience for developers who continue to implement
this pattern by returning focus in a more predictable manner.

Docs: ionic-team/ionic-docs#3432

Testing: 

Reviewers should manually test the following behaviors:

1. Create a modal with 2 buttons. Have one of the buttons present a
toast. Open the toast and verify that you can still Tab to cycle through
the buttons in the modal.
2. Create a modal with 2 buttons. Have one of the buttons present a
toast. Open the toast. Move focus to the toast and verify that you can
still Tab to cycle through the buttons in the modal (once focus is
returned to the modal).
  • Loading branch information
liamdebeasi authored Feb 14, 2024
1 parent 1fc4b76 commit 2ed0ada
Show file tree
Hide file tree
Showing 4 changed files with 196 additions and 25 deletions.
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');

/**
* 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);
} 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

0 comments on commit 2ed0ada

Please sign in to comment.