Undo/redo support for new editor #3267
Conversation
otherwise caret is put at editor start when undoing last step
src/editor/history.js
Outdated
| if (diff.added) { | ||
| // only append after every 5th keystroke while typing | ||
| this._newlyTypedCharCount += diff.added.length; | ||
| return this._newlyTypedCharCount > 5; |
There was a problem hiding this comment.
I'd recommend extracting this magic number out to a constant at the top of the module with a comment explaining it's purpose there.
src/editor/history.js
Outdated
| return true; | ||
| } | ||
| if (diff.added) { | ||
| // only append after every 5th keystroke while typing |
There was a problem hiding this comment.
Why every 5 chars? Would it make more sense to append at word boundaries instead, or is that too complex?
There was a problem hiding this comment.
That would be more complex, but probably worthwhile change. Will have a look 👍
| this._parts = serializedParts.map(p => this._partCreator.deserializePart(p)); | ||
| this._updateCallback(caret, inputType); | ||
| } | ||
|
|
||
| update(newValue, inputType, caret) { | ||
| const diff = this._diff(newValue, inputType, caret); |
There was a problem hiding this comment.
So we're diffing on every keystroke I guess? How expensive is that? Does this processing make the input experience feel sluggish at all? (I assume it shouldn't be sluggish at least, since the browser has already displayed the new character before the handler runs...?)
There was a problem hiding this comment.
why does this need a diffing algorithm rather than just snapshotting the editor state on every word boundary?
There was a problem hiding this comment.
For clarity, the diffing is part of how the editor works, and is unrelated to the undo support.
The diffing algorithm is very basic, and assumes there is only one change around the caret position, so this should be very inexpensive. See editor/diff.js for details. The most expensive part is probably to turn the content-editable content into a single string (with some logic, see getCaretOffsetAndText) on every keystroke. Some preliminary profiling shows that we can probably make some improvements still, and there is an item on element-hq/element-web#9866 for that.
As for the reason to use diffing; the alternative would be to recreate the model on every keystroke based on the DOM of the editor. Without considering the runtime cost/benefits of this, it would not allow us preserve state for a given section of the editor, like the part type, or the userid for a pill.
So on every keystroke, the editor dom is turned into a string, and the model updated: the model turns its current state into text, and compares the current and new text value. The difference between both is then applied to the existing parts of the model state, applying validation logic in the parts.
The benefit of this is that we can use the input event, which is broadly supported, to find changes in the editor. We don't have to rely on keyboard events, which poorly relate to text input or changes.
Once the parts of the model are updated, the DOM of the editor is then reconciled with the new model state, see renderModel in editor/render.js for this. If the model didn't reject the input and didn't make any additional changes, this won't make any changes to the DOM at all, and should thus be fairly efficient.
I should probably document all of this somewhere.
There was a problem hiding this comment.
ah, ok. i would definitely put all this into a massive /**/ comment at the top of the editor file, or possibly even docs/brunoEditor.md or whatever we're calling this ;)
There was a problem hiding this comment.
#3272 adds some documentation.
bwindels
force-pushed
the
bwindels/editor-history
branch
from
b9dc45a to
07b2e51
Compare
|
I thought the word boundary was going to be more complicated because right now I can only add an undo step including the last change, and constantly serializing the model might slow down things a bit. This means the undo step will include the last word boundary character, but in practice this seems fine.
|
Fixes element-hq/element-web#9831
The algorithm when to push history steps might still need a bit of fine-tuning. Right now it always pushes on every change, apart from when typing (and not overwriting), then it pushes a step on every 5th typed character.