yjs / y-prosemirror

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

fix (sync plugin)[Issue #43]: Restore relativeSelection using a NodeSelection if appropriate #118

Open jamesopti opened 2 years ago

jamesopti commented 2 years ago

Changes

Context

Attempts to fix #43

jamesopti commented 2 years ago

@dmonad I think the complete solution here to handle custom selections (like Cell Selection for tables) is to accept an optional function parameter restoreSelection(tr, relSel) that a consumer could implement to handle their own selection restoration.

If you think that makes sense to do now, I'm happy to do it as part of this PR or as a follow up.

E.g.

const defaultRestoreSelection = (tr, anchor, head) => {
  const selection = relSel.type === 'node'
    ? NodeSelection.create(tr.doc, Math.min(anchor, head))
    : TextSelection.create(tr.doc, anchor, head)
  tr = tr.setSelection(selection)
}

export const ySyncPlugin = (yXmlFragment, {
  colors = defaultColors,
  colorMapping = new Map(),
  permanentUserData = null,
  onFirstRender = () => {},
  restoreSelection = defaultRestoreSelection
} = {}) => {

  ...

})

const restoreRelativeSelection = (tr, relSel, binding, restoreSelection) => {
  if (relSel !== null && relSel.anchor !== null && relSel.head !== null) {
    const anchor = relativePositionToAbsolutePosition(binding.doc, binding.type, relSel.anchor, binding.mapping)
    const head = relativePositionToAbsolutePosition(binding.doc, binding.type, relSel.head, binding.mapping)
    if (anchor !== null && head !== null) {
      restoreSelection(tr, anchor, head)
    }
  }
}
jamesopti commented 2 years ago

@dmonad Any chance we could get this merged soon?

dmonad commented 2 years ago

FYI this is pretty low on my priority list. I just took some time off. Now I need to catch up on quite a lot of stuff. The good thing is that it is on some kind of priority list I guess :sweat_smile:

amilich commented 2 years ago

FYI this is pretty low on my priority list. I just took some time off. Now I need to catch up on quite a lot of stuff. The good thing is that it is on some kind of priority list I guess 😅

Hey all! Just noting that we could use this fix as well. Would help us out fixing some other selection types.

amilich commented 2 years ago

FYI this is pretty low on my priority list. I just took some time off. Now I need to catch up on quite a lot of stuff. The good thing is that it is on some kind of priority list I guess 😅

Hey all! Just noting that we could use this fix as well. Would help us out fixing some other selection types.

Checking in briefly on this

rebolyte commented 1 year ago

Just bumped into this as well. @jamesopti anything left before it can be merged?

Alecyrus commented 1 year ago

I have the same issue https://github.com/yjs/y-prosemirror/issues/43. I try this commit and it does not work, but https://github.com/yjs/y-prosemirror/issues/43#issuecomment-1253668582 works.