Use native character insertion to fix browser/OS text features

[bkrausz]

See #3293, which hasn't seen any action despite a change request. This takes that PR and makes TextString a functional component, the one requested change.

Huge thanks to @luddep for doing almost all of the work here.

Does this fix any issues or need any specific reviewers?

Fixes: #2061 (and some of #2051?)
Reviewers: @ianstormtaylor

[maccman]

This spell-checking bug essentially makes Slate unusable in production in my opinion.

I know Slate has some new core-contributors - I urge them to look at this PR.

[maccman]

Here's my hacky workaround I'm using for now:

  const debouncedSetSpellCheck = useMemo(() => debounce(setSpellCheck, 300), [])

  const toggleSpellCheck = () => {
    // Slate has a spell checking bug where spell checking gets invoked too
    // aggressively. We can work around this by toggling it when the user types.
    // https://rb.gy/qycfpa
    setSpellCheck(false)
    debouncedSetSpellCheck(true)
  }

And:

          onChange={newValue => {
              toggleSpellCheck()
            }
          }}>

          <EditablePlugins
            spellCheck={spellCheck}```

[ianstormtaylor]

Looks good to me generally. It just needs someone to verify that this doesn't break other things, since this is a pretty core change.

[bkrausz]

I've been using this in production on a fork for months now with no major issues, though all of our users are on Chrome.

The only issue I found was in doing complex normalizations on top of nativeOps, I just fixed that via the latest commit (only required an extra withoutNormalization.

Is there other verification you would like to see?

[luddep]

If you want to cherry-pick d7e444a, it solves a macOS chrome bug where a long press to select a special character (hold a + press 4 = ä) would double insert until native events flushed -- solved for now by just sidestepping it entirely and limiting native events to single char a-z inserts. In my limited testing, those are the only keyboard events (on US keyboards) that trigger spell check corrections.

[MohsenElgendy]

Is there an estimation on when would this PR be merged with the master?

[cmditch]

Very glad a fix for the spellcheck flicker is out there. Appreciate the work @ryanmitts, @luddep, @bkrausz have put towards this. 🎉

Pretty excited for this fix to get merged.

@thesunny do you see any issues with the logic of this PR?

[dylans]

I hate to ask @bkrausz given how long this PR (and its previous form have been going), but if you could rebase it against the latest in main, I will get this landed asap. If you don't have time I'll do the rebase.

[changeset-bot]

🦋 Changeset detected

Latest commit: ee08862

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

[bkrausz]

@dylans done. I'm having trouble getting tests to pass on my local machine, but the same 5 fail for me on main and on this branch, so hopefully nothing to fix.

[dylans]

@dylans done. I'm having trouble getting tests to pass on my local machine, but the same 5 fail for me on main and on this branch, so hopefully nothing to fix.

Ok, we do have one integration test that appears to be legitimately failing now @bkrausz , the test in https://github.com/ianstormtaylor/slate/blob/main/cypress/integration/examples/code-highlighting.ts#L19-L34

Here's the specific error: 1) code highlighting highlights javascript syntax: AssertionError: Timed out retrying after 4000ms: expected '<span.css-14qid8c>' to contain 'const' at Context.eval (http://localhost:3000/__cypress/tests?p=cypress/integration/examples/code-highlighting.ts:124:14)

Please have a look when you have time.

[bkrausz]

Is it possible that this test is flaky? I'm seeing it working correctly locally, both via test passing and manually checking on a new build. Would you mind triggering a re-run to confirm and/or snagging the video from the test server? I don't have access to either.

[bkrausz]

On further inspection it looks like this PR isn't actually doing what it claims to do anymore...we still get flickering on spellcheck. I'm using it to get other native actions (specifically so horizontal scroll will work so Slate can emulate Input boxes). This PR needs deeper investigation: we are still avoiding calling preventDefault, but it seems that's no longer sufficient.

I doubt I'll have time to go that deep on this PR and fix it again, if anyone else is interested in taking this over that would be more than welcome.

[lukesmurray]

If anyone wants to do it you could potentially use git bisect to find the commit where the change broke.

[bkrausz]

FYI this PR does in fact still fix autocorrect support in Safari, just not the spellcheck squiggle blink.

[bkrausz]

Ok, I got nerdsniped enough to bisect this...it's from #4158, which causes every string to rerender. I'm pushing up a revert for it, can someone who knows how to test IME see if the bug described there is also fixed by this one? We may need to add a few extra characters to the allowlist, as I don't know what characters it produces.

[dylans]

Ok, I got nerdsniped enough to bisect this...it's from #4158, which causes every string to rerender. I'm pushing up a revert for it, can someone who knows how to test IME see if the bug described there is also fixed by this one? We may need to add a few extra characters to the allowlist, as I don't know what characters it produces.

I know how to test IME. I’ll look at it over the weekend.

[dylans]

Overall this looks good to me @bkrausz... I'm going to let it sit for a couple more days so I can give it another look and in case anyone else wants to take a second look.

[githoniel]

@bkrausz sorry for the bug caused by my hacky PR that drove you crazy😥。

@dylans but this pr did not fix the IME double input when add mark bug. I have test in the main branch with this pr merged like below:

this anoy bug will cause slate to crash when click on the text that should not exist.

I have a new idea to fix this without force re-render string component. I will try to implement later

[dylans]

@bkrausz sorry for the bug caused by my hacky PR that drove you crazy😥。

@dylans but this pr did not fix the IME double input when add mark bug. I have test in the main branch with this pr merged like below:

this anoy bug will cause slate to crash when click on the text that should not exist.

I have a new idea to fix this without force re-render string component. I will try to implement later

ok @githoniel let me know. We've had quite a few IME related contributions lately and it would be great to be done with the double input issue resolved for good.

[dylans]

@bkrausz fyi, this update seems to have caused a regression for code highlighting/decorations, see the notes in #4445.

[jaked]

rather than defer the insertText call to the underlying Editor, would it make sense to accumulate native operations in onDOMBeforeInput and flush them in onInput entirely within Editable, so insertText is not called at all until onInput? It seems like a lot of the bugs around this (e.g. #4530, #4541) have to do with interactions with non-default insertText implementations.

If this seems like a reasonable idea I can try to do a PR.

[dylans]

rather than defer the insertText call to the underlying Editor, would it make sense to accumulate native operations in onDOMBeforeInput and flush them in onInput entirely within Editable, so insertText is not called at all until onInput? It seems like a lot of the bugs around this (e.g. #4530, #4541) have to do with interactions with non-default insertText implementations.

If this seems like a reasonable idea I can try to do a PR.

Seems worth a try to me.

[juliankrispel]

Was this fixed as part of these prs? I have built commits from both this pr and #4605 and have not found a working version. Pointers appreciated 🙏