Improve stability and performance of BottomSheet animations #37559
Labels
Mobile App - i.e. Android or iOS
Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
[Type] Performance
Related to performance efforts
Comments
dcalhoun
added
[Type] Code Quality
dcalhoun
added a commit
that referenced
this issue
Dec 21, 2021
The assertions of these tests do not tell the full story of the test expectation. Namely, due to bugs within react-native-testing-library and its dependencies, the tests do not assert navigation events that should occur _after_ the keyboard dismisses. Combining async renders and mocked timers is currently quite difficult. callstack/react-native-testing-library#379 (comment) To clarify the intent of the test, the test description was expanded to describe the expectation that the keyboard is dismissed prior to the navigation event occurring. Simultaneously dismissing the keyboard and navigating has caused performance issues in our bottom sheet before. #37559
hypest
added
the
[Priority] High
|
Marked this as |
dcalhoun
added a commit
that referenced
this issue
Jan 3, 2022
* Upgrade base React Native packages for 0.66.2 - react-native - react - react-dom - react-test-renderer - metro-react-native-babel-preset - metro-react-native-babel-transformer * Remove legacy module patches These patches have now arrived from upstream changes. - facebook/metro@1e6cec8 - facebook/react-native@842bcb9 * Apply React Native Upgrade Helper changes https://react-native-community.github.io/upgrade-helper/?from=0.64.0&to=0.66.1 * Apply possibly overlooked changes from React Native 0.64.0 upgrade https://react-native-community.github.io/upgrade-helper/?from=0.61.5&to=0.64.0 * Capture pod installation changes Result of `npm run native preios`. * Bump version of hermes we keep on the S3 mirror * Utilize store definition rather than store name string Errors were thrown from `useSelect` and `useDispatch` returning unexpected `undefined` values. Utilize the store defintion rather than the store name string resolved the errors. There is already a movement to use this approach in general: https://git.io/JPQW2 * Remove usage of deprecated EventEmitter.removeEventListener This function is marked as deprecated.We should be replaced by calling `remove` on the subscription itself. https://git.io/JPQzO * Patch react-native-modal to remove deprecated method usage Remove usage of deprecated `EventEmitter.removeEventListener`. This function is marked as deprecated.We should be replaced by calling `remove` on the subscription itself. https://git.io/JPQzO The patch can be removed once we upgrade to react-native-modal@^13.0.0. https://git.io/JPQgq * Fix native Android Demo app build Set the relative RN CLI path for the Demo app to fix broken builds when using Android Studio, as opposed to the RN CLI directly. It appears the [code used to locate the CLI was modified](https://git.io/JcNzp) for v0.64.0. So, our recent React Native upgrade is likely when this issue began. * Disable Metro inlineRequires configuration When enabled, this option converts top-level imports into inline requires as a performance optimization for large apps. However, it would appear that some import side effects break, e.g. `import './hooks'`. https://reactnative.dev/docs/ram-bundles-inline-requires#inline-requires * Patch @react-navigation/native to remove usage of deprecated method Remove usage of deprecated `EventEmitter.removeEventListener`. This function is marked as deprecated.We should be replaced by calling `remove` on the subscription itself. https://git.io/JPQzO The patch can be removed once we upgrade to react-navigation@^6.0.0. https://git.io/JP7OG * Remove ALWAYS_EMBED_SWIFT_STANDARD_LIBRARIES setting Installing pods resulted in the following warning. Given that the target is an unused test workspace, it felt safe to remove this setting. ``` [!] The `GutenbergDemoTests [Debug]` target overrides the `ALWAYS_EMBED_SWIFT_STANDARD_LIBRARIES` build setting defined in `Pods/Target Support Files/Pods-GutenbergDemo-GutenbergDemoTests/Pods-GutenbergDemo-GutenbergDemoTests.debug.xcconfig'. This can lead to problems with the CocoaPods installation - Use the `$(inherited)` flag, or - Remove the build settings from the target. [!] The `GutenbergDemoTests [Release]` target overrides the `ALWAYS_EMBED_SWIFT_STANDARD_LIBRARIES` build setting defined in `Pods/Target Support Files/Pods-GutenbergDemo-GutenbergDemoTests/Pods-GutenbergDemo-GutenbergDemoTests.release.xcconfig'. This can lead to problems with the CocoaPods installation - Use the `$(inherited)` flag, or - Remove the build settings from the target. ``` * Use Carthage --use-xcframeworks argument The Carthage documentation showcases this argument. Using it resolved the following error when running `npm run native preios:carthage`. https://github.com/Carthage/Carthage/tree/0.38.0#quick-start ``` Building universal frameworks with common architectures is not possible. The device and simulator slices for "Aztec" both build for: arm64 Rebuild with --use-xcframeworks to create an xcframework bundle instead. ``` * Add required EventEmitter methods to address warning The added methods are required by React Native. Otherwise, the below warning is displayed. https://stackoverflow.com/a/69650217/378228 ``` WARN `new NativeEventEmitter()` was called with a non-null argument without the required `addListener` method. WARN `new NativeEventEmitter()` was called with a non-null argument without the required `removeListeners` method. ``` * Enable LogBox to address deprecation warning `console.disableYellowBox` is deprecated. Additionally, totally disabling the logger increases the likelihood that we inadvertently overlook new warnings. * Patch react-native-keyboard-aware-scroll-view for deprecated method This patch changed from the deprecated `componentWillReceiveProps` to `componentDidUpdate`. We can remove this patch when we upgrade to `react-native-keyboard-aware-scroll-view@^0.9.2` https://git.io/JPbOK * Fix runtime error from invoking removed method Utilize the new versions of the scroll methods. This patch could be removed if we upgrade to `react-native-keyboard-aware-view@^0.9.5`. https://git.io/JPb6a * Remove redundant listener removal A few lines before this, `remove` is called on the subscription itself. * Remove usage of deprecated removeEventListener method We should use `remove` on the subscription itself instead. * Remove IPHONEOS_DEPLOYMENT_TARGET for all Pods Build errors occurred for `react-native-bridge` as it attempted to target 11.0, where the editor project targets 13.0. This change lets all Pods, except RCT-Folly, inherit the target of the project. RCT-Folly requires an explicit target. - https://stackoverflow.com/a/37289688/378228 - https://git.io/JPb7Y - https://git.io/JPb73 * Capture Podfile and Xcode project changes from initial build Result of running `npm run native ios`. * Replace deprecated Clipboard core module The `Clipboard` module was marked as deprecated and will be removed from React Native core. React Native documentation recommends relying upon the community package instead. ``` WARN Clipboard has been extracted from react-native core and will be removed in a future release. It can now be installed and imported from '@react-native-clipboard/clipboard' instead of 'react-native'. See https://github.com/react-native-clipboard/clipboard ``` * Capture lockfile changes from subsequent npm install These changes did not show up until the second run, unsure as to why. * Fix mock of AccessibilityInfo This aligns with the mock found within the React Native source itself. Prior to this change, `AccessibilityInfo` was `undefined` within tests. https://git.io/JXKgx * Capture Jest snapshot updates These changes occurred after upgrading React Native to 0.66.2. They do not appear suspicious. * Mark mock as an ESM default export This mock was not properly applied, which led to the error it resolved resurfacing in the test output. Setting `__esModule: true` signals the mocked file is an ESM default export. https://jestjs.io/docs/jest-object#jestmockmodulename-factory-options * Fix illegible iOS header in Demo app Changes to navigation bars in iOS 15 resulted in the navigation bar becoming unexpectedly transparent when content was scrolled all the way to the top. https://reactnative.dev/blog/2021/09/01/preparing-your-app-for-iOS-15-and-android-12#transparent-navigation-bar * Demote the warning to simple code comment While at it, expand the comment with more context about why there's no need to bump the event counter when already undefined. * Add global source to Gemfile This change addresses the following warning: ``` [DEPRECATED] This Gemfile does not include an explicit global source. Not using an explicit global source may result in a different lockfile being generated depending on the gems you have installed locally before bundler is run. Instead, define a global source in your Gemfile like this: source "https://rubygems.org". ``` * Remove unnecessary deprecated bundler --path argument A Bundler configuration file already sets this value. Also, this argument outputs the following deprecation warning: ``` [DEPRECATED] The `--path` flag is deprecated because it relies on being remembered across bundler invocations, which bundler will no longer do in future versions. Instead please use `bundle config set --local path 'vendor/bundle'`, and stop using this flag ``` * Fix non-dismissible modals caused by unnecessary height transitions These changes refactor animations for the `BottomSheetNavigationContainer` to reduce the likelihood of `LayoutAnimations` remaining unconsumed. In certain scenarios, the BottomSheet would erroneously register a `LayoutAnimation`, but then it would never be consumed for one reason or another. E.g. transitioning from full-height to full-height, computed pixel height was identical, stale height value captured within memoized callback. Unconsumed `LayoutAnimations` appear to cause issues for React Native's Modal component on iOS. Specifically, it can result in a transparent, non-dismissible modal sitting atop the rest of the app UI. Upgrading to React Native 0.66 appears to have increased the frequency of this occurring, for an unknown reason. - https://git.io/J1W3z - https://git.io/J1W32 - https://git.io/J1W3a - https://git.io/J1W3r - https://git.io/J1W36 * Fix non-dismissible modals caused by header layout transitions This change ensures changes to the header height are considered when computing the allowable maximum height for the `BottomSheet`. When the `BottomSheet` header changes its size, it configures a `LayoutAnimation` to manage the change. It then relies upon the `onSetHeight` callback to implement the change. In the scenario where the _header_ layout changed, but the rest of the content did not, it meant we attempted to set the height to the same value. This would result in an unconsumed `LayoutAnimation` lingering as no changes occurred which would consume the `LayoutAnimation`. Unconsumed `LayoutAnimations` appear to cause issues for React Native's Modal component on iOS. Specifically, it can result in a transparent, non-dismissible modal sitting atop the rest of the app UI. Upgrading to React Native 0.66 appears to have increased the frequency of this occurring, for an unknown reason. - https://git.io/J1W3z - https://git.io/J1W32 - https://git.io/J1W3a - https://git.io/J1W3r - https://git.io/J1W36 * Document ignored LogBox warnings Provide additional context as to why warnings are ignored and how we might address them properly. * Ignore overridden layout animations warnings `KeyboardAvoidingView`'s usage of `LayoutAnimation` collides with both `BottomSheet` and `NavigationContainer` usage of `LayoutAnimation` simultaneously. Each of these components may invoke overlapping animations, which produces this warning. The most appropriate way to handle this is likely to migrate to @gorhom/bottom-sheet and/or replace usage of `LayoutAnimation` with `Animated`. - https://git.io/J1lZv - https://git.io/J1lZY * Link picker dismisses iOS keyboard to ensure smooth animation Focus of the URL text input caused animation stutter while the iOS keyboard visibility toggled from hide to show to hide. To address this, the Android-specific keyboard dismiss logic is now applied to iOS as well. When the keyboard visibility changes, the `KeyboardingAvoidingView` configures a `LayoutAnimation` to manage the change. The quick toggle of hide to show to hide could result in an unconsumed `LayoutAnimation` lingering as no changes occurred which would consume the `LayoutAnimation`. Unconsumed `LayoutAnimations` appear to cause issues for React Native's Modal component on iOS. Specifically, it can result in a transparent, non-dismissible modal sitting atop the rest of the app UI. Upgrading to React Native 0.66 appears to have increased the frequency of this occurring, for an unknown reason. - https://git.io/J1W3z - https://git.io/J1W32 - https://git.io/J1W3a - https://git.io/J1W3r - https://git.io/J1W36 * Revert erroneous native editor package version bumps Automation to generate web releases seems to sporadically bump the package versions of the native editor packages by mistake. This reverts the erroneous version changes. * Install Pods after erroneous version bump revert * Revert erroneous package lock change Removing the `node_modules` directory entirely and `npm install` resulted in this package being added back. * Android: SafeArea sub can be null so, guard its removal * Remove unnecessary test mock code The implementation itself is irrelevant, we merely need the spy to assert against the call counts. * Avoid state updates on unmounted LinkSettingsScreen component State updates that triggerd from calling `submit` within `onHandleClosingBottomSheet` resulted in errors from React regarding a potential memory leak. Since the bottom sheet is closing and component is unmounting, there is no need to update the component state. The `onChange` callback, however, still needs to be invoked to persist the attribute changes. There is further opportunity to improve `onChange` as it currently empties the attributes causing a re-render during the close animation, which results in UI jumping. ``` ERROR Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in a useEffect cleanup function. in LinkSettingsScreen (at modal.native.js:25) ``` * Avoid unnecessary link removal while closing bottom sheet The absence of a checking the visibility of the link settings meant multiple calls to `removeLink` were performed while closing the bottom sheet. This resulted in unnecessary re-renders while the bottom sheet close animation was running, which led to poor animation performance and potential for unconsumed `LayoutAnimation`s. This change now only removes the link if the bottom sheet is still visible, which matches the behavior of submitting a link. Unconsumed `LayoutAnimation`s appear to cause issues for React Native's Modal component on iOS. Specifically, it can result in a transparent, non-dismissible modal sitting atop the rest of the app UI. Upgrading to React Native 0.66 appears to have increased the frequency of this occurring, for an unknown reason. - https://git.io/J1W3z - https://git.io/J1W32 - https://git.io/J1W3a - https://git.io/J1W3r - https://git.io/J1W36 * Pin react-native-url-polyfill to 1.1.2 that includes Buffer * Include rest of npm install changes in package-lock * Need to load the native part of the Clipboard package Note: haven't done the binary/release side setup yet. We'll need to fork the dependency to add Jitpack support as usual. * Point the android build to the forked react-native-clipboard * Point to the forked react-native-clipboard * Correct integrity hash for react-native-clipboard's tarball * Fix up the package name reference * Point to the JitPack fix commit * Update React Native version ref in third-party package forks (#36983) We currently maintain a few forks of third-party packages for modifications required for the Gutenberg mobile editor, e.g. Gradle configuration changes. This updates the dependency versions that reference React Native 0.66.2. * Update third-party package forks to tag references Tagging the third-party package fork repository is necessary for native bridge Android integration. For sake of consistency, this updates the git references in the `package.json` file to reference tags as well. https://git.io/JM2bA * Disable keyboard-related BottomSheet animations This particular animation exasperated a preexisting bug. We chose to disable this one animation as a temporary solution to keep the React Native upgrade moving forward. Long term, we should explore replacing `LayoutAnimation` usage with more nuanced `Animated` usage or replace our custom `BottomSheet` with `@gorhom/bottom-sheet`. https://git.io/JMPCV * Avoid non-dismissible modals caused by unnecessary height transitions Transitioning height between sub-pixel values is unnecessary and caused unconsumed LayoutAnimations which resulted in non-dismissible, transparent modals on iOS. These changes refactor animations for the `BottomSheetNavigationContainer` to reduce the likelihood of `LayoutAnimations` remaining unconsumed. In certain scenarios, the BottomSheet would erroneously register a `LayoutAnimation`, but then it would never be consumed for one reason or another. E.g. transitioning to height that is only a few decimal different. Unconsumed `LayoutAnimations` appear to cause issues for React Native's Modal component on iOS. Specifically, it can result in a transparent, non-dismissible modal sitting atop the rest of the app UI. Upgrading to React Native 0.66 appears to have increased the frequency of this occurring, for an unknown reason. - https://git.io/J1W3z - https://git.io/J1W32 - https://git.io/J1W3a - https://git.io/J1W3r - https://git.io/J1W36 * Adjust bottom sheet max height to account for header subtraction The percentage height required changing now that we subtract the header height from the max height calculation. The header is now subtracted to ensure that the `LayoutAnimation` scheduled is consumed if _only_ the header changes its height. Previously, a change in header height only scheduled a `LayoutAnimation`, but did not modify the max height. * Fix BottomSheet navigation hardware back button support (#37426) An earlier addition of `setHeight` to the dependencies array meant the callback was recreated multiple times, which led to unintentional updates to the hardware button handler. This resulted in navigating backwards multiple screen instead of one when pressing the hardward back button in a deeply nested screen. `onHandleHardwareButtonPress` stores a single value, which means future invocations from sibling screens can replace the callback for the currently active screen. Currently, the empty dependency array passed to `useCallback` here is what prevents erroneous callback replacements, but leveraging memoization to achieve this is brittle and explicitly discouraged in the React documentation. https://reactjs.org/docs/hooks-reference.html#usememo Ideally, we refactor `onHandleHardwareButtonPress` to manage multiple callbacks triggered based upon which screen is currently active. Related: https://git.io/JD2no * Further clarify link picker test intent The assertions of these tests do not tell the full story of the test expectation. Namely, due to bugs within react-native-testing-library and its dependencies, the tests do not assert navigation events that should occur _after_ the keyboard dismisses. Combining async renders and mocked timers is currently quite difficult. callstack/react-native-testing-library#379 (comment) To clarify the intent of the test, the test description was expanded to describe the expectation that the keyboard is dismissed prior to the navigation event occurring. Simultaneously dismissing the keyboard and navigating has caused performance issues in our bottom sheet before. #37559 * Replace usage of deprecated React Native Clipboard module The `Clipboard` module was relocated to a non-core package. This change mirrors earlier work in this branch, but needed to be applied to additional code that merged after the initial work. I.e. a merge resolution resulted in old references showing up. * Update React Native version in Gradle configuration These stale references lingered from prior iterations of the React Native upgrade work. * Remove unused React Native Clipboard module mock The React Native Clipboard module is deprecated. This project now utilizes the recommended, third-party module `@react-native-clipboard/clipboard` instead. * Reinstate version ranges for react and react-dom Using ranges avoids locking third-party projects to a specific version when consuming the `@wordpress/element` package. * Update outdated comments The code related to the comments changed in previous commits. Co-authored-by: Stefanos Togkoulidis <[email protected]>
|
Noting that React Native Bottom Sheet was brought up in an internal discussion recently, specifically related to the fact it'd help address the error related to VirtualizedList's described in #23902. |
This was referenced Jul 6, 2022
|
Removing the high priority label for a couple of reasons:
Additionally, I will note that #42192 captures tasks for addressing this issue with a third-party implementation, which has been explored at a surface level in #42201. If others disagree, please feel free to revert the removal and shared your perspective. Thanks! |
dcalhoun
added
[Type] Performance
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
What problem does this address?
Originally discussed in #36328 (comment), there is opportunity to refactor our
BottomSheetnavigation transitions to better manage keyboard visibility and improve performance. At times, changes to keyboard visibility can negatively impact the performance of animations and produce bugs, e.g. #30562. Currently, we implemented keyboard visibility logic in various one-off components to address performance issues, e.g. 71ae88a, 98684f1, f8b0275.Elsewhere, animations may appear jittery, as referenced in #32012.
What is your proposed solution?
Ideally, we architect a lower-level solution that abstracts keyboard visibility and performance improvements away from individual screens and applies a solution globally to our navigation transitions. A few possible approaches may be:
beforeRemoveevent to manage keyboard visibility withinBottomSheet.NavigationContainerprior to navigation transitions.keyboardShouldPersistTapsto identify changes that may improve the UX or navigation transition performance.LayoutAnimationto avoid stacking multiple registrations simultaneously, which can create competing registrations and sometimes leave instance unconsumed.BottomSheetcomponent with the third-party React Native Bottom Sheet to leverage improved performance.The text was updated successfully, but these errors were encountered: