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

[slate-react]: fix selection bugs when multiple editors share value #4475

Merged
merged 2 commits into from
Aug 25, 2021

Conversation

skogsmaskin
Copy link
Collaborator

@skogsmaskin skogsmaskin commented Aug 25, 2021

Description
The KEY_TO_ELEMENT WeakMap must be scoped to the editor instance to avoid collisions between multiple editors in the same DOM that share the same editor value (object equality on Node).

This is solved by wrapping it in another WeakMap keyed on the editor object, that returns the KEY_TO_ELEMENT WeakMap for that editor.

It could also be solved by just putting the KEY_TO_ELEMENT Weakmap directly on the editor instance, but I didn't want to tangle the ReactEditor interface with these (internal) things and keep using weak-maps only for this logic.

Issue
Fixes:
#3380
#3997
#4129

Example

This will illustrate the problem:

  1. Go to the current main branch deployment: https://deploy-preview-4469--slatejs.netlify.app/examples/editable-voids
  2. Click the + Button on top of the page to add another editor.
  3. Click in the middle of bold word "rich" in the top editor.
  4. Move the cursor one char to the right with arrow right key.
  5. Cursor is moved and focused on bottom editor word "rich"

Then try the same on the deployed preview: https://deploy-preview-4475--slatejs.netlify.app/examples/editable-voids to confirm the fix.

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 Aug 25, 2021

🦋 Changeset detected

Latest commit: 96e50f9

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

@skogsmaskin skogsmaskin force-pushed the fix/instance-weakmaps branch 2 times, most recently from 4aaba18 to 39114a3 Compare August 25, 2021 11:12
The KEY_TO_ELEMENT weakmap must be scoped to the instance to avoid collisions between multiple editors.

This is solved by wrapping it in another WeakMap keyed on the editor object, that returns the KEY_TO_ELEMENT Weakmap for that editor.
skogsmaskin added a commit to skogsmaskin/slate that referenced this pull request Aug 25, 2021
This can be ignored once we upgrade. Fixed upstream here: ianstormtaylor#4475
@skogsmaskin skogsmaskin merged commit c1433f5 into ianstormtaylor:main Aug 25, 2021
@skogsmaskin skogsmaskin removed the request for review from thesunny August 25, 2021 14:00
@github-actions github-actions bot mentioned this pull request Aug 25, 2021
@skogsmaskin skogsmaskin removed the request for review from ianstormtaylor August 26, 2021 07:10
@skogsmaskin skogsmaskin deleted the fix/instance-weakmaps branch August 31, 2021 14:14
@dylans dylans mentioned this pull request Sep 11, 2021
dylans added a commit to dylans/slate that referenced this pull request Sep 13, 2021
…anstormtaylor#4475)

* [slate-react]: fix selection bugs when multiple editors share value

The KEY_TO_ELEMENT weakmap must be scoped to the instance to avoid collisions between multiple editors.

This is solved by wrapping it in another WeakMap keyed on the editor object, that returns the KEY_TO_ELEMENT Weakmap for that editor.

* Add changeset

Co-authored-by: Dylan Schiemann <dylan@dojotoolkit.org>
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