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

Don't remove selection when hovering over a non-selectable DOM element #4577

Merged
merged 2 commits into from
Oct 12, 2021

Conversation

jameshfisher
Copy link
Contributor

@jameshfisher jameshfisher commented Oct 6, 2021

Description

To reproduce the buggy behavior:

  1. Go to https://codepen.io/jameshfisher/pen/OJgoeYw, or create a page that renders a Slate element with a contentEditable: false element in it.
  2. Start selecting some text with the mouse.
  3. During the drag, mouseover the contentEditable: false element.

Expected behavior: After doing a drag-to-select with the mouse, from a valid anchor point on mousedown to a valid focus point on mouseup, the selection is set to those anchor and focus points.

Actual behavior: your selection is removed as soon as your mouse hits the contentEditable: false element. This is because the current behavior clears the selection if it is momentarily not a valid Slate location.

Recording of buggy behavior

mouseover-bug.mp4

Issue
Fixes #4545

Context
If the focus is momentarily not a valid Slate location, just do nothing.

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

Fixes ianstormtaylor#4545

To reproduce the buggy behavior:

1. Create a page that renders a Slate element with a `contentEditable: false` element in it.
2. Start selecting some text with the mouse.
3. During the drag, mouseover the `contentEditable: false` element.

Expected behavior: After doing a drag-to-select with the mouse, from a valid anchor point on mousedown to a valid focus point on mouseup, the selection is set to those anchor and focus points.

Actual behavior: your selection is removed as soon as your mouse hits the `contentEditable: false` element. This is because the current behavior clears the selection if it is momentarily not a valid Slate location.
@changeset-bot
Copy link

changeset-bot bot commented Oct 6, 2021

🦋 Changeset detected

Latest commit: 4389d16

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

@jameshfisher
Copy link
Contributor Author

I discovered that this bug can be seen in a standard Slate example: https://www.slatejs.org/examples/check-lists.

When selecting text, mouseover one of the checkboxes. You'll see that the selection is removed.

Compare the same example with this patch, which does not have this buggy behavior: https://deploy-preview-4577--slatejs.netlify.app/examples/check-lists

@jameshfisher jameshfisher changed the title Don't remove selection when hovering over a non-selectable node Don't remove selection when hovering over a non-selectable DOM element Oct 7, 2021
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.

This seems reasonable. Not sure if it might introduce a scenario where you cannot clear out the selection, but I don't see that being the case with some quick tests. I'll leave it open for a few days for others to give feedback.

Could you add a quick changeset please? (yarn changeset from your branch, or there's an option in the GitHub UI). Thanks!

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.

Bug: selection is removed when selecting across element with contentEditable: false
2 participants