From 9dbd481f2856016b1da09296f7f9a7f4b80e509d Mon Sep 17 00:00:00 2001 From: Henry Heino <46334387+personalizedrefrigerator@users.noreply.github.com> Date: Fri, 2 Aug 2024 06:47:43 -0700 Subject: [PATCH] Desktop: Fix images fail to render in the preview pane for HTML notes (#10806) --- .eslintignore | 2 ++ .gitignore | 2 ++ .../integration-tests/main.spec.ts | 26 ++++------------ .../integration-tests/markdownEditor.spec.ts | 30 +++++++++++++++++++ .../integration-tests/models/MainScreen.ts | 10 +++++++ .../resources/html-import/image.svg | 1 + .../test-html-file-with-image.html | 8 +++++ .../util/getImageSourceSize.ts | 21 +++++++++++++ packages/renderer/HtmlToHtml.ts | 7 ++--- packages/renderer/htmlUtils.ts | 23 ++++++++++++-- 10 files changed, 102 insertions(+), 28 deletions(-) create mode 100644 packages/app-desktop/integration-tests/markdownEditor.spec.ts create mode 100644 packages/app-desktop/integration-tests/resources/html-import/image.svg create mode 100644 packages/app-desktop/integration-tests/resources/html-import/test-html-file-with-image.html create mode 100644 packages/app-desktop/integration-tests/util/getImageSourceSize.ts diff --git a/.eslintignore b/.eslintignore index 1d7eb8ac83..4cce905f20 100644 --- a/.eslintignore +++ b/.eslintignore @@ -455,6 +455,7 @@ packages/app-desktop/gui/utils/loadScript.js packages/app-desktop/gulpfile.js packages/app-desktop/integration-tests/goToAnything.spec.js packages/app-desktop/integration-tests/main.spec.js +packages/app-desktop/integration-tests/markdownEditor.spec.js packages/app-desktop/integration-tests/models/GoToAnything.js packages/app-desktop/integration-tests/models/MainScreen.js packages/app-desktop/integration-tests/models/NoteEditorScreen.js @@ -467,6 +468,7 @@ packages/app-desktop/integration-tests/simpleBackup.spec.js packages/app-desktop/integration-tests/util/activateMainMenuItem.js packages/app-desktop/integration-tests/util/createStartupArgs.js packages/app-desktop/integration-tests/util/firstNonDevToolsWindow.js +packages/app-desktop/integration-tests/util/getImageSourceSize.js packages/app-desktop/integration-tests/util/setFilePickerResponse.js packages/app-desktop/integration-tests/util/setMessageBoxResponse.js packages/app-desktop/integration-tests/util/test.js diff --git a/.gitignore b/.gitignore index dc20aaa8ca..c79632360d 100644 --- a/.gitignore +++ b/.gitignore @@ -434,6 +434,7 @@ packages/app-desktop/gui/utils/loadScript.js packages/app-desktop/gulpfile.js packages/app-desktop/integration-tests/goToAnything.spec.js packages/app-desktop/integration-tests/main.spec.js +packages/app-desktop/integration-tests/markdownEditor.spec.js packages/app-desktop/integration-tests/models/GoToAnything.js packages/app-desktop/integration-tests/models/MainScreen.js packages/app-desktop/integration-tests/models/NoteEditorScreen.js @@ -446,6 +447,7 @@ packages/app-desktop/integration-tests/simpleBackup.spec.js packages/app-desktop/integration-tests/util/activateMainMenuItem.js packages/app-desktop/integration-tests/util/createStartupArgs.js packages/app-desktop/integration-tests/util/firstNonDevToolsWindow.js +packages/app-desktop/integration-tests/util/getImageSourceSize.js packages/app-desktop/integration-tests/util/setFilePickerResponse.js packages/app-desktop/integration-tests/util/setMessageBoxResponse.js packages/app-desktop/integration-tests/util/test.js diff --git a/packages/app-desktop/integration-tests/main.spec.ts b/packages/app-desktop/integration-tests/main.spec.ts index 31ac996c13..40b1403bdb 100644 --- a/packages/app-desktop/integration-tests/main.spec.ts +++ b/packages/app-desktop/integration-tests/main.spec.ts @@ -8,6 +8,7 @@ import createStartupArgs from './util/createStartupArgs'; import firstNonDevToolsWindow from './util/firstNonDevToolsWindow'; import setFilePickerResponse from './util/setFilePickerResponse'; import setMessageBoxResponse from './util/setMessageBoxResponse'; +import getImageSourceSize from './util/getImageSourceSize'; test.describe('main', () => { @@ -108,27 +109,10 @@ test.describe('main', () => { await setMessageBoxResponse(electronApp, /^No/i); await editor.attachFileButton.click(); - const getImageSize = async () => { - const viewerFrame = editor.getNoteViewerIframe(); - const renderedImage = viewerFrame.getByAltText(filename); + const viewerFrame = editor.getNoteViewerIframe(); + const renderedImage = viewerFrame.getByAltText(filename); - // Use state: 'attached' -- we don't need the image to be on the screen (just present - // in the DOM). - await renderedImage.waitFor({ state: 'attached' }); - - // We load a copy of the image to avoid returning an overriden width set with - // .width = some_number - return await renderedImage.evaluate((originalImage: HTMLImageElement) => { - return new Promise<[number, number]>(resolve => { - const testImage = new Image(); - testImage.onload = () => { - resolve([testImage.width, testImage.height]); - }; - testImage.src = originalImage.src; - }); - }); - }; - const fullSize = await getImageSize(); + const fullSize = await getImageSourceSize(renderedImage); // To make it easier to find the image (one image per note), we switch to a new, empty note. await mainScreen.createNewNote('Image resize test (part 2)'); @@ -138,7 +122,7 @@ test.describe('main', () => { await setMessageBoxResponse(electronApp, /^Yes/i); await editor.attachFileButton.click(); - const resizedSize = await getImageSize(); + const resizedSize = await getImageSourceSize(renderedImage); expect(resizedSize[0]).toBeLessThan(fullSize[0]); expect(resizedSize[1]).toBeLessThan(fullSize[1]); diff --git a/packages/app-desktop/integration-tests/markdownEditor.spec.ts b/packages/app-desktop/integration-tests/markdownEditor.spec.ts new file mode 100644 index 0000000000..fdca08b423 --- /dev/null +++ b/packages/app-desktop/integration-tests/markdownEditor.spec.ts @@ -0,0 +1,30 @@ +import { test, expect } from './util/test'; +import MainScreen from './models/MainScreen'; +import { join } from 'path'; +import getImageSourceSize from './util/getImageSourceSize'; + + +test.describe('markdownEditor', () => { + test('preview pane should render images in HTML notes', async ({ mainWindow, electronApp }) => { + const mainScreen = new MainScreen(mainWindow); + await mainScreen.waitFor(); + + await mainScreen.importHtmlDirectory(electronApp, join(__dirname, 'resources', 'html-import')); + const importedFolder = mainScreen.sidebar.container.getByText('html-import'); + await importedFolder.waitFor(); + await importedFolder.click(); + + const importedHtmlFileItem = mainScreen.noteListContainer.getByText('test-html-file-with-image'); + await importedHtmlFileItem.click(); + + const viewerFrame = mainScreen.noteEditor.getNoteViewerIframe(); + // Should render headers + await expect(viewerFrame.locator('h1')).toHaveText('Test HTML file!'); + + // Should render images + const image = viewerFrame.getByAltText('An SVG image.'); + await expect(image).toBeAttached(); + await expect(await getImageSourceSize(image)).toMatchObject([117, 30]); + }); +}); + diff --git a/packages/app-desktop/integration-tests/models/MainScreen.ts b/packages/app-desktop/integration-tests/models/MainScreen.ts index 085ed3dee9..a1c7212523 100644 --- a/packages/app-desktop/integration-tests/models/MainScreen.ts +++ b/packages/app-desktop/integration-tests/models/MainScreen.ts @@ -3,6 +3,7 @@ import NoteEditorScreen from './NoteEditorScreen'; import activateMainMenuItem from '../util/activateMainMenuItem'; import Sidebar from './Sidebar'; import GoToAnything from './GoToAnything'; +import setFilePickerResponse from '../util/setFilePickerResponse'; export default class MainScreen { public readonly newNoteButton: Locator; @@ -56,4 +57,13 @@ export default class MainScreen { const searchBar = this.page.getByPlaceholder('Search...'); await searchBar.fill(text); } + + public async importHtmlDirectory(electronApp: ElectronApplication, path: string) { + await setFilePickerResponse(electronApp, [path]); + const startedImport = await activateMainMenuItem(electronApp, 'HTML - HTML document (Directory)', 'Import'); + + if (!startedImport) { + throw new Error('Unable to find HTML directory import menu item.'); + } + } } diff --git a/packages/app-desktop/integration-tests/resources/html-import/image.svg b/packages/app-desktop/integration-tests/resources/html-import/image.svg new file mode 100644 index 0000000000..346c8bea53 --- /dev/null +++ b/packages/app-desktop/integration-tests/resources/html-import/image.svg @@ -0,0 +1 @@ +Image \ No newline at end of file diff --git a/packages/app-desktop/integration-tests/resources/html-import/test-html-file-with-image.html b/packages/app-desktop/integration-tests/resources/html-import/test-html-file-with-image.html new file mode 100644 index 0000000000..e81244454b --- /dev/null +++ b/packages/app-desktop/integration-tests/resources/html-import/test-html-file-with-image.html @@ -0,0 +1,8 @@ + + + +

Test HTML file!

+

This is a test file. Resource:

+ An SVG image. + + \ No newline at end of file diff --git a/packages/app-desktop/integration-tests/util/getImageSourceSize.ts b/packages/app-desktop/integration-tests/util/getImageSourceSize.ts new file mode 100644 index 0000000000..b0f92a3067 --- /dev/null +++ b/packages/app-desktop/integration-tests/util/getImageSourceSize.ts @@ -0,0 +1,21 @@ +import { Locator } from '@playwright/test'; + +const getImageSourceSize = async (imageLocator: Locator) => { + // Use state: 'attached' -- we don't need the image to be on the screen (just present + // in the DOM). + await imageLocator.waitFor({ state: 'attached' }); + + // We load a copy of the image to avoid returning an overriden width set with + // .width = some_number + return await imageLocator.evaluate((originalImage: HTMLImageElement) => { + return new Promise<[number, number]>(resolve => { + const testImage = new Image(); + testImage.onload = () => { + resolve([testImage.width, testImage.height]); + }; + testImage.src = originalImage.src; + }); + }); +}; + +export default getImageSourceSize; diff --git a/packages/renderer/HtmlToHtml.ts b/packages/renderer/HtmlToHtml.ts index d5ba506318..7e01e5f535 100644 --- a/packages/renderer/HtmlToHtml.ts +++ b/packages/renderer/HtmlToHtml.ts @@ -92,7 +92,7 @@ export default class HtmlToHtml implements MarkupRenderer { ...options, }; - const cacheKey = md5(escape(JSON.stringify({ markup, options }))); + const cacheKey = md5(escape(JSON.stringify({ markup, options, baseUrl: this.resourceBaseUrl_ }))); let html = this.cache_.value(cacheKey); if (!html) { @@ -100,11 +100,10 @@ export default class HtmlToHtml implements MarkupRenderer { allowedFilePrefixes: options.allowedFilePrefixes, }); - // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied - html = htmlUtils.processImageTags(html, (data: any) => { + html = htmlUtils.processImageTags(html, (data) => { if (!data.src) return null; - const r = utils.imageReplacement(this.ResourceModel_, data.src, options.resources, this.resourceBaseUrl_, options.itemIdToUrl); + const r = utils.imageReplacement(this.ResourceModel_, data, options.resources, this.resourceBaseUrl_, options.itemIdToUrl); if (!r) return null; if (typeof r === 'string') { diff --git a/packages/renderer/htmlUtils.ts b/packages/renderer/htmlUtils.ts index ecc2e6995b..d35cf83325 100644 --- a/packages/renderer/htmlUtils.ts +++ b/packages/renderer/htmlUtils.ts @@ -56,14 +56,31 @@ export const isSelfClosingTag = (tagName: string) => { return selfClosingElements.includes(tagName.toLowerCase()); }; +type ProcessImageResult = { + type: 'replaceElement'; + html: string; +}|{ + type: 'replaceSource'; + src: string; +}|{ + type: 'setAttributes'; + attrs: Record; +}; +interface ProcessImageEvent { + src: string; + before: string; + after: string; +} +type ProcessImageCallback = (data: ProcessImageEvent)=> ProcessImageResult; + class HtmlUtils { // eslint-disable-next-line @typescript-eslint/ban-types -- Old code before rule was applied - public processImageTags(html: string, callback: Function) { + public processImageTags(html: string, callback: ProcessImageCallback) { if (!html) return ''; return html.replace(imageRegex, (_v, before, src, after) => { - const action = callback({ src: src }); + const action = callback({ src, before, after }); if (!action) return ``; @@ -80,7 +97,7 @@ class HtmlUtils { return ``; } - throw new Error(`Invalid action: ${action.type}`); + throw new Error(`Invalid action: ${(action as Record).type}`); }); }