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

[Proof of Concept] Make React resilient to Google Translate #13341

Closed
wants to merge 2 commits into from

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Aug 7, 2018

Maybe fixes #11538.

Google Translate modifies the DOM in an unsupported way: #11538 (comment). It seems like what it does is pretty limited though (wrapping text nodes into DOM elements). Given how often Google Translate is used, it seems like it would be nice to be resilient to this particular case.

My strategy is that if parentNode is missing we know somebody messed with our DOM. We search for a node at the same position in the host parent as the now-detached node. We do this by reusing the existing Fiber host sibling search logic that we apply at the commit phase. If we find a node at that position, and it's not managed by React, we assume it's the node we need to use for best effort. Worst case — we'll crash anyway.

The downside is that this adds a single .parentNode equality check on every insertion and deletion. Maybe there's a more scoped strategy. But maybe this is okay?

@nhunzaker
Copy link
Contributor

This might also fix the detachment issue in IE9 if we can't figure out how to avoid using the IE8 onpropertychange API for change events in IE9 (#11609)

@pull-bot
Copy link

pull-bot commented Aug 7, 2018

ReactDOM: size: 🔺+0.3%, gzip: 🔺+0.2%

Details of bundled changes.

Comparing: 3b3b7fc...90a9d78

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.1% +0.2% 643.97 KB 644.89 KB 151.3 KB 151.56 KB UMD_DEV
react-dom.production.min.js 🔺+0.3% 🔺+0.2% 96.37 KB 96.63 KB 31.23 KB 31.31 KB UMD_PROD
react-dom.development.js +0.1% +0.2% 640.11 KB 641.03 KB 150.12 KB 150.39 KB NODE_DEV
react-dom.production.min.js 🔺+0.3% 🔺+0.3% 96.36 KB 96.62 KB 30.77 KB 30.86 KB NODE_PROD
ReactDOM-dev.js +0.2% +0.2% 647.35 KB 648.37 KB 148.44 KB 148.7 KB FB_WWW_DEV
ReactDOM-prod.js 🔺+0.3% 🔺+0.4% 278.39 KB 279.31 KB 52.24 KB 52.45 KB FB_WWW_PROD
react-dom.profiling.min.js +0.3% +0.3% 97.49 KB 97.75 KB 31.17 KB 31.25 KB NODE_PROFILING
ReactDOM-profiling.js +0.3% +0.4% 281.68 KB 282.6 KB 52.97 KB 53.16 KB FB_WWW_PROFILING

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js 0.0% -0.0% 349.24 KB 349.24 KB 75.71 KB 75.7 KB NODE_DEV
react-reconciler.production.min.js 0.0% -0.1% 47.73 KB 47.73 KB 14.47 KB 14.47 KB NODE_PROD
react-reconciler-persistent.development.js 0.0% -0.0% 347.85 KB 347.85 KB 75.16 KB 75.14 KB NODE_DEV
react-reconciler-persistent.production.min.js 0.0% -0.0% 47.74 KB 47.74 KB 14.48 KB 14.47 KB NODE_PROD
react-reconciler-reflection.development.js +14.0% +10.8% 14.24 KB 16.22 KB 4.47 KB 4.96 KB NODE_DEV
react-reconciler-reflection.production.min.js 🔺+18.7% 🔺+12.6% 2.55 KB 3.02 KB 1.13 KB 1.27 KB NODE_PROD

Generated by 🚫 dangerJS

@nhunzaker
Copy link
Contributor

Does this handle the case where Japanese translations are wrapped in a font tag? Like:

Before
After

Does the Fiber have a reference to the parent node? To handle insertion of extra markup, Instead of checking document position, could we instead force an update of the closest parent component?

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 7, 2018

Aren’t all translation wrapped in a font tag? I thought that’s the case I was implementing — is what you’re showing different?

could we instead force an update of the closest parent component?

I think it sounds simpler than it is. We’re already in the commit phase so we can’t re-render without finishing first. Then it’s not clear why a re-render would be helpful. What we really want is to re-create the nodes that were replaced. But that requires us to add logic that walks the DOM and compares them. That’s kind of what hydration does, but we’d have to do it on an already mounted tree. It sounds significantly more complicated than what I’m doing.

@nhunzaker
Copy link
Contributor

Aren’t all translation wrapped in a font tag?

😶 Yes. Sorry, this was in the PR description but I confused myself.

What we really want is to re-create the nodes that were replaced. But that requires us to add logic that walks the DOM and compares them. [...] It sounds significantly more complicated than what I’m doing.

It is more complicated. Additionally, purging the DOM elements to do a clean re-render has a lot of problems too.

I like this approach. Can you think of any case where a re-render would cause canonical text to show up adjacent to translated text?

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 7, 2018

I like this approach. Can you think of any case where a re-render would cause canonical text to show up adjacent to translated text?

Not sure what you mean.

@nhunzaker
Copy link
Contributor

Like if a future render could cause the untranslated text to show up next to the translated text. Thinking on it a bit more, I don't think it is an issue.

Interestingly, it looks like Chrome is using MutationObserver to dynamically translate the text when the contents changes:

  1. Visit https://ia.net/ja/
  2. Translate
  3. Modify the contents of the headline back to Japanese with document.querySelector('h1').innerHTML = 'ミニマルデザインで効果最大'
  4. The content is re-translated

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 8, 2018

I've spent some time making a comprehensive test suite for this and it uncovered many edge cases where this code wouldn't work. Probably because by the time we're in the commit phase we can't actually rely on Fibers and nodes "lining up" — they've already been modified.

@nhunzaker
Copy link
Contributor

@gaearon is there anyway that you could share that test suite? I'd like to play around with this a bit too.

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 8, 2018

Sure. #13347

@gaearon gaearon closed this Aug 8, 2018
@gaearon
Copy link
Collaborator Author

gaearon commented Aug 30, 2018

For future reference, see a workaround: #11538 (comment)

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

Successfully merging this pull request may close these issues.

Make React resilient to DOM mutations from Google Translate
4 participants