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

If insertText does nothing, text is inserted into the DOM anyway, causing DOM/Slate desync (bug introduced in slate-react 0.66.0) #5152

Open
jameshfisher opened this issue Oct 11, 2022 · 12 comments
Labels

Comments

@jameshfisher
Copy link
Contributor

jameshfisher commented Oct 11, 2022

Description
It is valid for insertText to return without doing anything. The expected behavior is that the document is left unchanged. For example, you might want to disallow typing uppercase characters -- you can do this by implementing insertText, and if the text is uppercase, do nothing. With slate-react 0.74.2 and below, this works correctly. But with slate-react 0.75.0 and above, it is broken: the text is inserted into the DOM anyway. However, it is (correctly!) NOT inserted into Slate's document -- this means the DOM and the Slate document are then out of sync, leading to further bugs.

Sandbox

Example: https://codesandbox.io/s/inserttext-should-do-nothing-if-it-returns-wzc647?file=/index.js

Steps
To reproduce the behavior:

  1. Go to the sandbox above
  2. In the editor, type "X"
  3. Note that the character "X" is incorrectly inserted into the DOM
  4. Type "x"
  5. Note that the previous "X" has disappeared, and an incorrect character has been replaced with "x". (This is presumably due to desynchronization between DOM and Slate document)

Expectation
If my insertText does nothing, nothing should happen.

Environment

  • Slate Version: slate-react 0.66.0 and above. slate-react 0.65.3 works. Note that the bug will seem to not be present with Chrome v100+ and slate-react 0.66.0--0.75.0, but this is due to an unrelated bug (Fix chrome and edge three digit version check #4883, as noted by @BitPhinix).
  • Operating System: Windows
  • Browser: Chrome, Firefox
@jameshfisher
Copy link
Contributor Author

The diff is pretty small: [email protected]@0.75.0

@jameshfisher
Copy link
Contributor Author

jameshfisher commented Oct 11, 2022

It looks like this is the only substantial change: #4889

@jameshfisher
Copy link
Contributor Author

Temporary workaround: instead of doing nothing, make an invisible change. Example: https://codesandbox.io/s/workaround-for-slatejs-bug-inserttext-should-do-nothing-if-it-returns-wzc647?file=/index.js

@jameshfisher
Copy link
Contributor Author

jameshfisher commented Oct 11, 2022

I don't fully understand what #4889 is doing. I tried reverting that change, but the bug persists. Perhaps it is not the source of this bug, or there are further changes that are causing it. @dylans @zhugexinxin maybe you could help me here?

@jameshfisher
Copy link
Contributor Author

Okay, it gets weirder ... on Chrome, this breaks between [email protected] and [email protected]. But on Firefox, it breaks between [email protected] and [email protected]! Which again suggests that maybe #4889 is not the source of this bug.

@jameshfisher
Copy link
Contributor Author

The diff is much larger for the Firefox breakage: [email protected]@0.66.0. Although, like the diff that breaks Chrome, it makes changes to Slate.Transforms.setNodes.

@jameshfisher jameshfisher changed the title If insertText does nothing, text is inserted into the DOM anyway, causing DOM/Slate desync (as of slate-react 0.75.0) If insertText does nothing, text is inserted into the DOM anyway, causing DOM/Slate desync Oct 12, 2022
@BitPhinix
Copy link
Contributor

BitPhinix commented Oct 12, 2022

The issue is the native handling here:

Since slate doesn't preventDefault in all cases to avoid resulting issues with spell correct, doing nothing in insertText results in the default behavior not being prevented and slate not re-rendering/re-mounting the text node thus, the slate state and dom are out of sync.

Definitely a bug, we should always re-render the affected text node in this case.

@jameshfisher
Copy link
Contributor Author

Aha, thanks @BitPhinix! So #4889 is nothing to do with this; the cause was #4883. With [email protected], my modern Chrome was being identified as "legacy", in which case Slate does preventDefault.

So I suppose the bug has been around for longer.

@jameshfisher
Copy link
Contributor Author

To confirm this, I downloaded Chrome 99, as it is not affected by the Chrome regex thing, which was masking the real bug.

This time, Chrome v99 agrees with Firefox: the bug was introduced in slate-react 0.66.0. With slate-react 0.66.0, we see the buggy behavior, but with slate-react 0.65.3, it's fine.

@jameshfisher jameshfisher changed the title If insertText does nothing, text is inserted into the DOM anyway, causing DOM/Slate desync If insertText does nothing, text is inserted into the DOM anyway, causing DOM/Slate desync (bug introduced in slate-react 0.66.0) Oct 12, 2022
@jameshfisher
Copy link
Contributor Author

slate-react 0.66.0 introduced this "native event" thing in #3888. So the bug has existed ever since this was first added.

@xNarkon
Copy link

xNarkon commented Dec 1, 2022

Any update?

@delijah
Copy link
Contributor

delijah commented Oct 16, 2023

Any progress on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants