From dcde4701a5b31c88b3120722c3163af4214264d2 Mon Sep 17 00:00:00 2001 From: silverwind Date: Mon, 17 Apr 2023 12:10:22 +0200 Subject: [PATCH] Fix math and mermaid rendering bugs (#24049) 1. Fix multiple error display for math and mermaid: ![err](https://user-images.githubusercontent.com/115237/231126411-8a21a777-cd53-4b7e-ac67-5332623106e8.gif) 2. Fix height calculation of certain mermaid diagrams by reading the iframe inner height from it's document instead of parsing it from SVG: Before: Screenshot 2023-04-11 at 11 56 27 After: Screenshot 2023-04-11 at 11 56 35 3. Refactor error handling to a common function 4. Rename to `renderAsciicast` for consistency 5. Improve mermaid loading sequence Note: I did try `securityLevel: 'sandbox'` to make mermaid output a iframe directly, but that showed a bug in mermaid where the iframe style height was set incorrectly. Opened https://github.com/mermaid-js/mermaid/issues/4289 for this. --------- Co-authored-by: Giteabot --- web_src/css/base.css | 2 +- web_src/css/helpers.css | 3 +- web_src/css/markup/content.css | 2 +- web_src/js/markup/asciicast.js | 2 +- web_src/js/markup/common.js | 8 ++++++ web_src/js/markup/content.js | 4 +-- web_src/js/markup/math.js | 18 +++++------- web_src/js/markup/mermaid.js | 50 ++++++++++++++++------------------ 8 files changed, 46 insertions(+), 43 deletions(-) create mode 100644 web_src/js/markup/common.js diff --git a/web_src/css/base.css b/web_src/css/base.css index c48a36c85..bdf601951 100644 --- a/web_src/css/base.css +++ b/web_src/css/base.css @@ -9,7 +9,7 @@ /* non-color variables */ --border-radius: 0.28571429rem; --opacity-disabled: 0.55; - --height-loading: 12rem; + --height-loading: 16rem; /* base colors */ --color-primary: #4183c4; --color-primary-contrast: #ffffff; diff --git a/web_src/css/helpers.css b/web_src/css/helpers.css index 8d64bd751..beb93e1e8 100644 --- a/web_src/css/helpers.css +++ b/web_src/css/helpers.css @@ -25,7 +25,8 @@ .gt-overflow-x-scroll { overflow-x: scroll !important; } .gt-cursor-default { cursor: default !important; } .gt-items-start { align-items: flex-start !important; } -.gt-whitespace-pre { white-space: pre !important } +.gt-whitespace-pre { white-space: pre !important; } +.gt-invisible { visibility: hidden !important; } .gt-mono { font-family: var(--fonts-monospace) !important; diff --git a/web_src/css/markup/content.css b/web_src/css/markup/content.css index 90f8c7091..d0f11e8e7 100644 --- a/web_src/css/markup/content.css +++ b/web_src/css/markup/content.css @@ -542,7 +542,7 @@ .markup-block-error { display: block !important; /* override fomantic .ui.form .error.message {display: none} */ - border: 1px solid var(--color-error-border) !important; + border: none !important; margin-bottom: 0 !important; border-bottom-left-radius: 0 !important; border-bottom-right-radius: 0 !important; diff --git a/web_src/js/markup/asciicast.js b/web_src/js/markup/asciicast.js index 902cfcb73..97b18743a 100644 --- a/web_src/js/markup/asciicast.js +++ b/web_src/js/markup/asciicast.js @@ -1,4 +1,4 @@ -export async function renderAsciinemaPlayer() { +export async function renderAsciicast() { const els = document.querySelectorAll('.asciinema-player-container'); if (!els.length) return; diff --git a/web_src/js/markup/common.js b/web_src/js/markup/common.js new file mode 100644 index 000000000..aff4a3242 --- /dev/null +++ b/web_src/js/markup/common.js @@ -0,0 +1,8 @@ +export function displayError(el, err) { + el.classList.remove('is-loading'); + const errorNode = document.createElement('pre'); + errorNode.setAttribute('class', 'ui message error markup-block-error'); + errorNode.textContent = err.str || err.message || String(err); + el.before(errorNode); + el.setAttribute('data-render-done', 'true'); +} diff --git a/web_src/js/markup/content.js b/web_src/js/markup/content.js index e4ec3d0b4..1d29dc07f 100644 --- a/web_src/js/markup/content.js +++ b/web_src/js/markup/content.js @@ -1,7 +1,7 @@ import {renderMermaid} from './mermaid.js'; import {renderMath} from './math.js'; import {renderCodeCopy} from './codecopy.js'; -import {renderAsciinemaPlayer} from './asciicast.js'; +import {renderAsciicast} from './asciicast.js'; import {initMarkupTasklist} from './tasklist.js'; // code that runs for all markup content @@ -9,7 +9,7 @@ export function initMarkupContent() { renderMermaid(); renderMath(); renderCodeCopy(); - renderAsciinemaPlayer(); + renderAsciicast(); } // code that only runs for comments diff --git a/web_src/js/markup/math.js b/web_src/js/markup/math.js index dcc656ea8..8427637a0 100644 --- a/web_src/js/markup/math.js +++ b/web_src/js/markup/math.js @@ -1,14 +1,8 @@ -function displayError(el, err) { - const target = targetElement(el); - target.classList.remove('is-loading'); - const errorNode = document.createElement('div'); - errorNode.setAttribute('class', 'ui message error markup-block-error gt-mono'); - errorNode.textContent = err.str || err.message || String(err); - target.before(errorNode); -} +import {displayError} from './common.js'; function targetElement(el) { - // The target element is either the current element if it has the `is-loading` class or the pre that contains it + // The target element is either the current element if it has the + // `is-loading` class or the pre that contains it return el.classList.contains('is-loading') ? el : el.closest('pre'); } @@ -22,6 +16,8 @@ export async function renderMath() { ]); for (const el of els) { + const target = targetElement(el); + if (target.hasAttribute('data-render-done')) continue; const source = el.textContent; const displayMode = el.classList.contains('display'); const nodeName = displayMode ? 'p' : 'span'; @@ -33,9 +29,9 @@ export async function renderMath() { maxExpand: 50, displayMode, }); - targetElement(el).replaceWith(tempEl); + target.replaceWith(tempEl); } catch (error) { - displayError(el, error); + displayError(target, error); } } } diff --git a/web_src/js/markup/mermaid.js b/web_src/js/markup/mermaid.js index b519e2dcd..865a414c9 100644 --- a/web_src/js/markup/mermaid.js +++ b/web_src/js/markup/mermaid.js @@ -1,21 +1,12 @@ import {isDarkTheme} from '../utils.js'; import {makeCodeCopyButton} from './codecopy.js'; +import {displayError} from './common.js'; const {mermaidMaxSourceCharacters} = window.config; -const iframeCss = ` - :root {color-scheme: normal} - body {margin: 0; padding: 0; overflow: hidden} - #mermaid {display: block; margin: 0 auto} -`; - -function displayError(el, err) { - el.closest('pre').classList.remove('is-loading'); - const errorNode = document.createElement('div'); - errorNode.setAttribute('class', 'ui message error markup-block-error gt-mono'); - errorNode.textContent = err.str || err.message || String(err); - el.closest('pre').before(errorNode); -} +const iframeCss = `:root {color-scheme: normal} +body {margin: 0; padding: 0; overflow: hidden} +#mermaid {display: block; margin: 0 auto}`; export async function renderMermaid() { const els = document.querySelectorAll('.markup code.language-mermaid'); @@ -30,18 +21,19 @@ export async function renderMermaid() { }); for (const el of els) { - const source = el.textContent; + const pre = el.closest('pre'); + if (pre.hasAttribute('data-render-done')) continue; + const source = el.textContent; if (mermaidMaxSourceCharacters >= 0 && source.length > mermaidMaxSourceCharacters) { - displayError(el, new Error(`Mermaid source of ${source.length} characters exceeds the maximum allowed length of ${mermaidMaxSourceCharacters}.`)); + displayError(pre, new Error(`Mermaid source of ${source.length} characters exceeds the maximum allowed length of ${mermaidMaxSourceCharacters}.`)); continue; } try { await mermaid.parse(source); } catch (err) { - displayError(el, err); - el.closest('pre').classList.remove('is-loading'); + displayError(pre, err); continue; } @@ -49,26 +41,32 @@ export async function renderMermaid() { // can't use bindFunctions here because we can't cross the iframe boundary. This // means js-based interactions won't work but they aren't intended to work either const {svg} = await mermaid.render('mermaid', source); - const heightStr = (svg.match(/viewBox="(.+?)"/) || ['', ''])[1].split(/\s+/)[3]; - if (!heightStr) return displayError(el, new Error('Could not determine chart height')); const iframe = document.createElement('iframe'); - iframe.classList.add('markup-render'); - iframe.sandbox = 'allow-scripts'; - iframe.style.height = `${Math.ceil(parseFloat(heightStr))}px`; + iframe.classList.add('markup-render', 'gt-invisible'); iframe.srcdoc = `${svg}`; const mermaidBlock = document.createElement('div'); - mermaidBlock.classList.add('mermaid-block'); + mermaidBlock.classList.add('mermaid-block', 'is-loading', 'gt-hidden'); mermaidBlock.append(iframe); const btn = makeCodeCopyButton(); btn.setAttribute('data-clipboard-text', source); - mermaidBlock.append(btn); - el.closest('pre').replaceWith(mermaidBlock); + + iframe.addEventListener('load', () => { + pre.replaceWith(mermaidBlock); + mermaidBlock.classList.remove('gt-hidden'); + iframe.style.height = `${iframe.contentWindow.document.body.clientHeight}px`; + setTimeout(() => { // avoid flash of iframe background + mermaidBlock.classList.remove('is-loading'); + iframe.classList.remove('gt-invisible'); + }, 0); + }); + + document.body.append(mermaidBlock); } catch (err) { - displayError(el, err); + displayError(pre, err); } } }