Desktop: Accessibility: Restore keyboard focus when closing a dialog (#10817)

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<HTMLDialogElement>();
+const Dialog: React.FC<Props> = 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<HTMLDialogElement> = 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 = (
+		<div className='content' style={props.contentStyle}>
+			{props.children}
+		</div>
+	);
+	return <>
+		{dialogElement && createPortal(content, dialogElement)}
+	</>;
+};
+
+const useDialogElement = (onCancel: undefined|OnCancelListener) => {
+	const [dialogElement, setDialogElement] = useState<HTMLDialogElement|null>(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<HTMLDialogElement> = useCallback((event) => {
-		const onCancel = onCancelRef.current;
-		if (event.target === dialogElement && onCancel) {
-			onCancel();
-		}
-	}, [dialogElement]);
+	return dialogElement;
+};
 
-	return (
-		<dialog
-			ref={setDialogRef}
-			className={`dialog-modal-layer ${props.className}`}
-			onClose={props.onCancel}
-			onCancel={onCancel}
-			onClick={onContainerClick}
-		>
-			<div className='content' style={props.contentStyle}>
-				{props.children}
-			</div>
-		</dialog>
-	);
-}
+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;

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);

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');

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",

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"