use native character insertion to fix browser/OS text features.

[luddep]

Is this adding or improving a feature or fixing a bug?

fixing a bug

What's the new behavior?

Fixes spellcheck, text replacement, autocorrect and makes it so spellcheck squiggles dont blink in and out as you type.

This is a port of #3076 to 50+. It's fixing the same bug(s), with the same technique and comments from @ryanmitts.

Current behavior:

Fixed:

How does this change work?

Single character insert events are not cancelled, as browser features like spellcheck only fires on user events and are removed on DOM updates. Native events are allowed to pass through, and will cancel its render if the effective text value hasn't changed.

Native Operations are queued and applied onInput, as for the check to work the native events need to propagate.

As #3076, this is pretty narrow and is only applied for insertText onBeforeInput events, with no selection, not at the end of inline nodes (as the selection is moved on insert_text elsewhere) or at the very beginning of a selection.

#3076 has more details.

Have you checked that...?

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

Does this fix any issues or need any specific reviewers?

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

[ianstormtaylor]

@luddep this is amazing! Thank you for taking the time to work on this! I've added a few comments inline with some tweaks, but overall I like it.

[ianstormtaylor]

Can you keep this as a functional component and use useEffect and React.memo instead to achieve the changes?

[luddep]

Not entirely sure how to get the same behavior of both (1) stopping the render, and (2) making sure React still reconciles (the forceUpdateFlag part), using hook mechanics.

Spent a few hours experimenting, and was able to get (1) with a React.memo wrap on a subcomponent (could probably be lifted up to <Leaf />) and not updating render value if textContent is the same as text prop, but haven't been able to solve (2).

(2) Manifests itself as seeing ghost characters on delete (e.g. typing hi|, delete, expect to see h|, but see h|i) or selection moving around (hello |, delete, |hello.

I'm a little stumped, but also haven't quite grokked functional components enough to think of a clever way of doing it. Happy to give it another go if anyone's got any ideas.

[ajorkowski]

@luddep @ianstormtaylor The problem is that you want to prevent a normal render unless you want to reconcile the textContent with the native element, so it is not so simple to do it with hooks. I think you will need to have two hook based components to do this, something like the following (sorry if I mangled the TS a little bit)?

const TextString = (props: { text: string; isTrailing?: boolean }) => {
  const { text, isTrailing = false } = props

  const ref = useRef(null);
  const forceUpdateFlag = useRef(false);

  if (ref.current && ref.current.textContent !== text) {
    forceUpdateFlag.current = !forceUpdateFlag.current;
  }

  return <TextStringMemo ref={ref} text={text} isTrailing={isTrailing} forceUpdate={forceUpdateFlag.current} />
}

const TextStringMemo = memo(forwardRef((props: { text: string; isTrailing: boolean, forceUpdate: boolean }, ref) => {
  const { text, isTrailing, forceUpdate } = props
  return (
    <span
      data-slate-string
      ref={ref}
      key={forceUpdate ? 'A' : 'B'}
    >
      {text}	
      {isTrailing ? '\n' : null}
    </span>
  )
}));

[ajorkowski]

Actually, if I understand what you are trying to do is just make sure that the render is forced on text change it should be possible with a single component after you make it memo:

const TextString = React.memo((props: { text: string; isTrailing?: boolean }) => {
  const { text, isTrailing = false } = props

  const ref = useRef(null);
  const forceUpdateFlag = useRef(false);

  if (ref.current && ref.current.textContent !== text) {
    forceUpdateFlag.current = !forceUpdateFlag.current;
  }

  return (
    <span
      data-slate-string
      ref={ref}
      key={forceUpdateFlag.current ? 'A' : 'B'}
    >
      {text}	
      {isTrailing ? '\n' : null}
    </span>
  );
});

[ianstormtaylor]

Shoot it looks like one of my review comments was lost. I've just re-written it up again here.

[luddep]

Makes sense! Moved it all into slate-react, and removed any new interface (no more native?: flags, etc.)

[ryanmitts]

Thanks for porting this to the new version for me!

[luddep]

updated to work with latest changes, and fix an insertion bug when using marks.

[bkrausz]

Strong +1 to this PR: it fixes some other quirks. For example, we've been working on single-line Slate and this allows horizontal scroll as you type (like an input field) to work properly. Seems like the path forward.

I believe this breaks the markdown shortcuts example: since all of the operations get queued up, it's hard to figure out what to do to effectively "undo" the typing that will happen beforehand. I wonder if there's a good heuristic to still preventDefault if we're queueing up certain operations, or if we should provide some hook for plugins to tell that they don't want this functionality because they're messing with structure.

Edit: my quick fix (I'm running off this branch), which is obviously less than ideal, is to perform the actual modification inside a window.setTimeout(..., 1). That seems to work properly despite being ugly.

[bobjflong]

I'm not 100% sure, and not familiar with Slate internals, but I think this breaks the accented character input for us on mac:

I see an error like this upon selecting one of the accented forms:

Error: Cannot resolve a DOM point from Slate point: {"path":[0,0],"offset":2}

[luddep]

@bkrausz good catch. By bailing out of the native queue and immediately flushing if any non insert_text ops were being applied, the example works again.

@bobjflong I'm unable to reproduce that bug. Still happening? (was maybe fixed in master?)

[MikeAliG]

Hello! Any update on this one?

[gztomas]

Is there anything blocking this PR to be merged? It dramatically improves the UX and fixes a number of issues.

[emtesenair]

What's the status of this PR?

[timbuckley]

Per Ian's comment, can we make that class component into a functional component using hooks?

[brycedewitt]

Checking in on this, is anyone actively working on a functional conversion right now?

[bkrausz]

I have the conversion, can put up a new PR tomorrow with it.

…

On Sun, Sep 20, 2020 at 6:40 PM brycedewitt ***@***.***> wrote: Checking in on this, is anyone actively working on a functional conversion right now? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3293 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACY4JUDLV7ECTKNQHND733SG2VJNANCNFSM4JZHIYIQ> .

[bkrausz]

H'okay, #3888 is out with the requested change.

[dylans]

Superseded by #3888.