[Bug]: Mod-z Not Blocked When editor.isEditable Is False

[jan-schweiger]

Affected Packages

core, react, @tiptap/extension-history

Version(s)

2.6.6

Bug Description

1. Edit the content
2. Run editor.setEditable(false)
3. Press CTRL + Z
4. The undo operation is performed

However, this only happens for Undo but not for Redo.

Reproduce

I was also able to reproduce it by slightly adjusting the minimal setup: https://tiptap.dev/docs/examples/basics/minimal-setup

import Document from '@tiptap/extension-document'
import Paragraph from '@tiptap/extension-paragraph'
import Text from '@tiptap/extension-text'
import { EditorContent, useEditor } from '@tiptap/react'
import React from 'react'
import History from '@tiptap/extension-history'

function App() {
  const editor = useEditor({
    extensions: [
      Document,
      Paragraph,
      Text,
      History  // <-- Added History
    ],
    content: "<p>Tiptap content</p>",
  })

  return (
    <div className="App">
      <EditorContent editor={editor} />

      <button onClick={() => editor.setEditable(!editor.isEditable)}>
        Toggle isEdit
      </button>
    </div>
  );
}

export default App;

You just need to edit some content and click the button to experience the bug for yourself.

Thank you very much in advance for your help!

Browser Used

Chrome

Code Example URL

No response

Expected Behavior

Mod + Z shouldn't change the content when the editor is read-only.

Additional Context (Optional)

No response

Dependency Updates

• Yes, I've updated all my dependencies.

[jan-schweiger]

I also tried the following quickfix. But strangely the Mod-z function is only called when the isEditable is true.

     History.extend({
        addKeyboardShortcuts() {
          return {
            'Mod-z': () => {
              console.log('Mod-z', this.editor.isEditable);
              return this.editor.isEditable ? this.editor.commands.undo() : false;
            },
            'Mod-Shift-z': () => {
              return this.editor.isEditable ? this.editor.commands.redo() : false;
            },
          };
        },
      }),

[tanja-kovakic]

I can also reproduce the bug. Just tried it out in my application. Can anyone have a look on this one?

[nperez0111]

Maybe this is a browser default behavior? I wonder if the element is still a contenteditable when editable=false

[rosenbauerwillbacker]

I can also reproduce the bug.

@nperez0111 In my case, the content is initially set to editable=false. I however change the content programmatically. In this case everything works fine. CTRL + Z doesn't change the content back. Only, when I make the content editable and then make it read-only again, the bug occurs.

I personally do not think it is the browser's default behaviour, because the bug is only limited to CTRL + Z, but not CTRL + Shift + Z.

I tried to pinpoint the cause, but my knowledge of the inner workings of tiptap is unfortunately limited. Could maybe someone investigate this issue? This is unfortunately a severe problem for my application. Thank you!

[jan-schweiger]

Any updates on this one?

[nperez0111]

Feel free to look into it and contribute a PR fixing it.

[jan-schweiger]

Thank you @nperez0111. I have already looked into the potential causes but haven't found a solution yet. I will give it another try over the weekend.

[jan-schweiger]

Unfortunately, I was unable to identify the root cause. As a temporary "fix" you can use the following code to clear the history whenever you programmatically set the editor to read-only again. This is definitely not the best solution, rather a dirty quickfix for production applications.

import { history } from '@tiptap/pm/history';

// clear history:
editor.unregisterPlugin('history');
editor.registerPlugin(history());

[Nantris]

I'm finding nothing is blocked since TipTap 2.5.9 when setEditable(false) is used

I'm not sure if some particular configuration is required for this to manifest 2.5.8 works and anything above lets all document-modifying events right through. Determined by git bisect and confirmed manually - but haven't had time to make a repro yet.

I don't know what configuration is required but I'm quite confident 2.5.9 is where this issue was introduced. It obviously requires some combination of circumstances to become apparent, or else it would be more widely reported and easier to reproduce, but I'm not sure what those are right now.

[Nantris]

@nperez0111 I believe the root cause of this is that a change to how useEditor works considers the initial options, but that includes editable and if editable is initially true but later setEditable(false) is called, the useEditor setting takes precedence.

Repro: https://codesandbox.io/p/sandbox/tiptap-react-qidlsv For some reason the code sandbox code won't update and forking breaks too. Please copy paste this into App.js to repro:

import "./styles.scss";
import { useEffect, useState } from "react";
import { EditorContent, useEditor } from "@tiptap/react";
import StarterKit from "@tiptap/starter-kit";

export default function App() {
  const [counter, setCounter] = useState(0);
  const editor = useEditor({
    immediatelyRender: true,
    shouldRerenderOnTransaction: false,
    extensions: [StarterKit],
    editable: true,
    editorProps: {
      attributes: {
        class: "Editor",
      },
    },
    content: `
      <h1>Welcome to your fresh Tiptap Code Sandbox</h1>
      <p>You can create a demo for your issue inside of this sandbox and share it with us.</p>
    `,
  });

  useEffect(() => {
    editor.setEditable(false);
    setCounter(1); // Causes "App" to re-render, which seems to causes `editable` from `useEditor` to take precedence
  }, []);

  return (
    <div className="App">
      <EditorContent editor={editor} />
    </div>
  );
}

