Serialization to markdown fails when two marks start or end in the same position

[katemihalikova]

Steps to reproduce

Bold text:

Link/code added to one of the words:



The markdown output contains mixed marks:

Version

Detected in v0.3.0 and v0.4.0, using Windows Chrome 48. Most recent commit is the v0.4.0 now.

[ericandrewlewis]

Thanks for the bug report @katemihalikova! I was able to reproduce this as well.

We'll need to look into how the document is being transformed to Markdown, and whether it's a bug in PM or something to do with markdown-it, our Markdown utility library.

[marijnh]

It's almost certainly our serializer — markdown-it has been very solid so far. If someone knows of a Markdown-generation library (the escaping is non-trivial), I'd gladly switch to that. If it doesn't exist, we'll just have to continue shaping up ours.

[asteadman]

I've observed this behavior but was unable to pin down the source until now. Looking at the code for renderInline(parent) in to_markdown.js there appear to be a couple of issues.

Firstly, i'm not sure why the check for a code mark gets special treatment, but I think the .type == "code" checks are wrong (.type is not a string?). I think these checks should use the truthy .isCode ?

The more general problem seems to be that the stack is always traversed from k=0 to stack.length-1, even when flushing the stack /w progress(null), where as the stack should presumably be traversed in the reverse direction when flushing?

[ericandrewlewis]

the stack should presumably be traversed in the reverse direction when flushing

I was diagnosing this at the same time. Yep, that's the problem.

[asteadman]

Can you comment on the .type == "code" checks? I believe changing them to .type.isCode would inadvertently fix the 3rd test case, but i'm not sure why it's there at all. Is the code mark special for some reason? AFAICT, these checks currently never evaluate true, so the whole if statement and corresponding conditional block could presumably be removed.

[ericandrewlewis]

I believe changing them to .type.isCode would inadvertently fix the 3rd test case

#235 fixes the first and third test cases. It seems the second text case will be trickier to fix.

AFAICT, these checks currently never evaluate true

Yes, good find. Can you open up a separate issue and perhaps a pull request with the change?

[ericandrewlewis]

If someone knows of a Markdown-generation library (the escaping is non-trivial), I'd gladly switch to that.

I think what we're looking for is a library that supports an abstract syntax tree data type for Markdown (which we can transform our document model into), which also supports converting the AST to Markdown. This would be very desirable given the second test case originally shared on this issue.

Briefly Googled around for this today. commonmark.js already has an AST representation, and is discussing adding AST → Markdown support.

[marijnh]

Attached patches, along with b43d167, should address this issue.