Skip to content

Document editor: one undo takes back only part of an edit #903

Description

@andiwand

Problem

In the document editor (src/odr/internal/html/frontend/document.js), one undo does not take back one edit. If the reader edits inside a run and then presses undo, the page goes through intermediate states that the reader never made.

Cause

perform() pushes each step onto done as its own undo entry (document.js:101-110). But one beforeinput often performs several steps:

  • Enter inside a run. splitAt shortens the run, inserts its tail as a new run, and then splits the paragraph (document.js:1156-1160). That is three undo entries.
  • Formatting part of a run. markRange cuts the run at both ends and then styles each run (document.js:673-689).
  • Typing over a selection that covers several runs or paragraphs. replaceRange sets the text of both ends, removes what lies between, and merges the paragraphs (document.js:1022-1039).

undo() and redo() pop one entry at a time (document.js:1519-1538). So each undo reverts only a part of the edit, and the page shows the state between two parts.

This goes against the design note: docs/design/document-editing.md:140 says "One beforeinput is one undo step". The browser test codifies the current behaviour: test/browser/text/tests.html:378-379 expects two more undo steps after a split ("undo goes on to the run that was cut", "and to the run that was shortened").

Steps to reproduce

  1. Open an ODT and turn editing on.
  2. Put the caret in the middle of a run.
  3. Press Enter.
  4. Press Ctrl+Z three times.

Expected: the first Ctrl+Z puts the paragraph back as it was, and there is nothing more to undo.
Actual: the first Ctrl+Z joins the paragraphs, but the text stays cut into two runs. The next two Ctrl+Z take back the new run and the shortened run one after the other.

Possible fix

Give every step that one event handler performs the same gesture number, and let undo() and redo() take back or replay all steps of that gesture. A microtask can close the gesture after the handler returns, so no call site has to change:

var gesture = 0;
var gestureOpen = false;

function perform(step) {
  if (step === null) {
    return null;
  }
  if (!gestureOpen) {
    gestureOpen = true;
    gesture += 1;
    Promise.resolve().then(function () {
      gestureOpen = false;
    });
  }
  step.gesture = gesture;
  step.apply();
  done.push(step);
  undone.length = 0;
  odr.editing.changed();
  return step;
}

undo() then pops entries while done[done.length - 1].gesture is the same, and redo() does the same on undone. tests.html:378-380 must then expect that the second undo returns false.

Found while testing the editor on opendocument.app with a local 7.0.0 build.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions