diff --git a/Editing/visible_position/DESIGN.md b/Editing/visible_position/DESIGN.md new file mode 100644 index 000000000..e4870b26f --- /dev/null +++ b/Editing/visible_position/DESIGN.md @@ -0,0 +1,327 @@ +# Reduce VisiblePosition usage in Editing Commands + +This document proposes a phased plan to reduce `VisiblePosition` usage inside Blink's editing command bodies. + +--- + +## Background + +`VisiblePosition` is Blink's canonicalized cursor position type - it answers *where the caret visually appears given current layout*. It belongs at exactly two points in the editing pipeline: + +- **Entry** - renderer hands a `VisibleSelection` to the command system. +- **Exit** - command hands its result back to update the caret on screen. + +Inside `DoApply()`, commands work on DOM structure - raw `Position` questions, no layout needed. VisiblePosition is used throughout command bodies as a type adapter because `IsEndOfParagraph`, `MoveParagraph`, etc. had no `Position` overloads when written. Every such call silently forces a full layout update, interleaved with DOM mutations. + +--- + +## Problem Statement + +### User-visible symptoms +- **Wrong caret/selection bugs** - stale VisiblePositions are silently used after DOM mutations (`IsValid()` is `DCHECK`-only in release). Caret lands in wrong position after paste, list insertion, or indent. +- **Loss of developer intent** - raw positions get silently re-canonicalized into different positions than the caller intended, taking away flexibility from developers. +- **Typing lag** - each keystroke triggers multiple full style+layout passes against a partially-mutated tree. +- **Paste jank** - `ReplaceSelectionCommand` triggers dozens of layout passes for a single Ctrl+V on complex content. +- **Slow indent/outdent** - `IndentOutdentCommand` loops per-paragraph; VisiblePosition construction multiplies layout cost by paragraph count. +- **Blocks future work** - every VisiblePosition inside `DoApply()` is an implicit main-thread layout dependency, blocking batched undo and speculative editing. + +### Known bugs blocked on this work +- [Hotlist 7675360](https://issues.chromium.org/hotlists/7675360) - 25-30 open issues: wrong caret after paste, bad selection after list insertion, undo/redo producing wrong selection, indent leaving caret in detached subtree. +- Several WPT failures in `editing/` and `contenteditable/` where stale VisiblePosition after mutation is the root cause. + +### Scale + +| Metric | Count | +|--------|-------| +| `VisiblePosition` type usage | ~370 across 16 files | +| `CreateVisiblePosition()` calls | ~124 across 15 files | +| `.DeepEquivalent()` calls (VisiblePosition unwrapping) | ~271 across 16 files | + +Top offenders: `composite_edit_command.cc` (112), `replace_selection_command.cc` (98), `insert_list_command.cc` (91), `delete_selection_command.cc` (64), `indent_outdent_command.cc` (61). + +~110 of 124 `CreateVisiblePosition` calls (~89%) are unnecessary type adapters. Only ~14 sites are genuinely load-bearing. + +> Unit test files (`*_test.cc`) are not included in any of the counts above. + +### After this work + +| Metric | Before | After | +|--------|--------|-------| +| `CreateVisiblePosition` calls in `commands/` | 126 | ~16 | +| `VisiblePosition` type references in `commands/` | ~391 | ~66 | +| Unnecessary `UpdateStyleAndLayout` per editing op | multiple | 0 | + +--- + +## Design Principles + +### The Correct Abstraction Boundary + +``` +User Interaction + | + v +[Boundary-IN] VisibleSelection → SelectionInDOMTree (renderer → command) + | + v +[Command body] Works entirely with Position / PositionWithAffinity / EphemeralRange + | + v +[Boundary-OUT] SelectionInDOMTree → SetEndingSelection(...) (command → renderer) +``` + +`VisiblePosition` belongs only at the two boundary points. The problem is that commands create VisiblePositions deep inside `DoApply()`. + +--- + +## Anti-Patterns to Eliminate + +Patterns are ordered by priority: highest VisiblePosition count reduction first, then safety risk. + +### P1: VisiblePosition parameter forces caller canonicalization + +`MoveParagraph` / `MoveParagraphWithClones` / `MoveParagraphs` in +`composite_edit_command.h` take `const VisiblePosition&`. Every caller in +`indent_outdent_command.cc`, `insert_list_command.cc`, `delete_selection_command.cc`, +etc. must wrap raw positions into VisiblePositions before calling. Fixing the three method +signatures (Step 3) removes ~80 VisiblePositions across all callers at once. + +### P2: `EndingVisibleSelection()` called mid-command for raw position access + +`EndingVisibleSelection()` unconditionally calls `UpdateStyleAndLayout` then wraps +the result in a `VisibleSelection`. It appears in two unnecessary forms throughout +`replace_selection_command.cc`, `composite_edit_command.cc`, +`indent_outdent_command.cc`, `insert_list_command.cc`, and others: + +| Sub-pattern | Replacement | +|---|---| +| Raw position only | `EndingSelection().Start()` / `.End()` - no layout | +| After explicit layout gate, position used for navigation | Keep `UpdateStyleAndLayout`; replace `EndingVisibleSelection().VisibleStart()` with `EndingSelection().Start()` and use `Position` overloads | +| VisiblePosition from `EndingVisibleSelection()` passed directly to navigation function | `NavigationFn(EndingSelection().Start(), rule)` - explicit gate already covers layout | + +`EndingSelection()` returns a `SelectionForUndoStep` with raw `Position` values that +are already correct - no re-canonicalization needed. + + +### P3: VisiblePosition as a type requirement for navigation tests + +`IsEndOfParagraph` / `IsStartOfParagraph` have no `Position` overload today, so VisiblePosition +creation is forced purely to satisfy the type signature. High frequency across all +command files; mechanical to fix once Step 1 overloads exist. + +### P4: Create-then-extract + +A VisiblePosition is created from a `PositionWithAffinity` or raw `Position` and immediately +passed to a navigation function. The VisiblePosition serves only as a type adapter - the position +was already correct before the wrap. Fixed mechanically alongside P3 sites. + +--- + +## Proposed Approach: bottom-up foundation, top-down infrastructure + +1. **Add missing `Position`-based overloads** for navigation functions in `position_units` and `editing_commands_utilities` (bottom-up, purely additive). +2. **Change `CompositeEditCommand` infrastructure signatures** to take `Position` instead of `VisiblePosition` (top-down, uses the new overloads internally). +3. **Migrate command files** using the new overloads and the new infrastructure signatures, in ascending order of VisiblePosition count. + +--- + +## Step 1: Add Position-based Navigation Overloads + +**Nature:** Purely additive. No existing code is changed. Each overload is its own CL with tests. Zero regression risk. + +### New file: `position_units.h` / `position_units.cc` + +All `Position`-based navigation overloads live in a new dedicated pair. No +`VisiblePosition` type appears anywhere in these files. The existing +`Position`/`PositionWithAffinity` overloads currently in `visible_units.h` are +relocated here in CL 1-A; the originals are removed. Callers that needed only those +overloads switch to including `position_units.h`; files that still use VisiblePosition-returning +functions keep `visible_units.h`. + +### Gaps to fill + +New `Position` overloads to add to `position_units.h` / `position_units.cc`: + +**Paragraph boundary** (call algorithm templates in `visible_units_paragraph.cc`): +`StartOfParagraph`, `EndOfParagraph`, `IsStartOfParagraph`, `IsEndOfParagraph`, +`StartOfNextParagraph`, `InSameParagraph`. + +**Document boundary**: `EndOfDocument`, `IsStartOfDocument`, `IsEndOfDocument`. + +**Position traversal**: `PreviousPositionOf(const Position&, EditingBoundaryCrossingRule)`, +`NextPositionOf(const Position&, EditingBoundaryCrossingRule)`. These bypass the +`CreateVisiblePosition` wrap in the existing VisiblePosition overloads by calling +`PreviousVisuallyDistinctCandidate` / `NextVisuallyDistinctCandidate` directly. Do +not change existing VisiblePosition signatures. + +**Character query**: `CharacterAfter(const Position&)`. + +**Line boundary**: `IsStartOfLine` and `IsEndOfLine` taking `const PositionWithAffinity&` +and `const Position&`. No `Position` overloads exist today; needed because +`IsStartOfLine(CreateVisiblePosition(pos, affinity))` appears in +`editing_commands_utilities.cc` with no layout-free replacement. + +**In `editing_commands_utilities.h` / `.cc`** (not `position_units` - depends on +block-structure helpers private to that file): `StartOfBlock`, `EndOfBlock`, +`IsStartOfBlock`, `IsEndOfBlock`, `EnclosingEmptyListItem`. + +### Already in `visible_units.h` - relocate to `position_units.h` + +These already take `Position` or `PositionWithAffinity` and return the same. They are +moved in CL 1-A and removed from `visible_units.h`. + +**Word:** `StartOfWordPosition`, `EndOfWordPosition`, `MiddleOfWordPosition`, +`PreviousWordPosition`, `NextWordPosition`. + +**Sentence:** `StartOfSentencePosition`, `EndOfSentence(const Position&)`, +`PreviousSentencePosition`, `NextSentencePosition`, `ExpandEndToSentenceBoundary`, +`ExpandRangeToSentenceBoundary`. + +**Line:** `StartOfLine(PositionWithAffinity)`, `EndOfLine(PositionWithAffinity)`, +`InSameLine(PositionWithAffinity, PositionWithAffinity)`. + +**Document:** `StartOfDocument(Position)`. + +Note: `EndOfSentence(const VisiblePosition&)` stays in `visible_units.h` - it is a +VisiblePosition overload, not a `Position`-based one. + +### `PositionWithAffinity` ownership rule + +A function belongs in `position_units.h` if it answers a "where is the boundary of +unit X?" question using DOM structure, regardless of whether it takes or returns +`PositionWithAffinity`. `PositionWithAffinity` is just a `Position` with an affinity +tag - it is not a layout type. + +A function stays in `visible_units.h` if it is a layout or hit-test utility that +happens to use `PositionWithAffinity` as a carrier type (e.g. +`AdjustForwardPositionToAvoidCrossingEditingBoundaries`, +`PositionForContentsPointRespectingEditingBoundary`). These are tied to layout +machinery, not navigation units. + +### Implementation strategy + +New overloads in `position_units.cc` call through to the existing private algorithm +templates in the originating files (e.g. `StartOfParagraphAlgorithm` +in `visible_units_paragraph.cc`). Algorithm templates are not moved. For functions +with no existing `Position`-typed algorithm, write the algorithm first and have both +the VisiblePosition overload and the new `Position` overload call into it. + +--- + +## Step 2: Migrate Simple Commands + +Migrate the 5 lowest-VisiblePosition command files. These validate that the new overloads are +drop-in replacements against the WPT editing test suite. + +| File | VisiblePosition refs | Notes | +|------|---------|-------| +| `style_commands.cc` | 1 | | +| `undo_step.cc` | 2 | | +| `editor_command.cc` | 6 | | +| `insert_line_break_command.cc` | 6 | P3 and P4 examples | +| `insert_text_command.cc` | 7 | | + +### Migration patterns + +| Before | After | +|---|---| +| `CreateVisiblePosition(pos).DeepEquivalent()` | `pos` - drop entirely; do not substitute `CanonicalPositionOf` | +| `IsEndOfParagraph(CreateVisiblePosition(pos))` | `IsEndOfParagraph(pos)` | +| `StartOfParagraph(vp).DeepEquivalent()` | `StartOfParagraph(pos)` | +| `VisiblePosition::FirstPositionInNode(*node).DeepEquivalent()` | `Position::FirstPositionInNode(*node)` | +| `VisiblePosition::LastPositionInNode(*node).DeepEquivalent()` | `Position::LastPositionInNode(*node)` | +| `LineBreakExistsAtVisiblePosition(vp)` | `LineBreakExistsAtPosition(pos)` | +| `VisiblePosition::BeforeNode(*node)` | `Position::BeforeNode(*node)` | +| `EndingVisibleSelection().VisibleStart().DeepEquivalent()` | `EndingSelection().Start()` | + +--- + +## Step 3: Infrastructure Signature Changes + +Change the five `CompositeEditCommand` protected methods from `const VisiblePosition&` +to `const Position&`. Method bodies migrate simultaneously using the Step 1 overloads. + +- `MoveParagraph`, `MoveParagraphs` - three params each +- `MoveParagraphWithClones` - two params +- `CleanupAfterDeletion` - one param with default +- `ReplaceCollapsibleWhitespaceWithNonBreakingSpaceIfNeeded` - one param + +All callers in `indent_outdent_command.cc`, `insert_list_command.cc`, +`delete_selection_command.cc`, `apply_block_element_command.cc`, +`format_block_command.cc` drop their `CreateVisiblePosition` wrappers and pass +`Position` directly. + +--- + +## Step 4: Heavy Command Migration + +With Step 1 overloads and Step 3 infrastructure in place, the heavy files fall into +two categories: + +- **Category A** (fixed by Step 3): VisiblePosition created solely to pass to `MoveParagraph` / `MoveParagraphWithClones` / `CleanupAfterDeletion`. +- **Category B** (fixed by Step 1): VisiblePosition created solely to call `IsEndOfParagraph`, `IsStartOfParagraph`, `StartOfParagraph`, `EndOfParagraph`, `StartOfBlock`, etc. + +| File | VisiblePosition refs | Primary category | +|------|---------|-----------------| +| `insert_paragraph_separator_command.cc` | 19 | B | +| `apply_block_element_command.cc` | 37 | B + A | +| `indent_outdent_command.cc` | 61 | A | +| `delete_selection_command.cc` | 64 | A + B | +| `insert_list_command.cc` | 91 | A + B | +| `replace_selection_command.cc` | 98 | B | + +`composite_edit_command.cc` (112 refs): Step 3 covers the method signatures. The +remaining VisiblePosition in `RebalanceWhitespaceOnTextSubstring`, +`MoveParagraphContentsToNewBlockIfNecessary`, `BreakOutOfEmptyListItem`, +`BreakOutOfEmptyMailBlockquotedParagraph` are addressed in Step 4. + +### Estimated VisiblePosition reduction + +| Work item | Estimated VisiblePositions removed | +|-----------|-----------------------| +| Simple command migration (Step 2) | ~25 | +| Infrastructure signature changes (Step 3) | ~80 | +| Heavy command migration (Step 4) | ~220 | +| **Total** | **~325 of ~391 (83%)** | + +--- + +## What is Legitimately VisiblePosition (Do Not Remove) + +1. **Entry boundary.** `CompositeEditCommand` constructor calling `ComputeVisibleSelectionInDOMTreeDeprecated()`. + +2. **Exit boundary.** `EndingVisibleSelection()` in `Apply()` for the `IsRichlyEditablePosition` check. + +3. **Visual-caret equivalence checks.** When the question is "did the caret actually move?", VisiblePosition equivalence is the right tool - some positions are structurally distinct but render to the same caret. `MostForwardCaretPosition(x) == MostForwardCaretPosition(y)` is often a layout-free replacement, but each site requires individual confirmation. + +4. **`VisiblePosition::FirstPositionInNode` / `LastPositionInNode` as equality tests.** Replaceable with `Position::` variants only after per-site confirmation that positions are canonical. + +--- + +## Testing Strategy + +**Per-CL:** +- `blink_unittests --gtest_filter="*Edit*"` - all pass +- `third_party/blink/web_tests/editing/` - no new failures +- `third_party/blink/web_tests/editing/contenteditable/` - no new failures + +**Per-step gate:** +- Full `blink_unittests` suite +- `content_browsertests` editing subset +- Verify `visible_position.h` include count in `commands/` is decreasing + +**Manual smoke test:** Basic typing, delete/backspace, paste, undo/redo, list creation, indent/outdent, Enter in list items, table editing. + +**Regression monitoring:** Watch CQ for editing test failures for 2 weeks after each step lands. + +--- + +## Next Steps: Beyond Commands + +- `core/editing/selection_adjuster.cc` - 28 VisiblePosition refs, primarily P3/P4 +- `core/editing/editing_utilities.cc` - ~12 VisiblePosition refs in internal helpers +- `core/editing/serializers/styled_markup_serializer.cc` - 7 VisiblePosition refs, P3/P4 +- `core/editing/editor.cc` - ~5 VisiblePosition refs in internal helpers + +Out of scope until Step 4 lands. +