preferences: Render markdown descriptions #10431

msujew · 2021-11-18T12:42:49Z

What it does

Renders the markdownDescription property into html instead of just setting the textContent to the markdownDescription value. As there are some special semantics related to the rendered markdown like links that have to be opened in a new window or linked preferences that have to be rendered as links instead of code, I added a new MarkdownRenderer that allows easy post-processing on rendered markdown. Perhaps this is a bit overkill, but I couldn't find any other method to overwrite the code rule for markdown-it.

Also updates some preferences that were using description instead of markdownDescription.

Previously:

Now:

Compared to vscode:

How to test

Open the Preferences widget.
Assert that preferences with a markdownDescription property are rendered correctly to html.
Assert that preferences with links to other preferences are correctly rendered and clicking the links brings the preference into view
Assert that actual http links open a new tab/window when clicking on them.
Assert that no preference exists which should use markdownDescription instead of description. (i.e. no markdown syntax exists in description properties)

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

vince-fugnitto

The changes look good to me overall, believe it fixes #7742 if you'd like to link it 👍

I have a couple of suggestions for improvements, but they can be tackled in separate pull-requests:

ensuring that links (ex: read more) open in an external window and do not replace the app.
ensuring that preferences are anchors, so we can reference them when clicking the following #search.searchOnType#.

msujew · 2021-11-18T13:50:56Z

believe it fixes #7742 if you'd like to link it 👍

Ah, good to know 👍

I have a couple of suggestions for improvements, but they can be tackled in separate pull-requests:

I see, I will address them in this PR 👍

vince-fugnitto

Thank you for taking care of the updates, I can confirm that links open in new windows, and we can reference other preference ids through the markdown as anchors.

There are some instances where preferences are not anchorable:

Is this an issue with localization in general? We might reference invalid preference ids because we want to use strings present in nls.metadata.json for translation. For an end-user it might be confusing when we use a preference description and refer to an invalid id.

packages/core/src/browser/markdown-renderer.ts

packages/preferences/src/browser/preference-tree-model.ts

packages/preferences/src/browser/views/components/preference-node-renderer.ts

msujew · 2021-11-22T12:18:25Z

@vince-fugnitto Thanks for the feedback, I removed the references to non-existing preferences and used custom localization keys 👍

colin-grant-work · 2021-11-22T23:31:15Z

packages/core/src/browser/markdown-renderer.ts

