Desktop: Fix editor/viewer loses focus when visible panels are changed with ctrl-l (#11029)

.eslintignore

@@ -287,6 +287,7 @@ packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/v6/CodeMirror.js
 packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/v6/Editor.js
 packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/v6/useEditorCommands.js
 packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/v6/utils/useKeymap.js
+packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/v6/utils/useRefocusOnVisiblePaneChange.js
 packages/app-desktop/gui/NoteEditor/NoteBody/PlainEditor/PlainEditor.js
 packages/app-desktop/gui/NoteEditor/NoteBody/TinyMCE/TinyMCE.js
 packages/app-desktop/gui/NoteEditor/NoteBody/TinyMCE/styles/index.js

.gitignore

@@ -264,6 +264,7 @@ packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/v6/CodeMirror.js
 packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/v6/Editor.js
 packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/v6/useEditorCommands.js
 packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/v6/utils/useKeymap.js
+packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/v6/utils/useRefocusOnVisiblePaneChange.js
 packages/app-desktop/gui/NoteEditor/NoteBody/PlainEditor/PlainEditor.js
 packages/app-desktop/gui/NoteEditor/NoteBody/TinyMCE/TinyMCE.js
 packages/app-desktop/gui/NoteEditor/NoteBody/TinyMCE/styles/index.js

packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/v6/CodeMirror.tsx

@@ -28,6 +28,7 @@ import useWebviewIpcMessage from '../utils/useWebviewIpcMessage';
 import Toolbar from '../Toolbar';
 import useEditorSearchHandler from '../utils/useEditorSearchHandler';
 import CommandService from '@joplin/lib/services/CommandService';
+import useRefocusOnVisiblePaneChange from './utils/useRefocusOnVisiblePaneChange';
 
 const logger = Logger.create('CodeMirror6');
 const logDebug = (message: string) => logger.debug(message);
@@ -318,6 +319,8 @@ const CodeMirror = (props: NoteBodyEditorProps, ref: ForwardedRef<NoteBodyEditor
 		return output;
 	}, [styles.cellViewer, props.visiblePanes]);
 
