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

Make React resilient to DOM mutations from Google Translate #11538

Closed
fritz-c opened this issue Nov 13, 2017 · 85 comments
Closed

Make React resilient to DOM mutations from Google Translate #11538

fritz-c opened this issue Nov 13, 2017 · 85 comments

Comments

@fritz-c
Copy link

fritz-c commented Nov 13, 2017

Coming from search? See workaround here: #11538 (comment). And star this issue: https://bugs.chromium.org/p/chromium/issues/detail?id=872770.

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

Bug, though there's a decent chance it's a Chrome/Google Translate one

What is the current behavior?

When using Google Translate on a page using React 16, a certain code pattern produces a Javascript error (Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.) when the rendered content changes.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via https://jsfiddle.net or similar (template for React 16: https://jsfiddle.net/Luktwrdm/, template for React 15: https://jsfiddle.net/hmbg7e9w/).

(This has only been checked on macOS 10.13.1)

  1. Navigate to https://qq49kwjynj.codesandbox.io/ in a Chrome browser set to some language other than Japanese.
  2. Right click the page and select "Translate to English"
  3. Click the checkbox, and the error will show.

The source of the example can be found at https://codesandbox.io/s/qq49kwjynj
The part of the code that seems to cause it is the following two lines:

{this.state.checked && "選択済み"}
{!this.state.checked && "無選択"}

Changing this to the following fixes the behavior with Google Translate:

{this.state.checked ? "選択済み" : "無選択"}

What is the expected behavior?

It should not produce an error.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

I created an identical example with React 15 at the following pages:
https://p93xxmr0rq.codesandbox.io/
https://codesandbox.io/s/p93xxmr0rq
When repeating the same steps outlined above, no error was produced.
It only seems to affect React 16.
As this is a Chrome-only feature, it only affects Chrome.

@clemmy
Copy link
Contributor

clemmy commented Nov 13, 2017

Thanks for the report - I can also reproduce this bug.

@gaearon
Copy link
Collaborator

gaearon commented Nov 13, 2017

Could be related? #9836

@Tom-Bonnike
Copy link

Tom-Bonnike commented Nov 23, 2017

We can also reproduce this bug on our own app. Thanks for the report.

Edit: About the next comment: we also ended up applying this “fix”. It obviously hurts accessibility and users are still triggering this error by right clicking “Translate to X” on Chrome as this meta tag only removes the top-bar translation suggestion.

@MartijnHols
Copy link

MartijnHols commented Dec 7, 2017

I "fixed" my app for users running into this by disabling Chrome's translation using this method (until the underlying issue is fixed): https://stackoverflow.com/a/12238414/684353

Edit 2024: Skip to my write-up that covers the entire issue and all of the listed fixes: #11538 (comment) (the one in the OP is broken)

@f0urfingeredfish
Copy link

f0urfingeredfish commented Jan 22, 2018

This error also occurs if you use https://localizejs.com for translation. I'm assuming since they manipulate the DOM and React can't reconcile the changes. This took forever to track down and I'm putting it here in case anyone else is trying use localizejs and React16. The issue is with localizejs.

The workaround is to add notranslate tags to the containers of your react components so that localize doesn't mutate the dom. Obviously they will stop getting translated until localizejs comes up with a fix.

  import { render } from 'react-dom'

  const appRoot = document.getElementById('app')
  appRoot.setAttribute('notranslate', true)
  render(<App />, appRoot)

UPDATE
Localize.js support confirmed it is a bug on their end and they are working on a fix.

In the meantime this is the solution we are using:
https://gist.github.com/f0urfingeredfish/44a3c12b95caf157ad06839ba4b98524

Usage

translate string localize`translate me`
translate string but not expressions localize`translate me ${notMe}`
translate string and expressions localizeAll`translate me ${andMeToo}`
translate expressions aka variables localizeAll`${translateMe}`

@vsiao
Copy link
Contributor

vsiao commented Feb 7, 2018

Failed to execute 'removeChild' on 'Node' can be caused by various chrome extensions that replace text on a page. For example, one of our users reported that the "3CX Click to Call" extension was also causing this crash.

