Add markdown to notebook entries #7084

scottbell · 2023-09-25T14:52:56Z

Closes #6060

Describe your changes:

Add the Marked library, and consolidate the URL whitelisting/anchor tag generation.

All Submissions:

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?
Is this change backwards compatible? For example, developers won't need to change how they are calling the API or how they've extended core plugins such as Tables or Plots.

Author Checklist

Changes address original issue?
Tests included and/or updated with changes?
Command line build passes?
Has this been smoke tested?
Testing instructions included in associated issue OR is this a dependency/testcase change?

Reviewer Checklist

Changes appear to address issue?
Reviewer has tested changes by following the provided instructions?
Changes appear not to be breaking changes?
Appropriate automated tests included?
Code style and in-line documentation are appropriate?
Has associated issue been labelled unverified? (only applicable if this PR closes the issue)
Has associated issue been labelled bug? (only applicable if this PR is for a bug fix)

deploysentinel · 2023-09-25T14:57:30Z

Current Playwright Test Results Summary

✅ 14 Passing - ⚠️ 1 Flaky

Run may still be in progress, this comment will be updated as current testing workflow or job completes...

(Last updated on 10/02/2023 10:27:46pm UTC)

Run Details

Running Workflow e2e-couchdb on Github Actions

Commit: 9ed1ce9

Started: 10/02/2023 10:22:00pm UTC

⚠️ Flakes

📄 functional/plugins/displayLayout/displayLayout.e2e.spec.js • 1 Flake

Test Case Results

Test Case	Last 7 days Failures	Last 7 days Flakes
Display Layout When multiple plots are contained in a layout, we only ask for annotations once @couchdb Retry 1 • Initial Attempt	2.50% (1) 1 / 40 run failed over last 7 days	20% (8) 8 / 40 runs flaked over last 7 days

View Detailed Build Results

Current Playwright Test Results Summary

✅ 141 Passing - ⚠️ 1 Flaky

Run may still be in progress, this comment will be updated as current testing workflow or job completes...

(Last updated on 10/02/2023 10:27:46pm UTC)

Run Details

Running Job e2e-stable on CircleCI

Commit: 9ed1ce9

Started: 10/02/2023 10:21:12pm UTC

⚠️ Flakes

📄 functional/plugins/notebook/notebookSnapshots.e2e.spec.js • 1 Flake

Test Case Results

Test Case	Last 7 days Failures	Last 7 days Flakes
Snapshot image tests Can drop an image onto a notebook and create a new entry Retry 1 • Initial Attempt	15.73% (14) 14 / 89 runs failed over last 7 days	61.80% (55) 55 / 89 runs flaked over last 7 days

View Detailed Build Results

codecov · 2023-09-25T14:58:06Z

Codecov Report

Merging #7084 (c51aaef) into master (2243381) will decrease coverage by 13.68%.
The diff coverage is n/a.

❗ Current head c51aaef differs from pull request most recent head 9ed1ce9. Consider uploading reports for the commit 9ed1ce9 to get more accurate results

@@             Coverage Diff             @@
##           master    #7084       +/-   ##
===========================================
- Coverage   55.60%   41.92%   -13.68%     
===========================================
  Files         650      412      -238     
  Lines       26063    12792    -13271     
  Branches     2547        0     -2547     
===========================================
- Hits        14492     5363     -9129     
+ Misses      10871     7429     -3442     
+ Partials      700        0      -700

Flag	Coverage Δ
e2e-full	`41.92% <ø> (-0.01%)`	⬇️
e2e-stable	`?`
unit	`?`

see 519 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2243381...9ed1ce9. Read the comment docs.

scottbell · 2023-09-25T15:11:36Z

@jvigliotta Here's what I've got so far.

Screen.Recording.2023-09-25.at.4.53.44.PM.mov

There are few issues with it:

CSS wise, it looks like we're not enabling certain things (e.g., , and lists)
It looks like we're stripping out a lot of tags we'd like in markdown. I'll add them to the sanitize list.

…atted-entries-v2

scottbell · 2023-09-26T09:14:14Z

At this point, I'll probably need some CSS help from @rukmini-bose or @charlesh88. We're rendering this sample input:

sample.md

which renders the HTML:

