-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
fix: Prevent ReactEditor.toDOMRange crash in setDomSelection #5741
Conversation
🦋 Changeset detectedLatest commit: c50923a 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 |
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. |
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
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. |
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. |
…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 <dylan@dojotoolkit.org>
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 fromsetDomSelection
'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 adomPoint
.This pattern is already used in
ensureDomSelection
:Please let me know if I missed something!
Checks
yarn test
.yarn lint
. (Fix errors withyarn fix
.)yarn start
.)yarn changeset add
.)