@gaearon can you provide any guidance about if and when React could be patched to be resilient against this sort of bug?

@webmobiles
Copy link

4 of 7 customers had this problem with react, but it's not happen in all computers, chrome last version 63 working ok for some users but not in others, so it's produces by: no use some keys in react elements, google translate plugins and anothers plugins.. the only solutions is to tell people turn of his plugins...

@hyperknot
Copy link

hyperknot commented Apr 2, 2018

I've just run into this issue as well, which was really hard to figure out. The only clue was that all the strings in Sentry reports were non-English.

How is this not causing problems across all major websites using React 16? Any extension modifying the DOM (which there are a lot, translators, password managers, etc.) can totally break a React website.

The only workaround I found right now is to:

<meta name="google" content="notranslate">

As recommended above.

@hyperknot
Copy link

Is there any workaround for this? How is it solved on facebook.com, where Chrome translate works with React 16?

@webmobiles
Copy link

it's ok:

with that works, it's means you can't use google translate same time .. but I did realize to repair some errors still without
for that I had to implements "key" properties everywhere in my still when react did not complaint about keys absents and another things and another things that i don't remember well, it's about the render, google translate do lost some keys ...

@gaearon
Copy link
Collaborator

gaearon commented Apr 10, 2018

If you want to help fix this, please create a small reproducing case that doesn't involve extensions, but manually reproduces what they might do to the DOM. Then we can take a look.

@hyperknot
Copy link

hyperknot commented Apr 10, 2018

@gaearon: @fritz-c 's original snippet doesn't involve extensions:
https://codesandbox.io/s/qq49kwjynj

Full page here:
https://qq49kwjynj.codesandbox.io/

@gaearon
Copy link
Collaborator

gaearon commented Apr 10, 2018

It involves using Google Translate. I'm asking to create a reproduction case that does what Google Translate would do, but with DOM API calls. So that we can see what exactly is causing the issue.

@hyperknot
Copy link

I don't think anyone can answer that, except Google Chrome team. I'm not even sure if Translate is part of the open source Chromium project.

@gaearon
Copy link
Collaborator

gaearon commented Apr 10, 2018

I don't think you need to know the internals of what Google Translate is doing. Look at DOM before and after for a single word, set some DOM breakpoints, and that should tell you the manipulations necessary to reproduce it. A mutation observer can help too.

@hyperknot
Copy link

hyperknot commented Apr 10, 2018

OK, just simply looking at the DOM it's obvious that it gets changed significantly:

original
<div>無選択</div>

translated:
<div>
  <font style="vertical-align: inherit;">
    <font style="vertical-align: inherit;">No choice</font>
  </font>
</div>

It's interesting that this works well in React 15.

@gaearon
Copy link
Collaborator

gaearon commented Apr 10, 2018

Right, so I encourage you to write a minimal case in CodeSandbox that does similar mutations and try to reproduce the problem 🙂

@hyperknot
Copy link

I did the mutation observers, but haven't been able to replicate it in a way which wouldn't break React 15 as well. I pass it on to a more experienced person from here:
https://codesandbox.io/s/lrz2zwp5wl

@fritz-c
Copy link
Author

fritz-c commented Apr 11, 2018

I used Chrome DOM breakpoints to see what Google Translate was doing under the hood, and created a minimal reproduction that closely emulates how it replaces text.
Demo: https://5k0q7pl5y4.codesandbox.io/
Source: https://codesandbox.io/s/5k0q7pl5y4

Now with cross-browser compatibility, it breaks in Safari, Firefox and Chrome.

The key lines are at the bottom:

// Get the text node "checked"
const myEl = document.querySelector("div > div > div").childNodes[0];

// Create an arbitrary font element to replace it with
const fontEl = document.createElement("font");

myEl.parentElement.insertBefore(fontEl, myEl);
myEl.parentElement.removeChild(myEl);

This puts the DOM node in a state that will cause the next update by React to throw an error.

