ProseMirror / prosemirror

The ProseMirror WYSIWYM editor
http://prosemirror.net/
MIT License
7.66k stars 336 forks source link

Regression - codeblock newlines collapse during joinBackward or joinForward #1460

Closed pantryfight closed 5 months ago

pantryfight commented 5 months ago

We (Atlassian) have had some customer reports of code blocks collapsing into one line they are merged with the previous node by pressing backspace (or delete key from previous node). I've found that this behaviour was introduced in this PR: https://github.com/ProseMirror/prosemirror-transform/commit/8a50033d741f41fa5513d360439197bfd040b92d

I'm unsure if it's intentional since, the PR specifically mentions setBlockType and not joinForward/joinBackward however I can see in the code for joinBackward/joinForward that it also invokes tr.clearIncompatible() which is going to result in the same newline behaviour as setBlockType.

If this is the intentional behaviour, I'm wondering if you could please provide any advice on how best to either restore the legacy behaviour or ideally (since newline characters in a HTML document are problematic) replace them with hardBreak nodes. I considered overriding this in our own backspace/delete keymap handlers, however I can see there's a lot of logic in the default handlers for joinBackward/joinForward which as far as I can tell I would need to duplicate in order to prevent the newline removal from happening.

Note: Ideally setBlockType would also be able to use the legacy newline behaviour (or replace with hardBreaks) since we are also using it to convert codeBlocks back to text blocks in some circumstances - although that behaviour might be a little easier to work around than joinBackward/Forward.

Thanks so much for your time and any light you may be able to shed on this!

Steps to reproduce:

  1. Go to prosemirror.net and in the sample editor, hit enter after "Try it out by typing in here or see more examples."
  2. Select Type -> Code
  3. Enter multiple lines of text eg:
    line 1
    line 2
    line 3
  4. Move Cursor to start of code block. (before line 1)
  5. Press Backspace

Actual result: Newlines are replaced with spaces. The final sentence of the example text becomes:

Try it out by typing in here or see more examples.line 1 line 2 line 3

Expected result: Newlines are retained:

Try it out by typing in here or see more examples.line 1 line 2 line 3

Platform: MacOS Chrome 124.0.6367.92

marijnh commented 5 months ago

This is indeed an intentional. However, on closer look, the logic should maybe use the whitespace, rather than code property on the node spec.

Could you say a bit more about how you are treating newlines in regular paragraphs? Are you rendering them as line breaks? Do you allow people to edit them? Because editing would already clear them, no? Even before the patch you linked. Or are you already setting whitespace: "pre" on your textblock nodes?

pantryfight commented 5 months ago

Thanks for the reply. I can see that we have whiteSpace: 'pre-wrap' styling applied for all rendered content, which appears to have been applied to match ProseMirror's own styling. However, our Paragraph NodeSpec does not explicitly define a whitespace property which I guess would mean it defaults to normal.

I agree that we shouldn't be relying on whitespace newline characters in the rendered HTML for normal spec nodes - it seems like this has been overlooked in our editor for a long time, however it would be fantastic if there were a way we could control how this is handled in some way. While semantically correct I can see how it could be unexpected to users if they have a particularly large code block and it collapses to one line.

Ideally I'd like to convert the \n to hard_break/<br /> instead of spaces during the joinBackward/joinForward merge. It doesn't seem as if this is possible with the current API unless we duplicate PM's own backspace/delete behaviours though. Am I wrong there?

marijnh commented 5 months ago

Indeed, it's not currently possible to do that.

One solution might be to add a schema-wide option that marks a node to be used as line-break equivalent when normalizing node content. When converting from code-style nodes to regular texblocks, newlines would be replaced with this node. When going the other way, this node would be replaced by a newline. Being opt-in via an option, it would not impact existing setups. Does that sound like it would work for you?

pantryfight commented 5 months ago

Thanks for that, I'd discussed this proposed solution with my team and just wanted to confirm we are understanding correctly. Are you proposing that you could add a new optional configuration option to Prosemirror's schema settings that lets us determine which node to use for newline normalizing? So we could for example set our existing hardbreak node to be used and not have to add any additional nodes to the schema?

If I'm understanding it correctly then that does sound pretty useful.

marijnh commented 5 months ago

Yes, that is how it works. Please see attached patches. To use the feature, add a linebreakEquivalent: true property to your hard break node's node spec.

gpolzinatlassian commented 5 months ago

That's fantastic. Thank you