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

@TyMick
Copy link
Contributor

@TyMick TyMick commented Nov 6, 2025

Currently, defaultScrollSelectionIntoView only scrolls collapsed selections into view, citing the following concern:

// This was affecting the selection of multiple blocks and dragging behavior,
// so enabled only if the selection has been collapsed.

This causes a bug when undoing the deletion of a range of text. To reproduce:

  1. Go to https://www.slatejs.org/examples/richtext
  2. Add enough text so that you can't see the first paragraph when you scroll to the bottom of the page
  3. Select the whole first paragraph and delete it
  4. Scroll down to the bottom of the page
  5. Press Ctrl+Z or Z to undo
  6. Note that the page does not automatically scroll back to the restored text
  7. Scroll back manually to the top and confirm that the deleted text is restored
Bug_screen_capture.mp4

This PR updates defaultScrollSelectionIntoView to allow scrolling when the selection is expanded, but it specifically scrolls to the focus point of the selection (rather than trying to scroll to the entire selected range). This fixes the undo-after-delete bug:

Fix_screen_capture.mp4

And scrolling to the focus point appears to avoid the issues with "the selection of multiple blocks and dragging behavior" that the original comment mentions. I've experienced those issues in development when allowing scrolling to expanded selections if I'm still trying to scroll to the entire domRange. But we've made this domFocusPoint change in our project's custom scrollSelectionIntoView, and I can't find any undesired behaviors when creating large selections, using either the mouse or the keyboard.

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 Nov 6, 2025

🦋 Changeset detected

Latest commit: 0ecace7

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

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.

For the most part this looks reasonable, but it does cause one of the chunking integration tests to fail. @TyMick can you please take a look at that and see if it's the test or your change causing the failure?

@12joan
Copy link
Contributor

12joan commented Nov 7, 2025

Hi @dylans, it looks like the issue you're seeing on the Playwright integration test was a random flake, probably caused by the page taking slightly longer than usual to load. The same test succeeded on its second attempt. I'm pretty sure it's unrelated to this PR.

https://github.com/ianstormtaylor/slate/actions/runs/19149811365/job/54736850063?pr=5968#step:4:1328

@TyMick
Copy link
Contributor Author

TyMick commented Nov 7, 2025

Yeah the chunking test's flakiness seems unrelated to my changes. When I run the test locally, it's failing at the same rate on both my feature branch and the main branch.

@dylans dylans merged commit 49f28e5 into ianstormtaylor:main Nov 21, 2025
11 checks passed
This was referenced Nov 21, 2025
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.

3 participants