I ran the same mutation code in the demo for React 15 (changing .childNodes[0] to .childNodes[1] to select the "checked" text instead of the <!-- react-text: 6 --> comment), and no errors were thrown. So I will declare that this is as close as I can get to a reproduction of the Google Translate behavior.

@gaearon
Copy link
Collaborator

gaearon commented Apr 27, 2018

Thanks! Any idea why it inserts <font> tags instead of modifying the text node? There's probably a good reason for this but it's not obvious to me.

@fritz-c
Copy link
Author

fritz-c commented Apr 27, 2018

I have no idea. If I had to venture a guess, I'd say it's related to how they incrementally translate large blocks of text (only what's on the screen), and inconsistency between browsers in how bare text nodes are handled.

@shuhei
Copy link
Contributor

shuhei commented May 19, 2018

Adding a bit more of information. The examples below are based on the ones by @hyperknot and @fritz-c.

The problem is that Google Translate replaces text nodes with <font> tags containing translations while React keeps references to the text nodes that are no longer in the DOM tree.

React throws in the following cases:

  1. A text node is conditionally rendered and it's not the only child of its parent. Then React calls parent.removeChild(textNode) when the text node is removed and throws because textNode is no longer a child of parent. https://codesandbox.io/s/74k8lz417x
  2. A node before a text node is conditionally rendered. Then React calls parent.insertBefore(someNode, textNode) when the node is inserted and throws because textNode is no long a child of parent. https://codesandbox.io/s/q7n4mk7m86
// Case 1
<div>
  {condition && 'Welcome'}
  <span>Something</span>
</div>

// Doesn't throw
<div>
  {condition && 'Welcome'}
</div>

// Case 2
<div>
  {condition && <span>Something</span>}
  Welcome
</div>

Workaround

We can avoid these errors by invalidating the conditions above. The easiest workaround is to wrap those text nodes with <span> so that nodes referenced by React will stay in the DOM tree even though their contents are replaced with <font> tags.

// A workaround for case 1
<div>
  {condition && <span>Welcome</span>}
  <span>Something</span>
</div>

// A workaround for case 2
<div>
  {condition && <span>Something</span>}
  <span>Welcome</span>
</div>

@jasonrhodes
Copy link

Just curious, has anyone found an existing eslint rule that warns or errors on this? Our Sentry logs have been filled with these errors for a few months now, so it's great to identify the issue. Not sure we'll be able to have our team "remember" to do this convention, though, but it seems lintable.

@jaller94
Copy link

What is React’s common behaviour for dealing with modified DOM trees? I assume it is to override the changes and regain certainty about the DOM’s state.

Would it be ok to fully invalidate and rerender the currentParent, if removeChild() fails?

removeChild((currentParent: any), node.stateNode);

@gaearon
Copy link
Collaborator

gaearon commented Aug 7, 2018

@shuhei Thanks for great analysis.

@MartijnHols
Copy link

MartijnHols commented May 5, 2024

I would recommend against using the monkeypatch-workaround listed in #11538 (comment). It only makes things worse. Normally a machine translation induced crash leads to a rendering crash in React, triggering the closest error boundary. The monkeypatch changes this so React will simply skip removing the old content and add the new content alongside it. I believe misleading the user is worse than telling them nothing at all.

I made a demo to showcase this: https://martijnhols.nl/gists/demo/react-translation-reproduction. Now imagine instead of clicks, it showed money owed or something else vital. Showing wrong information could be very bad.

The solution proposed earlier of wrapping content in error boundaries that simply rerender the children will also not work, as any components in the subtree will still lose their state when this gets triggered.

The only two options I would deem viable are to block machine translation (as I suggested back in 2017) or wrap the content in spans. You could also consider some funny business where you detect machine translation and show a warning, but that won't help users too much (unless your app has i18n and you can change your apps language into the one they wanted).

eatyourgreens added a commit to eatyourgreens/Panoptes-Front-End that referenced this issue May 9, 2024
Monkey-patch `node.removeChild` and `node.insertBefore` to catch crashing errors generated by Google Translate. This should make projects usable in Chrome, in other languages, until the underlying bug is fixed.

See facebook/react#11538 (comment)
eatyourgreens added a commit to eatyourgreens/Panoptes-Front-End that referenced this issue May 9, 2024
Monkey-patch `node.removeChild` and `node.insertBefore` to catch crashing errors generated by Google Translate. This should make projects usable in Chrome, in other languages, until the underlying bug is fixed.

See facebook/react#11538 (comment)
eatyourgreens added a commit to eatyourgreens/Panoptes-Front-End that referenced this issue May 19, 2024
Monkey-patch `node.removeChild` and `node.insertBefore` to catch crashing errors generated by Google Translate. This should make projects usable in Chrome, in other languages, until the underlying bug is fixed.

See facebook/react#11538 (comment)
@reubenjh
Copy link

For those asking for an eslint rule for this - i've found this one works well https://github.com/sayari-analytics/eslint-plugin-sayari

@MartijnHols
Copy link

I dove into the issues of Google Translate some more and wrote up my findings in the following article: Everything about Google Translate crashing React (and other web apps). In it I write about the behavior of Google Translate that leads to the interference that breaks React (and other) apps, the exact issues you will run into with 5 different reproduction samples, how each of the workarounds proposed here affect it, and what I think might be the best solution for now. There are a couple of new things in this article that haven’t been mentioned in this thread yet:

Unfortunately the only ready-to-go “workaround” I can think of is the same as I posted here back in 2017. Wrapping TextNodes with <span>s is also an option, but takes a fair bit more effort and doesn't solve everything. There might be one possible workaround (outside of patching React or Google Translate) of hooking the __REACT_DEVTOOLS_GLOBAL_HOOK__ and forcing React to remount the parents of changed TextNodes if Google Translate is active, but it seemed too much work for me to dive into as the renderer used in the Devtools is over 4500 lines long.

@maxiedaniels
Copy link

@MartijnHols Amazing writeup! That's interesting about ternaries - I've been wondering why i'm still seeing these errors on Sentry and haven't been able to track them down.. I suspect it's because of that. What is the solution for ternaries??

@hapo-sonnx
Copy link

hapo-sonnx commented Jun 3, 2024

I fixed the error in my application by changing
{this.state.checked && "選択済み"}
to
{this.state.checked ? "選択済み" : ""},
and wrapped the text in the Button input box as follows:
<Button><span>ホームへ</span></Button>.

@hata6502
Copy link

Google Translate can be controlled even in React with translate="no" attribute and useTranslation hook.
https://jvvmnt-5173.csb.app/

@Strapchay
Copy link

Strapchay commented Jul 1, 2024

I had the same issue and this wasn't google translate specific. The error occurs because react still has state of the element being removed and can't find it in the component. This can be caused by arbitrarily removing an element being conditionally rendered by react. Take for example:

<div id="parent">
{childValue && <div>...</div>}
</div>

Now if probably based on an event in the childValue or on the parent value, you then do something like:

function handleEvent(e){
 e.target.textContent = null 
 //OR
 parentRef.current.textContent = null
}

That error would occur because, the state of the childValue still exist and its a derivedElement based off that conditional. So instead of having to remove it arbitrarily, you specifically need to deal with it through the childValue by setting childValue to null or whatever way fits your usecase

Ihara-kenta added a commit to Ihara-kenta/generative-ai-use-cases-jp that referenced this issue Jul 9, 2024
- Fix JavaScript errors occurring when Chrome's translation feature is enabled
- Resolve conflicts between elements inserted by the translation feature and application's DOM manipulation

References:
- facebook/react#11538 (comment)

Related Issue: aws-samples#518
Ihara-kenta added a commit to Ihara-kenta/generative-ai-use-cases-jp that referenced this issue Jul 9, 2024
- Fix JavaScript errors occurring when Chrome's translation feature is enabled
- Resolve conflicts between elements inserted by the translation feature and application's DOM manipulation

References:
- facebook/react#11538 (comment)

Related Issue: aws-samples#518
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.