Enabling Markdown for GristDocTour #1653

ogui11aume · 2025-06-03T11:15:34Z

Context

So far Grist only permits raw text to appear as a one-liner in modal during a document tour : not a very nice user experience for somebody to get a first impression of your super Grist document. 🙁
With Markdown, the document tours are much better:

rich text with new lines and line spacing
multiple clickable links (multiple) directly within the text, not handled as a separate data
images

Proposed solution

Technically :
Just parsing the Body cell from GristDocTour table with already available marked library, sanitize and following the renderer approach to better select which aspect of markdown to render.

Documentation : https://github.com/ogui11aume/hackdays2025/blob/ergonogrist/submissions/ergonogrist/assets/markdown-for-gristdoctour/deliverable1.md

Related issues

Didn't find any issue ( and I didn't open one although I made the change at the request of other users).

Has this been tested?

👍 yes, I added tests to the test suite
💭 no, because this PR is a draft and still needs work
🙅 no, because this is not relevant here
🙋 no, because I need help I created a sample Document to rapidly test the behaviour here https://github.com/ogui11aume/hackdays2025/blob/ergonogrist/submissions/ergonogrist/assets/markdown-for-gristdoctour/Markdown-for-GristDocTour.grist

Screenshots / Screencasts

https://github.com/ogui11aume/hackdays2025/blob/ergonogrist/submissions/ergonogrist/assets/markdown-for-gristdoctour/deliverable1.md

fflorent

Sounds good to me. Thank you very much!

I just wondered why the iframe were allowed in sanitizeTutorialHTML. Sounds like it is required and used in the official tutorial to embed a Youtube video.

We will ask for an official review from Grist Labs.

georgegevoian · 2025-06-20T15:31:23Z

app/client/ui/DocTour.ts

 import sortBy = require('lodash/sortBy');
+import {marked} from "marked";
+import {renderer} from 'app/client/ui/DocTutorialRenderer';
+import {sanitizeTutorialHTML} from "./sanitizeHTML";


Suggested change

import {sanitizeTutorialHTML} from "./sanitizeHTML";

import {sanitizeHTML} from "app/client/ui/sanitizeHTML";

You can use this variant instead, unless you need to tweak the default sanitization behavior, in which case you can just add a new variant like sanitizeTourHTML.

If no tweaking is necessary, you can just use the markdown helper from app/client/lib/markdown.ts.

Minor lint: absolute module path is preferred over relative.

Thanks for the feedback, I'm going the new variant way with 'sanitizeTourHTML' (with absolute module path) to keep things clear and have the ability to embed videos as @fflorent noted earlier.

georgegevoian · 2025-06-20T15:32:29Z

app/client/ui/DocTour.ts

 import {dom} from 'grainjs';
 import sortBy = require('lodash/sortBy');
+import {marked} from "marked";
+import {renderer} from 'app/client/ui/DocTutorialRenderer';


Suggested change

import {renderer} from 'app/client/ui/DocTutorialRenderer';

import {renderer} from 'app/client/ui/DocTourRenderer';

DocTourRenderer isn't being imported here.

georgegevoian · 2025-06-20T15:52:21Z

app/client/ui/DocTour.ts

+
+    const bodyHtmlContent = sanitizeTutorialHTML(marked.parse(getValue("Body"), {
+      async: false, renderer
+    }));
+    const element = document.createElement('div');
+    element.innerHTML = bodyHtmlContent;
+
+
+    let body: HTMLElement = element;
+
+


I think you can replace the dom('p', body), below to something like dom('p', markdown(body)), or dom('p', el => el.innerHTML = bodyHtmlContent), depending on whether you're using the markdown helper or your own renderer. This way, you only need at most bodyHtmlContent and none of the other changes.

Thank you @georgegevoian for the feedback.
I'm going to look into incorporating these changes and adding tests to check for MarkDown content rendering.

georgegevoian · 2025-06-20T16:07:35Z

Thanks for the PR @ogui11aume.

For tests, we'll work on moving some tour-related browser tests to this repo by next week. That way, you can test your changes against them and add a test. A test that asserts a tour containing Markdown text added the expected HTML to the DOM should be enough, but you're welcome to add more.

georgegevoian · 2025-06-25T15:13:25Z

Tests are now in grist-core: 2997ca8.

github-actions · 2025-07-11T14:16:47Z

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

hexaltation · 2025-07-16T14:39:28Z

@ogui11aume
It seems that you've broken the following test :

1) DocTour
       should create correct popups from a GristDocTour table:

You're modifications do not produce the same html has before, can you fix it ?

An can you add the mail you're signing your commits with in your email account and verify it? you will need It to sign the new CLA.

app/client/ui/DocTour.ts

berhalak · 2025-07-25T13:40:35Z

