Skip to content
This repository has been archived by the owner on Feb 6, 2023. It is now read-only.

Native browser spellcheck causes errors after replacement in Firefox and Safari #2171

Open
jalners opened this issue Aug 29, 2019 · 14 comments

Comments

@jalners
Copy link

jalners commented Aug 29, 2019

Do you want to request a feature or report a bug?
Bug.

What is the current behavior?
drafts_native_spellchecker_firefox

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. You can use this jsfiddle to get started: https://jsfiddle.net/gmertk/e61z7nfa/.

  • Use latest Firefox browser (version 68.0.2).
  • Go to https://draftjs.org/.
  • Open the browser console.
  • Type the next text in the editor: Hello maan.
  • Add bold markup for m in maan word.
  • Right mouse button click on the misspelled word (maan) and select a replacement moan.
  • Now you can see the next text: Hello moanaan.. The text is incorrect. It should be Hello moan..
  • Place cursor at the end of the string.
  • Press Ctrl/Command + A (Select All).
  • Take a look at the console. There will be an error - Error: invariant(...): Second argument must be a string..
  • Enter a letter. Other errors will appear in the console: Error: Got unexpected null or undefined.
  • After that try to type other letters. You will see an error for each key press.
  • Try to select a part of the text and make it bold/italic/etc. The changes will not be applied.
  • Try to press Enter key. It will not work.

What is the expected behavior?
The misspelled word should be replaced with a correct one. The editor should work properly without any errors and inconsistent behaviour.

Which versions of Draft.js, and which browser / OS are affected by this issue? Did this work in previous versions of Draft.js?
Current version in latest Firefox (version 68.0.2) on macOS Mojave (10.14.5). The same problems present in latest Safari (version 12.1.1 ) (but add Hello maaan text to the editor).

@jalners
Copy link
Author

jalners commented Sep 27, 2019

Any updates?

@fmagaldea
Copy link

Same issue here with latest draft js (0.11.4).

@jalners
Copy link
Author

jalners commented Feb 3, 2020

