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 stuck lightbox #6816

Merged
merged 2 commits into from
Nov 28, 2024
Merged

Fix stuck lightbox #6816

merged 2 commits into from
Nov 28, 2024

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Nov 28, 2024

Fixes https://bsky.app/profile/breakintoprogram.co.uk/post/3lbxo3eouis2v.

When you spam-tap the image many times, you may end up with nextLightbox changing midway through the animation:

  • First, nextLightbox is null (lightbox is closed)
  • Tap: nextLightbox is an object
  • Another quick tap: nextLightbox is a different object

This sequence would cause this effect to be cleaned up and re-set up:

React.useEffect(() => {
if (!nextLightbox) {
return
}
const canAnimate =
!PlatformInfo.getIsReducedMotionEnabled() &&
nextLightbox.images.every(
img => img.thumbRect && (img.dimensions || img.thumbDimensions),
)
// https://github.com/software-mansion/react-native-reanimated/issues/6677
requestAnimationFrame(() => {
openProgress.set(() =>
canAnimate ? withClampedSpring(1, SLOW_SPRING) : 1,
)
})
return () => {
// https://github.com/software-mansion/react-native-reanimated/issues/6677
requestAnimationFrame(() => {
openProgress.set(() =>
canAnimate ? withClampedSpring(0, SLOW_SPRING) : 0,
)
})
}
}, [nextLightbox, openProgress])

The logic there should handle that scenario fine, but it didn't. In particular, I used rAF to work around a Reanimated bug but bumped into a React Native bug. The expected order of operations in case of an interruption was:

  • Animate to 1
  • Effect cleanup: Animate to 0
  • Effect re-setup: Animate to 1

But due to the rAF callback order in React Native, we'd do:

  • Animate to 1
  • Effect cleanup: rAF(() => Animate to 0)
  • Effect re-setup: rAF(() => Animate to 1)
  • rAF: Animate to 1
  • rAF: Animate to 0

So we'd get stuck at 0 as the final frame and the screen would freeze in an inconsistent state.

The Fix

There are two parts to the fix:

  • Polyfill the correct rAF behavior (callbacks should be called in order). I didn't make this a global polyfill because I don't know where rAF is being used internally by other libraries. Don't want to risk Reanimated relying on the broken behavior somewhere inside. It's also harder to do this correctly if we wanted to support cancelAnimationFrame etc. So this is a scoped hackfix.
  • Disallow the "active lightbox" -> "another active lightbox" scenario at the state level. This doesn't completely remove the possibility of an interruption (maybe you could somehow spam the close button and the image) but at least it would ensure that the only possible flips are lightbox -> null and null -> lightbox which is easier to think about.
    • Nit: I'm not sure this is entirely true because in theory a fast lightbox -> null -> otherLightbox could get batched into lightbox -> otherLightbox. For now it seems unlikely, we can revisit with Fabric if that can happen.

Test Plan

First, go back to main. Do this for testing:

--- a/src/view/com/lightbox/ImageViewing/index.tsx
+++ b/src/view/com/lightbox/ImageViewing/index.tsx
@@ -67,13 +67,13 @@ const EDGES =
     : (['left', 'right'] satisfies Edge[]) // iOS, so no top/bottom safe area
 
 const SLOW_SPRING: WithSpringConfig = {
-  mass: isIOS ? 1.25 : 0.75,
+  mass: 100, // mass: isIOS ? 1.25 : 0.75,
   damping: 300,
   stiffness: 800,
   restDisplacementThreshold: 0.01,
 }
 const FAST_SPRING: WithSpringConfig = {
-  mass: isIOS ? 1.25 : 0.75,
+  mass: 100, // mass: isIOS ? 1.25 : 0.75,
   damping: 150,
   stiffness: 900,
   restDisplacementThreshold: 0.01,

Also add these logs:

+    console.log('queue 1')
     requestAnimationFrame(() => {
+      console.log('set 1')

...

+      console.log('queue 0')
       requestAnimationFrame(() => {
+        console.log('set 0')

Verify that you can still spam the lightbox into emitting:

 LOG  queue 1
 LOG  set 1
 LOG  queue 0
 LOG  queue 1
 LOG  set 1
 LOG  set 0

Note the flipped order of queue and set, that's the bug.

Then cherry-pick the first commit. Verify you can still spam the lightbox but you get:

 LOG  queue 1
 LOG  set 1
 LOG  queue 0
 LOG  queue 1
 LOG  set 0
 LOG  set 1

So it doesn't get stuck anymore.

Then undo the first fix (go back to requestAnimationFrame) and test the second commit. Verify that that fix alone is also sufficient on its own because you don't get interrupted at all:

 LOG  queue 1
 LOG  set 1

Finally, test with both commits. They should not interfere with each other.

For fun, you can also try this:

@@ -195,9 +195,9 @@ function ImageView({
 
   const containerStyle = useAnimatedStyle(() => {
     if (openProgress.get() < 1 || isFlyingAway.get()) {
-      return {pointerEvents: 'none'}
+      return {}
     }
-    return {pointerEvents: 'auto'}
+    return {}
   })

This disables the blocking overlay entirely. Verify you can't completely break the lightbox by spamming (despite more gestures being enabled) — it will eventually settle in some final state.

Try on both iOS and Android.

If you spam opening lightbox too fast, the effect that calls rAF will clean up and set up again midflight. Unfortunately, due to rAF order being unreliable, it may fire in reverse order, causing "open, open, close" instead of "open, close, open", so it would get stuck closed. This fixes the rAF order.
@arcalinea arcalinea temporarily deployed to fix-stuck-lb2 - social-app PR #6816 November 28, 2024 15:36 — with Render Destroyed
Copy link

Old size New size Diff
8.14 MB 8.14 MB 25 B (0.00%)

Copy link
Member

@mozzius mozzius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both fixes work independently and together, on both platforms. nice one!

@gaearon gaearon merged commit d08ce0d into main Nov 28, 2024
6 checks passed
@gaearon gaearon deleted the fix-stuck-lb2 branch November 28, 2024 16:55
gaearon added a commit that referenced this pull request Dec 4, 2024
* Fix lightbox getting stuck by fixing rAF order

If you spam opening lightbox too fast, the effect that calls rAF will clean up and set up again midflight. Unfortunately, due to rAF order being unreliable, it may fire in reverse order, causing "open, open, close" instead of "open, close, open", so it would get stuck closed. This fixes the rAF order.

* Don't allow opening another lightbox while it's open
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.

3 participants