From 2d29ad27181e861c1f28449fce36b471537e1c88 Mon Sep 17 00:00:00 2001
From: Mark Florian <mflorian@gitlab.com>
Date: Tue, 16 Apr 2024 19:21:52 +0100
Subject: [PATCH] Fix math block alert rendering
This replaces the static markup to recreate an alert with a real
`GlAlert` instance for maths blocks. This fixes incorrect styling of the
alert, incorrect positioning of the `<copy-code>` element, and also
simplifies wiring up of events.
Before, the `<copy-code>` button was displaying over the alert's dismiss
button. Now, it correctly displays within the maths code block it's
associated with.
Since `<copy-code>` is absolutely positioned, and its nearest positioned
ancestor is the `.js-markdown-code` element, and the alert was the first
child of that element, the `<copy-code>` button was rendered over the
alert instead of the code block below.
Now, the alert is the immediate previous sibling of the
`.js-markdown-code` element, instead of its first child.
Addresses https://gitlab.com/gitlab-org/gitlab/-/issues/455642 and
https://gitlab.com/gitlab-org/gitlab/-/issues/455665.
Changelog: fixed
---
.../behaviors/markdown/render_math.js | 120 +++++++++---------
locale/gitlab.pot | 3 +
spec/features/markdown/math_spec.rb | 9 +-
3 files changed, 67 insertions(+), 65 deletions(-)
diff --git a/app/assets/javascripts/behaviors/markdown/render_math.js b/app/assets/javascripts/behaviors/markdown/render_math.js
index 20450b4d6cb89..b9b68f680b38a 100644
--- a/app/assets/javascripts/behaviors/markdown/render_math.js
+++ b/app/assets/javascripts/behaviors/markdown/render_math.js
@@ -1,5 +1,6 @@
+import { GlAlert } from '@gitlab/ui';
import { escape } from 'lodash';
-import { spriteIcon } from '~/lib/utils/common_utils';
+import Vue from 'vue';
import { differenceInMilliseconds } from '~/lib/utils/datetime_utility';
import { s__, sprintf } from '~/locale';
@@ -14,6 +15,7 @@ const MAX_MATH_CHARS = 1000;
const MAX_MACRO_EXPANSIONS = 1000;
const MAX_USER_SPECIFIED_EMS = 20;
const MAX_RENDER_TIME_MS = 2000;
+const LAZY_ALERT_SHOWN_CLASS = 'lazy-alert-shown';
// Wait for the browser to reflow the layout. Reflowing SVG takes time.
// This has to wrap the inner function, otherwise IE/Edge throw "invalid calling object".
@@ -74,7 +76,6 @@ class SafeMathRenderer {
this.renderElement = this.renderElement.bind(this);
this.render = this.render.bind(this);
- this.attachEvents = this.attachEvents.bind(this);
this.pageName = document.querySelector('body').dataset.page;
}
@@ -86,48 +87,30 @@ class SafeMathRenderer {
const el = chosenEl || this.queue.shift();
const forceRender = Boolean(chosenEl) || !gon.math_rendering_limits_enabled;
const text = el.textContent;
+ const isTextTooLong = text.length > MAX_MATH_CHARS;
el.removeAttribute('style');
- if (!forceRender && (this.totalMS >= MAX_RENDER_TIME_MS || text.length > MAX_MATH_CHARS)) {
- // Show un-rendered math code
- const codeElement = document.createElement('pre');
-
- codeElement.className = 'code';
- codeElement.textContent = el.textContent;
- codeElement.dataset.mathStyle = el.dataset.mathStyle;
-
- let message;
- if (text.length > MAX_MATH_CHARS) {
- message = sprintf(
- s__(
- 'math|This math block exceeds %{maxMathChars} characters, and may cause performance issues on this page.',
- ),
- { maxMathChars: MAX_MATH_CHARS },
- );
- } else {
- message = s__('math|Displaying this math block may cause performance issues on this page.');
- }
-
- const html = `
- <div class="alert gl-alert gl-alert-warning alert-dismissible lazy-render-math-container js-lazy-render-math-container fade show" role="alert">
- ${spriteIcon('warning', 'gl-text-orange-600 s16 gl-alert-icon')}
- <div class="display-flex gl-alert-content">
- <div>${message}</div>
- <div class="gl-alert-actions">
- <button class="js-lazy-render-math btn gl-alert-action btn-confirm btn-md gl-button">Display anyway</button>
- </div>
- </div>
- <button type="button" class="close js-close" aria-label="Close">
- ${spriteIcon('close', 's16')}
- </button>
- </div>
- `;
-
- if (!el.classList.contains('lazy-alert-shown')) {
- // eslint-disable-next-line no-unsanitized/property
- el.innerHTML = html;
- el.append(codeElement);
- el.classList.add('lazy-alert-shown');
+ if (!forceRender && (this.totalMS >= MAX_RENDER_TIME_MS || isTextTooLong)) {
+ if (!el.classList.contains(LAZY_ALERT_SHOWN_CLASS)) {
+ el.classList.add(LAZY_ALERT_SHOWN_CLASS);
+
+ // Show un-rendered math code
+ const codeElement = document.createElement('pre');
+ codeElement.className = 'code';
+ codeElement.textContent = el.textContent;
+ codeElement.dataset.mathStyle = el.dataset.mathStyle;
+ el.replaceChildren(codeElement);
+
+ this.renderAlert({
+ // We do not want to put the alert in the <copy-code> element's nearest
+ // positioned ancestor, otherwise it will display over the alert instead of
+ // the code block. Instead, put the alert *before* that ancestor.
+ mountBeforeEl: el.closest('.js-markdown-code'),
+ isTextTooLong,
+ onDisplayAnyway: () => {
+ this.renderElement(codeElement);
+ },
+ });
}
// Render the next math
@@ -194,27 +177,45 @@ class SafeMathRenderer {
setTimeout(this.renderElement, 400);
}
- attachEvents() {
- document.body.addEventListener('click', (event) => {
- const alert = event.target.closest('.js-lazy-render-math-container');
+ // eslint-disable-next-line class-methods-use-this
+ renderAlert({ mountBeforeEl, isTextTooLong, onDisplayAnyway }) {
+ let alert;
- if (!alert) {
- return;
- }
+ const dismiss = () => {
+ alert.$destroy();
+ alert.$el.remove();
+ };
- // Handle alert close
- if (event.target.closest('.js-close')) {
- alert.remove();
- return;
- }
+ const displayAnyway = () => {
+ dismiss();
+ onDisplayAnyway();
+ };
- // Handle "render anyway"
- if (event.target.classList.contains('js-lazy-render-math')) {
- const pre = alert.nextElementSibling;
- alert.remove();
- this.renderElement(pre);
- }
+ const message = isTextTooLong
+ ? sprintf(
+ s__(
+ 'math|This math block exceeds %{maxMathChars} characters, and may cause performance issues on this page.',
+ ),
+ { maxMathChars: MAX_MATH_CHARS },
+ )
+ : s__('math|Displaying this math block may cause performance issues on this page.');
+
+ alert = new Vue({
+ render(h) {
+ return h(
+ GlAlert,
+ {
+ class: 'gl-mb-5',
+ props: { variant: 'warning', primaryButtonText: s__('math|Display anyway') },
+ on: { dismiss, primaryAction: displayAnyway },
+ },
+ message,
+ );
+ },
});
+
+ alert.$mount();
+ mountBeforeEl.before(alert.$el);
}
}
@@ -227,7 +228,6 @@ export default function renderMath(elements) {
.then(([katex]) => {
const renderer = new SafeMathRenderer(elements, katex);
renderer.render();
- renderer.attachEvents();
})
.catch(() => {});
}
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 6093f281b27db..32b72a4d95b30 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -61028,6 +61028,9 @@ msgstr ""
msgid "manual"
msgstr ""
+msgid "math|Display anyway"
+msgstr ""
+
msgid "math|Displaying this math block may cause performance issues on this page."
msgstr ""
diff --git a/spec/features/markdown/math_spec.rb b/spec/features/markdown/math_spec.rb
index 1835661b6a3c8..d59c9d474e0ca 100644
--- a/spec/features/markdown/math_spec.rb
+++ b/spec/features/markdown/math_spec.rb
@@ -70,7 +70,7 @@
create_and_visit_issue_with_description(lazy_load_description)
page.within '.description > .md' do
- expect(page).to have_selector('.js-lazy-render-math-container', text: /math block exceeds 1000 characters/)
+ expect(page).to have_selector('[role="alert"]', text: /math block exceeds 1000 characters/)
end
end
@@ -111,9 +111,9 @@
# the find is needed to ensure the lazy container is loaded, otherwise
# it can be a flaky test, similar to
# https://gitlab.com/gitlab-org/gitlab/-/merge_requests/25408
- find('.js-lazy-render-math-container')
+ find('[role="alert"]')
- expect(page).to have_selector('.js-lazy-render-math-container', text: /math block exceeds 1000 characters/)
+ expect(page).to have_selector('[role="alert"]', text: /math block exceeds 1000 characters/)
end
end
end
@@ -128,8 +128,7 @@
page.within '.description > .md' do
expect(page).not_to have_selector('button', text: 'Display anyway')
- expect(page)
- .not_to have_selector('.js-lazy-render-math-container', text: /math block exceeds 1000 characters/)
+ expect(page).not_to have_selector('[role="alert"]', text: /math block exceeds 1000 characters/)
end
end
--
GitLab