app/client/ui/DocTourRenderer.ts

+
+export const renderer = new marked.Renderer();
+
+renderer.link = ({href, text}) =>


I think that the default marked.Renderer allows arbitrary html, which potentially allows some phishing attacks. For example with <div style="position: fixed; color: red; inset: 0">Boo</div> you can cover whole Grist layout and provide your own.

What do you think about reusing the renderer from app/client/ui/MarkdownCellRenderer.ts? It clears out any html content.

I used ALLOWED_TAGS with marked.Renderer in app/client/ui/sanitizeHTML.ts to limit abuse and only allow those HTML elements that have been tested (+ table).

Agreed, this does not yet avoid the problem you described ( covering div ). To avoid the problem in your example, I would also need to limit the usage of style at least in div, should I go that far ?

The problem with app/client/ui/MarkdownCellRenderer.ts is that, according to comment, it will not permit images or other HTML elements. So I didn't go that way.

grist-core/app/client/ui/MarkdownCellRenderer.ts

Line 59 in 129a053

* This does not support HTML or images in the markdown value, and renders those as text.

:

This does not support HTML or images in the markdown value, and renders those as text.

@ogui11aume, what if we disallowed HTML for the time being?

renderer.html = ({raw}) => escape(raw);

Would there be an immediate need for tours to support arbitrary HTML?

Having a robust and shared sanitization approach across different UI components like tutorials, tours, etc. seems desirable, but could perhaps be reconciled later. (Tutorials in particular should probably also disallow HTML outside of the specific iframe that's permitted, but that's unrelated to your proposed changes.)

ogui11aume · 2025-07-25T14:21:07Z

I have read the CLA Document and I hereby sign the CLA

fflorent · 2025-07-30T14:34:26Z

@ogui11aume Do you need our review again?

ogui11aume · 2025-08-01T10:20:40Z

@ogui11aume Do you need our review again?
Yes please, I need a new review.

For my commits I have :

