diff --git a/entry_types/scrolled/package/src/editor/models/ScrolledEntry/Batch.js b/entry_types/scrolled/package/src/editor/models/ScrolledEntry/Batch.js index 95fd57f44e..f31eb3bb26 100644 --- a/entry_types/scrolled/package/src/editor/models/ScrolledEntry/Batch.js +++ b/entry_types/scrolled/package/src/editor/models/ScrolledEntry/Batch.js @@ -314,18 +314,28 @@ export function Batch(entry, section, {reviewSession} = {}) { }, success(response) { - applyAdditions(response); - applyPositions(); - applyDeletions(); + // Each step's intermediate state is observable in the iframe + // (Backbone change events propagate as separate postMessages + // and re-render React without batching), so the order + // resolves these dependencies: + // + // - permaIds before thread updates: thread migrations + // resolve `target.get('permaId')`. + // - Thread updates before configuration changes: the + // value-flip render in `useCachedValue` clears rangeRefs + // and falls back to `thread.subjectRange`, which must + // already be migrated. + // - Positions before additions: Backbone's collection + // comparator drops new elements into their sorted slot. + // - Configuration changes before deletions: a delete-merge + // survivor grows before its sibling disappears (no + // brief collapse). + applyPermaIds(response); applyThreadUpdates(); - // Apply configuration changes last so that the value flip - // observed by `useCachedValue` happens after `reviewSession` - // already holds the migrated thread ranges. `onReset` then - // clears `useCommentRangeRefs`'s map and the post-render - // effect rebuilds it from the new thread subject ranges - // against the new editor content — no reliance on - // `isValidRange` filtering out stale OLD ranges. applyConfigurationChanges(); + applyPositions(); + applyAdditions(); + applyDeletions(); section.contentElements.sort(); @@ -384,20 +394,26 @@ export function Batch(entry, section, {reviewSession} = {}) { // Functionality to apply the recorded changes to the underlying // section once the request succeeded: - function applyAdditions(response) { + function applyPermaIds(response) { contentElements.forEach((contentElement, index) => { if (contentElement.isNew()) { - section.contentElements.add(contentElement); - contentElement.set({ id: response[index].id, permaId: response[index].permaId }); } - else if (contentElement.section !== section) { + }); + } + + function applyAdditions() { + contentElements.forEach(contentElement => { + if (contentElement.section && contentElement.section !== section) { contentElement.section.contentElements.remove(contentElement); section.contentElements.add(contentElement); } + else if (!section.contentElements.contains(contentElement)) { + section.contentElements.add(contentElement); + } }); } diff --git a/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/BadgeColumn.js b/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/BadgeColumn.js index ac721fd1fd..97aec0b437 100644 --- a/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/BadgeColumn.js +++ b/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/BadgeColumn.js @@ -1,7 +1,7 @@ import React from 'react'; import {FloatingPortal} from '@floating-ui/react'; -import {useSlate} from 'slate-react'; +import {useSlate, ReactEditor} from 'slate-react'; import {Badge, useAnchoredFloating} from 'pageflow-scrolled/review'; import {useFloatingPortalRoot} from '../../FloatingPortalRootProvider'; @@ -11,11 +11,17 @@ import {highlightOverlapsSelection} from './highlightOverlapsSelection'; export function BadgeColumn({highlights, anchors}) { const editor = useSlate(); + // Treat `editor.selection` as a live cursor only while the editor + // is focused. After the user clicks away, slate-react's throttled + // `selectionchange` listener can sync a clamped DOM cursor back + // into `editor.selection`, which would otherwise flip badges back + // to overlap mode without any actual selection. + const editorSelection = ReactEditor.isFocused(editor) ? editor.selection : null; return highlights.map(highlight => ( )); } diff --git a/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/Selection.js b/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/Selection.js index c8e68b25db..8e73974995 100644 --- a/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/Selection.js +++ b/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/Selection.js @@ -78,6 +78,17 @@ export function Selection(props) { } if (!isContentElementSelected && !boundsRef.current) { + // Only treat the selection as user-initiated when the editor is + // actually focused. After an external value replacement, the + // browser may clamp its DOM cursor to the start of the + // shrunken contenteditable; slate-react's throttled + // `selectionchange` listener then re-syncs that cursor into + // `editor.selection`. Without this guard, that synthetic + // selection would re-select the content element here. + if (!ReactEditor.isFocused(editor)) { + return; + } + select(); } diff --git a/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/index.js b/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/index.js index 186ae80787..7273ca8f58 100644 --- a/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/index.js +++ b/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/index.js @@ -221,6 +221,19 @@ function resetSelectionIfOutsideNextValue(editor, nextValue) { if (editor.selection && (!hasTextAtPoint(nextEditor, editor.selection.anchor) || !hasTextAtPoint(nextEditor, editor.selection.focus))) { Transforms.deselect(editor); + // Also drop the browser's DOM cursor when it sits inside this + // editor's contenteditable. Otherwise slate-react's throttled + // `selectionchange` listener would later re-sync that stale + // cursor — clamped to the start of the now-shrunken content — + // back into `editor.selection`, and `Selection`'s auto-select + // would treat it as a user click and re-select this content + // element (overriding whatever the calling op selected, e.g. a + // freshly inserted image). + const domSelection = window.getSelection(); + const editorEl = ReactEditor.toDOMNode(editor, editor); + if (domSelection && editorEl.contains(domSelection.anchorNode)) { + domSelection.removeAllRanges(); + } } } diff --git a/entry_types/scrolled/package/src/frontend/inlineEditing/useCachedValue.js b/entry_types/scrolled/package/src/frontend/inlineEditing/useCachedValue.js index 72f855253e..a952e4b28a 100644 --- a/entry_types/scrolled/package/src/frontend/inlineEditing/useCachedValue.js +++ b/entry_types/scrolled/package/src/frontend/inlineEditing/useCachedValue.js @@ -7,26 +7,13 @@ export function useCachedValue(value, { const [cachedValue, setCachedValue] = useState(value || defaultValue); const previousValue = useRef(value); - // Adjust state during render: when the upstream value changes, - // call `onReset` and flip `cachedValue` immediately so the first - // committed render after the change already observes the new - // value. React detects the in-render setState and re-runs the - // component before commit, so no extra render lands on the - // screen — and `onReset` side effects are visible to the same - // render that observes the new `cachedValue`. - if (previousValue.current !== value && value !== cachedValue) { - onReset && onReset(value); - setCachedValue(value); - previousValue.current = value; - } + useEffect(() => { + if (previousValue.current !== value && value !== cachedValue) { + onReset && onReset(value); + setCachedValue(value); + } + }, [onReset, value, cachedValue]); - // Track the upstream value across renders. The branch above only - // assigns when an external change is observed; without this effect, - // a render that sees `value` already in sync with `cachedValue` (a - // local edit echoed back via `onDebouncedChange`) would leave - // `previousValue.current` stale. The next local edit would then - // misread that stale value as an external change and overwrite the - // edit with `onReset`. useEffect(() => { previousValue.current = value; });