search

yjs / y-prosemirror

ProseMirror editor binding for Yjs
https://demos.yjs.dev/prosemirror/prosemirror.html
MIT License
339 stars 118 forks source link

Did not deal with the problem of overlapping marks with same name #34

Open dongtc opened 3 years ago

dongtc commented 3 years ago

Describe the bug Prosemirror supports overlapping marks with same name(as long as they have different attributes):

excludes: ?⁠string Determines which other marks this mark can coexist with. Should be a space-separated strings naming other marks or groups of marks. When a mark is added to a set, all marks that it excludes are removed in the process. If the set contains any mark that excludes the new mark but is not, itself, excluded by the new mark, the mark can not be added an the set. You can use the value "_" to indicate that the mark excludes all marks in the schema.Defaults to only being exclusive with marks of the same type. You can set it to an empty string (or any string not containing the mark's own name) to allow multiple marks of a given type to coexist (as long as they have different attributes).

https://prosemirror.net/docs/ref/#model.MarkSpec.excludes

But prosemirror-binding seems didn't handle it well, i have found the problem code line: https://github.com/yjs/y-prosemirror/blob/25cea84874eace50745fd2433847aabceef92b65/src/plugins/sync-plugin.js#L670

If a prosemirror text with the marks:[{type: 'comment', attrs: {id: 1}}, {type: 'comment', attrs: {id: 2}}]

It will be convert to:{comment: {id: 2}}

You can see lost a mark.

dmonad commented 3 years ago

Right.. that makes sense. This type of information can currently not be properly represented in Yjs / Y.Text.

dongtc commented 3 years ago

In my project, i have a temporary solution. The main changed code is as follows:

image

Meantime, i change the createTextNodesFromYText and equalYTextPText functions.

It seems hack but works fine, do you have a better suggest? @dmonad

dmonad commented 3 years ago

Hi @dongtc ,

the approach might work, but it will break all existing documents because they use objects instead of arrays. I would accept a PR that is well tested and is backwards-compatible. I think it could be possible to use arrays as "attributes" when multiple marks with the same name are present.

dongtc commented 3 years ago

Tanks for your suggestion, i will create a PR later. @dmonad