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

The cursor jumps to the end of the content when using React Hook Form #91

Closed
EduardTrutsyk opened this issue Jul 10, 2023 · 5 comments
Closed

Comments

@EduardTrutsyk
Copy link

Describe the bug

When I try to use RichTextEditor with React Hook Form and change the form value via onUpdate prop with HTML format, the cursor jumps to the end of the content each time.

To Reproduce

Steps to reproduce the behavior:

  1. start typing

Expected behavior

The content should be updated, and the cursor should stay in the same position

Screenshots

Link to Reproduce

System (please complete the following information):

  • mui-tiptap version: [1.0.0-beta.9]
  • tiptap version: [2.0.3]
  • Browser: [Chrome]
  • Node version: [16.18.1]
  • OS: [macOS 13.4]
@sjdemartini
Copy link
Owner

Thanks for using mui-tiptap and for reporting this bug @EduardTrutsyk! I appreciate the CodeSandbox repro link.

I'm fairly certain what is causing this is the fact that I have the RichTextEditor calling setContent whenever its content prop changes, which makes it behave more like a controlled component if the content is being updated external to the editor:

// Update content if/when it changes
const previousContent = useRef(editorProps.content);
useEffect(() => {
if (
!editor ||
editor.isDestroyed ||
editorProps.content === undefined ||
editorProps.content === previousContent.current
) {
return;
}
// We use queueMicrotask to avoid any flushSync console errors as
// mentioned here
// https://github.com/ueberdosis/tiptap/issues/3764#issuecomment-1546854730
queueMicrotask(() => {
// Validate that editorProps.content isn't undefined again to appease TS
if (editorProps.content !== undefined) {
editor.commands.setContent(editorProps.content);
}
});
}, [editorProps.content, editor]);

But that above code means it doesn't work well when you're updating content upon changes within the editor itself. So I think I'll just remove that code so that RichTextEditor can be used in more flexible ways to support your sort of use-case.

What you can do in the meantime before I release an update that fixes this problem: use useEditor, RichTextEditorProvider, and RichTextField instead of the all-in-one RichTextEditor component, since it does not have that logic. Here's an example of that, forked from your CodeSandbox:
https://codesandbox.io/p/sandbox/mui-tiptap-demo-forked-2d74dp?file=%2Fsrc%2FEditor.tsx%3A1%2C1


Also, one separate general point/suggestion (not specific to mui-tiptap per se): I would not recommend calling editor.getHTML() inside of onUpdate, since that will be called upon every keystroke inside your editor and Tiptap's getHTML() can be expensive, particularly as the content gets longer. (See the source for what getHTML uses here and the underlying prosemirror-model code here—the serialization seemingly isn't "free".) Instead, you'll probably want to debounce updating your react-hook-form state and calling getHTML(), or perhaps not include the editor content in your react-hook-form state at all. For instance, you could just wait until form submission to call editor.getHTML() and include that in whatever API request, app state update, etc. you need.

@EduardTrutsyk
Copy link
Author

@sjdemartini thanks for your answer and explanation, it really helps!
Two more things which I found:

  1. RichTextReadOnly will not work without extensions: https://codesandbox.io/p/sandbox/mui-tiptap-demo-forked-fx5g2j?file=%2Fsrc%2FFormExample.tsx%3A34%2C28
  2. Editor is not controlled, and when we display the updated content on the same page, the editor will not re-render.

