Details

Reviewers

tomek
ashoat

Commits

rCOMMf4bc51387b80: [native] Migrate fling functionality in FullScreenViewModal

Summary

https://linear.app/comm/issue/ENG-8150/convert-fullscreenviewmodal-to-reanimated-2-syntax

The fling function is triggered after pan ends. If the image is zoomed in and user moves the image with a pan gesture, the image moves a little with an inertia. Note that there recenter function will run instead if the image is moved to the edge of the screen.

The builds will fail due to some leftover reanimated v1 code. I'll remove it in the next diffs for diffs readability.

Depends on D14307

Test Plan

Tested after migration: there is an inertia effect after pan ended

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

angelika created this revision.Feb 6 2025, 5:39 PM

angelika held this revision as a draft.

Herald added a subscriber: ashoat. · View Herald TranscriptFeb 6 2025, 5:39 PM

angelika added a child revision: D14309: [native] Cancel animations when gesture is started in FullScreenViewModal.Feb 6 2025, 5:42 PM

Harbormaster failed remote builds in B33423: Diff 46952!Feb 6 2025, 5:52 PM

Rebase & bugfixes

angelika edited the summary of this revision. (Show Details)Feb 12 2025, 9:04 PM

angelika edited the test plan for this revision. (Show Details)

angelika published this revision for review.Feb 12 2025, 9:09 PM

Harbormaster failed remote builds in B33502: Diff 47086!Feb 12 2025, 9:18 PM

I'm not sure what the right diff to apply this feedback to is, but I noticed a difference in behavior while testing the changes on my iOS device.

old	new

Basically, in the old version it seems like the inertia on the x axis is continued despite the "snap back" recentering. In the new version, when the pan concludes it appears that the inertia disappears, and it seems a bit jarring to me.

Note that I performed this testing based on yarn arcpatch D14313, so if that diff hasn't been rebased on the latest changes in an earlier diff, please rebase it and I can try testing again.

tomek accepted this revision.Feb 17 2025, 3:58 AM

This revision is now accepted and ready to land.Feb 17 2025, 3:58 AM

Basically, in the old version it seems like the inertia on the x axis is continued despite the "snap back" recentering. In the new version, when the pan concludes it appears that the inertia disappears, and it seems a bit jarring to me.

Note that I performed this testing based on yarn arcpatch D14313, so if that diff hasn't been rebased on the latest changes in an earlier diff, please rebase it and I can try testing again.

I guess it might be connected with the next diff D14309 where we cancel all the animations when e.g. pan animation is started.

tomek mentioned this in D14309: [native] Cancel animations when gesture is started in FullScreenViewModal.Feb 17 2025, 4:01 AM

ashoat mentioned this in D14306: [native] Migrate backdropOpacityUpdate in FullScreenViewModal.Feb 19 2025, 5:51 PM

ashoat added inline comments.Feb 19 2025, 8:36 PM

native/components/full-screen-view-modal.react.js
927 ↗	(On Diff #47086)	In the old system, it seems like we would run the fling update only if a recenter wasn't occurring. Whereas now, we seem to run the fling update more often: whenever a pan ends that doesn't result in a dismissal. Currently, the recenter will override the fling update, so it probably works out okay.
928–940 ↗	(On Diff #47086)	I have a theory about the issue I shared earlier in the videos: On `panEnd`, we set `curX` and `curY` here. Then the `useAnimatedReaction` in D14307 runs, and it sets `curX` and `curY`, resulting in these animations being overriden. My guess is that the difference with the old system is that the X and Y coordinates were distinct. I think what we see in my "old" video is the X axis being handled by `flingUpdate`, and the Y axis being handled by `recenter`.

ashoat requested changes to this revision.Feb 19 2025, 8:36 PM

This revision now requires changes to proceed.Feb 19 2025, 8:36 PM

Rebase & bugfixes

angelika added inline comments.Feb 20 2025, 3:11 PM

native/components/full-screen-view-modal.react.js
927 ↗	(On Diff #47086)	Yes, in the old system we stop the fling clock if recenter is running. In the new system we override fling update which is kind of the same as stopping the clock.
928–940 ↗	(On Diff #47086)	Yes, that's correct. Fixed in https://phab.comm.dev/D14307

angelika planned changes to this revision.Feb 20 2025, 3:35 PM

Just leaving a note here that we should address this comment in this diff

Rebase

Harbormaster failed remote builds in B33582: Diff 47202!Feb 21 2025, 8:22 PM

In D14308#399961, @ashoat wrote:

Just leaving a note here that we should address this comment in this diff

Answering the comment:
I added cancelAnimation(curScale) when dismissing from pan. Now fling should be stopped when activeInteraction is true. In case activeInteraction is triggered by:

panActive turning to true: we explicitly cancel all animations in panStart
pinchActive turning to true: we explicitly cancel all animation in pinchStart
isRunningDismissAnimation: curX and curY are overridden and we explicitly cancel curScale
isRunningDoubleTapZoomAnimation: curX, curY and curScale are are overridden

This also applies to the recenter animation.

Fix rebase

Harbormaster failed remote builds in B33588: Diff 47208!Feb 21 2025, 10:27 PM

ashoat accepted this revision.Feb 21 2025, 11:47 PM

This revision is now accepted and ready to land.Feb 21 2025, 11:47 PM

Rebase

Harbormaster failed remote builds in B33604: Diff 47224!Feb 24 2025, 4:18 AM

This revision was landed with ongoing or failed builds.Feb 25 2025, 2:48 AM

Closed by commit rCOMMf4bc51387b80: [native] Migrate fling functionality in FullScreenViewModal (authored by angelika). · Explain Why

This revision was automatically updated to reflect the committed changes.

angelika added a commit: rCOMMf4bc51387b80: [native] Migrate fling functionality in FullScreenViewModal.