[RNMobile] Use Reanimated in bottom sheet height animation #52563
Conversation
Pass `currentHeight` in bottom sheet navigation context
We need to pass pixel values in order to animate the height with Reanimated.
fluiddot
added
[Type] Bug
|
Size Change: 0 B Total Size: 1.42 MB ℹ️ View Unchanged
|
| @@ -6,7 +6,7 @@ import { createContext } from '@wordpress/element'; | |||
| // Navigation context in BottomSheet is necessary for controlling the | |||
| // height of navigation container. | |||
| export const BottomSheetNavigationContext = createContext( { | |||
| currentHeight: 1, | |||
| currentHeight: { value: 0 }, | |||
There was a problem hiding this comment.
This structure matches Reanimated's shared value. I changed the default value to 0 as I feel it's a more appropriate default value.
There was a problem hiding this comment.
I agree that 0 seems like a more reasonable default.
That said, my gut tells me moving to 0 may introduce a bug. I was unable to find a direct answer (indirectly not answered), but my hunch is that using 1 as the default may be a cryptic way of ensuring something occurs when first setting the height, but not if the height is set to 0 in the future. I.e. that "something" should not occur if the height comes through as 0, which more likely to organically occur than 1.
Does that make sense?
My fear may be irrational and not worth following, particularly if we do not observe any bugs while testing now.
There was a problem hiding this comment.
These default values will only be used when using the provider without setting the value prop. AFAIK this is not our case (reference), so similarly to setting an empty function for setHeight below, I used 0 as the empty value for currentHeight.
Regarding the default value to use for the height animation, I agree it's a bit cryptic why we use 1. I haven't checked this but my gut feeling is that, since we need to calculate the layout of the content to set the final height, using a 0 value would prevent the layout calculations to happen.
There was a problem hiding this comment.
The impact — or lack thereof — of the default Context value makes sense. Thank you for noting that.
| const defaultTheme = useMemo( | ||
| () => ( { | ||
| ...DefaultTheme, | ||
| colors: { | ||
| ...DefaultTheme.colors, | ||
| background: backgroundStyle.backgroundColor, | ||
| }, | ||
| } ), | ||
| [ backgroundStyle.backgroundColor ] | ||
| ); |
There was a problem hiding this comment.
This is memoized to avoid potentially generating a new object on every render in _theme.
| jest.useRealTimers(); | ||
| } ); | ||
| it( 'animates height transitioning from non-full-screen to non-full-screen', async () => | ||
| withReanimatedTimer( async () => { |
There was a problem hiding this comment.
Note the use of withReanimatedTimer helper, this is needed when testing Reanimated animations.
| if ( shouldAnimate ) { | ||
| currentHeight.value = withTiming( newHeight, { | ||
| duration: HEIGHT_ANIMATION_DURATION, | ||
| easing: Easing.out( Easing.cubic ), |
There was a problem hiding this comment.
You can check the easing function here.
There was a problem hiding this comment.
Thank you for exploring how we might move away from LayoutAnimation, it has caused numerous issues in the past.
I tested this using an Android emulator. I noticed a few things that I'd like to get your input on.
Full-height bottom sheets appear to no longer animate. I noticed the setHeight callback fires multiple times for this scenario, but not for non-full-height scenarios.
Full-Height Animation
| Before | After |
|---|---|
Screen_Recording_20230713_095648_Jetpack.mp4 |
Screen_Recording_20230713_095715_Gutenberg.mp4 |
Full-Height Logs
I also noticed the bottom sheet animation jitters when opening the text formatting options. Similar to full-height, it appears the setHeight callback runs multiple times in this scenario.
Text Formatting Jitter
Screen_Recording_20230713_095858_Gutenberg.mp4
Text Formatting Logs
Any thoughts as to what might be happening in these two scenarios?
...es/components/src/mobile/bottom-sheet/bottom-sheet-navigation/navigation-container.native.js
Show resolved
Hide resolved
| @@ -6,7 +6,7 @@ import { createContext } from '@wordpress/element'; | |||
| // Navigation context in BottomSheet is necessary for controlling the | |||
| // height of navigation container. | |||
| export const BottomSheetNavigationContext = createContext( { | |||
| currentHeight: 1, | |||
| currentHeight: { value: 0 }, | |||
There was a problem hiding this comment.
I agree that 0 seems like a more reasonable default.
That said, my gut tells me moving to 0 may introduce a bug. I was unable to find a direct answer (indirectly not answered), but my hunch is that using 1 as the default may be a cryptic way of ensuring something occurs when first setting the height, but not if the height is set to 0 in the future. I.e. that "something" should not occur if the height comes through as 0, which more likely to organically occur than 1.
Does that make sense?
My fear may be irrational and not worth following, particularly if we do not observe any bugs while testing now.
...es/components/src/mobile/bottom-sheet/bottom-sheet-navigation/navigation-container.native.js
Show resolved
Hide resolved
|
Thanks @dcalhoun for reviewing the PR, I appreciate your feedback 🙇 !
Oh, good catch. I need to check this further but it might be possible that full-screen sheets won't be animated.
That's interesting. I'll check this scenario further following what you shared about |
|
@dcalhoun I tried to debut the issues recently shared in this comment. Here are my findings so far:
When the bottom sheet is set to full screen, This makes any changes to the navigator's height not visually reflected. In fact, I noticed that we could even remove this line as it's not really animating the height. This wasn't the case when using Layout animations, as they animate any changes made to the layout. We could investigate a potential fix but based on the current state of the bottom sheet component, I'm concerned that it will end up being a major refactor and it will be probably out of the scope of this PR. At this point, I see two options:
WDYT?
I'm not experiencing this issue on a physical device 🤔 , although I see multiple calls to the I'll try to use Reanimated's layout animation and revert the removal of the |
I believe we should pursue Option 1. The lack of animations for full-screen sheets is annoying, but it is not disruptive and it is limited to a subset of the product. We should prioritize upgrading our foundational dependency in React Native. We can follow up on the animations later.
I retested using a Samsung Galaxy S20 5G FE running WPAndroid. The issue is definitely less notable in that context, but the animation is still less smooth than the previous version relying upon These complex issues are a good reminder of the value in pursuing #37559, potentially through #42192 and #42201.
Let me know if you'd like to move forward with the current implementation. I will approve it. |
I agree 👍, let's continue with this approach and follow up on this later.
Great, I'm glad it's not that bad when using WP-Android, that was my impression when testing too.
Definitely, sooner than later we should tackle these issues. I feel that every time we have to make any minor updates to the bottom sheet we end up hitting a block due to the current status of that component. In fact, when trying to apply the workaround I shared here related to using Reanimated's layout animation, I realized that it won't work out of the box due to how that component is structured and the usage of React Navigation. As an example, when navigating to the Colors sub-sheet from the block settings sheet, there are several components that impact the height:
It took me a while until I realized that
@dcalhoun In the spirit of unblocking the RN upgrade, I think we could move it forward as-is. And as you mentioned, we could follow up on improvements to the bottom sheet afterwards. Thanks! |
Agreed. It seems to be a frequent occurrence.
Yes, it can be difficult to understand their relationship. Atop its low comprehensibility, my hunch is that the competing "managers" may lead to bugs we have noted. |
Absolutely. I also have the gut feeling that most of the bugs we have in the bottom sheet are related to this. |
* Upgrade `react-native` dependency * Upgrade `@babel/runtime` dependency * Upgrade `metro-react-native-babel` preset and transformer dependencies * Upgrade `cocoapods` gem * Re-apply `react-devtools-core` patch to new version * Update jest snapshots with new a11y values * Mock `Linking.addEventListener` function `Linking.removeEventListener` has been removed in RN `0.71`. The library is mocked by default but doesn't return the `remove` function when calling `addEventListener`. * Update tests that fail due to use of debounce and link suggestions * Fix `MediaUpload` component test * Update `@react-navigation/native` package to version `6.0.14` * Update `react-native-reanimated` to version `2.17.0` * Update `react-native-gesture-handler` to version `2.10.2` * Fix `act` warnings produced during block insertion * Fix `act` warnings in Columns block tests * Fix `act` warnings in List block tests * Upgrade `react-native` dependency to version `0.71.11` It also upgrades `metro-react-native-babel` dependencies following the upgrade helper. * Mock return value of Linking `addEventListener` We only need to mock the return the value, hence we don't need to mock the entire library. * Remove `waitForModalVisible` usage in Paragraph block tests * Remove `waitFor` usage in Link settings tests * test: Fix act warning by awaiting LinkPicker loading indicator removal The loading indicator is displayed and subsequently removed once the suggestion fetches resolve. Explicitly awaiting this element's removal fixes the `act` warnings. * build: Update react-native-safe-area-context to 4.6.3 * build: Upgrade react-native-screens to 3.22.0 * build: Upgrade react-native-svg to 13.9.0 Based on the release notes breaking changes, we should look out for odd sizing or display of icons, particularly on Android. * build: Upgrade @react-native-masked-view/masked-view to 0.2.9 * build: Upgrade @react-native-clipboard/clipboard to 1.11.2 * build: Upgrade react-native-modal to 13.0.1 * test: Update link modal snapshot This change is a result of applying new props from the RN upgrade to a newly introduced snapshot in trunk: 71d2dc5 * Update `@react-navigation/stack` to version `6.3.5` * Upgrade `react-native-linear-gradient` to version `2.7.3` This commit also updates the `react-native-hsv-color-picker` library to point to the same version of `react-native-linear-gradient`. * Use `react-native-safe-area-context` mock provided by the library * Update link modal snapshot * Update `package-lock.json` file The integrity checksum of `react-native-hsv-color-picker` changed because the package has been modified (ref: wordpress-mobile/react-native-hsv-color-picker#10 (comment)) * Disable `react-native-screens` in navigators `react-native-screens` is meant to be used at root level to save memory when having inactive screens. This is not the case of the editor, as the stack navigators are used within the Bottom sheet component. As a side note, enabling `react-native-screens` here leads to the editor crashing. * Fix render order of animated view to highlight selected segment Rendering the animated view before the segments will ensure that is rendered behind them. * Update source of `react-native-hsv-color-picker` to use tag version * Revert "Update link modal snapshot" This reverts commit 7988b0e. This is needed after disabling `react-native-screens` in navigators (ref: e5838f4). * [RNMobile] Upgrade React Native `0.71.11` - iOS changes (#51386) * refactor: Extract bundle version number to var * refactor: Delete /.ruby-version, no longer needed * refactor: Update /podfile to align w/RN updates * refactor: Remove path names as part of RN upgrade * Update `Podfile` with changes from RN upgrade helper * Fix React Native path for `react_native_post_install` script * Update Pods * Add patch to fix Reanimated podspec Without this patch, Reanimated tries to use Hermes version of React Native and produced a build failure. Seems there's an issue in the `podspec` file, as the JSC module is not being added. Reference: software-mansion/react-native-reanimated#4254 * Update pods to reflect 0.71.11 target * Apply changes to pods following `pod install` * Update `Podfile.lock` file --------- Co-authored-by: Siobhan <[email protected]> * [RNMobile] Upgrade React Native `0.71.11` - Android changes (#51289) * Upgrade Gradle to version 7.5.1 * Upgrade Gradle plugin * Remove no longer needed files in new version * Update Flipper initialization * Update demo project main application * Remove gradle download task plugin * Bump ndk version * Remove no longer need logic related to `newArchEnabled` * Apply plugin React Native Gradle plugin * Use React Native and Hermes modules from Maven We no longer need to publish these binaries because React Native team is publishing React Native binaries to Maven. * Remove exclude group from Flipper * Update comments in `build.gradle` to align with new RN version * Remove deprecated Gradle property * Add `mavenLocal` repository to allow testing local binaries * Bump Reanimated and Gesture handler libraries * Revert "Upgrade Gradle plugin" This reverts commit 82764a2. * build: Resolve react-native-gradle-plugin incompatability Due to host app requirements, we must use AGP 7.2.1. The included patch disables logic requiring AGP 7.3. The logic appears to not be required for our use cases in the Demo editor or host apps. We should remove this patch once we upgrade past AGP 7.3. * Reduce priority of `mavenLocal` repository in Android build configurations Maven local is used to provide dependencies located locally, which is mainly used for testing and debugging. Hence, published dependencies should be prioritized over local ones. * Disable `react-native-screens` in navigators `react-native-screens` is meant to be used at root level to save memory when having inactive screens. This is not the case of the editor, as the stack navigators are used within the Bottom sheet component. As a side note, enabling `react-native-screens` here leads to the editor crashing. * Fix render order of animated view to highlight selected segment Rendering the animated view before the segments will ensure that is rendered behind them. * Bump Linear gradient Android library This library is now published via the `react-native-libraries-publisher` repository. --------- Co-authored-by: David Calhoun <[email protected]> * Avoid exception in E2E tests when typing an empty string on Android * Update a11y id queries for Android E2E tests Starting in React Native 0.71, the accessibility hint is no longer appended to the accessibility content description. Reference: facebook/react-native@0b70b38 * Update button inline appender query for Android E2E tests * Unify press keycode function for E2E tests * Update comments in functions related to pressing a keycode * Update block drop position using Reanimated's shared value Seems there's some kind of incompatibility on calling a JS function from a worklet invoked from a gesture handler. For this reason, the logic to set the dropping insertion point has been updated. It now uses a Reanimated's shared value to keep the dragging over position and `useDerivedValue` hook to listen for changes. * Remove unneeded `hidden` param in Paragraph block test case Co-authored-by: David Calhoun <[email protected]> * Revert removing `.ruby-version` file * Add inline comment in Reanimated patch * Use `waitForElementToBeRemoved` in Paragraph block test cases This way we can avoid waiting for any microtasks of link suggestions. * Remove `act` statements from Link Settings test cases * fix: Cover focal point drag handle visibility The lack of an explicit width or height resulted in a invisible drag handle. The logic passing the dimensions to the SVG expected a single style object. The reality is that it (1) referenced only the Sass styles and (2) the combined reference was actually an array of style objects. Updating the reference and flattening it ensures the appropriate width and height are passed to the SVG. It appears the absence of explicit dimensions was not an issue in earlier versions of React Native, but it makes sense why it might be required. * [RNMobile] Use Reanimated in bottom sheet height animation (#52563) * Expose max height properties in `BottomSheetProvider` * Animate bottom sheet's height with Reanimated Pass `currentHeight` in bottom sheet navigation context * Use pixel value when setting fullscreen height We need to pass pixel values in order to animate the height with Reanimated. * Rename `heightRef` to `maxHeight` * Re-enable `exhaustive-deps` lint rule in `BottomSheetNavigationContainer` * Avoid setting height using debounce * Add test ID to navigation container component * Mock Reanimated's `now` function * Update test cases related to bottom sheet height animation * Update test snapshots * Update `react-native-editor` changelog * Drop unsupported `--no-jetifier` from Android cmd The `--no-jetifier` option no longer appears to be supported and results in an error when attempting to build the Android demo app. Ref: wordpress-mobile/gutenberg-mobile#5881 (comment) * Revert accidental change to .ruby-version * Restore correct dependencies to package-lock.json * Update `react-native-editor` changelog * Update `package-lock.json` file to revert previous conflict resolutions In 4482b9d we had a conflict in `package-lock.json` that was solved using the changes from this branch. However, seems that something went wrong and that although the editor has no issues, some e2e tests are failing due to this. This has been solved by using the latest version of `package-lock.json` file from `trunk` and updating it with the package updates required in the React Native upgrade. * Re-apply `react-devtools-core` patch to new version * Update `Podfile.lock` file --------- Co-authored-by: David Calhoun <[email protected]> Co-authored-by: Siobhan <[email protected]>
This warning appears to no longer occur, presumably thanks to: #52563
* refactor: Remove unsupported line height Aztec warning While arguably educational, this warning increases the amount of noise within the server log, which can make it difficult to debug errors, warnings, or intentionally logged values. * refactor: Remove Hermes engine status log While informative, this un-filterable log increases the amount of noise in the server log, which can make it difficult to debug errors, warnings, or intentionally logged values. Additionally, the Hermes engine is now globally enabled for both iOS and Android. * refactor: Remove outdated LogBox configuration for LayoutAnimation This warning appears to no longer occur, presumably thanks to: #52563 * refactor: Remove outdated LogBox configuration for gesture handler This warning appears to no longer occur. The configuration was originally added in: a1b11c5
What?
The main goal of this PR is to update the animation framework used to animate the bottom sheet's height. Currently, we use layout animations (reference) that will be replaced with Reanimated.
Why?
On Android, using layout animations can lead to inconsistent visual states when used in combination with React navigation. Specifically in the Image block, the content of the bottom sheet gets transparent after setting a custom URL.
Additionally, layout animations are experimental on Android, so it would be great to avoid them.
How?
The height was being animated using the function
setHeight, which keeps the height value in the component's state and updates the styles, and then invokingperformLayoutAnimationto execute the layout animation. This logic has been replaced using Reanimated's shared value to keep the height value, andwithTimingin combination withuseAnimatedStyleto animate the value. This implementation can be seen in 634b2fd.Testing Instructions
Inconsistent visual state in "Link to" setting
Bottom sheet's height animation
NOTE: This covers a wide range of test cases, so we can't provide a specific case.
Testing Instructions for Keyboard
N/A
Screenshots or screencast
android-bottom-sheet-height-animation.mp4
ios-bottom-sheet-height-animation.mp4