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

Invalid position at 334 at https://docs.remirror.org/editors/wysiwyg #96

Closed
joonhocho opened this issue Jul 1, 2019 · 3 comments · Fixed by #98
Closed

Invalid position at 334 at https://docs.remirror.org/editors/wysiwyg #96

joonhocho opened this issue Jul 1, 2019 · 3 comments · Fixed by #98
Labels
package: remirror 🟣 Label for the remirror package and all relevant scoped `@remirror/*` packages. type: bug 🪲 Something isn't working

Comments

@joonhocho
Copy link

Go to https://docs.remirror.org/editors/wysiwyg

On load, select all content delete press undo and it breaks the page. there are error logs on console

@ifiokjr
Copy link
Member

ifiokjr commented Jul 2, 2019

@joonhocho thanks for raising this. I have confirmed the issue 👍

The offending line is in the positioners.

https://github.com/ifiokjr/remirror/blob/26f51ae94ca102c4282de45d3f123a929f0fc368/%40remirror/react/src/positioners.ts#L61

While there are several easy fixes, the fundamental issue is that the EditorState held by the remirror component is one step ahead of the EditorState stored by Prosemirror.

When updating the editor state the remirror component first calls this.setState({ newState: state }) and then after the component renders, view.updateState is called.

The updateHandler can be found here.

https://github.com/ifiokjr/remirror/blob/26f51ae94ca102c4282de45d3f123a929f0fc368/%40remirror/react/src/components/remirror.tsx#L374-L380

By simply moving the view.updateState to before the this.setState call, this bug and probably a whole host of future bugs are fixed.

The only problem is that currently, the @remirror/extension-mention package depends on this weird implementation. The package was developed at the very beginning of my journey with Prosemirror and for some reason delaying the update of the view's state fixed an issue I couldn't fix otherwise.

Before I can merge in the simple fix I'll need to update @remirror/extension-mention to be a Prosemirror mark rather than node. This will also provide much better support on Android keyboards.

@joonhocho
Copy link
Author

joonhocho commented Jul 2, 2019

@ifiokjr Thank you so much for details. Awesome work on your repo, btw. I've been searching for editors for past few days and ,after trying out so many options, decided yours is the best one for my use case (prosmirror + react + typescript). I still see performance issues with slatejs, unfortunately. I don't have much experience on editors yet, though I've read ProsMirror documentations.

You said your work is inspired by tiptap. Have you also taken a look at Atlaskit's editor? It's built on top of react + prosmirror + typescript. https://bitbucket.org/atlassian/atlaskit-mk-2/src/master/packages/editor/

They have implemented full featured editor, so they may have solutions for similar problems.

@ifiokjr
Copy link
Member

ifiokjr commented Jul 6, 2019

@joonhocho thanks for the pointer in the atlassian direction.

Credit where credit is due, I've used a lot of code from them in making this library. The current implementation of NodeView's with React components is basically a direct copy.

I've also got some work coming (at some point) for tables which borrows heavily from their library (albeit simplified).

ifiokjr added a commit that referenced this issue Jul 10, 2019
@ifiokjr ifiokjr added package: remirror 🟣 Label for the remirror package and all relevant scoped `@remirror/*` packages. and removed @remirror/editor-wysiwyg labels Aug 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: remirror 🟣 Label for the remirror package and all relevant scoped `@remirror/*` packages. type: bug 🪲 Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants