Repository navigation
Conversation
…ring keyframe Reanimated hides an element with an entering keyframe until the animation starts, but it can delete the keyframe before the next animation frame (for example in a background tab or during a busy startup). The backdrop then stays hidden for good. Animate opacity with a shared value, like Container does. Co-authored-by: nkdengineer <nkdengineer@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
|
🤖 Android test steps verification results: The PR's Tests section is still a TODO, so I used the steps from the linked issue plus a backdrop regression check. Sign-in worked and the app ran without errors or crashes. However, the confirm-modal backdrop checks were not completed: tapping Sign out while online signs you out right away by design (src/hooks/useSignOut.ts:73-75), so no modal appeared, and signing back in needed a magic code. This is a test-plan gap, not a regression. The PR only changes
|
|
🤖 Web test steps verification results: The app loaded cleanly, and a confirm modal ("Clear cache and restart") opened with a dimmed backdrop and closed fully. The PR's main repro was blocked: the test browser can't set the
|
|
|
||
| useEffect(() => { | ||
| if (!isBackdropVisible) { | ||
| progress.set(0); |
There was a problem hiding this comment.
@MelvinBot Let's remove this line; we shouldn't need to set the progress to 0 again as we need to keep the opacity for exiting animation.
There was a problem hiding this comment.
Done. I removed progress.set(0) in b93073a. When the backdrop hides, the effect now returns early, so the exiting fade-out keeps its opacity.
Lint, typecheck, and ReanimatedModalBackdropWebTest pass.
One edge case: if the modal reopens while the parent backdrop is still mounted on web, progress is already 1. The backdrop then shows at full opacity without fading in. Let me know if you want me to reset progress to 0 right before the fade-in starts.
There was a problem hiding this comment.
@MelvinBot Yeah, let's reset progress to 0 right before fade-in starts.
…m full opacity Co-authored-by: nkdengineer <nkdengineer@users.noreply.github.com>
|
@codex Review. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Co-authored-by: nkdengineer <nkdengineer@users.noreply.github.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f360bdc8c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| progress.set(0); | ||
| progress.set(withTiming(1, {duration: animationInTiming, easing, reduceMotion: ReduceMotion.Never})); | ||
| }, [animationInTiming, isBackdropVisible, progress]); |
There was a problem hiding this comment.
When an open centered or right-docked modal crosses the small-screen breakpoint, Modal/index.tsx changes animationInTiming (for example, from the desktop 120/150 ms timing to 300 ms). That dependency reruns this effect and resets progress to zero, making the already-visible backdrop disappear and fade in again even though the modal was never reopened. Trigger the reset only on a false-to-true visibility transition, or otherwise guard timing-only updates.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in 3123b35. A wasBackdropVisible ref now tracks the last visibility. The effect resets progress and fades in only when the backdrop goes from hidden to visible. If only animationInTiming changes, the visible backdrop stays as it is.
I used a ref instead of usePrevious because usePrevious starts with the current value. With it, a backdrop that mounts as visible would skip its first fade-in.
Lint, typecheck, and ReanimatedModalBackdropWebTest pass.
Co-authored-by: nkdengineer <nkdengineer@users.noreply.github.com>
|
@codex review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3123b353a8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| progress.set(0); | ||
| progress.set(withTiming(1, {duration: animationInTiming, easing, reduceMotion: ReduceMotion.Never})); |
There was a problem hiding this comment.
When isBackdropVisible changes false→true without unmounting Backdrop—for example, when the raw isVisible prop is toggled during an in-progress modal transition—progress still contains its previous nonzero value when the Animated.View is reinserted. Because this reset runs in a passive effect, the backdrop can paint once at the stale opacity, then snap to zero and begin fading in; the former entering keyframe kept it hidden until animation start. Reset the value before paint or otherwise ensure the newly inserted view initially receives zero opacity.
Useful? React with 👍 / 👎.
|
@MelvinBot Please address this by switching the fade-in effect from
Keep everything else as it is: the |
… never paints at the old opacity Co-authored-by: nkdengineer <nkdengineer@users.noreply.github.com>
|
Done in 6ce73bd. The fade-in effect now uses I added a comment above the effect that explains why it must stay a layout effect. The Lint, typecheck, and |
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
This branch has not been deployed
2 participants
Explanation of Change
On web, the modal backdrop faded in through a Reanimated
enteringkeyframe. Reanimated renders such an element withvisibility: hidden, applies the keyframe on the nextrequestAnimationFrame, and shows the element again only inonanimationstart. It also schedules a timer (about 160 ms with our 1 ms web duration) that deletes the generated@keyframesrule. If the next animation frame arrives after that timer, for example in a background tab or during a busy startup, the animation never starts and the backdrop stays hidden for good. That's why the app review modal, which opens by itself at startup, sometimes showed without the dimmed backdrop.This PR fades the backdrop in with a shared value and
withTiming, the same wayContaineralready fades the modal card in. Theenteringkeyframe is removed, so Reanimated never hides the element. A late frame can only delay the fade. TheExitingkeyframe stays as is, because exiting animations are applied right away. This fixes every modal that uses this backdrop on web. Native is not affected.I also added a unit test that checks the web backdrop renders without an
enteringanimation. It fails onmainand passes with this change.Fixed Issues
$ #104039
PROPOSAL: #104039 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
// Please describe what tests you performed that validates your changed worked.
Offline tests
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
// Please describe what QA needs to do to validate your changes and what areas do they need to test for regressions.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari