Fix issue with ReactEditor.focus + tests #5527

skogsmaskin · 2023-10-19T13:37:48Z

Description
This will make sure we don't try to create a DOM range while the editor is still applying operations in the ReactEditor.focus function. And that it retries doing this until this the editor has flushed all it's changes.

If a client requests ReactEditor.focus from the UI, while the editor is updating the DOM, this can result in unrecoverable errors like: Error: Cannot resolve a DOM node from Slate node: {"text":"hello"}

This has been reported by several people since #5516 was merged (see discussion there).

Have a look a these changes

I have made tests for this function, and in order to properly test it I replaced react-test-renderer with @testing-library/react which has a very similar API, but fully supports window. This means we can test code that constructs DOM ranges against the actual window.getSelection API.

This test will fail without my refactored code and provoke the above-mentioned error.

Issue
Fixes: see feedback on #5516

Example
See the new react-editor.spec.tsx tests

Context
See #5516

Checks

The new code matches the existing patterns and styles.
The tests pass with yarn test.
The linter passes with yarn lint. (Fix errors with yarn fix.)
The relevant examples still work. (Run examples with yarn start.)
You've added a changeset if changing functionality. (Add one with yarn changeset add.)

changeset-bot · 2023-10-19T13:37:52Z

🦋 Changeset detected

Latest commit: 0b01f5a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
slate-react	Minor

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

skogsmaskin · 2023-10-19T13:56:32Z

packages/slate-react/src/plugin/react-editor.ts

