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

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

Closed

Conversation

luddep
Copy link
Contributor

@luddep luddep commented Dec 11, 2019

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:
current

Fixed:
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

Copy link
Owner

@ianstormtaylor ianstormtaylor left a comment

@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.

packages/slate-react/src/components/editable.tsx Outdated Show resolved Hide resolved
packages/slate-react/src/components/editable.tsx Outdated Show resolved Hide resolved
@@ -55,15 +55,42 @@ const String = (props: {
/**
* Leaf strings with text in them.
*/
type TextStringProps = { text: string; isTrailing?: boolean }
class TextString extends React.Component<TextStringProps> {
Copy link
Owner

@ianstormtaylor ianstormtaylor Dec 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@luddep luddep Dec 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@ajorkowski ajorkowski May 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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>
  )
}));

Copy link

@ajorkowski ajorkowski May 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
  );
});

Copy link
Owner

@ianstormtaylor ianstormtaylor left a comment

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

packages/slate/src/create-editor.ts Outdated Show resolved Hide resolved
@luddep
Copy link
Contributor Author

luddep commented Dec 12, 2019

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

@luddep luddep requested a review from ianstormtaylor Dec 12, 2019
@ryanmitts
Copy link
Contributor

ryanmitts commented Dec 13, 2019

Thanks for porting this to the new version for me!

@luddep luddep force-pushed the lp-native-single-character-inserts branch from 3ecf569 to 1e8e456 Compare Jan 18, 2020
@luddep
Copy link
Contributor Author

luddep commented Jan 18, 2020

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

thallada pushed a commit to considerhq/slate that referenced this pull request Jan 29, 2020
…kering spellcheck, autocorrect, text shortcuts, etc.)

See ianstormtaylor#3293.

move some checks into previous if statement, remove commented out code

move native behavior into `slate-react`, and remove any external interface

dont use native editing if marks are set, as a new node will be inserted

match -> above

remove nativeOperationsQueue from editor
thallada added a commit to considerhq/slate that referenced this pull request Feb 4, 2020
This PR in slate: ianstormtaylor#3293 causes an
exception in Slate about not being able to find a DOM node for the selection
while typing after a call to `Editor.insertText("\n")`.

The workaround here is to turn off native insertion if the user is inserting
text in a text node that ends in a new line character ("\n").
@bkrausz
Copy link
Contributor

bkrausz commented Feb 18, 2020

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
Copy link

bobjflong commented Feb 19, 2020

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

Screen Shot 2020-02-19 at 5 02 13 PM

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
Copy link
Contributor Author

luddep commented Apr 30, 2020

@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?)

@luddep luddep force-pushed the lp-native-single-character-inserts branch from d88ae55 to b708490 Compare Apr 30, 2020
…kering spellcheck, autocorrect, text shortcuts, etc.)

move some checks into previous if statement, remove commented out code

move native behavior into `slate-react`, and remove any external interface

dont use native editing if marks are set, as a new node will be inserted

match -> above

remove nativeOperationsQueue from editor

bail out of native queueing and immediately flush events if non insert_text operation is being applied.
@luddep luddep force-pushed the lp-native-single-character-inserts branch from 2d58cb0 to c2e44d1 Compare May 15, 2020
@MikeAliG
Copy link

MikeAliG commented Jul 2, 2020

Hello! Any update on this one?

@gztomas
Copy link
Contributor

gztomas commented Jul 21, 2020

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

@emtesenair
Copy link

emtesenair commented Aug 11, 2020

What's the status of this PR?

@timbuckley
Copy link
Collaborator

timbuckley commented Sep 11, 2020

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

@brycedewitt
Copy link

brycedewitt commented Sep 21, 2020

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

@bkrausz
Copy link
Contributor

bkrausz commented Sep 21, 2020

@bkrausz
Copy link
Contributor

bkrausz commented Sep 25, 2020

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

…r or space, to limit potential bad side effects.
@dylans
Copy link
Collaborator

dylans commented Aug 4, 2021

Superseded by #3888.

@dylans dylans closed this Aug 4, 2021
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.

fix spellcheck blinking as you type