Support rendering raw HTML tags that appear in a MarkdownString

[felixfbecker]

From @borlak on December 21, 2017 0:1

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]

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]

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]

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]

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

[felixfbecker]

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]

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]

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)

[DanTup]

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]

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]

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]

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]

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:

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

[vscodebot]

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!

[felixfbecker]

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:

[jens1o]

@jrieken wut. I cannot understand that.

[mjbvz]

#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

[DanTup]

@mjbvz would this also be supported via LSP?

[mjbvz]

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]

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