fix(overlays): do not return focus if application has already moved focus manually - #28850
Merged
Merged
Conversation
aeharding
added a commit
to aeharding/voyager
that referenced
this pull request
Jan 19, 2024
aeharding
added a commit
to aeharding/voyager
that referenced
this pull request
Jan 19, 2024
Resolves #1061 * Upgrade Ionic for patch: ionic-team/ionic-framework#28850
liamdebeasi
self-requested a review
February 7, 2024 15:07
liamdebeasi
suggested changes
Feb 7, 2024
liamdebeasi
left a comment
Contributor
There was a problem hiding this comment.
Overall looks good! A few changes for tests and maintainability.
| * action sheet) then don't restore focus | ||
| * to previous element | ||
| */ | ||
| if (document.activeElement === null || document.activeElement === document.body) { |
Contributor
There was a problem hiding this comment.
We'll need a test for this. Something like the following should suffice:
test('should not return focus to another element if focus already manually returned', async ({
page,
skip,
}, testInfo) => {
skip.browser(
'webkit',
'WebKit does not consider buttons to be focusable, so this test always passes since the input is the only focusable element.'
);
testInfo.annotations.push({
type: 'issue',
description: 'https://github.com/ionic-team/ionic-framework/issues/28849',
});
await page.setContent(
`
<button id="open-action-sheet">open</button>
<ion-action-sheet trigger="open-action-sheet"></ion-action-sheet>
<input id="test-input" />
<script>
const actionSheet = document.querySelector('ion-action-sheet');
actionSheet.addEventListener('ionActionSheetWillDismiss', () => {
requestAnimationFrame(() => {
document.querySelector('#test-input').focus();
});
});
</script>
`,
config
);
const ionActionSheetDidPresent = await page.spyOnEvent('ionActionSheetDidPresent');
const actionSheet = page.locator('ion-action-sheet');
const input = page.locator('#test-input');
const trigger = page.locator('#open-action-sheet');
// present action sheet
await trigger.click();
await ionActionSheetDidPresent.next();
// dismiss action sheet
await actionSheet.evaluate((el: HTMLIonActionSheetElement) => el.dismiss());
// verify focus is in correct location
await expect(input).toBeFocused();
});You can put this in src/utils/test/overlays/overlays.e2e.ts
| * action sheet) then don't restore focus | ||
| * to previous element | ||
| */ | ||
| if (document.activeElement === null || document.activeElement === document.body) { |
Contributor
There was a problem hiding this comment.
Can we also add a comment that explains focus is always moved to the body when the overlay is dismissed? Maintainers may not know why we specifically check the body here.
Contributor
Author
There was a problem hiding this comment.
I added some MDN references. Apologies if its now a bit too wordy, feel free to make further suggestions!
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue number: resolves #28849
What is the current behavior?
If the developer tries to set focus to a custom element on overlay dismissal, Ionic will always override that focus.
What is the new behavior?
Does this introduce a breaking change?
Other information
In the before video, you can see the text box is focused by developer code when "Mention User" is tapped, which opens the keyboard. Shortly after that, when the bottom sheet fully dismisses, Ionic focuses the button, removing focus from the text box and hiding the keyboard.
In the after, Ionic detects that the developer has already focused the text box and does not change that focus.
RPReplay_Final1705547611.MP4
RPReplay_Final1705547768.MP4