Suspected problem commit: 7c8889a#diff-2e5eb93343b2ec03e98abdd0c8612de5492886ae9a7ddb6b1cdd22d222bb483cR10

[nperez0111]

Good intuition @Nantris, so editor.setEditable is just mutating the options object:

tiptap/packages/core/src/Editor.ts

Line 191 in 33e1305

this.setOptions({ editable })

Which in React, will recreate & reapply whenever it needs to switch editor instances like you found (which I firmly believe is the correct behavior and in-line with React's model of completely re-rendering things from scratch).

The tricky thing is that in the case of React recreating the instance, it is unclear when it should actually enforce the value. Like even in your example, you have editable: true, should that always apply (and you have to change it from there) or should setEditable takes precedence?

So, what I think is probably needed here is that the editable option should be copied to an instance variable and setEditable can modify that (and the editable extension can read from that instead). If editable is specified in the useEditor options, it will always be respected over the dynamic setEditable, but if it is not specified, then setEditable would persist somehow (likely needs to be copied from the old editor to the new editor, but I haven't thought this through completely).

Any opinions here? Different approaches?

[nperez0111]

As for history specifically, I think that all that may be needed is a check here:

tiptap/packages/extension-history/src/history.ts

Lines 58 to 63 in b941eea

	undo: () => ({ state, dispatch }) => {
	return undo(state, dispatch)
	},
	redo: () => ({ state, dispatch }) => {
	return redo(state, dispatch)
	},

Which would early exit with false if the editor is not editable. Would be happy to take a PR to fix that, since that is simpler than the issue that @Nantris brought up

[Nantris]

@nperez0111 thanks for your reply! I'm not sure what the best way would be really, but I can at least share what I believe might be most intuitive.

In my mind, I guess I was expecting the editable in useEditor to use the initially set value and then never update again unless editor.setEditable is called with a new value. This thinking might be biased by how useEditor used to work in older versions of TipTap v2, but I think there's some merit to it. Take a case where maybe a developer want sto call setEditable from inside of an extension, or might want to temporarily set it from the terminal for debugging. Then they'd have to entirely avoid setting editable in useEditor, and I guess this could be achieved by exclusively managing this via calling editor.setEditable inside of a useEffect but it seems a tad hacky.

Is it possible/reasonable to store the original editable value in a ref or something so that it could persist even as the editor is recreated and treat that ref as the single source of truth for the editable state? Any changes to the editable state would then need to be managed via editor.setEditable.

Thanks again for the great work @nperez0111 - really appreciate you!

[Nantris]

Created #5731 to address the original issue here.

[guarmo]

Hey @Nantris #5731 doesn't seem to be working.

Created #5745 to make sure that setEditable overrides the initial options provided to useEditor's editable option.

Regarding the history related issue:
We've found out that the Prosemirror's history plugin allows undoing when the editor is not editable.
We've looked into this with @nperez0111 and found out a workaround for it:

  addProseMirrorPlugins() { 
    return [ 
      new Plugin({
        key: new PluginKey('uneditableHistory'),
        props: {
          handleDOMEvents: {
            beforeinput: (view, e: Event) => {
              let inputType = (e as InputEvent).inputType
              if (inputType == 'historyUndo' || inputType == 'historyRedo') {
                if (!this.editor.isEditable) {
                  e.preventDefault()
                }
              }
            }
          }
        }
      })
    ]
  }

What this plugin does is blocking Prosemirror's history's default undo event when the editor is not editable.

[Nantris]

@guarmo great work! I can't test at the moment, but are we certain that historyUndo and historyRedo fire consistently on all platforms?

There's some more discussion on this topic here: https://discuss.prosemirror.net/t/native-undo-history/1823/20

PS: All commands that I tested are able to modify the document when it's otherwise uneditable, when they're called directly.

[nperez0111]

@guarmo great work! I can't test at the moment, but are we certain that historyUndo and historyRedo fire consistently on all platforms?

Those are the events that prosemirror history listens to so it would at least be consistent with what prosemirror history does so it won't undo in that case

[nperez0111]

Editable is user editable I think commands are out of scope for this

[nperez0111]

@guarmo's change is released with v2.9.0

[Nantris]

@nperez0111 is there a plan to incorporate @guarmo's plugin into Tiptap to fix the originally reported issue?

[Nantris]

Confirming this aspect of the issue is resolved: by #5745. Undo still goes through though.

[nperez0111]

@Nantris between @guarmo and I we decided that it was a little too hacky to incorporate into Tiptap itself. The bug has been there for a long time.

@guarmo was going to open an issue with prosemirror-history to see if we can get this resolved upstream since we identified it to be a 1 line fix in that package

[Nantris]

Sounds like a good approach.

@guarmo were you planning to make a PR to prosemirror-history or just open an issue on the wider prosemirror repo? I could open such an issue, but I'm not certain what the PR code would need to look like so I can't do that.

[guarmo]

@Nantris thanks for the reminder. I have just created a PR that aims to fix the issue.

[guarmo]

Alright PR is merged. You'd need to update prosemirror-history to see the changes. Closing this one.

[jan-schweiger]

Amazing!! Thank you @nperez0111 and @guarmo.