ProseMirror / prosemirror-markdown

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

Special case hard breaks #30

Closed klaftertief closed 6 years ago

klaftertief commented 6 years ago

This improves an edge case of hard breaks ending a marked node. See https://discuss.prosemirror.net/t/position-of-hard-break-in-markdown-just-after-em-or-strong-node/1644

I opted in to only remove whitelisted em and strong marks from hard beaks as the behaviour now ist that a surrounded hard break now splits marked content into two parts, see the new tests. I find this ok for emphasised text, but not ok for links. The implementation could also be changed to check if the hard break actually is trailing, but I'm not sure if this is needed.

I did not use Array.prototype.filter() as I couldn't find its usage in any of the prose mirror repos.

marijnh commented 6 years ago

Thanks!

Do you think it'd be reasonable to do this for all marks instead of hard-coding strong and emphasis marks?

Hard-coding the hard_break node name is also slightly problematic (another schema could use a different name for hard break nodes), but I don't really see a good way around that, so I guess we can just do it that way for the time being.

marijnh commented 6 years ago

Hm, it seems hard breaks in such marks is allowed by CommonMark, as long as there's non-whitespace text after the break. Maybe the test on line 231 can be refined to only fire when there's a non-whitespace-only text node with the same mark after the current node?

klaftertief commented 6 years ago

As said I would not do this for links, as hovering then is not nice, separate for each mark.

And yes, hard coding is not that nice, but as this plugin defines a schema, I figured I should optimise for this. I guess a proper fix would be to extend the schema.

Hm, it seems hard breaks in such marks is allowed by CommonMark, as long as there's non-whitespace text after the break. Maybe the test on line 231 can be refined to only fire when there's a non-whitespace-only text node with the same mark after the current node?

I played around with such an implementation using Fragment methods with the current offset and index. I did not find it to be that robust, but maybe I was overthinking things. (I'm quite new to ProseMirror.) This is what I meant with

The implementation could also be changed to check if the hard break actually is trailing, but I'm not sure if this is needed.

How would an idiomatic implementation look like? What about if the marks are only partially the same?

marijnh commented 6 years ago

I was thinking along the lines of a463544 . That'll still move trailing hard breaks out of links, but since there's nothing after them to split, I think that's not much of a problem. Does this work for you?

klaftertief commented 6 years ago

Perfect, this looks a lot like what I just started :-)

klaftertief commented 6 years ago

Thanks!

klaftertief commented 6 years ago

@marijnh Are you going to wrap this in a release or do you want to batch this with other changes?

marijnh commented 6 years ago

Ah, right, I've released 1.2.2