<div id="entry-37de3c3f-72e8-4603-a3d2-4bb69f0c1c73" class="c-ne__text c-ne__input" aria-label="Notebook Entry Input" tabindex="-1" contenteditable="true"><h1>h1 Heading</h1>
<h2>h2 Heading</h2>
<h3>h3 Heading</h3>
<h4>h4 Heading</h4>
<h5>h5 Heading</h5>
<h6>h6 Heading</h6>
<h2>Emphasis</h2>
<p><strong>This is bold text</strong></p>
<p><strong>This is bold text</strong></p>
<p><em>This is italic text</em></p>
<p><em>This is italic text</em></p>
<p><del>Strikethrough</del></p>
<h2>Blockquotes</h2>
<blockquote>
<p>Blockquote </p>
<blockquote>
<p>nested quote</p>
<blockquote>
<p>very nested</p>
</blockquote>
</blockquote>
</blockquote>
<h2>Lists</h2>
<p>Unordered</p>
<ul>
<li>apple</li>
<li>orange</li>
</ul>
<p>Ordered</p>
<ol>
<li>First item</li>
<li>Second item</li>
</ol>
<h2>Code Blocks</h2>
<p>Block code "fences"</p>
<pre><code>Sample text here...
</code></pre>
<h2>Tables</h2>
<table>
<thead>
<tr>
<th>Option</th>
<th>Description</th>
<th>Notes</th>
</tr>
</thead>
<tbody><tr>
<td>data</td>
<td>path to data files to supply the data that will be passed into templates.</td>
<td>first item</td>
</tr>
<tr>
<td>engine</td>
<td>engine to be used for processing templates. Handlebars is the default.</td>
<td>second item</td>
</tr>
<tr>
<td>ext</td>
<td>extension to be used for dest files.</td>
<td>third item</td>
</tr>
</tbody></table>
<h2>Links</h2>
<p>These need to be whitelisted:</p>
<p>link text</p>
<p>link with title</p>
</div>

This mostly works:

with the exception of:

lists - both unordered and ordered
italicized text
block quotes and code blocks might need a rectangle to distinguish?

jvigliotta · 2023-09-26T16:17:53Z

@jvigliotta Here's what I've got so far.

Screen.Recording.2023-09-25.at.4.53.44.PM.mov
There are few issues with it:

CSS wise, it looks like we're not enabling certain things (e.g., , and lists)

It looks like we're stripping out a lot of tags we'd like in markdown. I'll add them to the sanitize list.

This looks awesome Scott!

- CSS resets and styling for markdown-related HTML markup in Notebook entries. - Better styling and cursor affordances for Notebook entry selection and editing interaction flow.

charlesh88 · 2023-09-27T00:31:12Z

Have pushed up CSS changes that fix most of the problems you were seeing. An important note: we were using white-space: pre-wrap which preserves line breaks without the need for markup. But that CSS rule was totally whacking margins between  and other block tags, so I had to remove it. We should now rely on Marked's  tags and other block tags, and need to enable Marked's "breaks" option to true so single line returns get transformed into   tags.

I may still do some sanding and shimming tomorrow, minor stuff like table widths and text contrast.

Also ran into a couple issues and a question, in order of priority:

Single line breaks need to work for regular and blockquoted text as noted above.
Nested unordered (bullet) list items need to work. While "- List item" gets converted into a <li>, the renderer doesn't convert " - Nested list item" at all. Marked seems to support this out of the box (it works on their demo site), so not sure what's going on.
Leaving a space at the end of a list item before a double-return encodes a non-breaking space in a paragraph container between the two. The only way to fix it is to delete the trailing space at the end of the list item, which is really unintuitive. This sucks, can we do anything about this?
Dragging an image from a random webpage didn't embed for me, not sure if that's supposed to work. Dragging from the desktop worked great though. Very nice!

Exported JSON of my test Notebook used for screenshots below

Marked up entry test, editing

Marked up entry test, result

Item 3, editing

Item 3, result

scottbell · 2023-09-27T07:19:53Z

@charlesh88

Single line breaks need to work for regular and blockquoted text as noted above.

I think I've got that figured per your suggestion by adding breaks: true to the options. Here is the resultant HTML now:

<p>Here's a line with a double-return before it.&nbsp;<br>Here's a line with a single return before it. It doesn't add a <code>&lt;br&gt;</code> before it, but should.</p>

scottbell · 2023-09-27T07:22:45Z

@charlesh88

Dragging an image from a random webpage didn't embed for me, not sure if that's supposed to work. Dragging from the desktop worked great though. Very nice!

