Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Prevent ReactEditor.toDOMRange crash in setDomSelection #5741

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AdrienPoupa
Copy link

@AdrienPoupa AdrienPoupa commented Oct 16, 2024

Description
Somehow related to #5407 and #5571

I debugged an issue in our application where using the TextExpander extension on a Slate input would cause the editor to crash with Cannot resolve a DOM point from Slate point coming from setDomSelection's call.

I was able to prevent the crash in our codebase using an ErrorBoundary as suggested here, but wondered why it was crashing to begin with.

As far as I can tell, although the TextExpander text is not expanded, the editor recovers fine if we discard the error and the existing code supports having a null newDomRange so I figured I could let it be null if it can't calculate a domPoint.

This pattern is already used in ensureDomSelection:

          const ensureDomSelection = (forceChange?: boolean) => {
            try {
              const el = ReactEditor.toDOMNode(editor, editor)
              el.focus()

              setDomSelection(forceChange)
            } catch (e) {
              // Ignore, dom and state might be out of sync
            }
          }

Please let me know if I missed something!

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.)

Copy link

changeset-bot bot commented Oct 16, 2024

🦋 Changeset detected

Latest commit: c50923a

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 Patch

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

@dylans
Copy link
Collaborator

dylans commented Oct 16, 2024

In general I think as a project we try to avoid squashing errors/failures because they might hide an issue for someone else which can make debugging a real pain. Let me think about this a bit. In general I think slate would benefit from an option to configure how to handle failures, but then I always end up back at "add an ErrorBoundary" which isn't a great answer for many users.

@AdrienPoupa
Copy link
Author

AdrienPoupa commented Oct 16, 2024

I think ultimately we need better error management as in #5407 but until we do this could be an easy stop gap. If I understand why this PR got stale, it's because we'd want to

  1. Create a first PR that changes the return types to be | undefined
  2. Introduce error management

I'm willing to take a stab at the former, but I feel we'd still benefit from the current PR to begin with, especially since a similar try/catch is already used in the same portion of the code.

@dylans
Copy link
Collaborator

dylans commented Oct 31, 2024

I will land this, but we're about to land a refactor of slate-react into slate-react + a new slate-dom package. This will likely create a conflict so I'm holding until that lands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants