Skip to content

Migrate formatSelection off document.execCommand to a synchronous DOMParser-based transform #4637

Description

@raineorshine

Background

formatSelection currently applies formatting (bold, italic, strikethrough, underline, code, foreColor, backColor, removeFormat) via document.execCommand. Because execCommand mutates the contentEditable DOM directly, the resulting state change only re-enters Redux later via the contentEditable's onChange → debounced thoughtChangeHandlereditThought. This introduces an async gap between the command and the resulting editThought dispatch.

This async gap is the root cause of a whole class of undo/redo complexity:

  • Applying foreground + background color is two separate dispatches that must be stitched into a single undo step.
  • The follow-up cleanup pass (strip default bg/fg, unwrap empty font/span tags) is a second dispatch that also must merge.
  • To stitch these together across the async gap, we built the entire batchEditing apparatus: batchEditing.ts, mergePrev / MergePrevActionPayload, the edited flag, a deferred setTimeout-based batch close (to survive the async gap on mobile WebViews), and wiring in Editable.tsx, Note.tsx, and formatSelection.ts.

See #4628 (interim fix) and #4620 for the undo bug this complexity works around.

Proposal

Reimplement formatSelection to compute the new HTML synchronously using DOMParser (which we already use for the background-color/tag-cleanup pass) and dispatch a single editThought / setDescendant. With formatting applied in one synchronous dispatch:

  • foreColor + backColor land in a single dispatch → a single undo step with no stitching.
  • The cleanup pass merges into the same synchronous transform instead of being a second dispatch.
  • There is no async gap, so no batching is needed at all.

This lets us delete:

  • src/stores/batchEditing.ts
  • src/@types/MergePrevActionPayload.ts
  • mergePrev wiring in src/components/Editable.tsx, src/components/Note.tsx, and src/actions/formatSelection.ts
  • The mergePrev handling in undoRedoEnhancer (restoring it to a fully action-type-agnostic, patch-based undo system)

Design constraint

undoRedoEnhancer must remain a generalized, patch-based undo/redo system that works on any Redux state patch. It must not become aware of formatting-specific action types. Removing mergePrev entirely (rather than deriving it centrally) satisfies this.

Subtleties / acceptance criteria

The rewrite must preserve current behavior, including the parts where execCommand is doing non-trivial work:

  • Partial-selection formatting. Formatting applied to an arbitrary sub-range of a thought (e.g. bold on a few characters mid-word, color on a selected word) must correctly split text nodes, wrap the exact range, and merge adjacent identical spans — including nested (<b><i>) and overlapping ranges. This is the hardest case; the whole-thought path is straightforward.
  • Selection/caret restoration. After replacing innerHTML via a synchronous editThought, the caret/selection must be restored to the correct offset for both whole-thought and partial-selection cases. Existing machinery (selection.offsetThought(), savedSelection) can be reused but must cover the partial-range case.
  • All commands. Handle bold | italic | strikethrough | underline | code | removeFormat in addition to color, including toggle on/off based on current formatting state (equivalent to queryCommandState).
  • Note vs. thought. Preserve the note-vs-thought branching (setDescendant vs editThought) and note-specific default colors (fgNote).
  • Mobile behavior. Preserve the mobile edit-mode selection/keyboard behavior ([Android] Text formatting opens keyboard if keyboard is manually dismissed prior #3996) — no async gap means the setTimeout deferral in endBatchEditing is no longer needed.
  • Regression tests in src/commands/__tests__/undo-redo.ts and e2e/puppeteer/__tests__/undo.ts continue to pass (a single undo reverts only the most recent color application).

Sequencing

This is a follow-up to #4628, which lands an interim fix for the undo bug. This issue tracks the root-cause migration that makes the batching apparatus unnecessary.

Metadata

Metadata

Assignees

Labels

refactorRefactor without changing behavior

Type

No type

Fields

No fields configured for issues without a type.

Projects

No projects

Relationships

None yet

Development

No branches or pull requests

Issue actions