Dragging from a webpage will only work if CORS is enabled for the website for the server hosting Open MCT. You should have at least gotten a nice notification saying "Failed to fetch image" and a more specific error in the console.

scottbell · 2023-09-27T07:26:52Z

@charlesh88

Nested unordered (bullet) list items need to work. While "- List item" gets converted into a
, the renderer doesn't convert " - Nested list item" at all. Marked seems to support this out of the box (it works on their demo site), so not sure what's going on.

I think this got fixed by the breaks: true. Here's the rendering with that enabled:

scottbell · 2023-09-27T08:01:18Z

@charlesh88

Leaving a space at the end of a list item before a double-return encodes a non-breaking space in a paragraph container between the two. The only way to fix it is to delete the trailing space at the end of the list item, which is really unintuitive. This sucks, can we do anything about this?

This is a tough one to fix as Marked has already tokenized the extra space as separate from the list item, so we'd have to remove it either before being fed to Marked, or after. We could do this by either using regular expressions on the input text or resultant HTML, or parsing the output HTML into DOMs, manipulating it there, and then serializing the HTML back out again. Would it be OK to file this as a follow on issue?

scottbell · 2023-09-27T08:02:59Z

@charlesh88

Leaving a space at the end of a list item before a double-return encodes a non-breaking space in a paragraph container between the two. The only way to fix it is to delete the trailing space at the end of the list item, which is really unintuitive. This sucks, can we do anything about this?

This is a tough one to fix as Marked has already tokenized the extra space as separate from the list item, so we'd have to remove it either before being fed to Marked, or after. We could do this by either using regular expressions on the input text or resultant HTML, or parsing the output HTML into DOMs, manipulating it there, and then serializing the HTML back out again. Would it be OK to file this as a follow on issue?

I say that and the Marked demo isn't doing this. Let me dig into this further.

scottbell · 2023-09-27T11:11:04Z

@charlesh88 and thank you very much for the CSS assistance - it look way better!

- Tab

charlesh88 · 2023-09-27T16:28:35Z

breaks: true was the fix for single returns, looks great. But: the nested list item isn't fixed yet. See below.
Ok if we can't figure out the trailing space thing, but the biggest issue there is user frustration: you go to edit the entry to fix the extra space, delete what looks like 2 returns, save - and the extra space is back again.
Did get a "Failed to fetch image" message on browser window drag. That's a limited use case anyway, dragging from the desktop is probably much more valuable. All good.

Nested List item problem

In short: nested list items need to be encoded as <ul>'s in the <li> above them. The Marked demo does this, but we're currently treating nested list items as a string. See examples below.

Here's what we'd expect and want, from the Marked demo:

Note that the nested list items are encoded as <ul>'s in the <li> above them.

Here's what we're getting in a Notebook entry for the same structure:

Note that the nested list items are NOT encoded as <ul>'s, but are just treated as lines.

…/openmct into 6060-notebooks-formatted-entries-v2

scottbell · 2023-09-28T12:18:08Z

@charlesh88 For the trailing space and nested lists, I think they're both caused by having div with out a whitespace attribute on the CSS, thus when the content is loaded for editing, all the spaces are collapsed into a single space and the line breaks are tossed.

Setting white-space: pre-wrap; in the CSS I think is rendering (closer) to the HTML we want:

&__input {
    // Appended to __text element when Notebook is not in readOnly mode
    @include inlineInput;
    padding-left: $p;
    padding-right: $p;
    overflow: unset;
    margin-bottom: $interiorMargin;
    text-overflow: unset;
    white-space: pre-wrap;
  }

Here's the resultant trailing space HTML:

<ul>
  <li>Leaving a space at the end of a list item before a double-return encodes a non-breaking space in a paragraph container. The only way to fix it is to delete the trailing space at the end of list item, which is really unintuitive. This sucks, can we do anything about this?
  </li>
</ul>
<p>See?</p>

and the nested lists:

<ul>
    <li>List item<ul>
            <li>This nested list item isn't recognized but should be.</li>
        </ul>
    </li>
    <li>Another list item
        <ol>
            <li>Numbered item under an unordered list item&nbsp;isn't recognized but should be.</li>
            <li>Numbered item under an unordered list item&nbsp;isn't recognized but should be.</li>
        </ol>
    </li>
</ul>

Here are screenshots of the rendering, which still have a lot of space that we can probably compact:

