Delete->Undo will move cursor to the beginning of editor

[zhujinxuan]

Do you want to request a feature or report a bug?

bug

What's the current behavior?

See the gif below:

Delete a char and then undo will place the cursor to a wrong position.

What's the expected behavior?

Cursor shall be where the edit happens.

[ericedem]

May be related to this? #2098

[zhujinxuan]

@ericedem You are right. The problem is about undo the selection... I just found some strange behavior in logs.

It seems the the history does not batch operations in the right way. The slate editor undos all operations until the start state of the document.

[zhujinxuan]

Perhaps we can set history stack barrier in these events:
1, onKeyDown (if delete happens)
2. onBeforeInput (if value.isExpanded)
3. onInput (if delete event.data.length > 1 representing a spell check replacement)

[sudo-tee]

Has somebody found a workaround..

Undoing in the middle of a huge document will set the focus at the start of the document.

You can try it in the Huge document example: https://www.slatejs.org/#/huge-document

This is was not happening in older version of the editor. (0.3.*)

thanks for your help

[zhujinxuan]

@inkubux Two solutions

1. use change.snapShotSelection in onChange if the previous change is not undo
2. Await Add controller #2221

[sudo-tee]

@zhujinxuan

Thanks.

I'm having a bit of difficulty finding if the previous change is not a undo inside of the onChange callback

[zhujinxuan]

@inkubux
Two solutions:

1. (suggested) compare the history stack between prevState.value and state.value
2. (ugly) hook onKeyDown when cmd+z is pressed with setTimeout to detect whether undo happens in last 50ms.

[sudo-tee]

@zhujinxuan zhujinxuan

After multiple hours trying to find the issue. I finally got it.

The proposed workaround did not work unfortunately, but calling change.select with the last selection when it's not un undo worked (more or less) but created some other wierd bugs.

I ended up digging in the slate core to find the issue

When you call the change.snapshotSelection()

it then call the applyOperation with

change.applyOperation({ type: 'set_selection', value: value, properties: props, selection: selection.toJSON() }, snapshot ? { skip: false, merge: false } : {});

it tells the applyOperation to not merge BUT, the applyOperation method does not have a second parameter even though the comments says otherwise.

/** * Apply anoperation` to the current value, saving the operation to the
* history if needed.
*
* @param {Operation|Object} operation
* @param {Object} options
* @return {Change}
*/

value: function applyOperation(operation) {`

So the merge flag will be set to null because of the tmp check

// Default options to the change-level flags, this allows for setting // specific options for all of the operations of a given change. var _tmp = this.tmp, merge = _tmp.merge, save = _tmp.save

and then in the history.save the shouldMerge method will set the merge to true. so the snapshot is merged with the prevBatch

After this the deletion comes in it check the the non-commital operations and set the merge flag to true.. because of the snapshot being attached to this.operations

// If merge` is non-commital, and this is not the first operation in a new change
// then we should merge.

  if (merge == null && operations.size !== 0) {
    merge = true;
  }

`
... since the snapshot is merged with the previous batch from the history the delete method will also be merged with it.

I was able to make it work by fixing the applyOperation to take the options in consideration like so

`value: function applyOperation(operation, options={}) {

  var operations = this.operations;
  var value = this.value;
  var _value = value,
      history = _value.history;

  var oldValue = value;

  // Add in the current `value` in case the operation was serialized.
  if (isPlainObject(operation)) {
    operation = _extends({}, operation, { value: value });
  }

  operation = Operation.create(operation);

  // Default options to the change-level flags, this allows for setting
  // specific options for all of the operations of a given change.
  var _tmp = this.tmp,
      merge = _tmp.merge || options.merge,
      save = _tmp.save  || options.save;

  // If `merge` is non-commital, and this is not the first operation in a new change
  // then we should merge.

  if (merge == null && operations.size !== 0) {
    merge = true;
  }

  // Apply the operation to the value.
  debug$4('apply', { operation: operation, save: save, merge: merge });
  value = operation.apply(value);

  // If needed, save the operation to the history.
  if (history && save) {
    history = history.save(operation, { merge: merge });
    value = value.set('history', history);
  }

  // Get the keys of the affected nodes, and mark them as dirty.
  var keys = getDirtyKeys(operation, value, oldValue);
  this.tmp.dirty = this.tmp.dirty.concat(keys);

  // Update the mutable change object.
  this.value = value;
  this.operations = operations.push(operation);
  return this;
}` 

I'm putting this here since I might have broke something obvious. I can create a pull request , but I'm not sure of the guidelines to make it merged

Thanks for your help

[zhujinxuan]

@inkubux My pesudo code:

function createSnapshotPlugin() {
  let undoId;
  let isUndo = false;
  let prevDocument;
  function onKeyDown(event) {
     if (Hotkeys.isUnodo(event) {
        undoId = setTimeout(()=> {
           isUndo = false
        }, 50)
        isUndo = true;
     }
     isUndo = false
     window.clearTimeout(undoId);
  }
  function onChange(change) {
     if (change.value.document === prevDocument) return;
     prevDocument === change.value.document;
     if (isUndo) return;
     change.snapshotSelection();
   }
}

[sudo-tee]

@zhujinxuan

Thanks a lot again :)

Sorry if I sound insistant..

The actual issue is that snapshotSelection is getting merged with the previous batch for the current change.

This is causing multiple issues

Example

Insert text
Move your cursor to somewhere else
delete text

undo

the undo will undo the insertion and the deletion in one batch.

I created a pull request for the issue. Let me know if you need more informations

#2240

[zhujinxuan]

@inkubux Oh, I did not notice that. I am using a different version, so I did not know that the snapshot has changed. Sorry that I did not catch that in my previous post.

[sudo-tee]

Great thanks