From ad06fc73214dd51dd0c00b2c054524c527fd56d0 Mon Sep 17 00:00:00 2001 From: Charles Gagnon Date: Thu, 27 Jan 2022 15:00:00 -0800 Subject: [PATCH] Fix images in Notebook not showing (#18160) * Fix images in Notebook not showing * fixes * more fixes --- .../cellViews/markdownToolbar.component.ts | 9 +++- .../notebook/browser/htmlMarkdownConverter.ts | 7 ++- .../browser/outputs/notebookMarkdown.ts | 14 +++++- .../test/browser/notebookMarkdown.test.ts | 45 ++++++++++++++++--- src/vs/base/common/network.ts | 2 + 5 files changed, 65 insertions(+), 12 deletions(-) diff --git a/src/sql/workbench/contrib/notebook/browser/cellViews/markdownToolbar.component.ts b/src/sql/workbench/contrib/notebook/browser/cellViews/markdownToolbar.component.ts index 44a5a6b9c5..38008e603c 100644 --- a/src/sql/workbench/contrib/notebook/browser/cellViews/markdownToolbar.component.ts +++ b/src/sql/workbench/contrib/notebook/browser/cellViews/markdownToolbar.component.ts @@ -27,6 +27,7 @@ import { TextCellEditModes } from 'sql/workbench/services/notebook/common/contra import { NotebookLinkHandler } from 'sql/workbench/contrib/notebook/browser/notebookLinkHandler'; import { StandardKeyboardEvent } from 'vs/base/browser/keyboardEvent'; import { KeyCode } from 'vs/base/common/keyCodes'; +import { FileAccess } from 'vs/base/common/network'; export const MARKDOWN_TOOLBAR_SELECTOR: string = 'markdown-toolbar-component'; const linksRegex = /\[(?.+)\]\((?[^ ]+)(?: "(?.+)")?\)/; @@ -297,8 +298,12 @@ export class MarkdownToolbarComponent extends AngularDisposable { await insertFormattedMarkdown(linkCalloutResult?.insertEscapedMarkdown, this.getCellEditorControl()); } else if (type === MarkdownButtonType.IMAGE_PREVIEW) { if (imageCalloutResult.embedImage) { - let base64String = await this.getFileContentBase64(URI.file(imageCalloutResult.imagePath)); - let mimeType = await this.getFileMimeType(URI.file(imageCalloutResult.imagePath)); + // VS Code blocks loading directly from the file protocol - we have to transform it to a vscode-file URI + // first. Currently we assume that the path here is always going to be a path since we don't support + // embedding images from web links. + const uri = FileAccess.asBrowserUri(URI.file(imageCalloutResult.imagePath)); + let base64String = await this.getFileContentBase64(uri); + let mimeType = await this.getFileMimeType(uri); const originalImageName: string = path.basename(imageCalloutResult.imagePath).replace(/\s/g, ''); let attachmentName = this.cellModel.addAttachment(mimeType, base64String, originalImageName); if (originalImageName !== attachmentName) { diff --git a/src/sql/workbench/contrib/notebook/browser/htmlMarkdownConverter.ts b/src/sql/workbench/contrib/notebook/browser/htmlMarkdownConverter.ts index b2f8632554..6a937cd070 100644 --- a/src/sql/workbench/contrib/notebook/browser/htmlMarkdownConverter.ts +++ b/src/sql/workbench/contrib/notebook/browser/htmlMarkdownConverter.ts @@ -9,6 +9,7 @@ import * as path from 'vs/base/common/path'; import * as turndownPluginGfm from 'sql/workbench/contrib/notebook/browser/turndownPluginGfm'; import { IConfigurationService } from 'vs/platform/configuration/common/configuration'; import { findPathRelativeToContent, NotebookLinkHandler } from 'sql/workbench/contrib/notebook/browser/notebookLinkHandler'; +import { FileAccess } from 'vs/base/common/network'; // These replacements apply only to text. Here's how it's handled from Turndown: // if (node.nodeType === 3) { @@ -121,9 +122,11 @@ export class HTMLMarkdownConverter { filter: 'img', replacement: (content, node) => { if (node?.src) { - let imgPath = URI.parse(node.src); + // Image URIs are converted to vscode-file URIs for the underlying HTML so that they can be loaded by ADS, + // but we want to convert them back to their file URI when converting back to markdown for displaying to the user + let imgUri = FileAccess.asFileUri(URI.parse(node.src)); const notebookFolder: string = this.notebookUri ? path.join(path.dirname(this.notebookUri.fsPath), path.sep) : ''; - let relativePath = findPathRelativeToContent(notebookFolder, imgPath); + let relativePath = findPathRelativeToContent(notebookFolder, imgUri); if (relativePath) { return `![${node.alt}](${relativePath})`; } diff --git a/src/sql/workbench/contrib/notebook/browser/outputs/notebookMarkdown.ts b/src/sql/workbench/contrib/notebook/browser/outputs/notebookMarkdown.ts index 82c35933c9..ca7023982b 100644 --- a/src/sql/workbench/contrib/notebook/browser/outputs/notebookMarkdown.ts +++ b/src/sql/workbench/contrib/notebook/browser/outputs/notebookMarkdown.ts @@ -17,6 +17,7 @@ import { IMarkdownStringWithCellAttachments, MarkdownRenderOptionsWithCellAttach import { replaceInvalidLinkPath } from 'sql/workbench/contrib/notebook/common/utils'; import { IConfigurationService } from 'vs/platform/configuration/common/configuration'; import { useNewMarkdownRendererKey } from 'sql/workbench/contrib/notebook/common/notebookCommon'; +import { FileAccess, Schemas } from 'vs/base/common/network'; // Based off of HtmlContentRenderer export class NotebookMarkdownRenderer { @@ -98,7 +99,18 @@ export class NotebookMarkdownRenderer { } let attributes: string[] = []; if (href) { - attributes.push(`src="${href}"`); + // VS Code blocks loading directly from the file protocol - we have to transform it to a vscode-file URI + // first. Since the href here can be either a file path, an HTTP/S link or embedded data though we first + // check if it's any of the others and if so then don't need to do anything. + let uri = URI.parse(href); + if (!(uri.scheme === Schemas.https || + uri.scheme === Schemas.http || + uri.scheme === Schemas.data || + uri.scheme === Schemas.attachment || + uri.scheme === Schemas.vscodeFileResource)) { + uri = FileAccess.asBrowserUri(URI.file(href)); + } + attributes.push(`src="${uri.toString(true)}"`); } if (text) { attributes.push(`alt="${text}"`); diff --git a/src/sql/workbench/contrib/notebook/test/browser/notebookMarkdown.test.ts b/src/sql/workbench/contrib/notebook/test/browser/notebookMarkdown.test.ts index e399a4e557..0c6c31feef 100644 --- a/src/sql/workbench/contrib/notebook/test/browser/notebookMarkdown.test.ts +++ b/src/sql/workbench/contrib/notebook/test/browser/notebookMarkdown.test.ts @@ -18,7 +18,7 @@ suite('NotebookMarkdownRenderer', () => { const imageFromMarked = marked(markdown.value, { sanitize: true, renderer - }).trim(); + }).trim().replace('someimageurl', 'vscode-file://vscode-app/someimageurl'); assert.strictEqual(result.innerHTML, imageFromMarked); }); @@ -26,26 +26,26 @@ suite('NotebookMarkdownRenderer', () => { const markdown = { value: `![image](someimageurl)` }; const result: HTMLElement = notebookMarkdownRenderer.renderMarkdown(markdown); const renderer = new marked.Renderer(); - const imageFromMarked = marked(markdown.value, { + let imageFromMarked = marked(markdown.value, { sanitize: true, renderer - }).trim(); + }).trim().replace('someimageurl', 'vscode-file://vscode-app/someimageurl'); assert.strictEqual(result.innerHTML, imageFromMarked); }); test('image width from title params', () => { let result: HTMLElement = notebookMarkdownRenderer.renderMarkdown({ value: `![image](someimageurl|width=100 'caption')` }); - assert.strictEqual(result.innerHTML, `<p><img src="someimageurl" alt="image" title="caption" width="100"></p>`); + assert.strictEqual(result.innerHTML, `<p><img src="vscode-file://vscode-app/someimageurl" alt="image" title="caption" width="100"></p>`); }); test('image height from title params', () => { let result: HTMLElement = notebookMarkdownRenderer.renderMarkdown({ value: `![image](someimageurl|height=100 'caption')` }); - assert.strictEqual(result.innerHTML, `<p><img src="someimageurl" alt="image" title="caption" height="100"></p>`); + assert.strictEqual(result.innerHTML, `<p><img src="vscode-file://vscode-app/someimageurl" alt="image" title="caption" height="100"></p>`); }); test('image width and height from title params', () => { let result: HTMLElement = notebookMarkdownRenderer.renderMarkdown({ value: `![image](someimageurl|height=200,width=100 'caption')` }); - assert.strictEqual(result.innerHTML, `<p><img src="someimageurl" alt="image" title="caption" width="100" height="200"></p>`); + assert.strictEqual(result.innerHTML, `<p><img src="vscode-file://vscode-app/someimageurl" alt="image" title="caption" width="100" height="200"></p>`); }); test('link from local file path', () => { @@ -99,12 +99,43 @@ suite('NotebookMarkdownRenderer', () => { assert.strictEqual(result.innerHTML, `<p><img src="attachment:ads.png" alt="altText"></p>`, 'Cell attachment name not found failed'); result = notebookMarkdownRenderer.renderMarkdown({ value: `![altText](attachments:ads.png)`, isTrusted: true }, { cellAttachments: JSON.parse('{"ads2.png":{"image/png":"iVBORw0KGgoAAAANSUhEUgAAAggg=="}}') }); - assert.strictEqual(result.innerHTML, `<p><img src="attachments:ads.png" alt="altText"></p>`, 'Cell attachment scheme mismatch failed'); + assert.strictEqual(result.innerHTML, `<p><img src="vscode-file://vscode-app/attachments:ads.png" alt="altText"></p>`, 'Cell attachment scheme mismatch failed'); result = notebookMarkdownRenderer.renderMarkdown({ value: `![altText](attachment:ads.png)`, isTrusted: true }, { cellAttachments: JSON.parse('{"ads2.png":"image/png"}') }); assert.strictEqual(result.innerHTML, `<p><img src="attachment:ads.png" alt="altText"></p>`, 'Cell attachment no image data failed'); }); + suite('Schema validation', function () { + test('https', () => { + const result: HTMLElement = notebookMarkdownRenderer.renderMarkdown({ value: `![](https://example.com)` }); + assert.strictEqual(result.innerHTML, `<p><img src="https://example.com/"></p>`, 'HTTPS schema link should not be modified'); + }); + test('http', () => { + const result: HTMLElement = notebookMarkdownRenderer.renderMarkdown({ value: `![](http://example.com)` }); + assert.strictEqual(result.innerHTML, `<p><img src="http://example.com/"></p>`, 'HTTP schema link should not be modified'); + }); + test('attachment & data', () => { + const result = notebookMarkdownRenderer.renderMarkdown({ value: `![altText](attachment:ads.png)`, isTrusted: false }, { cellAttachments: JSON.parse('{"ads.png":{"image/png":"iVBORw0KGgoAAAANSUhEUgAAAggg=="}}') }); + assert.strictEqual(result.innerHTML, `<p><img src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAggg==" alt="altText"></p>`, 'Cell with attachment should have attachment URI not be modified'); + }); + test('vscode-file', () => { + const result: HTMLElement = notebookMarkdownRenderer.renderMarkdown({ value: `![](vscode-file://vscode-app/mypath)` }); + assert.strictEqual(result.innerHTML, `<p><img src="vscode-file://vscode-app/mypath"></p>`, 'vscode-file schema link should not be modified'); + }); + test('file', () => { + const filePath = URI.parse(__filename).fsPath; + const result: HTMLElement = notebookMarkdownRenderer.renderMarkdown({ value: `![](${filePath})` }); + let renderedPath = filePath.replace(/\\/g, '/'); + // Unix filesystems start with / which is deduplicated in the final src URI - so just remove that now if it exists + renderedPath = renderedPath.startsWith('/') ? renderedPath.slice(1) : renderedPath; + assert.strictEqual(result.innerHTML, `<p><img src="vscode-file://vscode-app/${renderedPath}"></p>`, 'file links should have vscode-file schema added'); + }); + test('unknown', () => { + const result: HTMLElement = notebookMarkdownRenderer.renderMarkdown({ value: `![](unknown://some/path)` }); + assert.strictEqual(result.innerHTML, `<p><img src="vscode-file://vscode-app/unknown://some/path"></p>`, 'unknown schema should be treated like a path and have vscode-file schema added'); + }); + }); + test('table followed by blank line with space and then header renders correctly (#16245)', function (): void { let result: HTMLElement = notebookMarkdownRenderer.renderMarkdown({ value: '<table></table>\n \n### Hello', isTrusted: true }); assert.strictEqual(result.innerHTML, '<table></table>\n \n<h3 id="hello">Hello</h3>\n'); diff --git a/src/vs/base/common/network.ts b/src/vs/base/common/network.ts index 2ed496b1c0..a6bc49a6d4 100644 --- a/src/vs/base/common/network.ts +++ b/src/vs/base/common/network.ts @@ -46,6 +46,8 @@ export namespace Schemas { export const data = 'data'; + export const attachment = 'attachment'; // {{SQL CARBON EDIT}} "Scheme" used for Notebook cell attachment data (not really a scheme but formatted like one...) + export const command = 'command'; export const vscodeRemote = 'vscode-remote';