Fix beforeinput: pass in view.state instead of view

BrianHung commented

2021-08-18 09:39:50 +02:00

(Migrated from github.com)

undo and redo both take in EditorState as arguments instead of EditorView.

The code surprisingly doesn't throw an error because pluginKey.getState(state) resolves to state[this.key] (which results in null); however, undo and redo always returns false, as !hist (where hist = historyKey.getState(state)) is a true condition.

`undo` and `redo` both take in `EditorState` as arguments instead of `EditorView`. The code surprisingly doesn't throw an error because [`pluginKey.getState(state)` resolves to `state[this.key]`](https://github.com/ProseMirror/prosemirror-state/blob/master/src/plugin.js) (which results in `null`); however, `undo` and `redo` always returns `false`, as `!hist` (where `hist = historyKey.getState(state)`) is a true condition.

BrianHung commented

2021-08-18 10:43:59 +02:00

(Migrated from github.com)

Tried going through the problem demo in https://github.com/ProseMirror/prosemirror/issues/1196 with the code only passing in EditorState to undo and redo; that alone doesn't fix the issue. Kinda makes sense because calling commands with only EditorState as input doesn't make the editor do anything:

When no dispatch callback is passed, the command should do a 'dry run', determining whether it is applicable, but not actually doing anything.

And default behavior of the beforeinput event is not prevented (since we're inside of handleDOMEvents), so it goes through. Second commit addresses both of these concerns.

--

Edit. One thing I'm unsure about is whether prosemirror-history should always call e.preventDefault() for historyRedo and historyUndo events regardless of the result of undo and redo, because what would be the default behavior for those two events if the undo and redo stacks are empty? Should it allow for default behavior, or should it result in a "no-op" by consuming the event but doing nothing?

Tried going through the problem demo in https://github.com/ProseMirror/prosemirror/issues/1196 with the code only passing in `EditorState` to `undo` and `redo`; that alone doesn't fix the issue. Kinda makes sense because calling commands with only `EditorState` as input [doesn't make the editor do anything](https://prosemirror.net/docs/ref/#commands): > When no dispatch callback is passed, the command should do a 'dry run', determining whether it is applicable, but not actually doing anything. And default behavior of the `beforeinput` event is not prevented (since we're inside of [`handleDOMEvents`](https://prosemirror.net/docs/ref/#view.EditorProps.handleDOMEvents)), so it goes through. Second commit addresses both of these concerns. -- Edit. One thing I'm unsure about is whether `prosemirror-history` should always call `e.preventDefault()` for `historyRedo` and `historyUndo` events regardless of the result of `undo` and `redo`, because what would be the default behavior for those two events if the `undo` and `redo` stacks are empty? Should it allow for default behavior, or should it result in a "no-op" by consuming the event but doing nothing?

marijnh commented

2021-08-18 12:26:53 +02:00

(Migrated from github.com)

Thanks for catching that! I copy-pasted these from the corresponding CodeMirror 6 file and failed to remember that the command interface is slightly different in ProseMirror.

I've pushed a slightly different patch. Not entirely sure about always preventing the default behavior, but that might actually be a good idea.

Thanks for catching that! I copy-pasted these from the corresponding CodeMirror 6 file and failed to remember that the command interface is slightly different in ProseMirror. I've pushed a slightly different patch. Not entirely sure about always preventing the default behavior, but that might actually be a good idea.

marijnh commented

2021-08-18 12:31:24 +02:00

(Migrated from github.com)

It seems the 'redo' entry in the context menu is just never going to be enabled when you preventDefault undo events. But I agree that we do need to preventDefault them always, because otherwise you get broken results when activating undo when the history is empty. Attached patch does this.

👍 1

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Rows
Columns

Fix beforeinput: pass in view.state instead of view #6

Pull request closed