+            for (const callback of calls) {
+                const elements: Element[] = Array.from(div.getElementsByTagName(tag));
+                for (const element of elements) {
+                    const result = callback(element);
+                    if (result) {
+                        div.replaceChild(result, element);


It's still pretty easy to get this to fail. .getElementsByTagName retrieves all descendants including remote descendants, but (in Chrome, at least) .replaceChild only works if the element to be replaced is an immediate child of the element on which .replaceChild is called, so if the original rendering or a previous callback creates a nested structure that includes the tag of interest, the next callback targeting that tag will fail. Since markdown-it already supports plugins, I wouldn't make this hard on yourself trying to invent a new plugin system for it: just apply the modifications you want to apply to the HTML.

I would like to use the plugin system of markdown-it, but it's just not powerful enough. The main issue is that it always renders to text, eliminating every event listener I place on the html elements in the process. The main purpose of the MarkdownRenderer class is to operate directly on the html elements. (I've tried implementing it using purely markdown-it at the start, but couldn't get around this limitation)

Anyway, changed this to only select direct children of the div.

Thanks for adding the tests. I don't see an obvious way to break the current implementation (though a triply nested for-loop is a little scary...) 👍. I'm still not sure whether it's worth adding that machinery (the MarkdownRenderer class) when the only current use case is a fairly simple one (running one callback on each of two different tags). Adding it puts us on the hook for its correctness in the general case when that level of complexity isn't (yet) needed. But it is working, and the tests suggest it should handle the general case, so I'll leave it up to you.

msujew · 2021-11-29T17:05:13Z

@colin-grant-work Updated the markdown rendering to behave better when selecting non-direct descendants. Also added some tests. Like I said here, I couldn't find a way by doing it purely through markdown-it. I guess working on the DOM is always messier than operating on the rendered strings, but it's necessary in this case.

colin-grant-work · 2021-11-30T15:40:17Z

packages/preferences/src/browser/util/preference-types.ts

@@ -64,6 +64,7 @@ export namespace Preference {
    export interface LeafNode extends BaseTreeNode {
        depth: number;
        preference: { data: PreferenceDataProperty };
+        preferenceId: string;


I'm Ok with adding this, but there is also a utility in the Preference.TreeNode namespace (getGroupAndIdFromNodeId) to get the group and id (i.e. preferenceId) from the ID of the node.

colin-grant-work · 2021-11-30T15:57:38Z

packages/core/src/browser/markdown-renderer.ts

+            for (const callback of calls) {
+                const elements: Element[] = Array.from(div.getElementsByTagName(tag));
+                for (const element of elements) {
+                    const result = callback(element);
+                    if (result) {
+                        div.replaceChild(result, element);


Thanks for adding the tests. I don't see an obvious way to break the current implementation (though a triply nested for-loop is a little scary...) 👍. I'm still not sure whether it's worth adding that machinery (the MarkdownRenderer class) when the only current use case is a fairly simple one (running one callback on each of two different tags). Adding it puts us on the hook for its correctness in the general case when that level of complexity isn't (yet) needed. But it is working, and the tests suggest it should handle the general case, so I'll leave it up to you.

colin-grant-work

This is behaving correctly and the implementation looks sound. The one outstanding issue that I'd like to discuss - it may or may not need to be addressed - is that in Electron, clicking an external link opens a new Electron window rather than external browser window as VSCode does. That can make Electron behave oddly, because events from that window are treated the same as events from 'real' Theia windows, which can be undesirable (I think it would mess with the code to close windows in #10379, for example, since the new window wouldn't have the code to confirm exit). FWIW, I think it would be better for it to open an external window, but the fact that it's so easy to open a non-IDE Electron window probably means that #10379 needs to be more careful about which windows it tries to ask for close confirmation.

Update

I tested this with #10379 and it didn't cause any trouble (to my relief...), but I still think it would be better to open in an external browser if possible. I notice that the links on the Getting Started page already do this, so they should be able to point the way.

msujew · 2021-12-05T13:30:11Z

@colin-grant-work Thanks, I only tested this in the browser environment. I wasn't aware of the whole WindowService.openNewWindow stuff. It should work as expected in electron now :)

vince-fugnitto

The changes work well for me 👍
I confirmed that:

markdown descriptions of preferences are correctly styled
clicking the anchor links of preferences correctly navigates to the corresponding preference
opening external links works correctly for the browser and electron
no visible regressions
- searching works
- scrolling works
- layout restoration works

colin-grant-work

Looking good and working well for me. 👍

msujew added preferences issues related to preferences ui/ux issues related to user interface / user experience labels Nov 18, 2021

vince-fugnitto reviewed Nov 18, 2021

View reviewed changes

msujew force-pushed the msujew/render-markdown-settings branch 2 times, most recently from ff923f3 to 8eea0a0 Compare November 19, 2021 02:25

msujew requested a review from vince-fugnitto November 19, 2021 02:25

vince-fugnitto requested a review from colin-grant-work November 19, 2021 13:11

vince-fugnitto reviewed Nov 19, 2021

View reviewed changes

colin-grant-work reviewed Nov 19, 2021

View reviewed changes

msujew force-pushed the msujew/render-markdown-settings branch from 8eea0a0 to d8169bb Compare November 22, 2021 12:16

msujew requested a review from colin-grant-work November 22, 2021 12:24

colin-grant-work reviewed Nov 22, 2021

View reviewed changes

msujew force-pushed the msujew/render-markdown-settings branch 2 times, most recently from 97f0414 to 1bdafc4 Compare November 29, 2021 17:01

colin-grant-work self-requested a review November 29, 2021 17:55

colin-grant-work reviewed Nov 30, 2021

View reviewed changes

msujew force-pushed the msujew/render-markdown-settings branch from 1bdafc4 to dac076f Compare December 5, 2021 12:57

Render markdown for preferences

0f042f3

msujew force-pushed the msujew/render-markdown-settings branch from dac076f to 0f042f3 Compare December 5, 2021 13:28

vince-fugnitto approved these changes Dec 6, 2021

View reviewed changes

colin-grant-work approved these changes Dec 9, 2021

View reviewed changes

msujew merged commit 82feef3 into master Dec 14, 2021

msujew deleted the msujew/render-markdown-settings branch December 14, 2021 13:07

github-actions bot added this to the 1.21.0 milestone Dec 14, 2021

msujew mentioned this pull request Dec 23, 2021

Redhat Java Plugin > 0.78.0 has some issues in Theia #10579

Closed

msujew mentioned this pull request Jan 3, 2022

Simplify markdown rendering for preferences #10589

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

preferences: Render markdown descriptions #10431

preferences: Render markdown descriptions #10431

msujew commented Nov 18, 2021 •

edited

Loading

vince-fugnitto left a comment

msujew commented Nov 18, 2021

vince-fugnitto left a comment

msujew commented Nov 22, 2021

colin-grant-work Nov 22, 2021 •

edited

Loading

msujew Nov 23, 2021 •

edited

Loading

colin-grant-work Nov 30, 2021 •

edited

Loading

msujew commented Nov 29, 2021

colin-grant-work Nov 30, 2021

colin-grant-work Nov 30, 2021 •

edited

Loading

colin-grant-work left a comment •

edited

Loading

msujew commented Dec 5, 2021

vince-fugnitto left a comment

colin-grant-work left a comment

preferences: Render markdown descriptions #10431

preferences: Render markdown descriptions #10431

Conversation

msujew commented Nov 18, 2021 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

vince-fugnitto left a comment

Choose a reason for hiding this comment

msujew commented Nov 18, 2021

vince-fugnitto left a comment

Choose a reason for hiding this comment

msujew commented Nov 22, 2021

colin-grant-work Nov 22, 2021 • edited Loading

Choose a reason for hiding this comment

msujew Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

colin-grant-work Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

msujew commented Nov 29, 2021

colin-grant-work Nov 30, 2021

Choose a reason for hiding this comment

colin-grant-work Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

colin-grant-work left a comment • edited Loading

Choose a reason for hiding this comment

Update

msujew commented Dec 5, 2021

vince-fugnitto left a comment

Choose a reason for hiding this comment

colin-grant-work left a comment

Choose a reason for hiding this comment

msujew commented Nov 18, 2021 •

edited

Loading

colin-grant-work Nov 22, 2021 •

edited

Loading

msujew Nov 23, 2021 •

edited

Loading

colin-grant-work Nov 30, 2021 •

edited

Loading

colin-grant-work Nov 30, 2021 •

edited

Loading

colin-grant-work left a comment •

edited

Loading