Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: fix ansi escaping for diffs #557

Merged
merged 1 commit into from
Nov 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
161 changes: 161 additions & 0 deletions src/ansi2html.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
/*
Copyright (c) Microsoft Corporation.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

export function ansi2html(text: string, defaultColors?: { bg: string, fg: string }): string {
const regex = /(\x1b\[(\d+(;\d+)*)m)|([^\x1b]+)/g;
const tokens: string[] = [];
let match;
let style: any = {};

let reverse = false;
let fg: string | undefined = defaultColors?.fg;
let bg: string | undefined = defaultColors?.bg;

while ((match = regex.exec(text)) !== null) {
const [, , codeStr, , text] = match;
if (codeStr) {
const code = +codeStr;
switch (code) {
case 0: style = {}; break;
case 1: style['font-weight'] = 'bold'; break;
case 2: style['opacity'] = '0.8'; break;
case 3: style['font-style'] = 'italic'; break;
case 4: style['text-decoration'] = 'underline'; break;
case 7:
reverse = true;
break;
case 8: style.display = 'none'; break;
case 9: style['text-decoration'] = 'line-through'; break;
case 22:
delete style['font-weight'];
delete style['font-style'];
delete style['opacity'];
delete style['text-decoration'];
break;
case 23:
delete style['font-weight'];
delete style['font-style'];
delete style['opacity'];
break;
case 24:
delete style['text-decoration'];
break;
case 27:
reverse = false;
break;
case 30:
case 31:
case 32:
case 33:
case 34:
case 35:
case 36:
case 37:
fg = ansiColors[code - 30];
break;
case 39:
fg = defaultColors?.fg;
break;
case 40:
case 41:
case 42:
case 43:
case 44:
case 45:
case 46:
case 47:
bg = ansiColors[code - 40];
break;
case 49:
bg = defaultColors?.bg;
break;
case 53: style['text-decoration'] = 'overline'; break;
case 90:
case 91:
case 92:
case 93:
case 94:
case 95:
case 96:
case 97:
fg = brightAnsiColors[code - 90];
break;
case 100:
case 101:
case 102:
case 103:
case 104:
case 105:
case 106:
case 107:
bg = brightAnsiColors[code - 100];
break;
}
} else if (text) {
let token = escapeHTML(text);
const isBold = style['font-weight'] === 'bold';
if (isBold)
token = `<b>${token}</b>`;
const isItalic = style['font-style'] === 'italic';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's have a type for style and also just check if style.italic is true?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would fork the code from the upstream a lot.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part is already different

if (isItalic)
token = `<i>${token}</i>`;
const hasOpacity = style['opacity'] === '0.8';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is so special about 0.8?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is assigned in the line 34, the idea is that we use upstream code for style definition, but then we sanitize it for vscode to accept it.

if (hasOpacity)
token = `<span style='color:#666;'>${token}</span>`;
const color = reverse ? (bg || '#000') : fg;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe default background will depend on dark/light mode.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I checked it in both modes and it is ok because we only do reverse on mid-light background.

if (color)
token = `<span style='color:${color};'>${token}</span>`;
const backgroundColor = reverse ? fg : bg;
if (backgroundColor)
token = `<span style='background-color:${backgroundColor};'>${token}</span>`;
tokens.push(token);
}
}
return tokens.join('');
}

const ansiColors: Record<number, string> = {
0: '#000',
1: '#f14c4c',
2: '#73c991',
3: '#ffcc66',
4: '#44a8f2',
5: '#b084eb',
6: '#afdab6',
7: '#fff',
};

const brightAnsiColors: Record<number, string> = {
0: '#808080',
1: '#f14c4c',
2: '#73c991',
3: '#ffcc66',
4: '#44a8f2',
5: '#b084eb',
6: '#afdab6',
7: '#fff',
};

function escapeHTML(text: string): string {
return text.replace(/[&"<> \n]/g, c => ({
' ': '&nbsp;',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this won't match other whitespaces, such as \t. Also, do we want to map all whitespaces to non-breaking?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has not changed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, this is what I see upstream:

function escapeHTML(text: string): string {
  return text.replace(/[&"<>]/g, c => ({ '&': '&amp;', '"': '&quot;', '<': '&lt;', '>': '&gt;' }[c]!));
}

'\n': '\n<br>\n',
'&': '&amp;',
'"': '&quot;',
'<': '&lt;',
'>': '&gt;'
}[c]!));
}
5 changes: 3 additions & 2 deletions src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,12 @@ import { SettingsModel } from './settingsModel';
import { SettingsView } from './settingsView';
import { TestModel, TestModelCollection } from './testModel';
import { TestTree } from './testTree';
import { NodeJSNotFoundError, ansiToHtml, getPlaywrightInfo, stripAnsi, stripBabelFrame } from './utils';
import { NodeJSNotFoundError, getPlaywrightInfo, stripAnsi, stripBabelFrame } from './utils';
import * as vscodeTypes from './vscodeTypes';
import { WorkspaceChange, WorkspaceObserver } from './workspaceObserver';
import { registerTerminalLinkProvider } from './terminalLinkProvider';
import { RunHooks, TestConfig } from './playwrightTestTypes';
import { ansi2html } from './ansi2html';

const stackUtils = new StackUtils({
cwd: '/ensure_absolute_paths'
Expand Down Expand Up @@ -712,7 +713,7 @@ export class Extension implements RunHooks {
const markdownString = new this._vscode.MarkdownString();
markdownString.isTrusted = true;
markdownString.supportHtml = true;
markdownString.appendMarkdown(ansiToHtml(this._linkifyStack(text)));
markdownString.appendMarkdown(ansi2html(this._linkifyStack(text)));
return new this._vscode.TestMessage(markdownString);
}

Expand Down
57 changes: 0 additions & 57 deletions src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,63 +40,6 @@ export function stripBabelFrame(text: string) {
return result.join('\n').trim();
}

export function ansiToHtml(text: string): string {
let isOpen = false;
let hasTags = false;
const tokens: string[] = [];
for (let i = 0; i < text.length; ++i) {
const c = text.charAt(i);
if (c === '\u001b') {
hasTags = true;
const end = text.indexOf('m', i + 1);
const code = text.substring(i + 1, end);
if (!code.match(/\[\d+/))
continue;
if (isOpen) {
tokens.push('</span>');
isOpen = false;
}
switch (code) {
case '[2': {
tokens.push(`<span style='color:#666;'>`);
isOpen = true;
break;
}
case '[22': break;
case '[31': {
tokens.push(`<span style='color:#f14c4c;'>`);
isOpen = true;
break;
}
case '[32': {
tokens.push(`<span style='color:#73c991;'>`);
isOpen = true;
break;
}
case '[39': break;
}
i = end;
} else {
if (c === '\n') {
// Don't close to work around html parsing bug.
tokens.push('\n<br>\n');
} else if (c === ' ') {
tokens.push('&nbsp;');
} else {
tokens.push(escapeHTML(c));
}
}
}
// Work around html parsing bugs.
if (hasTags)
tokens.push('\n</span></br>');
return tokens.join('');
}

function escapeHTML(text: string): string {
return text.replace(/[&"<>]/g, c => ({ '&': '&amp;', '"': '&quot;', '<': '<b>&lt;</b>', '>': '<b>&gt;</b>' }[c]!));
}

export async function spawnAsync(executable: string, args: string[], cwd?: string, settingsEnv?: NodeJS.ProcessEnv): Promise<string> {
const childProcess = spawn(executable, args, {
stdio: 'pipe',
Expand Down
5 changes: 1 addition & 4 deletions tests/run-tests.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,6 @@ test('should show error message', async ({ activate }) => {
Received: <span style='color:#f14c4c;'>1</span>
<br>
at tests/test.spec.ts:4:19
</span></br>
`);
});

Expand All @@ -286,7 +285,7 @@ test('should escape error log', async ({ activate }) => {
const testRun = await testController.run(testItems);

expect(testRun.renderLog({ messages: true })).toContain(
`<b>&lt;</b>div class=&quot;foo bar baz&quot;<b>&gt;</b><b>&lt;</b>/div<b>&gt;`);
`&lt;div class=&quot;foo bar baz&quot;&gt;&lt;/div&gt;`);
});

test('should show soft error messages', async ({ activate }) => {
Expand Down Expand Up @@ -321,7 +320,6 @@ test('should show soft error messages', async ({ activate }) => {
Received: <span style='color:#f14c4c;'>1</span>
<br>
at tests/test.spec.ts:4:24
</span></br>
test.spec.ts:[4:23 - 4:23]
Error: <span style='color:#666;'>expect(</span><span style='color:#f14c4c;'>received</span><span style='color:#666;'>).</span>toBe<span style='color:#666;'>(</span><span style='color:#73c991;'>expected</span><span style='color:#666;'>) // Object.is equality</span>
<br>
Expand All @@ -332,7 +330,6 @@ test('should show soft error messages', async ({ activate }) => {
Received: <span style='color:#f14c4c;'>2</span>
<br>
at tests/test.spec.ts:5:24
</span></br>
`);
});

Expand Down