Skip to content

Commit 010488e

Browse files
mjbvzmjbvz
committed
Avoid re-sanitizing markdown during rendering
Currently rendering markdown sanitizes the content twice. When creating the output element it also goes: `string -> dom-> string -> dom` (with other hops potentially in the implementation) This change changes to a single pass that goes `string -> dom` with a single sanitize call. Since we are moving away from DomParser, the output is slightly changed
1 parent b829292 commit 010488e

13 files changed

+61
-30
lines changed

‎src/vs/base/browser/markdownRenderer.ts‎

Lines changed: 21 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -147,21 +147,28 @@ export function renderMarkdown(markdown: IMarkdownString, options: MarkdownRende
147147
renderedMarkdown = elements.map(e => typeof e === 'string' ? e : e.outerHTML).join('');
148148
}
149149

150-
const htmlParser = new DOMParser();
151-
const markdownHtmlDoc = htmlParser.parseFromString(sanitizeRenderedMarkdown(renderedMarkdown, markdown.isTrusted ?? false, options.sanitizerConfig) as unknown as string, 'text/html');
150+
const renderedContent = document.createElement('div');
151+
const sanitizerConfig = getDomSanitizerConfig(markdown.isTrusted ?? false, options.sanitizerConfig ?? {});
152+
domSanitize.safeSetInnerHtml(renderedContent, renderedMarkdown, sanitizerConfig);
152153

153-
rewriteRenderedLinks(markdown, options, markdownHtmlDoc.body);
154+
// Rewrite links and images before potentially inserting them into the real dom
155+
rewriteRenderedLinks(markdown, options, renderedContent);
154156

155-
const element = target ?? document.createElement('div');
156-
element.innerHTML = sanitizeRenderedMarkdown(markdownHtmlDoc.body.innerHTML, markdown.isTrusted ?? false, options.sanitizerConfig) as unknown as string;
157+
let outElement: HTMLElement;
158+
if (target) {
159+
outElement = target;
160+
DOM.reset(target, ...renderedContent.children);
161+
} else {
162+
outElement = renderedContent;
163+
}
157164

