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 crash when deleting paragraph ending \n in Firefox #5549

Merged
merged 1 commit into from
Nov 9, 2023

Conversation

12joan
Copy link
Contributor

@12joan 12joan commented Nov 1, 2023

Description
Due to a Firefox browser quirk, the DOM selection sometimes includes the additional \n added when isTrailing is true in TextString. This results in an invalid focus point, causing a crash during delete operations.

Issue
Fixes: #5273

Example
Before:

before.mp4

After:

after.mp4

Context
It's possible that the logic for the check I added might not be perfect. However, I'm 99% sure that any new bugs it introduces will be harder to reproduce accidentally than the bug it fixes.

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 Nov 1, 2023

🦋 Changeset detected

Latest commit: 54bc0f2

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

Copy link
Collaborator

@dylans dylans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub actions is providing a linting warning that should be addressed. Please review that and then we can land this.

@12joan
Copy link
Contributor Author

12joan commented Nov 6, 2023

Hi @dylans, I can't see the linting warning you're referring to, unless you mean this warning that's also present on main?

/Users/joe/workspace/slate/packages/slate-react/src/components/editable.tsx
  193:6  warning  React Hook useLayoutEffect has a missing dependency: 'state'. Either include it or remove the dependency array  react-hooks/exhaustive-deps

✖ 1 problem (0 errors, 1 warning)

If that's the warning you mean, perhaps it should be fixed in a separate PR?

@12joan 12joan requested a review from dylans November 6, 2023 18:11
@dylans dylans merged commit f9cca97 into ianstormtaylor:main Nov 9, 2023
11 checks passed
@github-actions github-actions bot mentioned this pull request Nov 9, 2023
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.

Crash when deleting last paragraph ending in \n
2 participants