From 754ca3992622bcd05e12de6b01124773e8bac762 Mon Sep 17 00:00:00 2001 From: Laurent Cozic Date: Fri, 29 Dec 2023 16:08:09 +0000 Subject: [PATCH] Desktop: Fixes #9511: HTML notes are not readable in dark mode --- .../NoteBody/CodeMirror/v5/CodeMirror.tsx | 1 - .../NoteEditor/NoteBody/TinyMCE/TinyMCE.tsx | 116 +++++++++--------- .../app-desktop/gui/NoteEditor/NoteEditor.tsx | 8 +- .../app-desktop/gui/NoteEditor/utils/types.ts | 10 ++ .../gui/NoteEditor/utils/useMarkupToHtml.ts | 9 +- packages/renderer/HtmlToHtml.ts | 21 +++- packages/renderer/noteStyle.ts | 36 ++++++ 7 files changed, 132 insertions(+), 69 deletions(-) diff --git a/packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/v5/CodeMirror.tsx b/packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/v5/CodeMirror.tsx index 58bd44a1b0a..717f300de28 100644 --- a/packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/v5/CodeMirror.tsx +++ b/packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/v5/CodeMirror.tsx @@ -621,7 +621,6 @@ function CodeMirror(props: NoteBodyEditorProps, ref: ForwardedRef { useEffect(() => { const theme = themeStyle(props.themeId); + const backgroundColor = props.whiteBackgroundNoteRendering ? lightTheme.backgroundColor : theme.backgroundColor; const element = document.createElement('style'); element.setAttribute('id', 'tinyMceStyle'); @@ -503,7 +505,7 @@ const TinyMCE = (props: NoteBodyEditorProps, ref: any) => { } .joplin-tinymce .tox .tox-edit-area__iframe { - background-color: ${theme.backgroundColor} !important; + background-color: ${backgroundColor} !important; } .joplin-tinymce .tox .tox-toolbar__primary { @@ -524,7 +526,7 @@ const TinyMCE = (props: NoteBodyEditorProps, ref: any) => { // // tl;dr: editorReady is used here because the css needs to be re-applied after TinyMCE init // eslint-disable-next-line @seiyab/react-hooks/exhaustive-deps -- Old code before rule was applied - }, [editorReady, props.themeId]); + }, [editorReady, props.themeId, lightTheme, props.whiteBackgroundNoteRendering]); // ----------------------------------------------------------------------------------------- // Enable or disable the editor @@ -542,9 +544,6 @@ const TinyMCE = (props: NoteBodyEditorProps, ref: any) => { useEffect(() => { if (!scriptLoaded) return; - loadedCssFiles_ = []; - loadedJsFiles_ = []; - const loadEditor = async () => { const language = closestSupportedLocale(props.locale, true, supportedLocales); @@ -739,13 +738,10 @@ const TinyMCE = (props: NoteBodyEditorProps, ref: any) => { // Set the initial content and load the plugin CSS and JS files // ----------------------------------------------------------------------------------------- - const documentCssElements: Record = {}; - const documentScriptElements: Record = {}; - - const loadDocumentAssets = (editor: any, pluginAssets: any[]) => { - const theme = themeStyle(props.themeId); + const loadDocumentAssets = (themeId: number, editor: any, pluginAssets: any[]) => { + const theme = themeStyle(themeId); - let docHead_: any = null; + let docHead_: HTMLHeadElement = null; function docHead() { if (docHead_) return docHead_; @@ -768,58 +764,55 @@ const TinyMCE = (props: NoteBodyEditorProps, ref: any) => { .map((a: any) => a.path), ); + const filePathToElementId = (path: string) => { + return `jop-tiny-mce-${md5(escape(path))}`; + }; - // Remove all previously loaded files that aren't in the assets this time. - // Note: This is important to ensure that we properly change themes. - // See https://github.com/laurent22/joplin/issues/8520 - for (const cssFile of loadedCssFiles_) { - if (!allCssFiles.includes(cssFile)) { - documentCssElements[cssFile]?.remove(); - delete documentCssElements[cssFile]; - } - } + const existingElements = Array.from(docHead().getElementsByClassName('jop-tinymce-css')).concat(Array.from(docHead().getElementsByClassName('jop-tinymce-js'))); - for (const jsFile of loadedJsFiles_) { - if (!allJsFiles.includes(jsFile)) { - documentScriptElements[jsFile]?.remove(); - delete documentScriptElements[jsFile]; - } - } + const existingIds: string[] = []; + for (const e of existingElements) existingIds.push(e.getAttribute('id')); - const newCssFiles = allCssFiles.filter((path: string) => !loadedCssFiles_.includes(path)); - const newJsFiles = allJsFiles.filter((path: string) => !loadedJsFiles_.includes(path)); + const processedIds: string[] = []; - loadedCssFiles_ = allCssFiles; - loadedJsFiles_ = allJsFiles; + for (const cssFile of allCssFiles) { + const elementId = filePathToElementId(cssFile); + processedIds.push(elementId); + if (existingIds.includes(elementId)) continue; - // console.info('loadDocumentAssets: files to load', cssFiles, jsFiles); - - if (newCssFiles.length) { - for (const cssFile of newCssFiles) { - const style = editor.dom.create('link', { - rel: 'stylesheet', - type: 'text/css', - href: cssFile, - class: 'jop-tinymce-css', - }); + const style = editor.dom.create('link', { + id: elementId, + rel: 'stylesheet', + type: 'text/css', + href: cssFile, + class: 'jop-tinymce-css', + }); - documentCssElements[cssFile] = style; - docHead().appendChild(style); - } + docHead().appendChild(style); } - if (newJsFiles.length) { - const editorElementId = editor.dom.uniqueId(); + for (const jsFile of allJsFiles) { + const elementId = filePathToElementId(jsFile); + processedIds.push(elementId); + if (existingIds.includes(elementId)) continue; - for (const jsFile of newJsFiles) { - const script = editor.dom.create('script', { - id: editorElementId, - type: 'text/javascript', - src: jsFile, - }); + const script = editor.dom.create('script', { + id: filePathToElementId(jsFile), + type: 'text/javascript', + class: 'jop-tinymce-js', + src: jsFile, + }); + + docHead().appendChild(script); + } - documentScriptElements[jsFile] = script; - docHead().appendChild(script); + // Remove all previously loaded files that aren't in the assets this time. + // Note: This is important to ensure that we properly change themes. + // See https://github.com/laurent22/joplin/issues/8520 + for (const existingId of existingIds) { + if (!processedIds.includes(existingId)) { + const element = existingElements.find(e => e.getAttribute('id') === existingId); + if (element) docHead().removeChild(element); } } }; @@ -898,7 +891,14 @@ const TinyMCE = (props: NoteBodyEditorProps, ref: any) => { }; } - await loadDocumentAssets(editor, await props.allAssets(props.contentMarkupLanguage, { contentMaxWidthTarget: '.mce-content-body' })); + const allAssetsOptions: NoteStyleOptions = { + contentMaxWidthTarget: '.mce-content-body', + themeId: props.contentMarkupLanguage === MarkupLanguage.Html ? 1 : null, + whiteBackgroundNoteRendering: props.whiteBackgroundNoteRendering, + }; + + const allAssets = await props.allAssets(props.contentMarkupLanguage, allAssetsOptions); + await loadDocumentAssets(props.themeId, editor, allAssets); dispatchDidUpdate(editor); }; @@ -909,7 +909,7 @@ const TinyMCE = (props: NoteBodyEditorProps, ref: any) => { cancelled = true; }; // eslint-disable-next-line @seiyab/react-hooks/exhaustive-deps -- Old code before rule was applied - }, [editor, props.markupToHtml, props.allAssets, props.content, props.resourceInfos, props.contentKey]); + }, [editor, props.themeId, props.markupToHtml, props.allAssets, props.content, props.resourceInfos, props.contentKey, props.contentMarkupLanguage, props.whiteBackgroundNoteRendering]); useEffect(() => { if (!editor) return () => {}; diff --git a/packages/app-desktop/gui/NoteEditor/NoteEditor.tsx b/packages/app-desktop/gui/NoteEditor/NoteEditor.tsx index f0035e94819..2a9795b99b2 100644 --- a/packages/app-desktop/gui/NoteEditor/NoteEditor.tsx +++ b/packages/app-desktop/gui/NoteEditor/NoteEditor.tsx @@ -50,6 +50,7 @@ import CodeMirror6 from './NoteBody/CodeMirror/v6/CodeMirror'; import CodeMirror5 from './NoteBody/CodeMirror/v5/CodeMirror'; import { openItemById } from './utils/contextMenu'; import { namespacedKey } from '@joplin/lib/services/plugins/api/JoplinSettings'; +import { MarkupLanguage } from '@joplin/renderer'; const commands = [ require('./commands/showRevisions'), @@ -165,8 +166,11 @@ function NoteEditor(props: NoteEditorProps) { return Setting.value(namespacedKey(pluginId, key)); }, []); + const whiteBackgroundNoteRendering = formNote.markup_language === MarkupLanguage.Html; + const markupToHtml = useMarkupToHtml({ themeId: props.themeId, + whiteBackgroundNoteRendering, customCss: props.customCss, plugins: props.plugins, settingValue, @@ -178,7 +182,7 @@ function NoteEditor(props: NoteEditorProps) { ...options, }; - const theme = themeStyle(props.themeId); + const theme = themeStyle(options.themeId ? options.themeId : props.themeId); const markupToHtml = markupLanguageUtils.newMarkupToHtml({}, { resourceBaseUrl: `file://${Setting.value('resourceDir')}/`, @@ -188,6 +192,7 @@ function NoteEditor(props: NoteEditorProps) { return markupToHtml.allAssets(markupLanguage, theme, { contentMaxWidth: props.contentMaxWidth, contentMaxWidthTarget: options.contentMaxWidthTarget, + whiteBackgroundNoteRendering: options.whiteBackgroundNoteRendering, }); }, [props.themeId, props.customCss, props.contentMaxWidth]); @@ -433,6 +438,7 @@ function NoteEditor(props: NoteEditorProps) { ref: editorRef, contentKey: formNote.id, style: styles.tinyMCE, + whiteBackgroundNoteRendering, onChange: onBodyChange, onWillChange: onBodyWillChange, onMessage: onMessage, diff --git a/packages/app-desktop/gui/NoteEditor/utils/types.ts b/packages/app-desktop/gui/NoteEditor/utils/types.ts index 31d6feb70d9..6e402403dec 100644 --- a/packages/app-desktop/gui/NoteEditor/utils/types.ts +++ b/packages/app-desktop/gui/NoteEditor/utils/types.ts @@ -9,6 +9,8 @@ import { ProcessResultsRow } from '@joplin/lib/services/searchengine/SearchEngin export interface AllAssetsOptions { contentMaxWidthTarget?: string; + themeId?: number; + whiteBackgroundNoteRendering?: boolean; } export interface ToolbarButtonInfos { @@ -63,6 +65,14 @@ export interface NoteBodyEditorProps { style: any; ref: any; themeId: number; + + // When this is true it means the note must always be rendered using a white + // background theme. This applies to the Markdown editor note view, and to + // the RTE. It does not apply to other elements such as the toolbar, dialogs + // or the CodeMirror editor. This is used to correctly render HTML notes and + // avoid cases where black text is rendered over a dark background. + whiteBackgroundNoteRendering: boolean; + content: string; contentKey: string; contentMarkupLanguage: number; diff --git a/packages/app-desktop/gui/NoteEditor/utils/useMarkupToHtml.ts b/packages/app-desktop/gui/NoteEditor/utils/useMarkupToHtml.ts index 0aa1872db88..8c1ee70ade2 100644 --- a/packages/app-desktop/gui/NoteEditor/utils/useMarkupToHtml.ts +++ b/packages/app-desktop/gui/NoteEditor/utils/useMarkupToHtml.ts @@ -13,6 +13,7 @@ interface HookDependencies { customCss: string; plugins: PluginStates; settingValue: (pluginId: string, key: string)=> any; + whiteBackgroundNoteRendering: boolean; } export interface MarkupToHtmlOptions { @@ -27,13 +28,14 @@ export interface MarkupToHtmlOptions { vendorDir?: string; platformName?: string; allowedFilePrefixes?: string[]; + whiteBackgroundNoteRendering?: boolean; } export default function useMarkupToHtml(deps: HookDependencies) { - const { themeId, customCss, plugins } = deps; + const { themeId, customCss, plugins, whiteBackgroundNoteRendering } = deps; const markupToHtml = useMemo(() => { - return markupLanguageUtils.newMarkupToHtml(deps.plugins, { + return markupLanguageUtils.newMarkupToHtml(plugins, { resourceBaseUrl: `file://${Setting.value('resourceDir')}/`, customCss: customCss || '', }); @@ -69,10 +71,11 @@ export default function useMarkupToHtml(deps: HookDependencies) { externalAssetsOnly: true, codeHighlightCacheKey: 'useMarkupToHtml', settingValue: deps.settingValue, + whiteBackgroundNoteRendering, ...options, }); return result; // eslint-disable-next-line @seiyab/react-hooks/exhaustive-deps -- Old code before rule was applied - }, [themeId, customCss, markupToHtml]); + }, [themeId, customCss, markupToHtml, whiteBackgroundNoteRendering]); } diff --git a/packages/renderer/HtmlToHtml.ts b/packages/renderer/HtmlToHtml.ts index 5e482d2ac33..c424736a057 100644 --- a/packages/renderer/HtmlToHtml.ts +++ b/packages/renderer/HtmlToHtml.ts @@ -3,6 +3,8 @@ import linkReplacement from './MdToHtml/linkReplacement'; import utils, { ItemIdToUrlHandler } from './utils'; import InMemoryCache from './InMemoryCache'; import { RenderResult } from './MarkupToHtml'; +import noteStyle, { whiteBackgroundNoteStyle } from './noteStyle'; +import { Options as NoteStyleOptions } from './noteStyle'; const md5 = require('md5'); // Renderered notes can potentially be quite large (for example @@ -39,6 +41,7 @@ interface RenderOptions { enableLongPress: boolean; itemIdToUrl?: ItemIdToUrlHandler; allowedFilePrefixes?: string[]; + whiteBackgroundNoteRendering?: boolean; } // https://github.com/es-shims/String.prototype.trimStart/blob/main/implementation.js @@ -81,14 +84,22 @@ export default class HtmlToHtml { return this.fsDriver_; } - public async allAssets(/* theme*/): Promise { - return []; // TODO + public async allAssets(theme: any, noteStyleOptions: NoteStyleOptions = null) { + let cssStrings: string[] = []; + + if (noteStyleOptions.whiteBackgroundNoteRendering) { + cssStrings = [whiteBackgroundNoteStyle()]; + } else { + cssStrings = [noteStyle(theme, noteStyleOptions).join('\n')]; + } + + return [await this.fsDriver().cacheCssToFile(cssStrings)]; } // Note: the "theme" variable is ignored and instead the light theme is // always used for HTML notes. // See: https://github.com/laurent22/joplin/issues/3698 - public async render(markup: string, _theme: any, options: RenderOptions): Promise { + public async render(markup: string, theme: any, options: RenderOptions): Promise { options = { splitted: false, postMessageSyntax: 'postMessage', @@ -152,9 +163,7 @@ export default class HtmlToHtml { }; } - // const lightTheme = themeStyle(Setting.THEME_LIGHT); - // let cssStrings = noteStyle(lightTheme); - let cssStrings: string[] = []; + let cssStrings = options.whiteBackgroundNoteRendering ? [whiteBackgroundNoteStyle()] : noteStyle(theme); if (options.splitted) { const splitted = splitHtml(html); diff --git a/packages/renderer/noteStyle.ts b/packages/renderer/noteStyle.ts index b8d7870062f..bb29211f331 100644 --- a/packages/renderer/noteStyle.ts +++ b/packages/renderer/noteStyle.ts @@ -10,8 +10,44 @@ function formatCssSize(v: any): string { export interface Options { contentMaxWidth?: number; contentMaxWidthTarget?: string; + themeId?: number; + whiteBackgroundNoteRendering?: boolean; } +// If we are viewing an HTML note, it means it comes from the web clipper or +// emil-to-note, in which case we don't apply any specific theme. We just need +// to ensure the background is white so that we don't end up with a dark theme +// and dark font for example. https://github.com/laurent22/joplin/issues/9511 +export const whiteBackgroundNoteStyle = () => { + return ` + body { + background-color: #ffffff; + } + + /* TinyMCE adds a dashed border for tables that have no borders + to make it easier to view where the cells are and edit them. + However HTML notes may contain many nested tables used for + layout and we also consider that these notes are more or less + read-only. Because of this, we remove the dashed lines in this + case as it makes the note more readable. */ + + .mce-item-table:not([border]), + .mce-item-table:not([border]) caption, + .mce-item-table:not([border]) td, + .mce-item-table:not([border]) th, + .mce-item-table[border="0"], + .mce-item-table[border="0"] caption, + .mce-item-table[border="0"] td, + .mce-item-table[border="0"] th, + table[style*="border-width: 0px"], + table[style*="border-width: 0px"] caption, + table[style*="border-width: 0px"] td, + table[style*="border-width: 0px"] th { + border: none !important; + } + `; +}; + export default function(theme: any, options: Options = null) { options = { contentMaxWidth: 0,