158165
if (codeBlocks.length > 0) {
159166
Promise.all(codeBlocks).then((tuples) => {
160167
if (isDisposed) {
161168
return;
162169
}
163170
const renderedElements = new Map(tuples);
164-
const placeholderElements = element.querySelectorAll<HTMLDivElement>(`div[data-code]`);
171+
const placeholderElements = outElement.querySelectorAll<HTMLDivElement>(`div[data-code]`);
165172
for (const placeholderElement of placeholderElements) {
166173
const renderedElement = renderedElements.get(placeholderElement.dataset['code'] ?? '');
167174
if (renderedElement) {
@@ -172,7 +179,7 @@ export function renderMarkdown(markdown: IMarkdownString, options: MarkdownRende
172179
});
173180
} else if (syncCodeBlocks.length > 0) {
174181
const renderedElements = new Map(syncCodeBlocks);
175-
const placeholderElements = element.querySelectorAll<HTMLDivElement>(`div[data-code]`);
182+
const placeholderElements = outElement.querySelectorAll<HTMLDivElement>(`div[data-code]`);
176183
for (const placeholderElement of placeholderElements) {
177184
const renderedElement = renderedElements.get(placeholderElement.dataset['code'] ?? '');
178185
if (renderedElement) {
@@ -183,7 +190,7 @@ export function renderMarkdown(markdown: IMarkdownString, options: MarkdownRende
183190

184191
// Signal size changes for image tags
185192
if (options.asyncRenderCallback) {
186-
for (const img of element.getElementsByTagName('img')) {
193+
for (const img of outElement.getElementsByTagName('img')) {
187194
const listener = disposables.add(DOM.addDisposableListener(img, 'load', () => {
188195
listener.dispose();
189196
options.asyncRenderCallback!();
@@ -193,17 +200,17 @@ export function renderMarkdown(markdown: IMarkdownString, options: MarkdownRende
193200

194201
// Add event listeners for links
195202
if (options.actionHandler) {
196-
const onClick = options.actionHandler.disposables.add(new DomEmitter(element, 'click'));
197-
const onAuxClick = options.actionHandler.disposables.add(new DomEmitter(element, 'auxclick'));
203+
const onClick = options.actionHandler.disposables.add(new DomEmitter(outElement, 'click'));
204+
const onAuxClick = options.actionHandler.disposables.add(new DomEmitter(outElement, 'auxclick'));
198205
options.actionHandler.disposables.add(Event.any(onClick.event, onAuxClick.event)(e => {
199-
const mouseEvent = new StandardMouseEvent(DOM.getWindow(element), e);
206+
const mouseEvent = new StandardMouseEvent(DOM.getWindow(outElement), e);
200207
if (!mouseEvent.leftButton && !mouseEvent.middleButton) {
201208
return;
202209
}
203210
activateLink(markdown, options, mouseEvent);
204211
}));
205212

206-
options.actionHandler.disposables.add(DOM.addDisposableListener(element, 'keydown', (e) => {
213+
options.actionHandler.disposables.add(DOM.addDisposableListener(outElement, 'keydown', (e) => {
207214
const keyboardEvent = new StandardKeyboardEvent(e);
208215
if (!keyboardEvent.equals(KeyCode.Space) && !keyboardEvent.equals(KeyCode.Enter)) {
209216
return;
@@ -213,7 +220,7 @@ export function renderMarkdown(markdown: IMarkdownString, options: MarkdownRende
213220
}
214221

215222
// Remove/disable inputs
216-
for (const input of [...element.getElementsByTagName('input')]) {
223+
for (const input of [...outElement.getElementsByTagName('input')]) {
217224
if (input.attributes.getNamedItem('type')?.value === 'checkbox') {
218225
input.setAttribute('disabled', '');
219226
} else {
@@ -227,7 +234,7 @@ export function renderMarkdown(markdown: IMarkdownString, options: MarkdownRende
227234
}
228235

229236
return {
230-
element,
237+
element: outElement,
231238
dispose: () => {
232239
isDisposed = true;
233240
disposables.dispose();

‎src/vs/base/test/browser/markdownRenderer.test.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ suite('MarkdownRenderer', () => {
166166
mds.appendMarkdown(`[$(zap)-link](#link)`);
167167

168168
const result: HTMLElement = store.add(renderMarkdown(mds)).element;
169-
assert.strictEqual(result.innerHTML, `<p><a data-href="#link" href="" title="#link" draggable="false"><span class="codicon codicon-zap"></span>-link</a></p>`);
169+
assert.strictEqual(result.innerHTML, `<p><a draggable="false" title="#link" href="" data-href="#link"><span class="codicon codicon-zap"></span>-link</a></p>`);
170170
});
171171

172172
test('render icon in table', () => {
@@ -186,7 +186,7 @@ suite('MarkdownRenderer', () => {
186186
</thead>
187187
<tbody><tr>
188188
<td><span class="codicon codicon-zap"></span></td>
189-
<td><a data-href="#link" href="" title="#link" draggable="false"><span class="codicon codicon-zap"></span>-link</a></td>
189+
<td><a draggable="false" title="#link" href="" data-href="#link"><span class="codicon codicon-zap"></span>-link</a></td>
190190
</tr>
191191
</tbody></table>
192192
`);
@@ -253,7 +253,7 @@ suite('MarkdownRenderer', () => {
253253
});
254254

255255
const result: HTMLElement = store.add(renderMarkdown(md)).element;
256-
assert.strictEqual(result.innerHTML, `<p><a data-href="command:doFoo" href="" title="command:doFoo" draggable="false">command1</a> <a data-href="command:doFoo" href="">command2</a></p>`);
256+
assert.strictEqual(result.innerHTML, `<p><a draggable="false" title="command:doFoo" href="" data-href="command:doFoo">command1</a> <a href="" data-href="command:doFoo">command2</a></p>`);
257257
});
258258

259259
suite('PlaintextMarkdownRender', () => {
@@ -355,7 +355,7 @@ suite('MarkdownRenderer', () => {
355355
const result = store.add(renderMarkdown(mds)).element;
356356

357357
// Inputs should always be disabled too
358-
assert.strictEqual(result.innerHTML, `<p>text: \ncheckbox:<input type="checkbox" disabled=""></p>`);
358+
assert.strictEqual(result.innerHTML, `<p>text: \ncheckbox:<input disabled="" type="checkbox"></p>`);
359359
});
360360
});
361361

‎src/vs/workbench/contrib/chat/test/browser/__snapshots__/ChatMarkdownRenderer_CDATA.0.snap‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1,3 @@
1-
<div class="rendered-markdown"><p>&lt;!--[CDATA[&lt;div--&gt;content]]&gt;</p></div>
1+
<div class="rendered-markdown"><p>
2+
3+
&lt;!--[CDATA[&lt;div--&gt;content]]&gt;</p></div>

‎src/vs/workbench/contrib/chat/test/browser/__snapshots__/ChatMarkdownRenderer_html_comments.0.snap‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1,3 @@
1-
<div class="rendered-markdown">&lt;!-- comment1 &lt;div&gt;&lt;/div&gt; --&gt;<div>content</div><p>&lt;!-- comment2 --&gt;</p></div>
1+
<div class="rendered-markdown">
2+
3+
&lt;!-- comment1 &lt;div&gt;&lt;/div&gt; --&gt;<div>content</div><p>&lt;!-- comment2 --&gt;</p></div>

‎src/vs/workbench/contrib/chat/test/browser/__snapshots__/ChatMarkdownRenderer_invalid_HTML.0.snap‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1,3 @@
1-
<div class="rendered-markdown"><p>1&lt;canvas&gt;2&lt;/canvas&gt;</p>&lt;details&gt;3&lt;/details&gt;4<p></p></div>
1+
<div class="rendered-markdown">
2+
3+
<p>1&lt;canvas&gt;2&lt;/canvas&gt;</p>&lt;details&gt;3&lt;/details&gt;4<p></p></div>

‎src/vs/workbench/contrib/chat/test/browser/__snapshots__/ChatMarkdownRenderer_invalid_HTML_with_attributes.0.snap‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1,3 @@
1-
<div class="rendered-markdown"><p>1</p>&lt;details id="id1" style="display: none"&gt;2&lt;details id="my id 2"&gt;3&lt;/details&gt;&lt;/details&gt;4<p></p></div>
1+
<div class="rendered-markdown">
2+
3+
<p>1</p>&lt;details id="id1" style="display: none"&gt;2&lt;details id="my id 2"&gt;3&lt;/details&gt;&lt;/details&gt;4<p></p></div>

‎src/vs/workbench/contrib/chat/test/browser/__snapshots__/ChatMarkdownRenderer_mixed_valid_and_invalid_HTML.0.snap‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,7 @@
1-
<div class="rendered-markdown"><h1>heading</h1>
1+
<div class="rendered-markdown">
2+
3+
4+
<h1>heading</h1>
25
&lt;details&gt;
36
<ul>
47
<li><span>&lt;details&gt;<i>1</i>&lt;/details&gt;</span></li>

‎src/vs/workbench/contrib/chat/test/browser/__snapshots__/ChatMarkdownRenderer_remote_images_are_disallowed.0.snap‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1,3 @@
1-
<div class="rendered-markdown"><p></p><div>&lt;img src="http://disallowed.com/image.jpg"&gt;</div><p></p></div>
1+
<div class="rendered-markdown">
2+
3+
<p><div>&lt;img src="http://disallowed.com/image.jpg"&gt;</div></p></div>

‎src/vs/workbench/contrib/chat/test/browser/__snapshots__/ChatMarkdownRenderer_self-closing_elements.0.snap‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1,3 @@
1-
<div class="rendered-markdown"><p>&lt;area&gt;</p><hr><br>&lt;input type="text" value="test"&gt;<p></p></div>
1+
<div class="rendered-markdown">
2+
3+
<p>&lt;area&gt;</p><hr><br>&lt;input type="text" value="test"&gt;<p></p></div>

‎src/vs/workbench/contrib/chat/test/browser/__snapshots__/ChatMarkdownRenderer_self-closing_elements.1.snap‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1,3 @@
1-
<div class="rendered-markdown"><p>&lt;area&gt;</p><hr><br><input type="checkbox" disabled=""><p></p></div>
1+
<div class="rendered-markdown">
2+
3+
<p>&lt;area&gt;</p><hr><br><input disabled="" type="checkbox"><p></p></div>

0 commit comments

Comments
 (0)