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

Remove trailing newline from code blocks #3321

Merged
merged 2 commits into from
Nov 2, 2022

Conversation

mejo-
Copy link
Member

@mejo- mejo- commented Oct 27, 2022

Markdown-it serialized code blocks to <pre><code>...\n</pre></code>, which leads to trailing newlines in the code blocks as rendered by Tiptap.

Let the Tiptap CodeBlock node remove the trailing newline in parseHTML in order to avoid this.

There's another issue with trailing newlines that got added to code blocks on purpose being removed by prosemirror-markdown, but that's not affected by this change.

Fixes: #2344

Signed-off-by: Jonas jonas@freesources.org

  • Resolves: #
  • Target version: master

Summary

@mejo- mejo- added bug Something isn't working 3. to review labels Oct 27, 2022
@mejo- mejo- force-pushed the fix/markdownit_fence_trailing_newline branch from 1a449e7 to 74ec9e9 Compare October 27, 2022 17:13
@mejo- mejo- requested a review from susnux October 27, 2022 17:21
@mejo- mejo- force-pushed the fix/markdownit_fence_trailing_newline branch 2 times, most recently from f29c0ca to ced0ada Compare October 27, 2022 17:28
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks for the fix 🥳

@susnux
Copy link
Contributor

susnux commented Oct 27, 2022

This breaks the common mark tests as

```
code
```

is expected to render to

<pre><code>code
</code></pre>

But generally the last \n should not affect the markdown serialization as prosemirror-markdown checks if the last character of the content of the code block is a newline and only inserts one if it is not a new line.
So for the example above no new line would be inserted, but for <pre><code>code</code></pre> a newline would be inserted.


I noticed that sometimes a new line is added visually (but not in the markdown). I do not know why, but prosemirror adds sometimes a <br> tag with class ProseMirror-trailingBreak.
Origins here:
https://github.com/ProseMirror/prosemirror-view/blob/b8c83593a3bb81a8547874fe7a9142504b049dce/src/viewdesc.ts#L1286

@mejo-
Copy link
Member Author

mejo- commented Oct 28, 2022

This breaks the common mark tests

You're right. I broke commonmark on purpose in this case because the linebreak before closing HTML tags seems to cause ProseMirror to add the HTML break element. But I forgot to disable/edit the respective commonmark tests.

With my changes, the newline doesn't get added anymore.

Another solution would be to find out why ProseMirror adds the break element, but to be honest for me it was an acceptable tradeoff to break commonmark because I don't see a reason to have a linebreak before the closing HTML tags, but maybe I'm missing something?

Would you prefer another solution @susnux?

@susnux
Copy link
Contributor

susnux commented Oct 28, 2022

But it does not always add the newline, and the newline is only added visually, so are you sure it does not appear anymore?
Because currently the added break is only added randomly, at least I could not figure out any pattern so it could even be something else somewhere deeper.

@mejo-
Copy link
Member Author

mejo- commented Oct 31, 2022

So after some further debugging, my understanding is that createDocument() from Tiptap gets <pre><code>something\n</code></pre> for a code block (as serialized by markdown-it). The \n gets transformed into a <br> tag by prosemirror-view and I'd argue that's even expected behaviour.

I'm not sure yet why the break tag is only added sometimes by prosemirror-view, but at least this PR reproducibly fixes the issue for me.

Whenever a newline is added to the end of a code block, this is reproducible for the same document. Closing the document, resetting the Text session and opening the document again reliably adds the new line again to the same code blocks where it's been added before. And everytime, applying the PR in between removes the newline.

@susnux

@mejo- mejo- force-pushed the fix/markdownit_fence_trailing_newline branch from ced0ada to 06a08a5 Compare October 31, 2022 12:28
@mejo- mejo- force-pushed the fix/markdownit_fence_trailing_newline branch from 06a08a5 to 8fd57c3 Compare November 2, 2022 12:28
@mejo-
Copy link
Member Author

mejo- commented Nov 2, 2022

After digging into this again with @juliushaertl I went with another approach.

So first, it's reproducible that the newline from markdown-it results in the rendered extra newline in Tiptap. Everytime you close the editor, reset the session and open the editor again, the trailing newline is back.

So that means that we can assume the HTML as given from markdown-it always contains one trailing newline too much.

That's why I went with removing one trailing newline from the code block content in parseHTML() of CodeBlock node now.

@mejo- mejo- requested review from susnux and juliusknorr November 2, 2022 12:31
@mejo- mejo- force-pushed the fix/markdownit_fence_trailing_newline branch from 8fd57c3 to be3a2e2 Compare November 2, 2022 12:32
Markdown-it serialized code blocks to `<pre><code>...\n</pre></code>`,
which leads to trailing newlines in the code blocks as rendered by
 Tiptap.

Let the Tiptap CodeBlock node remove the trailing newline in parseHTML
in order to avoid this.

There's another issue with trailing newlines that got added to code
blocks on purpose being removed by prosemirror-markdown, but that's not
affected by this change.

Fixes: #2344

Signed-off-by: Jonas <jonas@freesources.org>
src/nodes/CodeBlock.js Outdated Show resolved Hide resolved
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small debugging leftover, otherwise 👍 as discussed :)

@mejo- mejo- force-pushed the fix/markdownit_fence_trailing_newline branch from be3a2e2 to e78f78f Compare November 2, 2022 12:49
@mejo-
Copy link
Member Author

mejo- commented Nov 2, 2022

/compile

Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@mejo- mejo- changed the title Remove trailing newline from fenced code blocks Remove trailing newline from code blocks Nov 2, 2022
@mejo- mejo- merged commit 5b086d5 into master Nov 2, 2022
@delete-merged-branch delete-merged-branch bot deleted the fix/markdownit_fence_trailing_newline branch November 2, 2022 13:25
@max-nextcloud
Copy link
Collaborator

/backport e78f78f to stable25

@backportbot-nextcloud
Copy link

The backport to stable25 failed. Please do this backport manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

empty lines added to the end of code <> blocks on exit
5 participants