Fix file:// link hover tooltip showing raw URL-encoded URI#299659
Fix file:// link hover tooltip showing raw URL-encoded URI#299659
Conversation
…renderer Co-authored-by: mjbvz <12821956+mjbvz@users.noreply.github.com>
| }); | ||
|
|
||
| test('Should not override explicit title for file:// links', () => { | ||
| const md = new MarkdownString(`[log](file:///home/user/project/lib.d.ts "Go to definition")`, {}); |
There was a problem hiding this comment.
@copilot these test fail on windows. Try using Uri.file(...).toString() to generate the uri instead of hardcoding it to unix style paths
There was a problem hiding this comment.
Fixed in 88a4ff6 — the tests now use URI.file(...) to generate the file URI and fileUri.fsPath for the expected value, making them platform-independent.
Co-authored-by: mjbvz <12821956+mjbvz@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Improves hover-tooltip readability for file:// links rendered from markdown (notably inside hover widgets) by showing a decoded OS path instead of the raw URL-encoded URI.
Changes:
- Added a helper to derive a human-readable title for
file://link hrefs by usingURI.parse(href).fsPath(+ fragment). - Updated the default markdown link renderer to use that computed title when the markdown link has no explicit title.
- Added unit tests to validate the new title behavior, fragment preservation, and that explicit titles are not overridden.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/vs/base/browser/markdownRenderer.ts | Computes a friendlier title attribute for file:// links so tooltips show decoded filesystem paths. |
| src/vs/base/test/browser/markdownRenderer.test.ts | Adds coverage for the updated file:// link-title behavior, including fragment handling and explicit title precedence. |
| test('Should use decoded file path as title for file:// links', () => { | ||
| const fileUri = URI.file('/home/user/project/lib.d.ts'); | ||
| const md = new MarkdownString(`[log](${fileUri.toString()})`, {}); | ||
|
|
||
| const result = store.add(renderMarkdown(md)).element; | ||
| const anchor = result.querySelector('a')!; | ||
| assert.ok(anchor); | ||
| assert.strictEqual(anchor.title, fileUri.fsPath); | ||
| }); |
There was a problem hiding this comment.
The new tests assert that file:// link titles use fsPath, but they don't cover the specific regression described in the PR (URL-encoded segments like %3A in Windows file URIs). Consider adding a case that feeds an explicitly URL-encoded file:///c%3A/... (or another %xx-encoded path segment such as %20) and asserts that the resulting anchor.title is decoded, so the behavior is exercised independent of the current OS and matches the reported bug.
| // For file:// URIs without an explicit title, show the decoded OS path instead of | ||
| // the raw URL-encoded URI (e.g. display "C:\Users\..." instead of "file:///c%3A/Users/...") | ||
| if (!title && href.startsWith(`${Schemas.file}:`)) { | ||
| title = getLinkTitle(href); |
There was a problem hiding this comment.
For file:// links, this sets title from URI.parse(href).fsPath, but it only escapes double quotes. Previously the implicit title fell back to href, which is fully HTML-encoded (including &, <, >, and '). Using a different escaping strategy for the file-path title can lead to the tooltip text being HTML-entity-decoded/altered for paths containing sequences like </&, and is inconsistent with how href is encoded. Consider encoding the computed title with the same attribute-encoding logic used for href (or using htmlAttributeEncodeValue from vs/base/common/strings) so the title is safe and preserves the exact text.
| title = getLinkTitle(href); | |
| title = getLinkTitle(href); | |
| // HTML Encode title derived from file path to match href encoding and avoid HTML entity decoding | |
| title = title.replace(/&/g, '&') | |
| .replace(/</g, '<') | |
| .replace(/>/g, '>') | |
| .replace(/"/g, '"') | |
| .replace(/'/g, '''); |
Hovering over a
file://link inside a hover widget (e.g., theloglink in TypeScript'sconsole.infohover) displayed the raw URL-encoded URI as the tooltip —file:///c%3A/Users/matb/.../lib.dom.d.ts#42864,5-42864,8— instead of a readable path.Changes
src/vs/base/browser/markdownRenderer.ts— AddedgetLinkTitle()helper that extractsURI.parse(href).fsPath(+ fragment) forfile://links. Thelinkrenderer indefaultMarkedRenderersnow calls this when no explicit markdown title is present, so thetitleattribute on rendered<a>elements shows a decoded OS path (e.g.,C:\Users\matb\...\lib.dom.d.ts#42864,5-42864,8) instead of the raw URI.src/vs/base/test/browser/markdownRenderer.test.ts— Added tests covering: decoded path as title, fragment preservation, and explicit title not overridden. Tests useURI.file(...).toString()to generate file URIs andURI.fsPathfor expected values, ensuring correctness on both Windows and Unix.💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.