Another alternative we could pursue is swapping out the div with a textarea, like Github and the Marked demo use, as it preserves whitespace out of the box.

- Conversion of contenteditable-div to textarea started. - Stubbed in textarea with styles.

charlesh88 · 2023-09-28T19:09:46Z

Just pushed up changes that stub in a textarea element and add some CSS classes for it. Note that I've moved classes in the markup to apply c-ne__text to the div and c-ne__input to the textarea. Aria labels also updated accordingly.

akhenry · 2023-10-02T20:13:10Z

Before this is merged, I want to make sure we are not allowing any unsanitized HTML or JavaScript from the user?

akhenry

See comments about sanitization

scottbell · 2023-10-02T20:29:52Z

See comments about sanitization

@akhenry We're sanitizing output from the Markdown here. We're doing the same thing before we render for annotation search results.

scottbell · 2023-10-02T20:58:45Z

See comments about sanitization

@akhenry We're sanitizing output from the Markdown here. We're doing the same thing before we render for annotation search results.

@akhenry Added a further input sanitization, to prevent it from being saved in the first place.

Screen.Recording.2023-10-02.at.10.56.42.PM.mov

akhenry · 2023-10-02T21:11:07Z

@scottbell Beautiful, thank you!

ozyx

Really awesome work! I tested all the usual Markdown syntax, including hyperlinks (which still respected the URL sanitization properly!) Looks solid.

ozyx · 2023-10-02T21:35:54Z

e2e/tests/functional/plugins/notebook/notebook.e2e.spec.js

@@ -482,4 +482,42 @@ test.describe('Notebook entry tests', () => {
    expect.soft(await sanitizedLink.count()).toBe(1);
    expect(await unsanitizedLink.count()).toBe(0);
  });
