Repository navigation
fix(slate-react): keep the DOM in sync when a native insertText is a no-op - #6084
a-y-ibrahim wants to merge 4 commits into
Conversation
🦋 Changeset detectedLatest commit: e410a17 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@12joan let me know if you have opinions on this one. |
|
@dylans Sure! I'll take a look later today. |
12joan
left a comment
There was a problem hiding this comment.
Nice work! The fix itself seems pretty robust. I tried changing the example to insert a different character to that which was typed, or to insert the character elsewhere in the text, and everything I tried behaved correctly.
I've left a few suggestions, mainly to improve code quality.
| ['Insert Text Noop', 'insert-text-noop'], | ||
| ['Insert Text Noop Decorated', 'insert-text-noop-decorated'], |
There was a problem hiding this comment.
Perhaps we should add these to HIDDEN_EXAMPLES, since they might confusing to people seeing them in the sidebar without explanation?
|
|
||
| const InsertTextNoopDecoratedExample = () => { | ||
| const editor = useMemo(() => withNoUppercase(withReact(createEditor())), []) | ||
| const decorateCallback = useCallback(decorate, []) |
There was a problem hiding this comment.
There's no need to use useCallback to wrap a constant function. It's only necessary for functions that are recreated as part of the render function.
There was a problem hiding this comment.
Would it do any harm to combine these into a single example where decoration is always enabled? At the very least, it would be good to combine the two Playwright tests since they're so similar.
| // COMPAT: Tracks a text node that just received a native | ||
| // (non-preventDefault'd) character insertion, so we can verify - once | ||
| // the deferred `Editor.insertText` has been flushed - that Slate's | ||
| // document actually changed. If a custom `insertText` ignored the | ||
| // character (see https://github.com/ianstormtaylor/slate/issues/5152), | ||
| // no Slate operation is applied, so no re-render happens to correct the | ||
| // DOM via <TextString>'s layout effect; we correct it manually instead, | ||
| // by undoing the browser's own single-character insertion at the exact | ||
| // DOM position it happened. This is leaf-agnostic (works regardless of | ||
| // how many marks/decorations split the text node into separate spans), | ||
| // since `domNode`/`domOffset` (from `ReactEditor.toDOMPoint`) already | ||
| // resolve to the specific leaf span the insertion landed in. |
There was a problem hiding this comment.
Having some explanation of this is good since it's pretty unintuitive otherwise, but please could you make this comment a bit more concise so that it pertains only to the lastNativeInsertion ref itself?
Also, I'm not sure COMPAT is quite right. It looks like it's used in a few existing comments with no clear pattern, but ideally I would expect it to appear only on code addressing browser-specific quirks (compatibility issues).
| const { path } = selection.anchor | ||
| const [node] = Editor.node(editor, path) | ||
|
|
||
| if (Text.isText(node)) { |
There was a problem hiding this comment.
How about const [node, path] = Editor.leaf(editor, selection.anchor)?
This removes the need to pull out the path and node separately, or to check that node is a text node. The path of a point should always refer to a text node, but if not (such as if the selection is invalid), the error thrown by Editor.leaf will be caught by the try...catch anyway.
There was a problem hiding this comment.
Or, if possible, it might be better to avoid the need for a try...catch entirely.
| const [domNode, domOffset] = ReactEditor.toDOMPoint( | ||
| editor, | ||
| selection.anchor | ||
| ) as [DOMText, number] |
There was a problem hiding this comment.
Adding an if (domNode instanceof DOMText) might be safer than a type assertion here.
| // COMPAT: If a native insertion's deferred `Editor.insertText` | ||
| // turned out to be a no-op (e.g. a custom `insertText` ignored | ||
| // the character), the browser has already mutated the DOM, but | ||
| // since Slate's document didn't change, no re-render happens to | ||
| // correct it via <TextString>'s layout effect. Undo the | ||
| // browser's own single-character insertion directly, at the | ||
| // exact DOM position it happened - this works regardless of | ||
| // how many leaves (marks, decorations) the surrounding text | ||
| // node is split into, since `domNode`/`domOffset` already | ||
| // identify the specific leaf span the insertion landed in. | ||
| // https://github.com/ianstormtaylor/slate/issues/5152 |
There was a problem hiding this comment.
| // COMPAT: If a native insertion's deferred `Editor.insertText` | |
| // turned out to be a no-op (e.g. a custom `insertText` ignored | |
| // the character), the browser has already mutated the DOM, but | |
| // since Slate's document didn't change, no re-render happens to | |
| // correct it via <TextString>'s layout effect. Undo the | |
| // browser's own single-character insertion directly, at the | |
| // exact DOM position it happened - this works regardless of | |
| // how many leaves (marks, decorations) the surrounding text | |
| // node is split into, since `domNode`/`domOffset` already | |
| // identify the specific leaf span the insertion landed in. | |
| // https://github.com/ianstormtaylor/slate/issues/5152 | |
| // If a native insertion's deferred `Editor.insertText` did | |
| // nothing, undo the browser's native insertion to remove the | |
| // character from the DOM. |
|
|
||
| if (nativeInsertion) { | ||
| try { | ||
| const [node] = Editor.node(editor, nativeInsertion.path) |
There was a problem hiding this comment.
Node.get (or Node.getIf to avoid throwing an error, or Node.leaf to check if it's a text node) returns a node rather than a node entry, which would avoid the need to destructure it here.
| } catch { | ||
| // The path may no longer point to a valid node (e.g. | ||
| // it was affected by some other operation) - nothing | ||
| // to correct in that case. | ||
| } |
There was a problem hiding this comment.
We might not need a try...catch if Node.getIf is used instead of Editor.node.
| const [node] = Editor.node(editor, path) | ||
|
|
||
| if (Text.isText(node)) { | ||
| const [domNode, domOffset] = ReactEditor.toDOMPoint( |
There was a problem hiding this comment.
- this runs
toDOMPointeven whenIS_NODE_MAP_DIRTYis set - the gate above skips its own
toDOMPointin that case, since the anchor can't be trusted - the gate already computes
[node, offset]for this same anchor when the map is clean - reuse that point, and skip the capture when the map is dirty, instead of a second lookup in a try/catch
| }) => { | ||
| const textbox = page.getByRole('textbox') | ||
| await textbox.click() | ||
| await page.keyboard.press('End') |
There was a problem hiding this comment.
- both tests type at End, inside the undecorated leaf
- at offset 3,
toDOMPointreturns the end of the highlighted leaf - the browser caret can sit at the start of the next leaf's text node instead
- then the
charAt(domOffset)check fails and the uppercase letter stays in the DOM - does it hold if you type right after the highlight?
…rmtaylor#5152) Single a-z/space character insertion is handled natively for performance: preventDefault is skipped so the browser inserts the character directly, and the corresponding Editor.insertText call is deferred until the following input event. If a custom insertText override ignores the character (e.g. to disallow uppercase), no Slate operation gets applied, so no re-render happens to reconcile the DOM via <TextString>'s layout effect - the native insertion's character was left stranded in the DOM even though the Slate document (correctly) didn't change. Track the path of a native insertion at defer time, and after flushing deferred operations, verify the affected text node's DOM content against the model, correcting it if they've diverged. Restore the caret afterward, since replacing textContent resets it to the start of the (recreated) text node. Extracted <TextString>'s inline text-content computation into a shared getLeafDomText helper, reused by both the normal reconciliation path and this new correction path, rather than duplicating the logic. Added a demo (insert-text-noop) and Playwright regression test reproducing the exact bug from the issue, verified fail-before/pass after against the real built package. Full existing test suites (playwright, jest) pass with no regressions.
The previous commit only corrected a text node split into a single `[data-slate-string]` span, bailing out whenever marks or decorations split it into multiple leaves. Decorations aren't checked by the native-insertion gate at all (only `editor.marks` is), so a decorated text node could still hit the native fast path and leave a stray character stuck in the DOM with no correction applied. Replace the whole-leaf textContent comparison with a surgical fix: capture the exact DOM text node/offset the browser is about to insert into, and if the deferred `Editor.insertText` left the Slate model unchanged, delete just that one character at that exact position. This is leaf-agnostic by construction, since `ReactEditor.toDOMPoint` already resolves to the specific leaf span a given Slate point falls into, decorations included. It also drops the need to reconstruct each leaf's expected text, so `string.tsx` reverts to its original, unexported form. Also confirmed (by rebuilding from the pre-fix commit and re-testing) that Android's separate AndroidInputManager input path bypasses this correction entirely and has the same pre-existing gap independent of this change - out of scope here, called out in the changeset.
…ianstormtaylor#5152 Verifies the single-slot lastNativeInsertion ref doesn't lose track of an earlier swallowed character when several native insertions happen back to back - each keystroke's beforeinput/input cycle completes synchronously before the next one starts, so this isn't an actual race, but it wasn't exercised by the existing tests.
Description
When an
insertTextoverride rejects a native insertion, the browser can retain a character that never entered the Slate value. Remove that native insertion from its actual DOM text node when the model is unchanged. Capture the browser's target range, or its selection when no target range is available, so the correction reaches the right leaf at a decoration boundary.The fix reuses the existing resolved DOM point, skips capture when the node map is dirty, and uses
Node.getIfinstead of exception handling. It checks the expected DOM edit before undoing it and preserves the caret when the Slate selection remains at the insertion point.Consolidated the two regression pages and test files into one hidden decorated example, removed redundant callbacks and comments, and generated its JavaScript version. The original commits by @a-y-ibrahim are preserved.
Issue
Fixes #5010
Example
Verified behavior :
Xtyped intoabcdefabcdefXwhile the model remainsabcdefabcdefabcXdefin FirefoxChecks
f399a7cafails both no-op DOM reconciliation tests. The contributor implementation fails the event-level boundary and dirty-map tests, plus the real Firefox boundary test. All three unit tests pass with the revision.check.shexited 0 on Node 24: 1,468 Mocha tests, 144 Jest tests, build, TypeScript, ESLint, and Prettier. Playwright TypeScript also passes separately.3242334bearlier in this session; all new browser regressions pass. The markdown-shortcuts failure is a test-timing flake (a click processed after the keys that follow it), fixed separately in Make the markdown-shortcuts and shadow-dom editing tests deterministic #6205.slate-reactpatch changeset.Android's pre-existing rejected-insertion reconciliation gap is outside this desktop native-input correction.