+	useRefocusOnVisiblePaneChange({ editorRef, webviewRef, visiblePanes: props.visiblePanes });
+
 	useEditorSearchHandler({
 		setLocalSearchResultCount: props.setLocalSearchResultCount,
 		searchMarkers: props.searchMarkers,

packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/v6/utils/useRefocusOnVisiblePaneChange.ts

@@ -0,0 +1,44 @@
+import { RefObject, useRef, useEffect } from 'react';
+import { focus } from '@joplin/lib/utils/focusHandler';
+import CodeMirrorControl from '@joplin/editor/CodeMirror/CodeMirrorControl';
+import NoteTextViewer from '../../../../../NoteTextViewer';
+
+interface Props {
+	editorRef: RefObject<CodeMirrorControl>;
+	webviewRef: RefObject<NoteTextViewer>;
+	visiblePanes: string[];
+}
+
+const useRefocusOnVisiblePaneChange = ({ editorRef, webviewRef, visiblePanes }: Props) => {
+	const lastVisiblePanes = useRef(visiblePanes);
+	useEffect(() => {
+		const editorHasFocus = editorRef.current?.cm6?.dom?.contains(document.activeElement);
+		const viewerHasFocus = webviewRef.current?.hasFocus();
+
+		const lastHadViewer = lastVisiblePanes.current.includes('viewer');
+		const hasViewer = visiblePanes.includes('viewer');
+		const lastHadEditor = lastVisiblePanes.current.includes('editor');
+		const hasEditor = visiblePanes.includes('editor');
+
+		const viewerJustHidden = lastHadViewer && !hasViewer;
+		if (viewerJustHidden && viewerHasFocus) {
+			focus('CodeMirror/refocusEditor1', editorRef.current);
+		}
+
+		// Jump focus to the editor just after showing it -- this assumes that the user
+		// shows the editor to start editing the note.
+		const editorJustShown = !lastHadEditor && hasEditor;
+		if (editorJustShown && viewerHasFocus) {
+			focus('CodeMirror/refocusEditor2', editorRef.current);
+		}
+
+		const editorJustHidden = lastHadEditor && !hasEditor;
+		if (editorJustHidden && editorHasFocus) {
+			focus('CodeMirror/refocusViewer', webviewRef.current);
+		}
+
+		lastVisiblePanes.current = visiblePanes;
+	}, [visiblePanes, editorRef, webviewRef]);
+};
+
+export default useRefocusOnVisiblePaneChange;

packages/app-desktop/gui/NoteTextViewer.tsx

@@ -26,8 +26,7 @@ export default class NoteTextViewerComponent extends React.Component<Props, any>
 
 	private initialized_ = false;
 	private domReady_ = false;
-	// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
+	private webviewRef_: React.RefObject<HTMLIFrameElement>;
-	private webviewRef_: any;
 	// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
 	private webviewListeners_: any = null;
 	private removePluginAssetsCallback_: RemovePluginAssetsCallback|null = null;
@@ -131,10 +130,21 @@ export default class NoteTextViewerComponent extends React.Component<Props, any>
 
 	public focus() {
 		if (this.webviewRef_.current) {
+			// Calling focus on webviewRef_ seems to be necessary when NoteTextViewer.focus
+			// is called outside of a user event (e.g. in a setTimeout) or during automated
+			// tests:
 			focus('NoteTextViewer::focus', this.webviewRef_.current);
+
+			// Calling .focus on this.webviewRef.current isn't sufficient.
+			// To allow arrow-key scrolling, focus must also be set within the iframe:
+			this.send('focus');
 		}
 	}
 
+	public hasFocus() {
+		return this.webviewRef_.current?.contains(document.activeElement);
+	}
+
 	public tryInit() {
 		if (!this.initialized_ && this.webviewRef_.current) {
 			this.initWebview();

packages/app-desktop/integration-tests/main.spec.ts

@@ -36,7 +36,7 @@ test.describe('main', () => {
 		await mainWindow.keyboard.type('New note content!');
 
 		// Should render
-		const viewerFrame = editor.getNoteViewerIframe();
+		const viewerFrame = editor.getNoteViewerFrameLocator();
 		await expect(viewerFrame.locator('h1')).toHaveText('Test note!');
 	});
 
@@ -78,7 +78,7 @@ test.describe('main', () => {
 		}
 
 		// Should render mermaid
-		const viewerFrame = editor.getNoteViewerIframe();
+		const viewerFrame = editor.getNoteViewerFrameLocator();
 		await expect(
 			viewerFrame.locator('pre.mermaid text', { hasText: testCommitId }),
 		).toBeVisible();
@@ -115,7 +115,7 @@ test.describe('main', () => {
 		await setMessageBoxResponse(electronApp, /^No/i);
 		await editor.attachFileButton.click();
 
-		const viewerFrame = editor.getNoteViewerIframe();
+		const viewerFrame = editor.getNoteViewerFrameLocator();
 		const renderedImage = viewerFrame.getByAltText(filename);
 
 		const fullSize = await getImageSourceSize(renderedImage);

packages/app-desktop/integration-tests/markdownEditor.spec.ts

@@ -3,6 +3,7 @@ import MainScreen from './models/MainScreen';
 import { join } from 'path';
 import getImageSourceSize from './util/getImageSourceSize';
 import setFilePickerResponse from './util/setFilePickerResponse';
+import activateMainMenuItem from './util/activateMainMenuItem';
 
 
 test.describe('markdownEditor', () => {
@@ -24,7 +25,7 @@ test.describe('markdownEditor', () => {
 			await importedHtmlFileItem.click({ timeout: 300 });
 		}).toPass();
 
-		const viewerFrame = mainScreen.noteEditor.getNoteViewerIframe();
+		const viewerFrame = mainScreen.noteEditor.getNoteViewerFrameLocator();
 		// Should render headers
 		await expect(viewerFrame.locator('h1')).toHaveText('Test HTML file!');
 
@@ -44,7 +45,7 @@ test.describe('markdownEditor', () => {
 		await setFilePickerResponse(electronApp, [join(__dirname, 'resources', 'small-pdf.pdf')]);
 		await editor.attachFileButton.click();
 
-		const viewerFrame = mainScreen.noteEditor.getNoteViewerIframe();
+		const viewerFrame = mainScreen.noteEditor.getNoteViewerFrameLocator();
 		const pdfLink = viewerFrame.getByText('small-pdf.pdf');
 		await expect(pdfLink).toBeVisible();
 
@@ -106,7 +107,7 @@ test.describe('markdownEditor', () => {
 		await setFilePickerResponse(electronApp, [join(__dirname, 'resources', 'video.mp4')]);
 		await editor.attachFileButton.click();
 
-		const videoLocator = editor.getNoteViewerIframe().locator('video');
+		const videoLocator = editor.getNoteViewerFrameLocator().locator('video');
 		const expectVideoToRender = async () => {
 			await expect(videoLocator).toBeSeekableMediaElement(6.9, 7);
 		};
@@ -172,7 +173,7 @@ test.describe('markdownEditor', () => {
 		await mainWindow.keyboard.press('Enter');
 		await mainWindow.keyboard.type('This is a test of search. `Test inline code`');
 
-		const viewer = noteEditor.getNoteViewerIframe();
+		const viewer = noteEditor.getNoteViewerFrameLocator();
 		await expect(viewer.locator('h1')).toHaveText('Testing');
 
 		const matches = viewer.locator('mark');
@@ -213,5 +214,39 @@ test.describe('markdownEditor', () => {
 		await expect(noteEditor.codeMirrorEditor).toBeVisible();
 		await expect(noteEditor.editorSearchInput).not.toBeVisible();
 	});
+
+	test('should move focus when the visible editor panes change', async ({ mainWindow, electronApp }) => {
+		const mainScreen = new MainScreen(mainWindow);
+		await mainScreen.waitFor();
+		const noteEditor = mainScreen.noteEditor;
+
+		await mainScreen.createNewNote('Note');
+
+		await noteEditor.focusCodeMirrorEditor();
+		await mainWindow.keyboard.type('test');
+		const focusInMarkdownEditor = noteEditor.codeMirrorEditor.locator(':focus');
+		await expect(focusInMarkdownEditor).toBeAttached();
+
+		const toggleEditorLayout = () => activateMainMenuItem(electronApp, 'Toggle editor layout');
+
+		// Editor only
+		await toggleEditorLayout();
+		await expect(noteEditor.noteViewerContainer).not.toBeVisible();
+		// Markdown editor should be focused
+		await expect(focusInMarkdownEditor).toBeAttached();
+
+		// Viewer only
+		await toggleEditorLayout();
+		await expect(noteEditor.codeMirrorEditor).not.toBeVisible();
+		// Viewer should be focused
+		await expect(noteEditor.noteViewerContainer).toBeFocused();
+
+		// Viewer and editor
+		await toggleEditorLayout();
+		await expect(noteEditor.noteViewerContainer).toBeAttached();
+		await expect(noteEditor.codeMirrorEditor).toBeVisible();
+		// Editor should be focused
+		await expect(focusInMarkdownEditor).toBeAttached();
+	});
 });
 

packages/app-desktop/integration-tests/models/NoteEditorScreen.ts

@@ -3,6 +3,7 @@ import { Locator, Page } from '@playwright/test';
 
 export default class NoteEditorPage {
 	public readonly codeMirrorEditor: Locator;
+	public readonly noteViewerContainer: Locator;
 	public readonly richTextEditor: Locator;
 	public readonly noteTitleInput: Locator;
 	public readonly attachFileButton: Locator;
@@ -12,7 +13,7 @@ export default class NoteEditorPage {
 	public readonly viewerSearchInput: Locator;
 	private readonly containerLocator: Locator;
 
-	public constructor(private readonly page: Page) {
+	public constructor(page: Page) {
 		this.containerLocator = page.locator('.rli-editor');
 		this.codeMirrorEditor = this.containerLocator.locator('.cm-editor');
 		this.richTextEditor = this.containerLocator.locator('iframe[title="Rich Text Area"]');
@@ -20,6 +21,7 @@ export default class NoteEditorPage {
 		this.attachFileButton = this.containerLocator.getByRole('button', { name: 'Attach file' });
 		this.toggleEditorsButton = this.containerLocator.getByRole('button', { name: 'Toggle editors' });
 		this.toggleEditorLayoutButton = this.containerLocator.getByRole('button', { name: 'Toggle editor layout' });
+		this.noteViewerContainer = this.containerLocator.locator('iframe[src$="note-viewer/index.html"]');
 		// The editor and viewer have slightly different search UI
 		this.editorSearchInput = this.containerLocator.getByPlaceholder('Find');
 		this.viewerSearchInput = this.containerLocator.getByPlaceholder('Search...');
@@ -29,11 +31,11 @@ export default class NoteEditorPage {
 		return this.containerLocator.getByRole('button', { name: title });
 	}
 
-	public getNoteViewerIframe() {
+	public getNoteViewerFrameLocator() {
 		// The note viewer can change content when the note re-renders. As such,
 		// a new locator needs to be created after re-renders (and this can't be a
 		// static property).
-		return this.page.frameLocator('[src$="note-viewer/index.html"]');
+		return this.noteViewerContainer.frameLocator(':scope');
 	}
 
 	public getTinyMCEFrameLocator() {

packages/app-desktop/integration-tests/noteList.spec.ts

@@ -32,7 +32,7 @@ test.describe('noteList', () => {
 		await mainWindow.keyboard.type('[Testing...](http://example.com/)');
 
 		// Wait to render
-		await expect(editor.getNoteViewerIframe().locator('a', { hasText: 'Testing...' })).toBeVisible();
+		await expect(editor.getNoteViewerFrameLocator().locator('a', { hasText: 'Testing...' })).toBeVisible();
 
 		// Updating the title should force the sidebar to update sooner
 		await expect(editor.noteTitleInput).toHaveValue('note-1');

packages/app-desktop/integration-tests/richTextEditor.spec.ts

@@ -18,7 +18,7 @@ test.describe('richTextEditor', () => {
 		await editor.attachFileButton.click();
 
 		// Wait to render
-		const viewerFrame = editor.getNoteViewerIframe();
+		const viewerFrame = editor.getNoteViewerFrameLocator();
 		await viewerFrame.locator('a[data-from-md]').waitFor();
 
 		// Should have an attached resource