search

ProseMirror / prosemirror-markdown

ProseMirror Markdown integration
https://prosemirror.net
MIT License
345 stars 81 forks source link

Backslash escapes are duplicated during serialization in some cases #3

Closed marijnh closed 2 years ago

marijnh commented 7 years ago

Via https://github.com/ProseMirror/prosemirror/issues/562:

It sounds like a feature, but it's a bug :1st_place_medal:

Repro

http://prosemirror.net/demo/markdown.html

Type this into the Markdown editor 

`~`

Switch between WYSIWYG and Markdown mode

Result

image

torifat commented 7 years ago

It's because of this -> https://github.com/ProseMirror/prosemirror-markdown/blob/7b3a63d054f0fd3bf7f3a4a54dd446dac2afbf47/src/to_markdown.js#L228

There's no isCode in code mark.

marijnh commented 7 years ago

The information of whether to escape inside a given mark should probably be put in the parser spec, rather than the schema, anyway (since it's Markdown-specific).

mhuebert commented 7 years ago

Just to see if the rest of the implementation is working, I've tried adding an isCode attribute (true) on the code mark, and it doesn't appear to resolve the issue. But I might be missing something w.r.t. how it is supposed to work. (update: had not modified the MarkType effectively)

Gozala commented 6 years ago

The information of whether to escape inside a given mark should probably be put in the parser spec, rather than the schema, anyway (since it's Markdown-specific).

@marijnh I would like to fix this, but I'm not sure I understand your suggestion here. It does not seems to be markdown parser issue but rather of markdown serializer which does escape text with in the code blocks. I also think issue is here:

https://github.com/ProseMirror/prosemirror-markdown/blob/39224ecc4a043cf84834b7ec292d87e08031f6fb/src/to_markdown.js#L240

As far as I can tell there is no way to make isCode true there.

Gozala commented 6 years ago

Here is the solution I would like to propose:

  1. Extend MarkSpec interface to add code?: boolean filed similar to one in NodeSpec
  2. Add isCode boolean field to both MarkType and NodeType that would be set assigned as this.isCode = spec.code === true in the constructor.
  3. Add code:true to the code mark in the schema

Alternatively I could just add this.isCode = name == "code" in MarkType constructor which would be similar to how isText is set on NodeType, although I would prefer more general approach described above since I'm using some custom markdown extensions that this would not cover.

Yet another and least favorite option would be to substitute following line: https://github.com/ProseMirror/prosemirror-markdown/blob/39224ecc4a043cf84834b7ec292d87e08031f6fb/src/to_markdown.js#L240

With

 let code = marks.length && marks[marks.length - 1].type.spec.isCode && marks[marks.length - 1]

That would require no changes to prosemirror-model but seems dirty since, MarkSpec interface defines isCode field.

marijnh commented 6 years ago

It does not seems to be markdown parser issue but rather of markdown serializer

Yeah, sorry, I think I meant serializer when I wrote that.

Extend MarkSpec interface to add code?: boolean filed similar to one in NodeSpec

No, as I mentioned, I'd prefer for this to stay in the serializer, since it's Makdown-specific and doesn't need to concern the schema.

susnux commented 2 years ago

It seems this was fixed in version 1.2.0, with https://github.com/ProseMirror/prosemirror-markdown/commit/eff71629d88fb93007aa33f32de4b8d043b19b7d and can be closed (?)