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

Optimize TextString rendering to support browser/OS text features, eg fix native spellcheck #4733

Merged
merged 4 commits into from
Dec 18, 2021

Conversation

Schipy
Copy link
Contributor

@Schipy Schipy commented Dec 15, 2021

Description
This fix is the last or at least the next piece on the journey to Use native character insertion to fix browser/OS text features With all the good progress so far, native spellcheck as you type is still broken on practically all browsers (no spellcheck underlining on Safari & FF and blinking underlines on Chrome).

Issue
Fixes: #4320
Related: #3888, #2051

Example
Safari, before (spellcheck prevented)
video of Safari, before (spellcheck prevented)

Chrome, before (spellcheck blinks)
Chrome-with-react-text

Context
The reason for this is that the continuously changing text will rerender the virtual DOM on each keystroke, which in turn updates the DOM TextNode, which will opt out browsers from checking spelling in that - now programatically updated, not user typed - text node.

Essentially now that data flow is two-way (simple characters are inserted natively, and propagated to slate, while complex characters and events are caught and happen in slate and propagated to native DOM), we need to tweak the leaf text node rendering to reflect this and instead of using the one way dataflow of virtual DOM, implement the leaf TextNode rendering as an Effect that does proper diffing between the actual DOM and the virtual DOM, and not based on diffing between older and newer version of the virtual DOM. We would like to replace the text content in a slate span exactly if and only if its current content does not match our current virtual DOM (and for natively inserted characters, it will already match and need no interference from code)

Safari, after
video of Safari, after

Chrome, after
video of Chrome, after

While this change may seem like quite fundamental and risky, I think it's actually quite locally confined: the TextString component behaves almost identical as before.

  • it is rendered by React exactly on the same occasions when the previous version rendered (its props did not change)
  • the result of the render will exactly be the same for the new and old version (both the traditional update via the virtual DOM, and the effect will achieve the same thing, namely that the span will contain the textContent)

A note on null handling: it may be unnecessary, but I added an extra measure to handle the extreme case if the incoming text is null/undefined, and also thereby preserving the previous render behavior in the TextString component (which also did not fail anddid not output any text node children for the span, should this happen).

Note 2: even with this fix spellcheck will still opt-out (or blink on Chrome) for all characters and events that are not natively handled (eg punctuation, backspace, shift+characters etc). Will have to see, this may be acceptable tradeoff, or if not, likely further steps need to be taken to handle as much natively as possible.

Checks

  • 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.)
  • functionality did not change, likely needs a patch changeset in slate-react package?

… via virtual DOM, which implements diffing with real DOM avoiding interference with native TextNode behaviors for example spellcheck
@changeset-bot
Copy link

changeset-bot bot commented Dec 15, 2021

⚠️ No Changeset found

Latest commit: 4b4af79

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Schipy Schipy changed the title Don't rerender TextNodes if they dont change Optimize TextNode rendering to support browser/OS text features, eg fix native spellcheck Dec 15, 2021
@Schipy Schipy changed the title Optimize TextNode rendering to support browser/OS text features, eg fix native spellcheck Optimize TextString rendering to support browser/OS text features, eg fix native spellcheck Dec 15, 2021
Copy link
Collaborator

@dylans dylans left a comment

Choose a reason for hiding this comment

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

Thanks @Schipy, looks promising. I need a day or two to think through this one and to give others a change to give feedback, before we would land it.

Comment on lines 75 to 78
// making sure we're not outputing "null" in the extreme case the text is nullish at runtime
if (text == null) {
textWithTrailing = isTrailing ? '\n' : null
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment states we want to make sure we're not outputting null, but we do have a null. Are the comment and code out of sync or am i reading it wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I meant in the comment to avoid outputting "null" as a string, eg "null\n" as a result of null + '\n'.
So the intent is to set actual null in case text == null && !isTrailing.
Actually, we could also set empty string in this case instead of null, as it seems to two is equivalent (both will actually result in no childNodes under the span and textContent === '').
I will clarify the comment, and simplify this also in the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

You kind of addressed both things i wanted to ask there, looks good!

Copy link
Contributor

@nemanja-tosic nemanja-tosic left a comment

Choose a reason for hiding this comment

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

Love the fix, the spellchecker "lag" was just annoying...

packages/slate-react/src/components/string.tsx Outdated Show resolved Hide resolved
Co-authored-by: Nemanja Tosic <netosic90@gmail.com>
@dylans dylans merged commit ccafb69 into ianstormtaylor:main Dec 18, 2021
@dylans dylans mentioned this pull request Dec 18, 2021
@hueyhe
Copy link
Contributor

hueyhe commented Jan 20, 2022

This fix breaks server-side rendering. All text won't get rendered on ssr, because the useLayoutEffect hook won't fire until the JavaScript is downloaded. Should use useIsomorphicLayoutEffect instead.

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.

Blinking spelling highlight
4 participants