feat(Android, Stack v5): prevent native dismiss support#3620
Open
feat(Android, Stack v5): prevent native dismiss support#3620
Conversation
I don't know how our events have worked so far. I thought that overriding this method is obligatory. Apparently not. Maybe its just hint for the React machinery. I'm not sure right now, but I've verified that this code is indeed called & React gathers this information.
This commit adds support for "prevent native dismiss" behavior on Android for stack screens. The added interface consists of prop on `StackScreen`: `preventNativeDismiss` and event `onNativeDismissPrevented`, which lets the programmer react to the dismiss attempt. This is achieved by leveraging a couple of native mechanisms. First, I lean into usage of `OnBackPressedCallback` AndroidX API, which lets us handle this very gracefully & hopefully correct on many SDK levels. > [!note] > TODO: The behavior on lower SDK levels has not been tested yet. Now, for each `StackScreenFragment` we add a dedicated callback, which is enabled or disabled depending on the value. I do not add/remove the callbacks because their order in the `OnBackPressedDispatcher` matter, and we can add a callback only to the end of the list. > [!caution] > This implementation assumes that the fragments will move to `created` > state in the order they are in the fragment manager (stack order). > If this ever turns out to not be the case, we'll need to modify this > implementation. This implementation follows [ RFC-0717 ](https://github.com/software-mansion/react-native-screens-labs/blob/main-issue-tracker/rfcs/0717-stack-prevent-dismiss.md)
I've made StackContainer expose routeOptions, so that it is easier to read what current options applied to scree are. Usually in production-grade navigator, this could not be a best idea, however in our case I think it is pretty useful. Also, I don't test "dynamic" case yet, where the value is changed for a screen depending on a condition. This would require way more infrastructure code, which we do not have yet. I intend to add this later.
stack Previous code has not covered that case, because it assumed, that the callback will disable itself when fragment lifecycle reaches CREATED. However, we do not update trigger lifecycle changes for non-top fragments **yet**, therefore currently the callback isEnabled state dependent solely on the configuration value sourced from the screen. This led to situation, where a callback from a non-top fragment would be enabled & would prevent native dismiss. This commit changes that. First I decided to stay with approach that each fragment owns a `OnBackPressedCallback`. Alternative would be to have only a single callback owned by `StackContainer`, however registering a callback requires access to `OnBackPressedDispatcher` which is owned by the activity. `StackContainer` does not have easy access to activity. I figure, that we could either retrieve it from one of the fragments after it is attached to the activity, or from `reactContext`, but currently `StackContainer` is not aware of excessive React Native symbols directly & I want to keep it like that. Therefore keeping the callbacks in fragment allows for their easier lifecycle management. Regardless of above considerations, we need now to have the "top fragment" distinguished - and this is mostly what this commit does. Every time, we update primary navigation fragment we now also denote it as the "top fragment". Please note, that I use fragment manager state to determine the top-fragment. This is done deliberately! `stackModel` can be out of sync with the fragment manager state, e.g. by already having additional fragments for which the transactions have not yet been executed & therefore it can have fragments which aren't yet attached to fragment manager. I dislike two things this commit does: 1. it moves the `isTopFragment` to the fragment itself. I think this state should be, if at all, kept inside the `StackContainer`, where there is less risk that it will become out-of-sync (e.g. two top fragments). 2. The updateTopFragment code is not tightly coupled with SetPrimaryNavFragmentOp - I add a callback in every call site. I think right now that these two should be coupled. Finally, we should reconsider this approach once we have proper attach/detach system for "invisible" fragments. Please note, however, that it will still need to have some "top-fragment" mechanism, because if we e.g. top-screen will be transparent, we won't want the second-top screen to be detached & its callback will still block native dismiss, when clearly it shouldn't.
Member
Author
|
This is how I use the test for now: import React from 'react';
import { enableFreeze } from 'react-native-screens';
import Example from './Example';
// import * as Test from './src/tests/issue-tests';
import { COMPONENT_SCENARIOS } from './src/tests/single-feature-tests';
enableFreeze(true);
export default function App() {
const Component = COMPONENT_SCENARIOS.StackHost[0].screen;
return <Component />;
// return <Test.TestStackNesting />;
}Currently it is not reachable via Example app, because there are some bugs in the example app code I need to investigate. Will do it separately, its out of the scope of this PR |
Interesting behavior here is that native pop of nested stack is blocked by two screens: the last one in nested stack & the hosting one.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add initial implementation of prevent native dismiss on Android
This commit adds support for "prevent native dismiss" behavior on
Android for stack screens.
The added interface consists of prop on
StackScreen:preventNativeDismissand eventonNativeDismissPrevented, whichlets the programmer react to the native dismiss attempt.
This implementation follows RFC-0717.
It does not handle JS-side of the mechanism.
Closes https://github.com/software-mansion/react-native-screens-labs/issues/881
Changes
This is achieved by leveraging a couple of native mechanisms.
First, I lean into usage of
OnBackPressedCallbackAndroidX API, whichlets us handle this very gracefully & hopefully correct on many
SDK levels.
Note
TODO: The behavior on lower SDK levels has not been tested yet.
Now, for each
StackScreenFragmentwe add a dedicated callback, whichis enabled or disabled depending on the value. I do not add/remove the
callbacks because their order in the
OnBackPressedDispatchermatter,and we can add a callback only to the end of the list.
Caution
This implementation assumes that the fragments will move to
createdstate in the order they are in the fragment manager (stack order).
If this ever turns out to not be the case, we'll need to modify this
implementation.
Beside above ☝🏻 the added code also introduces "top-fragment" mechanism.
It is necessary to prevent non-top-fragment from blocking native dismiss.
Likely, when we introduce proper attach/detach mechanism for "non visible" fragments we will be able to get rid of this new mechanism, but currently it is necessary. Please see commit description: 1212f36
Add missing "exported custom direct event type constants"
I don't know how our events have worked so far. I thought that
overriding this method is obligatory. Apparently not. Maybe its
just hint for the React machinery. I'm not sure right now,
but I've verified that this code is indeed called & React gathers
this information.
Visual documentation
Screen.Recording.2026-02-05.at.16.00.28.mov
Test plan
These do not test "dynamic" case. It requires a bit more setup in
StackContainercode that is too much outside of the scope of this PR. I'll work on it separately.prevent-native-dismiss-single-stackprevent-native-dismiss-nested-stackChecklist