Fix incorrect surround behaviour next to <br> tag

[ajweeks]

When applying bold to text that sits immediately after an existing <b>…</b> which contains a <br>, the formatting merge logic could incorrectly move the <br> out of the bold element.

Example minimal reproduction steps

Input HTML:

<b><i>1</i> 2<br>ABC</b>3

Action:

• Select "3"
• Apply bold

Actual output:

<b><i>1</i> 2</b><br>ABC3

Expected output:

<b><i>1</i> 2<br>ABC3</b>

High-level explanation

The surround algorithm builds a formatting tree, may unwrap/remove matching elements (like existing <b>), and then re-applies formatting using FlatRange sibling indices.

Previously, when no matching ancestor was found, we sometimes started the build/apply process from the selection’s text node (range.commonAncestorContainer). During tree building, removing/unwrapping adjacent formatting elements can change sibling structure, making those precomputed sibling indices effectively stale and causing the final wrap to exclude the <br> portion.

Change: When the computed “correct node” is a text/comment node, we now build/apply starting from its parent node instead. This keeps the build/apply operations anchored to a stable container and preserves <br> inside the merged bold.

The root cause of this issue appears to be that the surround algorithm can reference stale sibling indices caused build-tree's structural splices (replaceWith(...children)). The apply step only compensates for the splice effect of surroundContents, not for the earlier splices. The fix authored here works by anchoring the operation to a stable container by starting from the parent element when the chosen build node is a text/comment node.

---

Note: This fix was discovered and authored primarily by GPT-5.2, but initiated, reviewed, and tested by me. When asked about the deeper issue it provided some ideas for solutions, which I'm happy to get it to generate if there is interest. The ideas are:

• Option A: Make build-phase removals inform the shifting model by calling build.announceElementRemoval(element) before element.replaceWith(...) and extending SplitRange and/or FlatRange shifting to track removals that happen before apply.
• Option B: Reduce reliance on sibling indices. Move away from FlatRange(parent,index,index) and toward something anchored to actual nodes: i.e. store boundary nodes (startNode, endNode) and re-derive indices late, or store Range boundary points more directly (but you’d need to keep it “surroundContents-safe”).

[abdnh]

I'm worried about regressions here. Would be nice to fix the root cause, but that might require a deep understanding of the surrounding logic.

[ajweeks]

I let GPT run for an hour or two and put together this fix of the deeper issue. I looked it over and it seems mostly reasonable, but it could definitely use a thorough review from someone who knows the codebase. It works for the cases I've tested and I haven't found any regressions, though more testing is warranted.

Here is its report:

---

PR: Node-anchored FlatRange for Surround (remove index shifting; add regression tests)

Summary

This change replaces the index-based FlatRange used by the surround formatting pipeline with a node-anchored representation and removes apply-time index shifting. Instead of persisting (parent, startIndex, endIndex) (which becomes stale when the DOM is mutated during build/apply), FlatRange now stores boundary anchor nodes (start/end, inclusive) plus a parent container, and derives startIndex/endIndex on demand. This makes the pipeline robust against DOM mutations like element unwrapping (replaceWith(...)) and formatting operations that restructure siblings.

Why this change was made

The surround pipeline performs DOM mutations during:

• Build: matching elements can be removed/“unwrapped” via element.replaceWith(...element.childNodes).
• Apply: formatting typically uses Range.surroundContents() (or custom formatters do similar work).

When FlatRange is index-based, any of those mutations can invalidate stored sibling indices, leading to subtle mis-selection or IndexSizeError/incorrect merging behavior. The most visible manifestation was the historical “BR moved out of bold” class of bugs, which was traced back to stale range boundaries.

By anchoring FlatRange to actual nodes rather than indices, we avoid having to “repair” indices as the DOM changes and can remove the complex apply-time shifting logic entirely.

Implementation details

1) FlatRange is now node-anchored

File: ts/lib/domlib/surround/flat-range.ts

• Before: stored parent, startIndex, endIndex as mutable numbers.
• After: stores:
  • parent: Node
  • start: Node (inclusive anchor)
  • end: Node (inclusive anchor)

Derived boundary behavior:

• firstChild/lastChild are computed by “rebasing” each anchor upward until it is a direct child of parent.
• startIndex/endIndex are now getters, computed via indexOf(parent.childNodes, firstChild/lastChild).

This keeps the external FlatRange API largely intact (consumers can still call toDOMRange(), access firstChild/lastChild, and use startIndex/endIndex when needed), but prevents mutation of indices.

2) Remove apply-time shifting

File: ts/lib/domlib/surround/apply/index.ts

• Before: apply traversal maintained leftShift/innerShift and mutated node.range.startIndex/endIndex to compensate for inserted wrappers.
• After: apply traversal is a straightforward post-order walk:
  • recurse into children
  • call format.applyFormat(node) for each FormattingNode

Because FlatRange indices are now derived, there is no longer any valid “shift” operation to apply.

3) Adjust unwrap rebasing hook usage

File: ts/lib/domlib/surround/build/build-tree.ts

• The unwrap path continues to call child.range.rebaseIfParentIs(...), but the method signature no longer accepts the indexInNewParent argument (it’s not needed with node anchors).

How it works end-to-end

1. The surround pipeline builds a formatting tree from DOM nodes.
2. Nodes to be formatted are represented as FormattingNodes with a FlatRange.
3. During build, matching wrappers may be removed/unwrapped, which shifts siblings and would previously stale indices.
4. With node anchors, FlatRange can always re-derive the correct boundary indices from the current DOM at the time toDOMRange() is invoked.
5. During apply, we format in post-order; each formatter creates a DOM Range via FlatRange.toDOMRange() and applies changes (usually surroundContents).

Potential concerns / risks

• Anchor detachment edge cases: If an anchor node is removed from the DOM entirely (not merely unwrapped/moved upward), FlatRange currently throws an error when it can’t rebase the anchor into parent. The current surround pipeline generally moves nodes via unwrap rather than deleting the boundary text nodes, so this should be rare, but it’s worth noting.
• Performance: startIndex/endIndex are derived via indexOf over parent.childNodes. These ranges are typically small and localized, but it is more work than cached indices. If this becomes hot, we could consider caching per materialization or only computing indices once per toDOMRange() call.
• Assumption about parent rebasing: FlatRange assumes anchors remain descendants of its parent and can be walked upward to a direct child. This matches how the algorithm constructs ranges today (anchored to text nodes and ascended elements) but should be kept in mind if future refactors start anchoring to nodes that are not stable descendants of the original parent.

[abdnh]

Will keep this until someone has the chance to dive into the surround logic.