+  test('Can add markdown to a notebook entry', async ({ page }) => {


This doesn't need to block this PR, but we should remember to add a visual test for this.

I'll put in a ticket for that.

ozyx · 2023-10-02T22:10:14Z

src/ui/layout/search/AnnotationSearchResult.vue

@@ -119,6 +105,9 @@ export default {
      return this.result.fullTagModels[0].foregroundColor;
    }
  },
+  beforeMount() {


not sure what difference it makes, but this could be in created() as well

charlesh88 · 2023-10-03T00:01:48Z

@scottbell Fantastic work, was truly a pleasure working on this with you. Just ran the merged code locally and it's working great!

scottbell added 2 commits September 25, 2023 15:29

try marked out

6b36534

fix url validation

dbade4c

scottbell linked an issue Sep 25, 2023 that may be closed by this pull request

[Notebooks] Support Markdown in Notebook Entries #6060

Closed

scottbell added 3 commits September 26, 2023 09:49

Merge remote-tracking branch 'origin/master' into 6060-notebooks-form…

d2060a9

…atted-entries-v2

now rendering blockquotes properly

6e2c1ed

add abbrv, link titles, and strikethrough

0f29c52

fix tests and lint

3802a80

Closes #6060

1116915

- CSS resets and styling for markdown-related HTML markup in Notebook entries. - Better styling and cursor affordances for Notebook entry selection and editing interaction flow.

Merge branch 'master' into 6060-notebooks-formatted-entries-v2

476dcdf

add line breaks option

811b1eb

Closes #6060

90bea00

- Tab

charlesh88 and others added 2 commits September 27, 2023 09:30

Merge branch '6060-notebooks-formatted-entries-v2' of github.com:nasa…

f3d79cb

…/openmct into 6060-notebooks-formatted-entries-v2

Merge branch 'master' into 6060-notebooks-formatted-entries-v2

654e6ce

Closes #6060

a069efb

- Conversion of contenteditable-div to textarea started. - Stubbed in textarea with styles.

have it markdown with a textarea and adjust size automatically

f596cab

scottbell requested review from jvigliotta, ozyx and charlesh88 October 2, 2023 18:30

ozyx added this to the Target:3.1.0 milestone Oct 2, 2023

akhenry self-requested a review October 2, 2023 20:13

akhenry requested changes Oct 2, 2023

View reviewed changes

add another sanitization step

de287c3

scottbell added the pr:e2e:couchdb npm run test:e2e:couchdb label Oct 2, 2023

scottbell requested a review from akhenry October 2, 2023 21:06

github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label Oct 2, 2023

scottbell removed request for charlesh88 and ozyx October 2, 2023 21:22

akhenry removed their request for review October 2, 2023 21:31

akhenry approved these changes Oct 2, 2023

View reviewed changes

ozyx approved these changes Oct 2, 2023

View reviewed changes

Merge branch 'master' into 6060-notebooks-formatted-entries-v2

9ed1ce9

ozyx added the pr:e2e:couchdb npm run test:e2e:couchdb label Oct 2, 2023

ozyx enabled auto-merge (squash) October 2, 2023 22:19

github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label Oct 2, 2023

ozyx merged commit 6947c91 into master Oct 2, 2023

ozyx deleted the 6060-notebooks-formatted-entries-v2 branch October 2, 2023 22:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add markdown to notebook entries #7084

Add markdown to notebook entries #7084

scottbell commented Sep 25, 2023 •

edited

Loading

deploysentinel bot commented Sep 25, 2023 •

edited

Loading

Test Case Results

Test Case Results

codecov bot commented Sep 25, 2023 •

edited

Loading

scottbell commented Sep 25, 2023

scottbell commented Sep 26, 2023 •

edited

Loading

jvigliotta commented Sep 26, 2023

charlesh88 commented Sep 27, 2023 •

edited

Loading

scottbell commented Sep 27, 2023 •

edited

Loading

scottbell commented Sep 27, 2023 •

edited

Loading

scottbell commented Sep 27, 2023

scottbell commented Sep 27, 2023

scottbell commented Sep 27, 2023

scottbell commented Sep 27, 2023

charlesh88 commented Sep 27, 2023 •

edited

Loading

scottbell commented Sep 28, 2023 •

edited

Loading

charlesh88 commented Sep 28, 2023

akhenry commented Oct 2, 2023

akhenry left a comment

scottbell commented Oct 2, 2023

scottbell commented Oct 2, 2023

akhenry commented Oct 2, 2023

ozyx left a comment

ozyx Oct 2, 2023

scottbell Oct 3, 2023

scottbell Oct 3, 2023

ozyx Oct 2, 2023

charlesh88 commented Oct 3, 2023

Add markdown to notebook entries #7084

Add markdown to notebook entries #7084

Conversation

scottbell commented Sep 25, 2023 • edited Loading

Describe your changes:

All Submissions:

Author Checklist

Reviewer Checklist

deploysentinel bot commented Sep 25, 2023 • edited Loading

Current Playwright Test Results Summary

⚠️ Flakes

Test Case Results

Current Playwright Test Results Summary

⚠️ Flakes

Test Case Results

codecov bot commented Sep 25, 2023 • edited Loading

Codecov Report

scottbell commented Sep 25, 2023

scottbell commented Sep 26, 2023 • edited Loading

jvigliotta commented Sep 26, 2023

charlesh88 commented Sep 27, 2023 • edited Loading

scottbell commented Sep 27, 2023 • edited Loading

scottbell commented Sep 27, 2023 • edited Loading

scottbell commented Sep 27, 2023

scottbell commented Sep 27, 2023

scottbell commented Sep 27, 2023

scottbell commented Sep 27, 2023

charlesh88 commented Sep 27, 2023 • edited Loading

Nested List item problem

scottbell commented Sep 28, 2023 • edited Loading

charlesh88 commented Sep 28, 2023

akhenry commented Oct 2, 2023

akhenry left a comment

Choose a reason for hiding this comment

scottbell commented Oct 2, 2023

scottbell commented Oct 2, 2023

akhenry commented Oct 2, 2023

ozyx left a comment

Choose a reason for hiding this comment

ozyx Oct 2, 2023

Choose a reason for hiding this comment

scottbell Oct 3, 2023

Choose a reason for hiding this comment

scottbell Oct 3, 2023

Choose a reason for hiding this comment

ozyx Oct 2, 2023

Choose a reason for hiding this comment

charlesh88 commented Oct 3, 2023

scottbell commented Sep 25, 2023 •

edited

Loading

deploysentinel bot commented Sep 25, 2023 •

edited

Loading

codecov bot commented Sep 25, 2023 •

edited

Loading

scottbell commented Sep 26, 2023 •

edited

Loading

charlesh88 commented Sep 27, 2023 •

edited

Loading

scottbell commented Sep 27, 2023 •

edited

Loading

scottbell commented Sep 27, 2023 •

edited

Loading

charlesh88 commented Sep 27, 2023 •

edited

Loading

scottbell commented Sep 28, 2023 •

edited

Loading