WARNING: THIS SITE IS A MIRROR OF GITHUB.COM / IT CANNOT LOGIN OR REGISTER ACCOUNTS / THE CONTENTS ARE PROVIDED AS-IS / THIS SITE ASSUMES NO RESPONSIBILITY FOR ANY DISPLAYED CONTENT OR LINKS / IF YOU FOUND SOMETHING MAY NOT GOOD FOR EVERYONE, CONTACT ADMIN AT ilovescratch@foxmail.com
Skip to content

Conversation

@AdrienPoupa
Copy link
Contributor

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

@changeset-bot
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
Contributor 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.

@dylans dylans merged commit 90fbcde into ianstormtaylor:main Nov 19, 2024
7 checks passed
@github-actions github-actions bot mentioned this pull request Nov 19, 2024
zhi-zhi-zhi added a commit to zhi-zhi-zhi/slate that referenced this pull request Jan 13, 2025
zhi-zhi-zhi added a commit to zhi-zhi-zhi/slate that referenced this pull request Jan 13, 2025
zhi-zhi-zhi added a commit to zhi-zhi-zhi/slate that referenced this pull request Jan 13, 2025
dylans added a commit that referenced this pull request Jan 27, 2025
…omSelection #5741) (#5792)

* fix: supplement of (fix: Prevent ReactEditor.toDOMRange crash in setDomSelection #5741)

* Update packages/slate-dom/src/plugin/with-dom.ts

* Update packages/slate-dom/src/plugin/with-dom.ts

* Create gold-tomatoes-grab.md

* Update packages/slate-dom/src/plugin/with-dom.ts

* Update packages/slate-dom/src/plugin/with-dom.ts

---------

Co-authored-by: Dylan Schiemann <[email protected]>
This was referenced Jan 27, 2025
RavenColEvol pushed a commit to RavenColEvol/slate that referenced this pull request Mar 16, 2025
…omSelection ianstormtaylor#5741) (ianstormtaylor#5792)

* fix: supplement of (fix: Prevent ReactEditor.toDOMRange crash in setDomSelection ianstormtaylor#5741)

* Update packages/slate-dom/src/plugin/with-dom.ts

* Update packages/slate-dom/src/plugin/with-dom.ts

* Create gold-tomatoes-grab.md

* Update packages/slate-dom/src/plugin/with-dom.ts

* Update packages/slate-dom/src/plugin/with-dom.ts

---------

Co-authored-by: Dylan Schiemann <[email protected]>
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