It seems that something went wrong with this editor (in the latest Chrome, Firefox and Safari on the https://draftjs.org/ site):

  • Each keypress throws an error.
  • ctrl + A causes the same errors too.
  • Selecting text and adding some formatting (bold for example) doesn't work.

@mrkev
Copy link
Contributor

mrkev commented Feb 13, 2020

Yeah, issue is here #2280

@mrkev
Copy link
Contributor

mrkev commented Mar 23, 2020

#2280 is fixed.

@mrkev mrkev closed this as completed Mar 23, 2020
@jalners
Copy link
Author

jalners commented Mar 23, 2020

Re-open this bug please. Because editor works incorrectly after spellcheck replacement (steps 1-7).

@mrkev mrkev reopened this Mar 24, 2020
@jalners
Copy link
Author

jalners commented Mar 25, 2020

Thank you @mrkev.

@mrkev
Copy link
Contributor

mrkev commented Mar 25, 2020

Just repro-d using the steps provided. I don't see the exceptions, those might've been fixed. Essentially everything from step 8+ works for me. The text still autocorrects to Hello moanaan. after spellcheck though.

@SRugina
Copy link

SRugina commented Apr 14, 2020

This also occurs in Electron apps (albeit tested using Draft-js v0.10.5, but I suspect a version change will not fix this), where if using webContents' replace, replaceMisspelling, or insertText methods on text contained in a Decorator, a duplicate immutable piece of text is created at the start of the input field, which if attempted to be edited causes an Uncaught DOMException: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node that also means the input field disappears.

I.e. similar behaviour to this issue, and to this issue, but with Electron.

@chutchi2
Copy link

chutchi2 commented May 20, 2020

So I looked into this and I've characterized the issue a bit more. I don't know the code though, so I'm throwing this in here in case anyone who knows better can solve it before I can.

So when I use draftjs.org as my experiment area, I can produce the issue as follows.

Lets say I want to write Galley but misspell it galsey. I then format it as Ga ls e y.
This will create 4 groups to apply all the different formats as follows.

<div data-offset-key="b6bad-0-0" class="public-DraftStyleDefault-block public-DraftStyleDefault-ltr">
  <span data-offset-key="b6bad-0-1" style="font-weight: bold;">
    <span data-text="true">Ga</span>
  </span>
  <span data-offset-key="b6bad-0-2">
    <span data-text="true">ls</span>
  </span>
  <span data-offset-key="b6bad-0-3" style="text-decoration: italic;">
    <span data-text="true">e</span>
  </span>
  <span data-offset-key="b6bad-0-4">
    <span data-text="true">y</span>
  </span>
</div>

After when I finally have spell check applied, I will get Galleyy with the following html.

<div data-offset-key="b6bad-0-0" class="public-DraftStyleDefault-block public-DraftStyleDefault-ltr">
  <span data-offset-key="b6bad-0-0"  style="font-weight: bold;">
    <span data-text="true">Galley</span>
  </span>
  <span data-offset-key="b6bad-0-4">
    <span data-text="true">y</span>
  </span>
</div>

I've observed a few things:

  • The format for the first grouping will be applied to the entire newly spell corrected word.
  • No matter how many different formats are in the word, the last span grouping in the div group will always be left behind with a spell check.
  • The issue persists whether the spell corrected is less than, more than, or the same length of the misspelled word.

I believe logically that when the spell check is applied that it will collect all the span contents together into the first tag and remove the rest of the tags following. So I expect the bug is with the concatenation of the word removing the following tags and not removing the full length of them (e.g. remove the len-1 instead of just the len).

@mrkev
Copy link
Contributor

mrkev commented May 20, 2020

Ah, I think I know exactly why this happens, and it's related to other bugs. First two things to know for background:

  • Draft is built on top of contenteditable. contenteditable editors are managed (state, rendering) by the browser. What Draft does is "intercepts" contenteditable events, updates its state, and then uses it's state to render the editor through React.
  • Different browsers implement events slightly differently, so React builds on top top of browser events and abstracts their differences away.

In a nutshell, the issue is this:

  1. React sends a not-ideal beforeinput event (see Use the native beforeinput event if it's supported facebook/react#11211). This is not an InputEvent, but rather either a TextEvent or a KeypressEvent.
  2. This event tells us what text got added, but not what text got removed. InputEvents (which would be ideal here) can tell you if text got replaced.
  3. Draft has to guess what text got replaced, but its current implementation can only guess/replace text in a single leaf. (see editOnInput)

Therefore, if the text that is supposed to be replaced spans more than one leaf, only part of it gets replaced.

There's two ways to go about fixing this: a patch that tries to hack its way around this limitation, or a change in React fixing facebook/react#11211 and then an alternative implementation of editOnBeforeInput and editOnInput using the new, better events. To be honest, neither seem easy, but this is an issue that's definitely on the radar.

@jalners
Copy link
Author

jalners commented Jan 19, 2021

Are there any updates on this issue?

@evanschwarz90
Copy link

This is very closely related (possibly the same) as what I am experiencing. In my case, when using the native autocomplete in Chrome and Safari, it does a re-render with the "insert-characters" changeType first, which gives a concatenation of the partially completed word and the completed word (for example, I type "f" and then autocomplete with the word "for" and the "insert-characters" change will show "ffor"). Next, DraftJS handles this with another update with the changeType of "apply-entity" and the partial word being removed ("ffor" then becomes "for"). If the text has a CompositeDecorator involved then the second "correction" update is not taking place. My guess is that there is a missing test case in editOnInput.js.

@mrkev
Copy link
Contributor

mrkev commented Sep 20, 2021

I guess the biggest update is that unfortunately I've left Facebook, so I don't know the status of this anymore /:

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants