y-prosemirror icon indicating copy to clipboard operation
y-prosemirror copied to clipboard

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

Open jamesopti opened this issue 2 years ago • 7 comments

Changes

  • Update getRelativeSelection to capture whether or not the current selection is a NodeSelection
  • Update restoreRelativeSelection to use a node selection if restoring one

Context

Attempts to fix #43

jamesopti avatar May 29 '22 00:05 jamesopti

@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 avatar May 29 '22 17:05 jamesopti

@dmonad Any chance we could get this merged soon?

jamesopti avatar Jun 19 '22 19:06 jamesopti

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:

dmonad avatar Jun 19 '22 20:06 dmonad

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 avatar Aug 02 '22 18:08 amilich

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

amilich avatar Aug 11 '22 00:08 amilich

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

rebolyte avatar Nov 09 '22 00:11 rebolyte

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.

Alecyrus avatar Dec 01 '22 23:12 Alecyrus