search

ianstormtaylor / slate

A completely customizable framework for building rich text editors. (Currently in beta.)
http://slatejs.org
MIT License
30.02k stars 3.26k forks source link

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 2 years ago

jameshfisher commented 2 years ago

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

jameshfisher commented 2 years ago

The diff is pretty small: https://github.com/ianstormtaylor/slate/compare/slate-react%400.74.2...slate-react%400.75.0

jameshfisher commented 2 years ago

It looks like this is the only substantial change: https://github.com/ianstormtaylor/slate/pull/4889

jameshfisher commented 2 years ago

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 commented 2 years ago

I don't fully understand what https://github.com/ianstormtaylor/slate/pull/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 commented 2 years ago

Okay, it gets weirder ... on Chrome, this breaks between slate-react@0.74.2 and slate-react@0.75.0. But on Firefox, it breaks between slate-react@0.65.3 and slate-react@0.66.0! Which again suggests that maybe https://github.com/ianstormtaylor/slate/pull/4889 is not the source of this bug.

jameshfisher commented 2 years ago

The diff is much larger for the Firefox breakage: https://github.com/ianstormtaylor/slate/compare/slate-react%400.65.3...slate-react%400.66.0. Although, like the diff that breaks Chrome, it makes changes to Slate.Transforms.setNodes.

BitPhinix commented 2 years ago

The issue is the native handling here: https://github.com/ianstormtaylor/slate/blob/d39943741f88c2d3d85375b58057b3dbca7d8c42/packages/slate-react/src/components/editable.tsx#L464

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 commented 2 years ago

Aha, thanks @BitPhinix! So https://github.com/ianstormtaylor/slate/pull/4889 is nothing to do with this; the cause was https://github.com/ianstormtaylor/slate/pull/4883. With slate-react@0.74.2, 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 commented 2 years ago

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 commented 2 years ago

slate-react 0.66.0 introduced this "native event" thing in https://github.com/ianstormtaylor/slate/pull/3888. So the bug has existed ever since this was first added.

xNarkon commented 2 years ago

Any update?

delijah commented 1 year ago

Any progress on this?