+        editor,
+        setTimeout(() => {
+          // Flush changes before trying to focus again
+          editor.onChange()


Not sure if we should actually call onChange here, or just retry until there are no operations pending.

onChange should already have been called by this point, right? I can't think of any situations where manually flushing the changes would be beneficial, and it might result in some weird edge cases. If, somehow, there are still pending operations after the timeout, retrying is probably safest.

Do we still need a weak map if we're not calling onChange? I can't think of any adverse side effects of running the timeout callback twice if a consumer happens to call ReactEditor.focus twice.

The only other thing that occurs to me is what happens if something goes wrong and editor.operations never gets emptied? That might happen if we call ReactEditor.focus after the editor has been unmounted, for example.

Perhaps we should add an options: { retries?: number } argument that defaults to 5 and is decremented on each attempt?

@12joan - thank you for your comments. I've been looking a bit closer at this now and I'm unsure if this is actually the right fix, though it seems to help in some cases.

EDIT: I'm currently working on setting up some tests for this in the mono repo. Will come back with something later.

onChange should already have been called by this point, right? I can't think of any situations where manually flushing the changes would be beneficial, and it might result in some weird edge cases. If, somehow, there are still pending operations after the timeout, retrying is probably safest.

Yes, I have refactored this now. It will now call onChange, but only because it will try to set a selection in the top of the document if the editor currently isn't selected.

Do we still need a weak map if we're not calling onChange? I can't think of any adverse side effects of running the timeout callback twice if a consumer happens to call ReactEditor.focus twice.

I agree. I have removed it - also one less WeakMap :)

The only other thing that occurs to me is what happens if something goes wrong and editor.operations never gets emptied? That might happen if we call ReactEditor.focus after the editor has been unmounted, for example.
Perhaps we should add an options: { retries?: number } argument that defaults to 5 and is decremented on each attempt?

Yeah, good thinking. I have implemented a retry (default 5) and then throw an error if the editor is still stuck with operations.

This will make sure we don't try to focus the editor while it's in the midst of applying operations. If this is the case, retry setting focus in the next tick.

We need to be able to test against window features, like the DOM selection. @testing-library/react has a very similar API, but have also these features, which react-test-renderer is missing.

This will rewrite the existing tests for Editable and move them into a own file.

skogsmaskin · 2023-11-07T22:47:36Z

I have updated this PR quite a bit today. Please have another look.

I can make a separate PR on the test upgrades if you'd prefer. But I prefer to have the tests together in this PR to illustrate the problem. If not pulled out, this PR should probably be rebased on top of main to keep those commits separate from the actual bug fix.

skogsmaskin · 2023-11-08T08:58:11Z

packages/slate-react/src/plugin/react-editor.ts

+  focus: (editor, options = { retries: 5 }) => {
+    // Return if already focused
+    if (IS_FOCUSED.get(editor)) {
+      return


This makes logical sense, but in real life maybe it's safer to just always run the code and re-set it to true?

skogsmaskin · 2023-11-08T09:00:21Z

packages/slate-react/src/plugin/react-editor.ts

      el.focus({ preventScroll: true })
+      IS_FOCUSED.set(editor, true)


Moved this to the end to ensure that we were able to set everything properly first.

skogsmaskin · 2023-11-08T09:01:33Z

packages/slate-react/src/plugin/react-editor.ts

+      // Create a new selection in the top of the document if missing
+      if (!editor.selection) {
+        Transforms.select(editor, Editor.start(editor, []))
+        editor.onChange()
+      }


This seems like it's missing. Added it as the tests showed that the editor would be stuck with a null selection after focus was set.

skogsmaskin · 2023-11-08T09:02:28Z

packages/slate-react/test/react-editor.spec.tsx

+        await act(async () => {
+          Transforms.removeNodes(editor, { at: [0] })
+          Transforms.insertNodes(editor, propagatedValue)
+          ReactEditor.focus(editor) // Note: calling focus in the middle of these transformations.


This focus call here will provoke that Error: Cannot resolve a DOM node from Slate node: {"text":"hello"}

This was a workaround for missing functionality in slate-react which is now fixed upstream ianstormtaylor/slate#5527

* chore(portable-text-editor): upgrade slate and slate-react to latest * refactor(portable-text-editor): remove workaround This was a workaround for missing functionality in slate-react which is now fixed upstream ianstormtaylor/slate#5527 * fix(core/form/inputs): remove workaround This is not needed anymore as it's fixed upstream. * feat(portable-text-editor): create a selection on focus if it doesn't exist

skogsmaskin mentioned this pull request Oct 19, 2023

Retain editor selection when using ReactEditor.focus() #5516

Merged

5 tasks

skogsmaskin commented Oct 19, 2023

View reviewed changes

skogsmaskin added 4 commits November 7, 2023 23:10

Fix issue with slate-react static ReactEditor.focus method

afb11f2

This will make sure we don't try to focus the editor while it's in the midst of applying operations. If this is the case, retry setting focus in the next tick.

Replace react-test-renderer with @testing-library/react

59133ff

We need to be able to test against window features, like the DOM selection. @testing-library/react has a very similar API, but have also these features, which react-test-renderer is missing.

Rewrite tests for @testing-library/react

72bb78b

This will rewrite the existing tests for Editable and move them into a own file.

Add tests for ReactEditor.focus

649a36d

skogsmaskin force-pushed the fix-focus-issue branch from cd93734 to 649a36d Compare November 7, 2023 22:19

skogsmaskin changed the title ~~Fix issue with slate-react static ReactEditor.focus method~~ Fix issue with ReactEditor.focus method + tests Nov 7, 2023

Add changeset

0b01f5a

skogsmaskin marked this pull request as ready for review November 7, 2023 22:47

skogsmaskin requested a review from dylans November 7, 2023 22:53

skogsmaskin changed the title ~~Fix issue with ReactEditor.focus method + tests~~ Fix issue with ReactEditor.focus + tests Nov 7, 2023

skogsmaskin requested a review from 12joan November 7, 2023 23:02

skogsmaskin commented Nov 8, 2023

View reviewed changes

dylans approved these changes Nov 10, 2023

View reviewed changes

dylans merged commit fc08181 into ianstormtaylor:main Nov 10, 2023
11 checks passed

github-actions bot mentioned this pull request Nov 10, 2023

Version Packages #5552

Merged

skogsmaskin added a commit to sanity-io/sanity that referenced this pull request Nov 11, 2023

refactor(portable-text-editor): remove workaround

d03b259

This was a workaround for missing functionality in slate-react which is now fixed upstream ianstormtaylor/slate#5527

skogsmaskin mentioned this pull request Nov 11, 2023

chore(portable-text-editor): upgrade slate + remove workarounds sanity-io/sanity#5175

Merged

skogsmaskin added a commit to sanity-io/sanity that referenced this pull request Nov 13, 2023

refactor(portable-text-editor): remove workaround

f3c39e8

This was a workaround for missing functionality in slate-react which is now fixed upstream ianstormtaylor/slate#5527

philicious mentioned this pull request Nov 19, 2023

Regression related to focus since 0.99.0 (Cannot resolve DOM point...) #5561

Closed

philicious mentioned this pull request Nov 27, 2023

Regression in ReactEditor.focus since 0.101.0 #5568

Open

12joan mentioned this pull request Jan 22, 2024

Fix: Calling ReactEditor.focus doesn't update useFocused when running in @testing-library/react #5593

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue with ReactEditor.focus + tests #5527

Fix issue with ReactEditor.focus + tests #5527

skogsmaskin commented Oct 19, 2023 •

edited

Loading

changeset-bot bot commented Oct 19, 2023 •

edited

Loading

skogsmaskin Oct 19, 2023

12joan Oct 20, 2023

12joan Oct 20, 2023

12joan Oct 20, 2023

skogsmaskin Nov 7, 2023 •

edited

Loading

skogsmaskin Nov 7, 2023

skogsmaskin commented Nov 7, 2023 •

edited

Loading

skogsmaskin Nov 8, 2023

skogsmaskin Nov 8, 2023

skogsmaskin Nov 8, 2023

skogsmaskin Nov 8, 2023

		el.focus({ preventScroll: true })
		IS_FOCUSED.set(editor, true)

Fix issue with ReactEditor.focus + tests #5527

Fix issue with ReactEditor.focus + tests #5527

Conversation

skogsmaskin commented Oct 19, 2023 • edited Loading

changeset-bot bot commented Oct 19, 2023 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skogsmaskin Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skogsmaskin commented Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skogsmaskin commented Oct 19, 2023 •

edited

Loading

changeset-bot bot commented Oct 19, 2023 •

edited

Loading

skogsmaskin Nov 7, 2023 •

edited

Loading

skogsmaskin commented Nov 7, 2023 •

edited

Loading