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

Fix crash when using Safari Translation feature #124

Merged
merged 1 commit into from Oct 7, 2023

Conversation

zigcccc
Copy link
Contributor

@zigcccc zigcccc commented Sep 28, 2023

Summary

This PR is to address an issue where the app would crash (unhandled exception is thrown) in Safari, when the following is true:

  • Safari's translation feature is enabled
  • at least one LinesEllipsis with and one without truncated text is first rendered on the screen and then unmounted

Steps to reproduce

  • Render 2 LinesEllipsis components on the screen; 1 should truncate the text and one should not
    • in my case I set basedOn="letters" but I think that doesn't really play a role
  • Open the page in Safari (mobile/desktop doesn't matter)
  • Translate the page to something other than what the original language was; make sure that the text inside LinesEllipsis gets translated
  • unmount the components
  • 💥

Minimal reproducible example

https://github.com/zigcccc/react-lines-ellipsis-crash-minimal-reproducible-example

Demo

React.Lines.Ellipsis.Safari.Crash.mp4

The problem

  • for some reason, when Safari applies translations, it manipulates DOM nodes in a way to trigger this.canvas to be null
  • in componentWillUnmount, we're assuming that this.canvas is always defined
  • since that's not the case, we're accessing property (parentNode) on null, which causes the crash
image

I'm not sure how exactly that happens, and why this is scoped to Safari only (applying translations in Chrome works without any issues). But simply verifying that we have a canvas element to work with before doing any operations on it prevents the app from crashing, which is the goal of this PR.

@adamjaffeback
Copy link

Thanks @zigcccc for opening this PR. We're getting hundreds of errors per week because of this on a payments platform; it's blocking people from collecting a paycheck. Hope @xiaody can help you move fast on this—it's such a minor change. Good find!

@adamjaffeback
Copy link

Introduced here #104

@srajchevski
Copy link

We've been stuck on this for a while, and just moved it to the backlog...
Would be nice if this could get fixed soon!!

@xiaody xiaody merged commit e225807 into xiaody:master Oct 7, 2023
1 check passed
@xiaody xiaody self-requested a review October 7, 2023 08:31
@xiaody
Copy link
Owner

xiaody commented Oct 7, 2023

Released as react-lines-ellipsis@0.15.4. Thx everyone.

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.

None yet

4 participants