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

Support rendering raw HTML tags that appear in a MarkdownString #40607

Closed
felixfbecker opened this issue Dec 21, 2017 · 22 comments
Closed

Support rendering raw HTML tags that appear in a MarkdownString #40607

felixfbecker opened this issue Dec 21, 2017 · 22 comments
Assignees
Labels
api api-finalization editor-hover Editor mouse hover extensions Issues concerning extensions insiders-released Patch has been released in VS Code Insiders on-testplan
Milestone

Comments

@felixfbecker
Copy link
Contributor

From @borlak on December 21, 2017 0:1

screen shot 2017-12-20 at 4 00 20 pm

In the screenshot, you can see the "p" and "br" tags are not rendered. I don't know if I need another extension to do this properly -- I searched the extensions and didn't see anything that seemed to fit.

I'm on a Mac, using Code version 1.19.1 and the intellisense extension version 2.1.0.

Copied from original issue: felixfbecker/php-language-server#557

@felixfbecker
Copy link
Contributor Author

This is controlled by vscode - Markdown can contain HTML, but vscode chooses to escape all HTML. Imo simple HTML like p/b/ul/li etc should be whitelisted, there are npm modules that do this easily.

@mjbvz
Copy link
Contributor

mjbvz commented Dec 21, 2017

Yes, this is by-design. Our built-in markdown parser for intellisense documentation explicitly disables html tags so that we maintain more control over the presentation. Is html documentation widely used in the php community?

@felixfbecker
Copy link
Contributor Author

I am using the same API stubs for the standard libraries that JetBrains PHPStorm is using, they use HTML everywhere unfortunately:

https://github.com/JetBrains/phpstorm-stubs/blob/43061d78f1980dedab671a30b419074b9e6d79f8/Core/Core.php#L76-L95

It would be really awesome if simple HTML tags could be whitelisted, e.g. with https://www.npmjs.com/package/sanitize-html

@jrieken
Copy link
Member

jrieken commented Jan 3, 2018

Fair, html to markdown to html sounds a little weird. One idea is to have HtmlString similar to MarkdownString

@felixfbecker
Copy link
Contributor Author

felixfbecker commented Jan 3, 2018

Well I don't know generally in advance what content the string is, and since markdown can contain HTML per spec, I don't think HtmlString is needed

@mjbvz
Copy link
Contributor

mjbvz commented Jan 3, 2018

I've had one or two issues about supporting html in jsdocs, but haven't seen that pattern being widespread enough to justify pushing for this. And all the requests were to support html tags like <b> or <img> or <p> which already have markdown equivalents.

My questions and concerns:

  • How many extensions/use-cases would benefit from the change?
  • How painful are current workarounds?
  • Should there be multiple ways to express the same thing? (i.e. *text* and <i>text</i>) Which one would we encourage?
  • This would make the supported html subset part of the VS Code api. Do we want that?
  • Does this open us up too much to API creep, such as: why not whitelist <video> tags or style="color: red;" attributes?

@felixfbecker
Copy link
Contributor Author

felixfbecker commented Jan 3, 2018

How many extensions/use-cases would benefit from the change?

Can't comment on that, but there are thousands of PHP devs who would benefit directly from this.

How painful are current workarounds?

I see only two options:

  • Change the complete PHPStorm stubs repo to pure markdown. Not an option.
  • Use an HTML-to-markdown converter to optimistically convert it. Problem would be first detecting HTML in the first place and then not escaping existing markdown parts. Might actually be impossible. Will have a perf impact.

Should there be multiple ways to express the same thing? (i.e. text and text) Which one would we encourage?
This would make the supported html subset part of the VS Code api. Do we want that?

I don't think vscode needs to be concerned with that. The markdown spec allows HTML to be used, LSP uses markdown, so vscode should support markdown as full as possible.

Does this open us up too much to API creep, such as: why not whitelist

It should be possible to strike a reasonable balance here. sanitize-html has some reasonable defaults that can serve as an inspiration: https://www.npmjs.com/package/sanitize-html#what-are-the-default-options

allowedTags: [ 'h3', 'h4', 'h5', 'h6', 'blockquote', 'p', 'a', 'ul', 'ol',
  'nl', 'li', 'b', 'i', 'strong', 'em', 'strike', 'code', 'hr', 'br', 'div',
  'table', 'thead', 'caption', 'tbody', 'tr', 'th', 'td', 'pre' ],
allowedAttributes: {
  a: [ 'href', 'name', 'target' ],
  // We don't currently allow img itself by default, but this
  // would make sense if we did
  img: [ 'src' ]
},
// Lots of these won't come up by default because we don't allow them
selfClosing: [ 'img', 'br', 'hr', 'area', 'base', 'basefont', 'input', 'link', 'meta' ],
// URL schemes we permit
allowedSchemes: [ 'http', 'https', 'ftp', 'mailto' ],
allowedSchemesByTag: {},
allowProtocolRelative: true