Added the mail I'm signing my commits with in Github account and verified it.
Signed my commits
Taken into account your advice regarding some code refactoring (thanks @georgegevoian, I've only just seen @berhalak sanitizeHTMLIntoDOM suggestion from last week )
Fixed existing nbrowser tests without altering content in existing lines in doctour.grist (thanks @hexaltation )
Added new lines in doctour.grist to test rendering Markdown formatting syntax, headings, image, link, quote, lists (ordered and unordered), code blocks.

Most other Markdown extended syntax are not currently being rendered so I didn't create tests for them. Let me know if you thinki other tests would be useful.

ogui11aume · 2025-08-01T14:11:55Z

Hi, please help ...
I'm not sure why this nbrowser test won't pass : https://github.com/gristlabs/grist-core/actions/runs/16676842047/job/47205430863?pr=1653#step:23:850

test/nbrowser/DocTour.ts

Discussion regarding the changes gristlabs#1653 (comment)

Refactor using DOM_FRAGMENT like in sanitizeHTMLIntoDOM gristlabs#1653 (comment) and specify allowed HTML tags to limit abuse according to gristlabs#1653 (comment)

fflorent · 2025-08-07T13:18:25Z

@ogui11aume The LeftPanel test is unstable, not related to your work (though I can't make it fail locally).
I take a look a the tests for the Grist Tour.

fflorent

@ogui11aume I don't seem to be able to make the test file fail locally (I ran the DocTour tests 20 times with no failure).

Do you reproduce the failure on your side?

ogui11aume · 2025-08-08T08:27:21Z

Hi @fflorent

@ogui11aume I don't seem to be able to make the test file fail locally (I ran the DocTour tests 20 times with no failure).

With my latest commit, all tests pass.

If you refer to my previous call for help then @georgegevoian suggested that the test was the wrong way around and I provided confirmation with the actual intended purpose of the test. I then fixed the problem 😄 .

Do you reproduce the failure on your side?

What that the test failure you were trying to reproduce ?

fflorent · 2025-08-08T08:44:00Z

@ogui11aume I was trying to reproduce this failure your mentioned earlier:
https://github.com/gristlabs/grist-core/actions/runs/16676842047/job/47205430863?pr=1653#step:23:850

As Github collapses the resolved comments, I miss that point (I should be more vigilant next time).

Anyway, glad you fixed the issue :)

georgegevoian · 2025-08-12T02:13:38Z

app/client/ui/DocTour.ts

 import {dom} from 'grainjs';
 import sortBy = require('lodash/sortBy');
+import {marked} from "marked";
+import {renderer} from 'app/client/ui/DocTutorialRenderer';


DocTourRenderer isn't being imported here.

georgegevoian · 2025-08-12T02:15:12Z

app/client/ui/DocTour.ts

+    if (!title && !(bodyValue.trim()) ) {
+      return null;
+    }
+    //const bodyHtmlContent


Suggested change

//const bodyHtmlContent

georgegevoian · 2025-08-12T02:30:09Z

app/client/ui/DocTourRenderer.ts

+
+export const renderer = new marked.Renderer();
+
+renderer.link = ({href, text}) =>


@ogui11aume, what if we disallowed HTML for the time being?

renderer.html = ({raw}) => escape(raw);

Would there be an immediate need for tours to support arbitrary HTML?

Having a robust and shared sanitization approach across different UI components like tutorials, tours, etc. seems desirable, but could perhaps be reconciled later. (Tutorials in particular should probably also disallow HTML outside of the specific iframe that's permitted, but that's unrelated to your proposed changes.)

dsagal · 2025-10-02T22:09:18Z

What do you think of the suggestion in #1864 to respect the setting of the Body field in the GristDocTour table, and only rendering markdown when that field is set to markdown? The main reason I am thinking of it is to avoid accidentally affecting existing doc-tours.

If we do that, it would also make sense to me to reuse the same markdown renderer + sanitizing as used for cells. You would then be able to expect the doc tour to show content in the same way you see it in the cells of the GristDocTour table. What do you think?

Discussion regarding the changes gristlabs#1653 (comment)

Refactor using DOM_FRAGMENT like in sanitizeHTMLIntoDOM gristlabs#1653 (comment) and specify allowed HTML tags to limit abuse according to gristlabs#1653 (comment)

ogui11aume · 2025-10-10T13:54:37Z

Hi @georgegevoian
Thanks for the review, I've fixed and I'm about to push :

✅ importing the dedicated DocTourRenderer .
✅ removing dead code //const bodyHtmlContent
✅ disabling "arbitrary HTML" within the Markdown in the dedicated DocTourRender settings

Just found how it was implemented in the Cell Renderer and yes, this is quite an important one.
Here how it goes :

If I leave the "arbirary HTML" rendering, then with a Markdown that includes HTML tags I get this rendering :

Now, if I use renderer.html = ({raw}) => escape(raw); I get a more controlled output with Markdown being rendered but not the HTML within :

- Prevent raw HTML from being rendered within DocTour markdown content - Improves security and consistency of displayed documentation - Updated related tests accordingly

ogui11aume · 2025-10-10T15:10:51Z

Hi @dsagal

What do you think of the suggestion in #1864 to respect the setting of the Body field in the GristDocTour table, and only rendering markdown when that field is set to markdown? The main reason I am thinking of it is to avoid accidentally affecting existing doc-tours.

My understanding of this suggestion is that we would render Markdown ONLY IF for the Body column of Type Text also indicates Cell Format as Markdown ? Below is a screenshot of the setting.

I may be able to add this condition, but I tried so far to keep this feature simple with limited refactoring (the way the body content is handled has already many conditions).
👉 The benefit I see in doing this is that it would handle all previous DocTour just as before (one liner text), and only start behaving rendering Mardown if the column Cell Format is switched to Markdown.
👉 But then, this optional behaviour would have to be properly documented somewhere ?

If we do that, it would also make sense to me to reuse the same markdown renderer + sanitizing as used for cells. You would then be able to expect the doc tour to show content in the same way you see it in the cells of the GristDocTour table. What do you think?

I agree. As this refactoring would need to be done also to TutorialTour at least, maybe this could be handled in a different issue where we could discuss and approach this refactoring in a larger scope ?

I've tested swapping DocTourRenderer for MarkdownCellRenderer and the rendering work, only it does not render images :

//import {renderer} from 'app/client/ui/DocTourRenderer';
import {renderer} from 'app/client/ui/MarkdownCellRenderer';

Difference in tests outputs (image in not rendered) :

-    "body": "<span><p>![Grist text as image alt](https://www.getgrist.com/wp-content/uploads/2023/03/Grist-Logo.png)</p>\n</span>"
      +    "body": "<span><p><img alt=\"Grist text as image alt\" src=\"https://www.getgrist.com/wp-content/uploads/2023/03/Grist-Logo.png\"></p>\n</span>"

Should we open another Refactoring TutorialTour and DocTour using MarkdownCellRenderer issue and this go ahead with this PR using DocTourRenderer ?

fflorent approved these changes Jun 5, 2025

View reviewed changes

fflorent requested a review from paulfitz June 5, 2025 09:53

fflorent added this to French administration Board Jun 5, 2025

fflorent moved this to Needs feedback in French administration Board Jun 5, 2025

georgegevoian requested review from georgegevoian and removed request for paulfitz June 18, 2025 14:29

georgegevoian reviewed Jun 20, 2025

View reviewed changes

berhalak self-requested a review June 25, 2025 15:48

hexaltation moved this from Needs feedback to In Progress in French administration Board Jul 9, 2025

ogui11aume force-pushed the markdown-for-gristdoctour branch from 391b30b to 5c4f6a7 Compare July 11, 2025 14:16

ogui11aume force-pushed the markdown-for-gristdoctour branch from 5c4f6a7 to a3d0ab6 Compare July 25, 2025 12:42

berhalak requested changes Jul 25, 2025

View reviewed changes

ogui11aume force-pushed the markdown-for-gristdoctour branch from a3d0ab6 to c8fb883 Compare July 25, 2025 14:14

github-actions bot added a commit that referenced this pull request Jul 25, 2025

@ogui11aume has signed the CLA in #1653

cdb1b76

ogui11aume force-pushed the markdown-for-gristdoctour branch from 1b18447 to 4c3ddcd Compare August 1, 2025 09:52

ogui11aume force-pushed the markdown-for-gristdoctour branch from 4c3ddcd to 26d9687 Compare August 1, 2025 13:52

georgegevoian reviewed Aug 3, 2025

View reviewed changes

test/nbrowser/DocTour.ts Outdated Show resolved Hide resolved

ogui11aume added a commit to ogui11aume/grist-core that referenced this pull request Aug 6, 2025

fix(test): Change the parent html element and change assert to true

de030e1

Discussion regarding the changes gristlabs#1653 (comment)

ogui11aume requested a review from berhalak August 6, 2025 13:38

fflorent moved this from In Progress to Needs Internal Feedback in French administration Board Aug 6, 2025

fflorent reviewed Aug 7, 2025

View reviewed changes

georgegevoian reviewed Aug 12, 2025

View reviewed changes

hexaltation moved this from Needs Internal Feedback to In Progress in French administration Board Aug 13, 2025

fflorent mentioned this pull request Oct 2, 2025

Support markdown in DocTour #1864

Closed

ogui11aume added 4 commits October 10, 2025 14:48

Enabling Markdown for GristDocTour

91c961e

Tests for Markdown in DocTour

8726c35

fix(test): Change the parent html element and change assert to true

03cbfb5

Discussion regarding the changes gristlabs#1653 (comment)

Refactor DOM_FRAGMENT and ALLOWED_TAGS

978e3d7

Refactor using DOM_FRAGMENT like in sanitizeHTMLIntoDOM gristlabs#1653 (comment) and specify allowed HTML tags to limit abuse according to gristlabs#1653 (comment)

ogui11aume force-pushed the markdown-for-gristdoctour branch from 7223e3c to 978e3d7 Compare October 10, 2025 12:56

feat(doc-tour): disable HTML rendering in DocTour Markdown

19c9509

- Prevent raw HTML from being rendered within DocTour markdown content - Improves security and consistency of displayed documentation - Updated related tests accordingly

	import {sanitizeTutorialHTML} from "./sanitizeHTML";
	import {sanitizeHTML} from "app/client/ui/sanitizeHTML";

	import {renderer} from 'app/client/ui/DocTutorialRenderer';
	import {renderer} from 'app/client/ui/DocTourRenderer';


		export const renderer = new marked.Renderer();

		renderer.link = ({href, text}) =>

Uh oh!

Enabling Markdown for GristDocTour #1653

Are you sure you want to change the base?

Enabling Markdown for GristDocTour #1653

Uh oh!

Conversation

ogui11aume commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Proposed solution

Related issues

Has this been tested?

Screenshots / Screencasts

Uh oh!

fflorent left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

georgegevoian Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

georgegevoian commented Jun 20, 2025

Uh oh!

georgegevoian commented Jun 25, 2025

Uh oh!

github-actions bot commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hexaltation commented Jul 16, 2025

Uh oh!

Uh oh!

berhalak Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogui11aume commented Jul 25, 2025

Uh oh!

fflorent commented Jul 30, 2025

Uh oh!

ogui11aume commented Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ogui11aume commented Aug 1, 2025

Uh oh!

Uh oh!

fflorent commented Aug 7, 2025

Uh oh!

fflorent left a comment

Choose a reason for hiding this comment

Uh oh!

ogui11aume commented Aug 8, 2025

Uh oh!

fflorent commented Aug 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsagal commented Oct 2, 2025

Uh oh!

ogui11aume commented Oct 10, 2025

Uh oh!

ogui11aume commented Oct 10, 2025

Uh oh!

ogui11aume commented Jun 3, 2025 •

edited

Loading

georgegevoian Jun 20, 2025 •

edited

Loading

github-actions bot commented Jul 11, 2025 •

edited

Loading

berhalak Jul 25, 2025 •

edited

Loading

ogui11aume commented Aug 1, 2025 •

edited

Loading