zooniverse / Panoptes-Front-End

Front end for zooniverse/Panoptes
https://www.zooniverse.org
Apache License 2.0
64 stars 75 forks source link

fix: catch Google Translate DOM errors #7096

Open eatyourgreens opened 1 month ago

eatyourgreens commented 1 month ago

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 https://github.com/facebook/react/issues/11538#issuecomment-417504600

Staging branch URL: None at this time (external PR contributor)

Describe your changes.

Required Manual Testing

Review Checklist

Optional

coveralls commented 1 month ago

Coverage Status

coverage: 56.98%. remained the same when pulling 23068a816303fa6c9338cdafcbecbc21d6b308e9 on eatyourgreens:fix-google-translate-crash into d270c1a2c9553654df5a32cca1705f503b3f5910 on zooniverse:master.

eatyourgreens commented 1 month ago

From Dan Abramov's original comment:

Finally, there is a workaround you can use that will fix the error. It will make your app a little bit slower, so it is up to you to try it and determine whether the tradeoff is worth it. But it's equivalent to what React would have to do if we were to fix it in React — so you wouldn't have a better solution anyway.

shaunanoordin commented 1 month ago

(Copy-pasting our follow up from our discussion from Slack)

Update on the "Chrome's Translate feature crashes PFE" bug , post-dev standup.

(Please see the Slack thread for additional comments on performance.)

eatyourgreens commented 1 month ago

We're not sure how to measure the performance hit. 🤷

Here's a couple of tools you can use, not just for this PR but to measure the performance of your code in general:

Here's a couple of example page speed tests for live Zooniverse projects:

Here are the performance scores from Page Speed Insights for both those pages. To be honest, both Zooniverse React apps are so slow that I suspect any additional slowness introduced by Dan Abramov's patch will be negligible by comparison. The DOM size is reasonably small for both, too.

App Mobile Desktop JS execution (mobile) JS execution (desktop) DOM size
Panoptes Front End 7% 21% 8.1s 1.8s 366
Monorepo 26% 32% 11.7s 3.9s 586

Also: kind of interesting that the new classifier is significantly slower to run on both desktop and mobile. I would have expected it to be faster.

eatyourgreens commented 1 month ago

if/when we hear more people complain

People generally won't complain when a site/app doesn't work (because the site didn't work, so how would they get the contact info?) However, these errors should be logged to Sentry, so you can use that to estimate how often this problem comes up.

Be wary of seeing an issue as an outlier or rarity, just because you don't often hear from people about it. If possible, use analytics and logging to find out who is struggling to use your site.

The Inaccessibility Cycle: inaccessible pages exclude people, who don't participate, so they don't contact you, so you see them as outliers, so you don't design for them.