With some slight modifications given this is running in an editor, not a browser (e.g. we clearly don't need target, meta etc)

@jrieken jrieken added the under-discussion Issue is under discussion for relevance, priority, approach label Jan 4, 2018
@felixfbecker felixfbecker changed the title HTML inside intellisense popup not rendering HTML inside hover/signature help markdown not rendering Feb 28, 2018
@felixfbecker felixfbecker changed the title HTML inside hover/signature help markdown not rendering HTML inside hover/signature help markdown is not rendered Feb 28, 2018
@DanTup
Copy link
Contributor

DanTup commented Mar 7, 2018

Hit this in Dart too:

Dart-Code/Dart-Code#638

Though our use is even more weird, because the API docs have this:

/// <p><i class="material-icons md-36">access_alarms</i> &#x2014; material icon named "access alarms".</p>
static const IconData access_alarms = const IconData(0xe191, fontFamily: 'MaterialIcons');

I'm not sure how to handle this. It basically comes with a requirement of some custom CSS for it to render correctly.

@jrieken
Copy link
Member

jrieken commented Mar 8, 2018

requirement of some custom CSS for it to render correctly

It is a lot, lot less likely that we will support custom, free form CSS there

@felixfbecker
Copy link
Contributor Author

I would expect that snippet to render like this:

access_alarms — material icon named "access alarms".

The writers of that documentation seem to have put the "alt text" access_alarms inside <i> there on purpose.

@DanTup
Copy link
Contributor

DanTup commented Mar 8, 2018

It is a lot, lot less likely that we will support custom, free form CSS there

Yeah, I figured that so I'm trying to attack it from the other end too (get them replaced with images like some of the other docs).

I would expect that snippet to render like this:

access_alarms — material icon named "access alarms".

That's how IntelliJ renders it, but in VS Code you just see all of the HTML. If this case is implemented then VS Code will look like IntelliJ (eg. alt text, not icon). I'm trying to get the docs changed, but if that doesn't happen I might just regex it out for my own image or something (they're consistent).

@felixfbecker
Copy link
Contributor Author

Here is a another use case for it: It would be absolutely awesome if in this hover, I could see an inline marble diagram for the RxJS operator:

image

And the <span class="informal"> that is styled for the docs page simply disappears.

@jrieken jrieken added the *out-of-scope Posted issue is not in scope of VS Code label Sep 11, 2018
@vscodebot
Copy link

vscodebot bot commented Sep 11, 2018

This issue is being closed to keep the number of issues in our inbox on a manageable level, we are closing issues that are not going to be addressed in the foreseeable future: We look at the number of votes the issue has received and the number of duplicate issues filed. If you disagree and feel that this issue is crucial: We are happy to listen and to reconsider.

If you wonder what we are up to, please see our roadmap and issue reporting guidelines.

Thanks for your understanding and happy coding!

@vscodebot vscodebot bot closed this as completed Sep 11, 2018
@felixfbecker
Copy link
Contributor Author

Really don't understand why this is out of scope. I've implemented the same thing for our hovers on sourcegraph.com with the solution I proposed here:

https://github.com/sourcegraph/codeintellify/blob/5eef578cfc508dbcc6a97f5ba08675768b0a5187/src/helpers.ts#L90-L102

which works really well:

image

@jens1o
Copy link
Contributor

jens1o commented Sep 11, 2018

@jrieken wut. I cannot understand that.

mjbvz added a commit that referenced this issue Sep 3, 2021
Fixes #40607

This change introduces a new `supportsHtml` property on `MarkdownString` that can be used to enable rendering of a safe subset of tags and attributes inside of markdown strings

For backwards compatability, `supportsHtml` will default to false and must be explicitly enabled by extensions

This PR will need to go in after we adopt dompurify (#131950) which should provide better control over how we actually go about sanitizing rendered html
mjbvz added a commit that referenced this issue Sep 3, 2021
Fixes #40607

This change introduces a new `supportsHtml` property on `MarkdownString` that can be used to enable rendering of a safe subset of tags and attributes inside of markdown strings

For backwards compatibility, `supportsHtml` will default to false and must be explicitly enabled by extensions

This PR will need to go in after we adopt dompurify (#131950) which should provide better control over how we actually go about sanitizing rendered html
@mjbvz mjbvz closed this as completed in 8b7264a Sep 3, 2021
@mjbvz mjbvz added this to the September 2021 milestone Sep 3, 2021
@mjbvz
Copy link
Contributor

mjbvz commented Sep 3, 2021

#132182 proposes adding a supportHtml property onMarkdownString (it is disabled by default). When enabled, we will render a limited subset of html tags and attributes that appear in markdown strings

Please give this a test and let me know how it works

Reopening to track api status

@mjbvz mjbvz reopened this Sep 3, 2021
@DanTup
Copy link
Contributor

DanTup commented Sep 6, 2021

@mjbvz would this also be supported via LSP?

@mjbvz
Copy link
Contributor

mjbvz commented Sep 7, 2021

Please file a separate feature request against the LSP. I imagine the API would look similar but folks on the LSP side would have to confirm this feature makes sense for them

@DanTup
Copy link
Contributor

DanTup commented Sep 8, 2021

I've filed microsoft/language-server-protocol#1344 for LSP.

@mjbvz mjbvz modified the milestones: September 2021, October 2021 Sep 14, 2021
@mjbvz mjbvz changed the title HTML inside hover/signature help markdown is not rendered Support rendering raw HTML tags that appear in a MarkdownString Oct 4, 2021
@mjbvz mjbvz closed this as completed in 03cc7e9 Oct 12, 2021
@mjbvz mjbvz added on-testplan and removed under-discussion Issue is under discussion for relevance, priority, approach labels Oct 12, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Nov 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api api-finalization editor-hover Editor mouse hover extensions Issues concerning extensions insiders-released Patch has been released in VS Code Insiders on-testplan
Projects
None yet
Development

No branches or pull requests

9 participants