From bcb5218e1af5ca320d23abe5d26169d96c4a0db7 Mon Sep 17 00:00:00 2001 From: Henry Heino <46334387+personalizedrefrigerator@users.noreply.github.com> Date: Thu, 12 Sep 2024 09:54:10 -0700 Subject: [PATCH] Desktop: Fix editor/viewer loses focus when visible panels are changed with ctrl-l (#11029) --- .eslintignore | 1 + .gitignore | 1 + .../NoteBody/CodeMirror/v6/CodeMirror.tsx | 3 ++ .../v6/utils/useRefocusOnVisiblePaneChange.ts | 44 +++++++++++++++++++ packages/app-desktop/gui/NoteTextViewer.tsx | 14 +++++- .../integration-tests/main.spec.ts | 6 +-- .../integration-tests/markdownEditor.spec.ts | 43 ++++++++++++++++-- .../models/NoteEditorScreen.ts | 8 ++-- .../integration-tests/noteList.spec.ts | 2 +- .../integration-tests/richTextEditor.spec.ts | 2 +- 10 files changed, 110 insertions(+), 14 deletions(-) create mode 100644 packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/v6/utils/useRefocusOnVisiblePaneChange.ts diff --git a/.eslintignore b/.eslintignore index f996d668f..4ae168a3d 100644 --- a/.eslintignore +++ b/.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 diff --git a/.gitignore b/.gitignore index 43f790bba..17a9cd028 100644 --- a/.gitignore +++ b/.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 diff --git a/packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/v6/CodeMirror.tsx b/packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/v6/CodeMirror.tsx index 718439d6b..14cf9e219 100644 --- a/packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/v6/CodeMirror.tsx +++ b/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; + webviewRef: RefObject; + 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; diff --git a/packages/app-desktop/gui/NoteTextViewer.tsx b/packages/app-desktop/gui/NoteTextViewer.tsx index 8029b4d1e..debb4451e 100644 --- a/packages/app-desktop/gui/NoteTextViewer.tsx +++ b/packages/app-desktop/gui/NoteTextViewer.tsx @@ -26,8 +26,7 @@ export default class NoteTextViewerComponent extends React.Component private initialized_ = false; private domReady_ = false; - // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied - private webviewRef_: any; + private webviewRef_: React.RefObject; // 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 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(); diff --git a/packages/app-desktop/integration-tests/main.spec.ts b/packages/app-desktop/integration-tests/main.spec.ts index f6e4e6728..736059b25 100644 --- a/packages/app-desktop/integration-tests/main.spec.ts +++ b/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); diff --git a/packages/app-desktop/integration-tests/markdownEditor.spec.ts b/packages/app-desktop/integration-tests/markdownEditor.spec.ts index 75cb53734..7fa4cf208 100644 --- a/packages/app-desktop/integration-tests/markdownEditor.spec.ts +++ b/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(); + }); }); diff --git a/packages/app-desktop/integration-tests/models/NoteEditorScreen.ts b/packages/app-desktop/integration-tests/models/NoteEditorScreen.ts index 8f4da25e3..3f336509b 100644 --- a/packages/app-desktop/integration-tests/models/NoteEditorScreen.ts +++ b/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() { diff --git a/packages/app-desktop/integration-tests/noteList.spec.ts b/packages/app-desktop/integration-tests/noteList.spec.ts index 74ea67d28..3eef4c4eb 100644 --- a/packages/app-desktop/integration-tests/noteList.spec.ts +++ b/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'); diff --git a/packages/app-desktop/integration-tests/richTextEditor.spec.ts b/packages/app-desktop/integration-tests/richTextEditor.spec.ts index d55cdf7d1..3b40f88d3 100644 --- a/packages/app-desktop/integration-tests/richTextEditor.spec.ts +++ b/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