but we can achieve this behavior in the following way (I'm not sure that this is the best solution)

  useEffect(() => {
    if (!editable) {
      editor?.commands.setContent(value || '');
    }
  }, [value, editor, editable]);

I know that serialization/deserialization is an expensive operation, we can use debounce or even better update react-hook-form state on onBlur event but looks like onBlur fired when the user clicks on the extension button and we will lose these changes.

@sjdemartini
Copy link
Owner

RichTextReadOnly will not work without extensions

This is correct. It still has to use Tiptap under the hood in order to properly render all of the content in whatever way you desire, which depends on what extensions are configured and how they're configured. (It's not merely rendering the raw serialized HTML directly, but rather is set up to accommodate the full capabilities of Tiptap's renderer, which can add interaction functionality with node views, JS-based logic, etc. even in read-only mode.) I can make the README more explicit about that.

It (like RichTextEditor) inherits the typing that Tiptap's useEditor uses, where it doesn't list extensions as a required prop. Even if that field were required in TS though, you could still pass in an empty array and satisfy the type, but the same error would come up since you need to include the Document extension at the very least. This is due to the extension design of Tiptap itself, where the extension list must always be specified. I'll probably update the types though as more of a "hint" that you need to pass in extensions.

Editor is not controlled, and when we display the updated content on the same page, the editor will not re-render.

Right, this is what I was aiming to handle with <RichTextEditor /> in the code I linked above, though it comes with the caveat you discovered about the user's caret/selection when it's editable, with its current implementation. I'd used code like the following before with a setContent-based approach, in order to try to preserve the user's selection position, though I would still recommend only using this in situations like blur/debounce, and not trying to use as a fully-controlled component where content changes on every call to onUpdate (which I don't imagine could work well). Something like this might work for you:

    // Only reset the content to the latest `value` if the user isn't focused on
    // the editor or currently in editing mode. This tries to avoid losing any
    // in-progress the user might be continuing to make. This approach isn't perfect; a better
    // solution could involve "collaborative editing" (https://tiptap.dev/guide/collaborative-editing)
    if (!editor.isFocused || !editable) {
      // Preserve the previous editor cursor state when calling `setContent` (which
      // otherwise would reset the selection/cursor)
      const currentSelection = editor.state.selection;
      editor
        .chain()
        .setContent(value)
        .setTextSelection(currentSelection)
        .run();
    }

I know that serialization/deserialization is an expensive operation, we can use debounce or even better update react-hook-form state on onBlur event but looks like onBlur fired when the user clicks on the extension button and we will lose these changes.

Yeah, this is tricky. Why is it that you say you will lose changes with that approach though? If you're saving the latest serialized content in onBlur, it seems you wouldn't have to worry about then losing changes, even if onBlur happens when interacting with menu buttons, bubble menus, etc. (Tiptap docs describe this expected blur/focus phenomenon here: https://tiptap.dev/api/commands/focus.)

For what it's worth, there's a mui-tiptap hook that may be helpful for you: useDebouncedFocus (available to import, though I didn't document it in the README yet), if you need some way to track the user's focus state more easily in a "debounced" way. Could at least be inspiration for other ideas if not immediately usable for your app:

/**
* A hook for getting the Tiptap editor focused state, but debounced to prevent
* "flashing" for brief blur/refocus moments, like when interacting with the
* menu bar buttons.
*
* This is useful for showing the focus state visually, as with the `focused`
* prop of <FieldContainer />.
*/
export default function useDebouncedFocus({

sjdemartini added a commit that referenced this issue Jul 12, 2023
See discussion here
#91 (comment)

These components won't work without some extensions provided, so improve
upon the typing we get from Tiptap and specify `extensions` as a
required prop. Note that an empty array could still "pass" typing, but
at least it's a hint that a real value is required, so shouldn't be as
likely to run into a confusing error, like in
#91 (comment).
sjdemartini added a commit that referenced this issue Jul 12, 2023
See discussion here
#91 (comment)

These components won't work without some extensions provided, so improve
upon the typing we get from Tiptap and specify `extensions` as a
required prop. Note that an empty array could still "pass" typing, but
at least it's a hint that a real value is required, so shouldn't be as
likely to run into a confusing error, like in
#91 (comment).
@EduardTrutsyk
Copy link
Author

@sjdemartini you are right, saving changes on onBlur works as expected. Thank you for your help!

This is the final working example mui-tiptap with react-hook-form:
https://codesandbox.io/p/sandbox/mui-tiptap-demo-forked-fx5g2j

sjdemartini added a commit that referenced this issue Jul 15, 2023
This is related to the discussion here
#91, and should make the
component more generally flexible, with the option for users to
customize when/how they want to `setContent`.
sjdemartini added a commit that referenced this issue Jul 15, 2023
This is related to the discussion here
#91, and should make the
component more generally flexible, with the option for users to
customize when/how they want to `setContent`.
sjdemartini added a commit that referenced this issue Jul 15, 2023
This is related to the discussion here
#91, and should make the
component more generally flexible, with the option for users to
customize when/how they want to `setContent`.
sjdemartini added a commit that referenced this issue Jul 15, 2023
This is related to the discussion here
#91, and should make the
component more generally flexible, with the option for users to
customize when/how they want to `setContent`.
sjdemartini added a commit that referenced this issue Jul 15, 2023
This is related to the discussion here
#91, and should make the
component more generally flexible, with the option for users to
customize when/how they want to `setContent`.
@sjdemartini
Copy link
Owner

Just as a heads up, I removed the automatic setContent behavior in the all-in-one RichTextEditor component, per the above discussion (see the PR here #98). I've also changed RichTextReadOnly so that that component does update when content changes, since I think that doesn't have the same caveats as an editable editor.

I've updated the README accordingly, and added some more suggestions to it based on this thread (e.g. about calling setContent yourself to update if needed, here) and published those changes as a part of the first non-beta v1.0.0 release.

Thanks again for filing the issue and discussing! Glad you were able to get your react-hook-form setup working well!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants