diff --git a/.changeset/remote-children-replacement-selection.md b/.changeset/remote-children-replacement-selection.md new file mode 100644 index 0000000000..73e988bf9b --- /dev/null +++ b/.changeset/remote-children-replacement-selection.md @@ -0,0 +1,12 @@ +--- +'@portabletext/editor': patch +--- + +fix: preserve selections through remote children replacements that keep the text + +When a remote change replaces a block's children with re-keyed spans +(a collaborator toggling a mark that splits spans), the local cursor +and any other tracked selection no longer jump to the start of the +block. As long as the block's text is unchanged, selections keep +their exact textual position; replacements that change the text keep +the previous behavior. diff --git a/packages/editor/src/engine/point/transform-point.test.ts b/packages/editor/src/engine/point/transform-point.test.ts index a71e564c31..ac1fff645d 100644 --- a/packages/editor/src/engine/point/transform-point.test.ts +++ b/packages/editor/src/engine/point/transform-point.test.ts @@ -217,4 +217,309 @@ describe(transformPoint.name, () => { offset: 0, }) }) + + test('set children: remaps a point through a text-preserving replacement', () => { + const point = { + path: [{_key: 'b1'}, 'children', {_key: 's1'}], + offset: 5, + } + const op: EngineOperation = { + type: 'set', + path: [{_key: 'b1'}, 'children'], + value: [ + {_type: 'span', _key: 'n1', text: 'foo', marks: []}, + {_type: 'span', _key: 'n2', text: 'bar', marks: ['strong']}, + {_type: 'span', _key: 'n3', text: 'baz', marks: []}, + ], + inverse: { + type: 'set', + path: [{_key: 'b1'}, 'children'], + value: [{_type: 'span', _key: 's1', text: 'foobarbaz', marks: []}], + }, + } + expect(transformPoint(point, op)).toEqual({ + path: [{_key: 'b1'}, 'children', {_key: 'n2'}], + offset: 2, + }) + }) + + test('set children: a boundary offset lands at the end of the earlier span', () => { + const point = { + path: [{_key: 'b1'}, 'children', {_key: 's1'}], + offset: 3, + } + const op: EngineOperation = { + type: 'set', + path: [{_key: 'b1'}, 'children'], + value: [ + {_type: 'span', _key: 'n1', text: 'foo', marks: []}, + {_type: 'span', _key: 'n2', text: 'barbaz', marks: []}, + ], + inverse: { + type: 'set', + path: [{_key: 'b1'}, 'children'], + value: [{_type: 'span', _key: 's1', text: 'foobarbaz', marks: []}], + }, + } + expect(transformPoint(point, op)).toEqual({ + path: [{_key: 'b1'}, 'children', {_key: 'n1'}], + offset: 3, + }) + }) + + test('set children: inline objects occupy no text offset on either side', () => { + const point = { + path: [{_key: 'b1'}, 'children', {_key: 's2'}], + offset: 1, + } + const op: EngineOperation = { + type: 'set', + path: [{_key: 'b1'}, 'children'], + value: [ + {_type: 'span', _key: 'n1', text: 'foo', marks: []}, + {_type: 'stock-ticker', _key: 'n2'}, + {_type: 'span', _key: 'n3', text: 'bar', marks: []}, + ], + inverse: { + type: 'set', + path: [{_key: 'b1'}, 'children'], + value: [ + {_type: 'span', _key: 's1', text: 'foo', marks: []}, + {_type: 'stock-ticker', _key: 'o1'}, + {_type: 'span', _key: 's2', text: 'bar', marks: []}, + ], + }, + } + expect(transformPoint(point, op)).toEqual({ + path: [{_key: 'b1'}, 'children', {_key: 'n3'}], + offset: 1, + }) + }) + + test('set children: a surviving span keeps its identity over offset mapping', () => { + const point = { + path: [{_key: 'b1'}, 'children', {_key: 's1'}], + offset: 2, + } + const op: EngineOperation = { + type: 'set', + path: [{_key: 'b1'}, 'children'], + value: [ + {_type: 'span', _key: 'n1', text: 'bar', marks: []}, + {_type: 'span', _key: 's1', text: 'foo', marks: []}, + ], + inverse: { + type: 'set', + path: [{_key: 'b1'}, 'children'], + value: [ + {_type: 'span', _key: 's1', text: 'foo', marks: []}, + {_type: 'span', _key: 'o2', text: 'bar', marks: []}, + ], + }, + } + expect(transformPoint(point, op)).toBe(point) + }) + + test('set children: a text-changing replacement leaves the point untransformed', () => { + const point = { + path: [{_key: 'b1'}, 'children', {_key: 's1'}], + offset: 5, + } + const op: EngineOperation = { + type: 'set', + path: [{_key: 'b1'}, 'children'], + value: [{_type: 'span', _key: 'n1', text: 'hello', marks: []}], + inverse: { + type: 'set', + path: [{_key: 'b1'}, 'children'], + value: [{_type: 'span', _key: 's1', text: 'foobarbaz', marks: []}], + }, + } + expect(transformPoint(point, op)).toBe(point) + }) + + test('set children: no inverse leaves the point untransformed', () => { + const point = { + path: [{_key: 'b1'}, 'children', {_key: 's1'}], + offset: 5, + } + const op: EngineOperation = { + type: 'set', + path: [{_key: 'b1'}, 'children'], + value: [{_type: 'span', _key: 'n1', text: 'foobarbaz', marks: []}], + } + expect(transformPoint(point, op)).toBe(point) + }) + + test('set children: a point in another block is untouched', () => { + const point = { + path: [{_key: 'b2'}, 'children', {_key: 's9'}], + offset: 5, + } + const op: EngineOperation = { + type: 'set', + path: [{_key: 'b1'}, 'children'], + value: [{_type: 'span', _key: 'n1', text: 'foobarbaz', marks: []}], + inverse: { + type: 'set', + path: [{_key: 'b1'}, 'children'], + value: [{_type: 'span', _key: 's1', text: 'foobarbaz', marks: []}], + }, + } + expect(transformPoint(point, op)).toBe(point) + }) + + test('set children: container blocks in a field named `children` are not offset-mapped', () => { + // A container's array field may be named `children` and hold + // blocks. Blocks carry no `text`, so a point addressing a replaced + // block keeps the untransformed-point behavior. + const point = { + path: [{_key: 'callout1'}, 'children', {_key: 'block1'}], + offset: 0, + } + const op: EngineOperation = { + type: 'set', + path: [{_key: 'callout1'}, 'children'], + value: [ + { + _type: 'block', + _key: 'newBlock1', + children: [{_type: 'span', _key: 'n1', text: 'foo', marks: []}], + }, + ], + inverse: { + type: 'set', + path: [{_key: 'callout1'}, 'children'], + value: [ + { + _type: 'block', + _key: 'block1', + children: [{_type: 'span', _key: 's1', text: 'foo', marks: []}], + }, + ], + }, + } + expect(transformPoint(point, op)).toBe(point) + }) + + test('set children: a point deeper inside replaced container blocks is untouched', () => { + const point = { + path: [ + {_key: 'callout1'}, + 'children', + {_key: 'block1'}, + 'children', + {_key: 's1'}, + ], + offset: 2, + } + const op: EngineOperation = { + type: 'set', + path: [{_key: 'callout1'}, 'children'], + value: [ + { + _type: 'block', + _key: 'newBlock1', + children: [{_type: 'span', _key: 'n1', text: 'foo', marks: []}], + }, + ], + inverse: { + type: 'set', + path: [{_key: 'callout1'}, 'children'], + value: [ + { + _type: 'block', + _key: 'block1', + children: [{_type: 'span', _key: 's1', text: 'foo', marks: []}], + }, + ], + }, + } + expect(transformPoint(point, op)).toBe(point) + }) + + test('set children: remaps span points of a text block nested in a container', () => { + const point = { + path: [ + {_key: 'table1'}, + 'rows', + {_key: 'row1'}, + 'cells', + {_key: 'cell1'}, + 'content', + {_key: 'block1'}, + 'children', + {_key: 's1'}, + ], + offset: 5, + } + const op: EngineOperation = { + type: 'set', + path: [ + {_key: 'table1'}, + 'rows', + {_key: 'row1'}, + 'cells', + {_key: 'cell1'}, + 'content', + {_key: 'block1'}, + 'children', + ], + value: [ + {_type: 'span', _key: 'n1', text: 'foo', marks: []}, + {_type: 'span', _key: 'n2', text: 'bar', marks: ['strong']}, + {_type: 'span', _key: 'n3', text: 'baz', marks: []}, + ], + inverse: { + type: 'set', + path: [ + {_key: 'table1'}, + 'rows', + {_key: 'row1'}, + 'cells', + {_key: 'cell1'}, + 'content', + {_key: 'block1'}, + 'children', + ], + value: [{_type: 'span', _key: 's1', text: 'foobarbaz', marks: []}], + }, + } + expect(transformPoint(point, op)).toEqual({ + path: [ + {_key: 'table1'}, + 'rows', + {_key: 'row1'}, + 'cells', + {_key: 'cell1'}, + 'content', + {_key: 'block1'}, + 'children', + {_key: 'n2'}, + ], + offset: 2, + }) + }) + + test('set children: a raw block-offset point is never offset-mapped', () => { + // Selections can carry raw block-offset points ({path: [block], + // offset: N}) that resolve lazily. The children remap only applies + // to points addressing a direct child of the replaced array, so + // block-path points pass through untouched. + const point = { + path: [{_key: 'b1'}], + offset: 5, + } + const op: EngineOperation = { + type: 'set', + path: [{_key: 'b1'}, 'children'], + value: [{_type: 'span', _key: 'n1', text: 'foobarbaz', marks: []}], + inverse: { + type: 'set', + path: [{_key: 'b1'}, 'children'], + value: [{_type: 'span', _key: 's1', text: 'foobarbaz', marks: []}], + }, + } + expect(transformPoint(point, op)).toBe(point) + }) }) diff --git a/packages/editor/src/engine/point/transform-point.ts b/packages/editor/src/engine/point/transform-point.ts index 673b8ae158..a34713db07 100644 --- a/packages/editor/src/engine/point/transform-point.ts +++ b/packages/editor/src/engine/point/transform-point.ts @@ -1,4 +1,8 @@ import {pathContains} from '../../traversal/path-contains' +import { + childAtTextOffset, + textOffsetOfChild, +} from '../../utils/util.child-text-offset' import {isKeyedSegment} from '../../utils/util.is-keyed-segment' import type {EngineOperation} from '../interfaces/operation' import type {Path} from '../interfaces/path' @@ -98,6 +102,30 @@ export function transformPoint( } } + // When a node's children are replaced wholesale (the sync + // machine's fallback for re-keyed remote children), a point + // inside the old children references keys that no longer exist. + // When the replacement preserves the concatenated span text (a + // remote mark toggle: same characters, new span boundaries and + // keys), the point maps losslessly through its text offset + // within the node. Points whose span survived keep their + // identity, and text-changing replacements keep the + // untransformed point, offset mapping would be guesswork there. + if (propertyName === 'children' && Array.isArray(op.value)) { + const remappedPoint = remapPointThroughChildrenReplacement( + point, + op.path, + op.value as Array, + op.inverse?.type === 'set' && Array.isArray(op.inverse.value) + ? (op.inverse.value as Array) + : undefined, + ) + if (remappedPoint) { + path = remappedPoint.path + offset = remappedPoint.offset + } + } + if (path === point.path && offset === point.offset) { return point } @@ -134,3 +162,109 @@ export function transformPoint( return point } } + +/** + * Map a point through a wholesale children replacement via its text + * offset within the node. Returns `undefined` (leave the point + * untransformed) unless every precondition holds: the point addresses + * a direct child of the node whose children were replaced, that + * child's key is gone from the new children (identity did not + * survive), the old children are known (the op's inverse), and the + * old and new children carry identical concatenated span text, the + * condition that makes offset mapping lossless. + * + * Spans are detected structurally (`text` being a string) rather than + * through the schema: `transformPoint` is deliberately schema-free, + * and text-carrying children are the only ones that occupy offsets. + * + * The offset arithmetic is shared with the snapshot-aware block-offset + * utils through `utils/util.child-text-offset.ts`, so the boundary + * convention (an offset landing exactly on a span boundary stays at + * the end of the earlier span) is the same by construction. Only the + * span detection differs: structural here, schema-based there. + * + * A container's array field may also be named `children` and hold + * blocks rather than spans. The text requirement makes that a no-op by + * construction: blocks carry no `text` of their own, so a point + * addressing a replaced block bails at the offset walk, and points + * deeper inside such blocks fail the direct-child path guard. + */ +function remapPointThroughChildrenReplacement( + point: Point, + childrenPath: Path, + newChildren: Array, + oldChildren: Array | undefined, +): Point | undefined { + if (!oldChildren) { + return undefined + } + + if ( + point.path.length !== childrenPath.length + 1 || + !pathEquals(point.path.slice(0, childrenPath.length), childrenPath) + ) { + return undefined + } + + const pointSegment = point.path[point.path.length - 1] + if (!isKeyedSegment(pointSegment)) { + return undefined + } + + if (newChildren.some((child) => childKey(child) === pointSegment._key)) { + // The point's span survived the replacement; identity wins over + // offset mapping. + return undefined + } + + const nodeTextOffset = textOffsetOfChild( + oldChildren, + childText, + pointSegment._key, + point.offset, + ) + if (nodeTextOffset === undefined) { + return undefined + } + + if (concatenatedText(oldChildren) !== concatenatedText(newChildren)) { + return undefined + } + + const placed = childAtTextOffset(newChildren, childText, nodeTextOffset) + if (placed === undefined) { + return undefined + } + + return { + path: [...childrenPath, {_key: placed.key}], + offset: placed.offset, + } +} + +function childText(child: unknown): string | undefined { + if (child !== null && typeof child === 'object' && 'text' in child) { + const text = (child as {text: unknown}).text + return typeof text === 'string' ? text : undefined + } + return undefined +} + +function childKey(child: unknown): string | undefined { + if (child !== null && typeof child === 'object' && '_key' in child) { + const key = (child as {_key: unknown})._key + return typeof key === 'string' ? key : undefined + } + return undefined +} + +function concatenatedText(children: Array): string { + let text = '' + for (const child of children) { + const childTextValue = childText(child) + if (childTextValue !== undefined) { + text += childTextValue + } + } + return text +} diff --git a/packages/editor/src/utils/util.block-offset.ts b/packages/editor/src/utils/util.block-offset.ts index 90eaf48639..0c8a6e84cf 100644 --- a/packages/editor/src/utils/util.block-offset.ts +++ b/packages/editor/src/utils/util.block-offset.ts @@ -5,6 +5,7 @@ import type {TraversalSnapshot} from '../traversal/traversal-snapshot' import type {BlockOffset} from '../types/block-offset' import type {EditorSelectionPoint} from '../types/editor' import type {ChildPath} from '../types/paths' +import {childAtTextOffset, textOffsetOfChild} from './util.child-text-offset' import {isKeyedSegment} from './util.is-keyed-segment' /** @@ -28,29 +29,32 @@ export function blockOffsetToSpanSelectionPoint({ const block = blockEntry.node const blockPath = blockEntry.path + if (direction === 'forward') { + const placed = childAtTextOffset( + block.children, + (child) => + isSpan(snapshot.context, child as (typeof block.children)[number]) + ? (child as {text: string}).text + : undefined, + blockOffset.offset, + ) + return placed + ? { + path: [ + ...blockPath, + 'children', + {_key: placed.key}, + ] satisfies ChildPath, + offset: placed.offset, + } + : undefined + } + let offsetLeft = blockOffset.offset let selectionPoint: {path: ChildPath; offset: number} | undefined let skippedInlineObject = false for (const child of block.children) { - if (direction === 'forward') { - if (!isSpan(snapshot.context, child)) { - continue - } - - if (offsetLeft <= child.text.length) { - selectionPoint = { - path: [...blockPath, 'children', {_key: child._key}], - offset: offsetLeft, - } - break - } - - offsetLeft -= child.text.length - - continue - } - if (!isSpan(snapshot.context, child)) { skippedInlineObject = true continue @@ -110,22 +114,18 @@ export function spanSelectionPointToBlockOffset({ return undefined } - let offset = 0 - - for (const child of textBlock.node.children) { - if (!isSpan(snapshot.context, child)) { - continue - } - - if (child._key === spanSegment._key) { - return { - path: textBlock.path, - offset: offset + selectionPoint.offset, - } - } - - offset += child.text.length - } - - return undefined + const offset = textOffsetOfChild( + textBlock.node.children, + (child) => + isSpan( + snapshot.context, + child as (typeof textBlock.node.children)[number], + ) + ? (child as {text: string}).text + : undefined, + spanSegment._key, + selectionPoint.offset, + ) + + return offset === undefined ? undefined : {path: textBlock.path, offset} } diff --git a/packages/editor/src/utils/util.child-text-offset.ts b/packages/editor/src/utils/util.child-text-offset.ts new file mode 100644 index 0000000000..22dfdc770d --- /dev/null +++ b/packages/editor/src/utils/util.child-text-offset.ts @@ -0,0 +1,72 @@ +/** + * Pure text-offset arithmetic over a node's children array, shared by + * the snapshot-aware block-offset utils (`util.block-offset.ts`) and + * the schema-free point transform (`engine/point/transform-point.ts`). + * Span-ness is the caller's concern, expressed as an accessor that + * returns the child's text when the child occupies text offsets and + * `undefined` when it does not (inline objects, blocks). + */ + +/** + * The text offset of `(childKey, offsetInChild)` within `children`: + * the total text of the children before that child, plus the offset + * into it. `undefined` when the child is missing or carries no text. + */ +export function textOffsetOfChild( + children: ReadonlyArray, + getSpanText: (child: unknown) => string | undefined, + childKey: string, + offsetInChild: number, +): number | undefined { + let precedingTextLength = 0 + + for (const child of children) { + const text = getSpanText(child) + if (keyOf(child) === childKey) { + return text === undefined + ? undefined + : precedingTextLength + offsetInChild + } + if (text !== undefined) { + precedingTextLength += text.length + } + } + + return undefined +} + +/** + * The child and child-local offset at `textOffset` within `children`. + * Forward-boundary convention: an offset landing exactly on a span + * boundary stays at the end of the earlier span. `undefined` when the + * offset lies beyond the children's total text. + */ +export function childAtTextOffset( + children: ReadonlyArray, + getSpanText: (child: unknown) => string | undefined, + textOffset: number, +): {key: string; offset: number} | undefined { + let remainingOffset = textOffset + + for (const child of children) { + const text = getSpanText(child) + if (text === undefined) { + continue + } + if (remainingOffset <= text.length) { + const key = keyOf(child) + return key === undefined ? undefined : {key, offset: remainingOffset} + } + remainingOffset -= text.length + } + + return undefined +} + +function keyOf(child: unknown): string | undefined { + if (child !== null && typeof child === 'object' && '_key' in child) { + const key = (child as {_key: unknown})._key + return typeof key === 'string' ? key : undefined + } + return undefined +} diff --git a/packages/editor/tests/selection-after-remote-patches.test.tsx b/packages/editor/tests/selection-after-remote-patches.test.tsx index c82a100e9b..44a90bc6de 100644 --- a/packages/editor/tests/selection-after-remote-patches.test.tsx +++ b/packages/editor/tests/selection-after-remote-patches.test.tsx @@ -554,4 +554,133 @@ describe('Feature: Selection adjustment after remote patches', () => { expect(editor.getSnapshot().context.selection).toEqual(afterBarSelection) }) }) + + test('Scenario: Remote mark toggle with re-keyed spans preserves the caret', async () => { + const keyGenerator = createTestKeyGenerator() + const b1 = keyGenerator() + const s1 = keyGenerator() + + const initialValue = [ + { + _type: 'block', + _key: b1, + children: [{_type: 'span', _key: s1, text: 'foobarbaz', marks: []}], + markDefs: [], + style: 'normal', + }, + ] + + const {editor, locator} = await createTestEditor({ + keyGenerator, + schemaDefinition: defineSchema({ + decorators: [{name: 'strong'}], + }), + initialValue, + }) + + await userEvent.click(locator) + await whenTheCaretIsPutAfter(editor, 'foobarbaz') + + const newSpanFoo = keyGenerator() + const newSpanBar = keyGenerator() + const newSpanBaz = keyGenerator() + + editor.send({ + type: 'update value', + value: [ + { + _type: 'block', + _key: b1, + children: [ + {_type: 'span', _key: newSpanFoo, text: 'foo', marks: []}, + {_type: 'span', _key: newSpanBar, text: 'bar', marks: ['strong']}, + {_type: 'span', _key: newSpanBaz, text: 'baz', marks: []}, + ], + markDefs: [], + style: 'normal', + }, + ], + }) + + await vi.waitFor(() => { + expect(editor.getSnapshot().context.value[0]).toEqual({ + _type: 'block', + _key: b1, + children: [ + {_type: 'span', _key: newSpanFoo, text: 'foo', marks: []}, + {_type: 'span', _key: newSpanBar, text: 'bar', marks: ['strong']}, + {_type: 'span', _key: newSpanBaz, text: 'baz', marks: []}, + ], + markDefs: [], + style: 'normal', + }) + }) + + await vi.waitFor(() => { + const expectedSelection = getSelectionAfterText( + editor.getSnapshot().context, + 'baz', + ) + expect(editor.getSnapshot().context.selection).toEqual(expectedSelection) + }) + }) + + test('Scenario: Remote children replacement with different text keeps the current fallback', async () => { + const keyGenerator = createTestKeyGenerator() + const b1 = keyGenerator() + const s1 = keyGenerator() + + const {editor, locator} = await createTestEditor({ + keyGenerator, + schemaDefinition: defineSchema({decorators: [{name: 'strong'}]}), + initialValue: [ + { + _type: 'block', + _key: b1, + children: [{_type: 'span', _key: s1, text: 'foobarbaz', marks: []}], + markDefs: [], + style: 'normal', + }, + ], + }) + + await userEvent.click(locator) + await whenTheCaretIsPutAfter(editor, 'foobarbaz') + + const newSpan = keyGenerator() + + editor.send({ + type: 'update value', + value: [ + { + _type: 'block', + _key: b1, + children: [{_type: 'span', _key: newSpan, text: 'hello', marks: []}], + markDefs: [], + style: 'normal', + }, + ], + }) + + await vi.waitFor(() => { + const children = ( + editor.getSnapshot().context.value[0] as { + children: Array<{text?: string}> + } + ).children + expect(children[0]?.text).toBe('hello') + }) + + // Text changed, so offset mapping would be guesswork; the selection + // keeps the pre-existing fallback (start of the replaced children). + await vi.waitFor(() => { + const selection = editor.getSnapshot().context.selection + expect(selection?.focus.path).toEqual([ + {_key: b1}, + 'children', + {_key: newSpan}, + ]) + expect(selection?.focus.offset).toBe(0) + }) + }) })