From 292d2fbc15acd1b8edaf7a5de2d564421ebd57f6 Mon Sep 17 00:00:00 2001 From: Henry Heino <46334387+personalizedrefrigerator@users.noreply.github.com> Date: Sat, 3 Aug 2024 08:43:16 -0700 Subject: [PATCH] Desktop: Accessibility: Restore keyboard focus when closing a dialog (#10817) --- packages/app-desktop/gui/Dialog.tsx | 153 +++++++++++++----- .../gui/styles/dialog-modal-layer.scss | 4 + .../integration-tests/goToAnything.spec.ts | 33 ++++ packages/app-desktop/package.json | 1 + yarn.lock | 10 ++ 5 files changed, 161 insertions(+), 40 deletions(-) diff --git a/packages/app-desktop/gui/Dialog.tsx b/packages/app-desktop/gui/Dialog.tsx index 299210b97..59e6b514e 100644 --- a/packages/app-desktop/gui/Dialog.tsx +++ b/packages/app-desktop/gui/Dialog.tsx @@ -1,55 +1,128 @@ import * as React from 'react'; -import { MouseEventHandler, ReactEventHandler, ReactNode, useCallback, useEffect, useRef, useState } from 'react'; +import { ReactNode, useEffect, useRef, useState } from 'react'; +import { createPortal } from 'react-dom'; +import { blur, focus } from '@joplin/lib/utils/focusHandler'; + +type OnCancelListener = ()=> void; interface Props { className?: string; - onCancel?: ()=> void; + onCancel?: OnCancelListener; contentStyle?: React.CSSProperties; children: ReactNode; } -export default function Dialog(props: Props) { - const [dialogElement, setDialogRef] = useState(); +const Dialog: React.FC = props => { + // For correct focus handling, the dialog element needs to be managed separately from React. In particular, + // just after creating the dialog, we need to call .showModal() and just **before** closing the dialog, we + // need to call .close(). This second requirement is particularly difficult, as this needs to happen even + // if the dialog is closed by removing its parent from the React DOM. + // + // Because useEffect cleanup can happen after an element is removed from the HTML DOM, the dialog is managed + // using native HTML APIs. This allows us to call .close() while the dialog is still attached to the DOM, which + // allows the browser to restore the focus from before the dialog was opened. + const dialogElement = useDialogElement(props.onCancel); + useDialogClassNames(dialogElement, props.className); + + const [contentRendered, setContentRendered] = useState(false); useEffect(() => { - if (!dialogElement) return; + if (!dialogElement || !contentRendered) return; - // Use .showModal instead of the open attribute: .showModal correctly - // traps the keyboard focus in the dialog - dialogElement.showModal(); - }, [dialogElement]); - - const onCancelRef = useRef(props.onCancel); - onCancelRef.current = props.onCancel; - - const onCancel: ReactEventHandler = useCallback((event) => { - const canCancel = !!onCancelRef.current; - if (!canCancel) { - // Prevents [Escape] from closing the dialog. In many places, this is handled - // elsewhere. - // See https://stackoverflow.com/a/61021326 - event.preventDefault(); + if (!dialogElement.open) { + dialogElement.showModal(); } + }, [dialogElement, contentRendered]); + + if (dialogElement && !contentRendered) { + setContentRendered(true); + } + + const content = ( +
+ {props.children} +
+ ); + return <> + {dialogElement && createPortal(content, dialogElement)} + ; +}; + +const useDialogElement = (onCancel: undefined|OnCancelListener) => { + const [dialogElement, setDialogElement] = useState(null); + + const onCancelRef = useRef(onCancel); + onCancelRef.current = onCancel; + + useEffect(() => { + const dialog = document.createElement('dialog'); + dialog.addEventListener('click', event => { + const onCancel = onCancelRef.current; + const isBackgroundClick = event.target === dialog; + if (isBackgroundClick && onCancel) { + onCancel(); + } + }); + dialog.classList.add('dialog-modal-layer'); + dialog.addEventListener('cancel', event => { + const canCancel = !!onCancelRef.current; + if (!canCancel) { + // Prevents [Escape] from closing the dialog. In many places, this is handled + // by external logic. + // See https://stackoverflow.com/a/61021326 + event.preventDefault(); + } + }); + + const removedReturnValue = 'removed-from-dom'; + dialog.addEventListener('close', () => { + const closedByCancel = dialog.returnValue !== removedReturnValue; + if (closedByCancel) { + onCancelRef.current?.(); + } + + // Work around what seems to be an Electron bug -- if an input or contenteditable region is refocused after + // dismissing a dialog, it won't be editable. + // Note: While this addresses the issue in the note title input, it does not address the issue in the Rich Text Editor. + if (document.activeElement?.tagName === 'INPUT') { + const element = document.activeElement as HTMLElement; + blur('Dialog', element); + focus('Dialog', element); + } + }); + document.body.appendChild(dialog); + + setDialogElement(dialog); + + return () => { + if (dialog.open) { + // .close: Instructs the browser to restore keyboard focus to whatever was focused + // before the dialog. + dialog.close(removedReturnValue); + } + dialog.remove(); + }; }, []); - const onContainerClick: MouseEventHandler = useCallback((event) => { - const onCancel = onCancelRef.current; - if (event.target === dialogElement && onCancel) { - onCancel(); - } - }, [dialogElement]); + return dialogElement; +}; - return ( - -
- {props.children} -
- - ); -} +const useDialogClassNames = (dialogElement: HTMLElement|null, classNames: undefined|string) => { + useEffect(() => { + if (!dialogElement || !classNames) { + return () => {}; + } + + // The React className prop can include multiple space-separated classes + const newClassNames = classNames + .split(/\s+/) + .filter(name => !dialogElement.classList.contains(name)); + dialogElement.classList.add(...newClassNames); + + return () => { + dialogElement.classList.remove(...newClassNames); + }; + }, [dialogElement, classNames]); +}; + +export default Dialog; diff --git a/packages/app-desktop/gui/styles/dialog-modal-layer.scss b/packages/app-desktop/gui/styles/dialog-modal-layer.scss index 67b7821d3..2800afb5e 100644 --- a/packages/app-desktop/gui/styles/dialog-modal-layer.scss +++ b/packages/app-desktop/gui/styles/dialog-modal-layer.scss @@ -11,6 +11,10 @@ margin: 0; background-color: transparent; + &:not([open]) { + display: none; + } + > .content { background-color: var(--joplin-background-color); color: var(--joplin-color); diff --git a/packages/app-desktop/integration-tests/goToAnything.spec.ts b/packages/app-desktop/integration-tests/goToAnything.spec.ts index b3dcc7c9c..c8d421095 100644 --- a/packages/app-desktop/integration-tests/goToAnything.spec.ts +++ b/packages/app-desktop/integration-tests/goToAnything.spec.ts @@ -1,6 +1,7 @@ import { test, expect } from './util/test'; import MainScreen from './models/MainScreen'; +import { Locator } from '@playwright/test'; test.describe('goToAnything', () => { test('clicking outside of go to anything should close it', async ({ electronApp, mainWindow }) => { @@ -31,6 +32,38 @@ test.describe('goToAnything', () => { } }); + test('closing go to anything should restore the original keyboard focus', async ({ electronApp, mainWindow }) => { + const mainScreen = new MainScreen(mainWindow); + await mainScreen.createNewNote(''); + + const initialFocusLocators: [Locator, boolean][] = [ + [mainScreen.noteEditor.noteTitleInput, true], + [mainScreen.noteEditor.codeMirrorEditor, false], + ]; + + // Focus and start to fill the editor + for (const [originalFocusLocator, isInput] of initialFocusLocators) { + await originalFocusLocator.click(); + await mainWindow.keyboard.type('Test'); + + const goToAnything = mainScreen.goToAnything; + await goToAnything.open(electronApp); + + await goToAnything.expectToBeOpen(); + await goToAnything.inputLocator.press('Escape'); + await goToAnything.expectToBeClosed(); + + // Keyboard focus should have returned to the editor + await mainWindow.keyboard.type('ing...'); + if (isInput) { + await expect(originalFocusLocator).toBeFocused(); + await expect(originalFocusLocator).toHaveValue('Testing...'); + } else { + await expect(originalFocusLocator).toHaveText('Testing...'); + } + } + }); + test('should be possible to show the set tags dialog from goToAnything', async ({ electronApp, mainWindow }) => { const mainScreen = new MainScreen(mainWindow); await mainScreen.createNewNote('Test note'); diff --git a/packages/app-desktop/package.json b/packages/app-desktop/package.json index 27992aa3a..e098a3317 100644 --- a/packages/app-desktop/package.json +++ b/packages/app-desktop/package.json @@ -128,6 +128,7 @@ "@types/jest": "29.5.8", "@types/node": "18.19.31", "@types/react": "18.2.58", + "@types/react-dom": "18.2.0", "@types/react-redux": "7.1.33", "@types/styled-components": "5.1.32", "@types/tesseract.js": "2.0.0", diff --git a/yarn.lock b/yarn.lock index b4f33ef2c..03b54bbef 100644 --- a/yarn.lock +++ b/yarn.lock @@ -7575,6 +7575,7 @@ __metadata: "@types/mustache": 4.2.5 "@types/node": 18.19.31 "@types/react": 18.2.58 + "@types/react-dom": 18.2.0 "@types/react-redux": 7.1.33 "@types/styled-components": 5.1.32 "@types/tesseract.js": 2.0.0 @@ -12435,6 +12436,15 @@ __metadata: languageName: node linkType: hard +"@types/react-dom@npm:18.2.0": + version: 18.2.0 + resolution: "@types/react-dom@npm:18.2.0" + dependencies: + "@types/react": "*" + checksum: 9212b3793707a763f10e2c608f6ccb5f729029da6cb84204f333634d3d7baafbc1b20b7faf7cf788a8fd385050a1fb579902031f9462473ef10607116f33f33c + languageName: node + linkType: hard + "@types/react-dom@npm:18.2.19": version: 18.2.19 resolution: "@types/react-dom@npm:18.2.19"