Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prosemirror SelectedNode Issue #43

Open
1 of 2 tasks
akashkamboj opened this issue Apr 2, 2021 · 13 comments · May be fixed by #118
Open
1 of 2 tasks

Prosemirror SelectedNode Issue #43

akashkamboj opened this issue Apr 2, 2021 · 13 comments · May be fixed by #118
Assignees
Labels
bug Something isn't working

Comments

@akashkamboj
Copy link

I checked everywhere and there is no solution to it.

Describe the bug
Prosemirror provides the ability to create NodeSelection. But when content is updated through YJS. The node selection gets removed and become the TextSelection.

To Reproduce
Steps to reproduce the behavior:

  1. Go to Y-Prosemirror Demo
  2. Insert an Image and click on the Image, the Image shows a border, indication of the NodeSelection.
  3. Now if The other collaborator does any change in the document. The border goes away and it creates a TextSelection of that image.
  4. This behaviour is very weird because I am showing a toolbar on Image Selection and that gets disappear every time.

Expected behavior
Ideally it should retain the NodeSelection and not convert it to TextSelection.

Environment Information
Everywhere

Additional context
I can detect the changes and move TextSelection back to NodeSelection, that requires an additional update to the state and behaviour will not be smooth at all. That doesn't seem the right way to me. Ideally y-prosemirror should handle this scenario itself.

@akashkamboj akashkamboj added the bug Something isn't working label Apr 2, 2021
@akashkamboj
Copy link
Author

I believe we can solve this here:

const restoreRelativeSelection = (tr, relSel, binding) => {
  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) {
      tr = tr.setSelection(TextSelection.create(tr.doc, anchor, head))
    }
  }
}

@dmonad
Copy link
Member

dmonad commented Apr 18, 2021

Hi @akashkamboj, could you please create a PR with the change? If it doesn't break anything, I'm happy to merge it after I was able to test it.

@jamesopti
Copy link

We are seeing an error in this same function that is causing YJS to pickup an empty change and effectively wipe out our document by persisting it (TipTap + YJS client with Hocuspocus YJS server).

Any chance these are related? We've noticed this more when someone has a tab open with the document and comes back online after closing their laptop lid.

YSync Error

@jamesopti
Copy link

jamesopti commented Mar 23, 2022

@akashkamboj Did you ever find a workaround to this?

We're seeing issues with NodeSelections turned into TextSelections as well, notably our drag handle implementation that lets you move an entire node.

We set the NodeSelection so that Prosemirror-view's drop handler will correctly delete the node on drop, but the NodeSelection turns into a TextSelection if someone else edits while youre dragging.

@milesingrams
Copy link

Have the same issue. I think on any change all node selections are converted to text selections. Not sure how to fix but assume it's related to the restoreRelativeSelection function. Perhaps if the original selection is a node then leave it as is?

@jevakallio
Copy link

@jamesopti I'm seeing the same issue with the same setup (TipTap + YJS client with Hocuspocus YJS server).

Did you have any luck fixing this?

For now I've patched y-prosemirror to not call tr.setSelection which avoid crashes and data loss, but I'm investigating the root cause and looking for a more appropriate fix.

@jamesopti
Copy link

@dmonad Any thoughts on this one? We've realized this makes collaborative editing with atom nodes (like an image) impossible as whenever user1 types, user2 loses their selection of the image.

@jamesopti
Copy link

Hi @akashkamboj, could you please create a PR with the change? If it doesn't break anything, I'm happy to merge it after I was able to test it.

I just threw up #118 after testing locally and verifying that my selected NodeView remains selected when another user types.

@akashbit
Copy link

hi @jamesopti I also did same kind of code to fix it temporarily . But this is not sufficient. Because apart from TextSelection and NodeSelection I also have CellSelection etc. And that becomes a bit complicated to calculate anchor and head.

@mweidner037
Copy link

mweidner037 commented Aug 12, 2022

hi @jamesopti I also did same kind of code to fix it temporarily . But this is not sufficient. Because apart from TextSelection and NodeSelection I also have CellSelection etc. And that becomes a bit complicated to calculate anchor and head.

For CellSelection, what worked for me is:

// oldState is old EditorState, tr is Transaction dispatched by y-prosemirror
if (oldState.selection instanceof CellSelection) {
    tr.setSelection(CellSelection.create(tr.doc, tr.selection.$anchor.before(), tr.selection.$head.before()));
}

hamflx added a commit to hamflx/y-prosemirror that referenced this issue Sep 21, 2022
hamflx added a commit to hamflx/y-prosemirror that referenced this issue Sep 21, 2022
@hamflx
Copy link

hamflx commented Sep 21, 2022

here's my solution: hamflx@2ed27a0

features:

  1. no switch/case
  2. no additional parameters

Use selection.map to record pos and convert to relative position when we need to save position.

Use the recorded pos to execute selection.map when we need to restore the position.

For custom SomeSelection, you only need to implement a proper map method to make the recovery of the selection work properly.

@akashkamboj
Copy link
Author

@hamflx awesome work, it's working great. They should really merge it.

One more question can we utilize the same in cursors as well somehow. So that if a node is selected the collaborator can see the selection as an overlay over the node?

romansp added a commit to Axure-Software/y-prosemirror that referenced this issue Oct 18, 2023
- introduce RecoverableSelection that wraps current selection and allows it to be restored later
- get ySyncPlugin state inside undoManager event hooks

fixes yjs#43, yjs#101, yjs#139
Keimdjo added a commit to Seraphin-legal/y-prosemirror that referenced this issue Apr 30, 2024
@xuliuzhu1834
Copy link

xuliuzhu1834 commented Oct 28, 2024

Hi @dmonad,

This issue has been open for 2 years, and the commit c99cd89 seems to provide a solution. It has successfully resolved the problem in my project. Could you please prioritize merging it at your earliest convenience?

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
9 participants