-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Make animated components use different tags for events #5960
Conversation
I haven't checked detail, but |
@gronxb if you were able to provide a small example where the react-native-bottom-sheet behaves strangely I’d be able to check it out and make sure it works as well |
I've been busy and haven't prepared a repro, I'll do it as soon as I can. When calling Pressable onPress from within the bottom sheet, the bottom sheet is forced down. |
@gronxb I'm sorry to bother you but we are about to go through with reviewing this PR - do you maybe have time to provide the example of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it's fine for me that we might have two tags, first for events and the second for other operations, I think that we should focus on the "why" here.
-
How comes that we actually need two different tags? Do all operations that require
_componentViewTag
work properly when_eventViewTag
is correct? Or is it a "switch-like" situation? -
Why didn't we need it before? Which change exactly forces us to make the split here? Or maybe it was always like this and only now we have detected it?
-
What about
useScrollViewOffset
here? Is it always able to get the tag it actually requires?
We definitely need to try to answer those questions.
Sorry for the delay, here's the reproduction, I think it's the backdrop, if you press anywhere on the bottom sheet, it forces it to go down. If I don't apply that patch, it only goes down normally when I press backdrop. It seems like something conflicts and thinks backdrop is pressed even if you press anywhere. https://github.com/gronxb/reanimated-repro/blob/main/App.tsx
2024-05-07.12.02.04.mov
2024-05-07.12.09.12.mov |
@tjzel I think all of the questions have answers:
The
We had it before, but the implementation wasn't well readable and clear. It worked and then was removed in WorkletEventHandler revamp under the assumption that is was redundant. What I'm doing here is reverting it and making it readable so that anyone that stumbles upon it in the future will know why it is this way.
Yeah, native implementation needs some changes in this regard. Thanks for pointing that out, I will add it to this PR. |
@gronxb alright I found the issue, could you please try out this patch - |
@szydlovsky it works fine 👍 |
@tjzel |
@szydlovsky I assume you tested it natively on iOS only? Android can have a different view hierarchy, please check it too. Is this PR something that could be verified by runtime tests? |
@tjzel I tested it on all: iOS and Android (both Paper and Fabric) and Web. Works fine everywhere. If it comes to runtime tests, we would have to use |
Oh, that's great then! We can list those as dev dependencies, that's not a problem, but let's not incorporate 3rd party libraries into our runtime tests yet. I will add it as a potential task for the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but I'd like @piaskowyk or @tomekzaw to take a look at it as well.
@szydlovsky Works perfectly. |
@szydlovsky I am facing this issue #4730. |
@sraut1 This patch will be in the next release after 3.11. And no, this one most likely doesn't fix the issue since it has nothing to do with animated styles. Nevertheless, feel free to use the patch, maybe it sorts something out. |
@szydlovsky the patch fixes issue #4730. thanks |
@sraut1 woah, I'll let the ppl know then 🤯 |
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [react-native-reanimated](https://togithub.com/software-mansion/react-native-reanimated) | [`~3.11.0` -> `~3.12.0`](https://renovatebot.com/diffs/npm/react-native-reanimated/3.11.0/3.12.0) | [![age](https://developer.mend.io/api/mc/badges/age/npm/react-native-reanimated/3.12.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/react-native-reanimated/3.12.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/react-native-reanimated/3.11.0/3.12.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/react-native-reanimated/3.11.0/3.12.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>software-mansion/react-native-reanimated (react-native-reanimated)</summary> ### [`v3.12.0`](https://togithub.com/software-mansion/react-native-reanimated/releases/tag/3.12.0) [Compare Source](https://togithub.com/software-mansion/react-native-reanimated/compare/3.11.0...3.12.0) #### What's Changed - Give more meaningful warning when modifying a Shareable by [@​tjzel](https://togithub.com/tjzel) in [software-mansion/react-native-reanimated#5548 - Make animated components use different tags for events by [@​szydlovsky](https://togithub.com/szydlovsky) in [software-mansion/react-native-reanimated#5960 - Add `warning` and `failing` test decorators by [@​Latropos](https://togithub.com/Latropos) in [software-mansion/react-native-reanimated#5929 - Update useAnimatedKeyboard docs by [@​maciekstosio](https://togithub.com/maciekstosio) in [software-mansion/react-native-reanimated#5866 - Change the docs to mention, that `.springify()` works with `.duration()` by [@​Latropos](https://togithub.com/Latropos) in [software-mansion/react-native-reanimated#5990 - \[Android]\[Keyboard] More consistent inequality check to compute keyboard state by [@​antFrancon](https://togithub.com/antFrancon) in [software-mansion/react-native-reanimated#5874 - fix typo on object key for `targetValues` in custom-animations.mdx by [@​JDMathew](https://togithub.com/JDMathew) in [software-mansion/react-native-reanimated#5994 - Remove outdated code for unsupported React Native versions by [@​tomekzaw](https://togithub.com/tomekzaw) in [software-mansion/react-native-reanimated#5979 - Remove REAInitializer by [@​tomekzaw](https://togithub.com/tomekzaw) in [software-mansion/react-native-reanimated#5681 - Prevent crash on non-existent view updates in Android by [@​thomas-rx](https://togithub.com/thomas-rx) in [software-mansion/react-native-reanimated#5767 - Fix location after shared element transition by [@​piaskowyk](https://togithub.com/piaskowyk) in [software-mansion/react-native-reanimated#6010 - Make `useScrollviewOffset` ref nullable and simplify its code by [@​szydlovsky](https://togithub.com/szydlovsky) in [software-mansion/react-native-reanimated#6009 - fix: use proper classes for bridgeless by [@​WoLewicki](https://togithub.com/WoLewicki) in [software-mansion/react-native-reanimated#5997 - docs: add [@​swmansion/t-rex-ui](https://togithub.com/swmansion/t-rex-ui) by [@​patrycjakalinska](https://togithub.com/patrycjakalinska) in [software-mansion/react-native-reanimated#6015 - Remove `CellRendererComponent` from Animated.FlatList props by [@​Latropos](https://togithub.com/Latropos) in [software-mansion/react-native-reanimated#5951 - Add `useComposedEventHandler` hook by [@​szydlovsky](https://togithub.com/szydlovsky) in [software-mansion/react-native-reanimated#5890 - Align handling colors with RN by [@​maciekstosio](https://togithub.com/maciekstosio) in [software-mansion/react-native-reanimated#5825 - Add more tests - useSharedValue, useAnimatedStyle , useDerivedValue by [@​Latropos](https://togithub.com/Latropos) in [software-mansion/react-native-reanimated#5981 - Make animation RollInLeft work with modifers by [@​Latropos](https://togithub.com/Latropos) in [software-mansion/react-native-reanimated#6039 - Test predefined entering animation by [@​Latropos](https://togithub.com/Latropos) in [software-mansion/react-native-reanimated#5995 - Tests: cancelAnimation, useFrameCallback, measure, withDecay by [@​Latropos](https://togithub.com/Latropos) in [software-mansion/react-native-reanimated#6016 - docs: fix useAnimatedKeyboard page crash by [@​patrycjakalinska](https://togithub.com/patrycjakalinska) in [software-mansion/react-native-reanimated#6056 - \[Web LA] Remove `existingTransform` by [@​m-bert](https://togithub.com/m-bert) in [software-mansion/react-native-reanimated#6060 #### New Contributors [@​antFrancon](https://togithub.com/antFrancon), [@​JDMathew](https://togithub.com/JDMathew), [@​thomas-rx](https://togithub.com/thomas-rx), [@​exploIF](https://togithub.com/exploIF) #### 🙌 Thank you for your contributions! **Full Changelog**: software-mansion/react-native-reanimated@3.11.0...3.12.0 Package build: https://github.com/software-mansion/react-native-reanimated/actions/runs/9287839734 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/GSTJ/react-native-magic-modal). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zNzcuOCIsInVwZGF0ZWRJblZlciI6IjM3LjM3Ny44IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Summary
Turns out the usual component tag (used for Layout Animations, Shared Element Transitions and Animated Styles) won't always work for events if the component with nested event emitters is made animated. Thus I am splitting these two into different tag variables.
Test plan
You can check out the following code (logs in console):
Code
Current tests work on: