Skip to content
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

Add the ability to "retain" a dismissed screen, to support iOS Picture In Picture overlays. #11097

Open
wants to merge 10 commits into
base: 6.x
Choose a base branch
from
5 changes: 5 additions & 0 deletions packages/native-stack/src/index.tsx
Expand Up @@ -8,6 +8,11 @@ export { default as createNativeStackNavigator } from './navigators/createNative
*/
export { default as NativeStackView } from './views/NativeStackView';

/**
* Contexts
*/
export { RetainContext } from './views/RetainContext';
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me know if you want this to go into a /contexts/ directory, or somewhere else


/**
* Types
*/
Expand Down
150 changes: 83 additions & 67 deletions packages/native-stack/src/views/NativeStackView.native.tsx
Expand Up @@ -40,6 +40,7 @@
import useInvalidPreventRemoveError from '../utils/useInvalidPreventRemoveError';
import DebugContainer from './DebugContainer';
import HeaderConfig from './HeaderConfig';
import { RetainContext, useRetainContext } from './RetainContext';

const isAndroid = Platform.OS === 'android';

Expand Down Expand Up @@ -120,6 +121,7 @@
descriptor: NativeStackDescriptor;
previousDescriptor?: NativeStackDescriptor;
nextDescriptor?: NativeStackDescriptor;
hidden: boolean;
onWillDisappear: () => void;
onAppear: () => void;
onDisappear: () => void;
Expand All @@ -134,6 +136,7 @@
descriptor,
previousDescriptor,
nextDescriptor,
hidden,
onWillDisappear,
onAppear,
onDisappear,
Expand Down Expand Up @@ -252,6 +255,7 @@
return (
<Screen
key={route.key}
hidden={hidden}

Check failure on line 258 in packages/native-stack/src/views/NativeStackView.native.tsx

View workflow job for this annotation

GitHub Actions / lint

Type '{ children: Element; key: string; hidden: boolean; enabled: true; style: RegisteredStyle<AbsoluteFillStyle>; customAnimationOnSwipe: boolean | undefined; ... 25 more ...; freezeOnBlur: boolean | undefined; }' is not assignable to type 'IntrinsicAttributes & AnimatedProps<Omit<ScreenProps, "ref"> & RefAttributes<NativeScreen>>'.
enabled
style={StyleSheet.absoluteFill}
customAnimationOnSwipe={customAnimationOnGesture}
Expand Down Expand Up @@ -397,74 +401,86 @@

useInvalidPreventRemoveError(descriptors);

const { retainContext, hiddenRoutes, hiddenDescriptors, retainedKeys } =
useRetainContext(state, navigation, descriptors);
descriptors = { ...hiddenDescriptors, ...descriptors };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd avoid overriding props, as ot may be confusing for someone reading the code. I guess it's a mental model that we don't mutate values as long as possible.

How about mergedDescriptors and mergedRoutes.

It's a bit confusing (at least, I can imagine, it can be) that you use create an array and object without memorizing. Of course, it makes sense here as we don't pass them as props, but I'd add a comment, e.g., "no need to memorize as we don't pass them directly as props"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to memorize as we don't pass them directly as props

imo the comment isn't necessary since memorizing depends on other factors and not about props. it'd be confusing for me to see such a comment.


const routes = hiddenRoutes.concat(state.routes);

return (
<ScreenStack style={styles.container}>
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

best viewed with whitespace ignored

{state.routes.map((route, index) => {
const descriptor = descriptors[route.key];
const isFocused = state.index === index;
const previousKey = state.routes[index - 1]?.key;
const nextKey = state.routes[index + 1]?.key;
const previousDescriptor = previousKey
? descriptors[previousKey]
: undefined;
const nextDescriptor = nextKey ? descriptors[nextKey] : undefined;

return (
<SceneView
key={route.key}
index={index}
focused={isFocused}
descriptor={descriptor}
previousDescriptor={previousDescriptor}
nextDescriptor={nextDescriptor}
onWillDisappear={() => {
navigation.emit({
type: 'transitionStart',
data: { closing: true },
target: route.key,
});
}}
onAppear={() => {
navigation.emit({
type: 'transitionEnd',
data: { closing: false },
target: route.key,
});
}}
onDisappear={() => {
navigation.emit({
type: 'transitionEnd',
data: { closing: true },
target: route.key,
});
}}
onDismissed={(event) => {
navigation.dispatch({
...StackActions.pop(event.nativeEvent.dismissCount),
source: route.key,
target: state.key,
});

setNextDismissedKey(route.key);
}}
onHeaderBackButtonClicked={() => {
navigation.dispatch({
...StackActions.pop(),
source: route.key,
target: state.key,
});
}}
onNativeDismissCancelled={(event) => {
navigation.dispatch({
...StackActions.pop(event.nativeEvent.dismissCount),
source: route.key,
target: state.key,
});
}}
/>
);
})}
</ScreenStack>
<RetainContext.Provider value={retainContext}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about ScreensRetentionContext? I'd prefer to use nouns if possible.

<ScreenStack style={styles.container}>
{routes.map((route, offsetIndex) => {
const index = offsetIndex - hiddenRoutes.length;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's confusing. I don't think we have any offset here.

Suggested change
const index = offsetIndex - hiddenRoutes.length;
const isHidden = index < hiddenRoutes.length;

and then hidden={isHidden}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const isHidden = hiddenRouteKeys.includes(route.key) maybe more descriptive.

const descriptor = descriptors[route.key];
const isFocused = state.index === index;
const previousKey = state.routes[index - 1]?.key;
const nextKey = state.routes[index + 1]?.key;
const previousDescriptor = previousKey
? descriptors[previousKey]
: undefined;
const nextDescriptor = nextKey ? descriptors[nextKey] : undefined;

return (
<SceneView
key={route.key}
hidden={index < 0}
index={index}
focused={isFocused}
descriptor={descriptor}
previousDescriptor={previousDescriptor}
nextDescriptor={nextDescriptor}
onWillDisappear={() => {
navigation.emit({
type: 'transitionStart',
data: { closing: true },
target: route.key,
});
}}
onAppear={() => {
navigation.emit({
type: 'transitionEnd',
data: { closing: false },
target: route.key,
});
}}
onDisappear={() => {
navigation.emit({
type: 'transitionEnd',
data: { closing: true },
target: route.key,
});
}}
onDismissed={(event) => {
navigation.dispatch({
...StackActions.pop(event.nativeEvent.dismissCount),
source: route.key,
target: state.key,
});

if (!retainedKeys.includes(route.key)) {
setNextDismissedKey(route.key);
}
Comment on lines +461 to +463
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this warning fires spuriously when restoring a previously dismissed screen

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaredly yea this one has some issue, need to address it later

}}
onHeaderBackButtonClicked={() => {
navigation.dispatch({
...StackActions.pop(),
source: route.key,
target: state.key,
});
}}
onNativeDismissCancelled={(event) => {
navigation.dispatch({
...StackActions.pop(event.nativeEvent.dismissCount),
source: route.key,
target: state.key,
});
}}
/>
);
})}
</ScreenStack>
</RetainContext.Provider>
);
}

Expand Down
138 changes: 138 additions & 0 deletions packages/native-stack/src/views/RetainContext.tsx
@@ -0,0 +1,138 @@
import type {
ParamListBase,
Route,
StackNavigationState,
} from '@react-navigation/native';
import * as React from 'react';

import type {
NativeStackDescriptorMap,
NativeStackNavigationHelpers,
} from '../types';

type RetainContextT = {
retain(): string;
release(key: string): boolean;
restore(key: string): boolean;
supported: boolean;
};

/**
* This context allows you to retain screens in memory, even after
* they have been popped off the stack. This is important for
* supporting native iOS "Picture in Picture" mode, as the
* originating UIViewController must be retained in memory for the
* PiP overlay to continue running.
* Retained screens can also be "restored" to the top of the stack,
* for example when the user taps the button in the PiP overlay
* to return to the app.
*/
Comment on lines +21 to +29
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* This context allows you to retain screens in memory, even after
* they have been popped off the stack. This is important for
* supporting native iOS "Picture in Picture" mode, as the
* originating UIViewController must be retained in memory for the
* PiP overlay to continue running.
* Retained screens can also be "restored" to the top of the stack,
* for example when the user taps the button in the PiP overlay
* to return to the app.
*/
* This context allows you to retain screens in memory, even after
* they have been popped off the stack. That is important for
* supporting native iOS "Picture in Picture" mode, as the
* originating UIViewController must be retained in memory for the
* PiP overlay to continue running.
* Retained screens can also be restored to the top of the stack,
* for example, when the user taps the button in the PiP overlay
* to return to the app.
*/

I'm not native so feel free to ignore. I believe there's no need for a quotation as the description is precise enough.

export const RetainContext = React.createContext<RetainContextT>({
retain() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd do something even stronger.

Suggested change
retain() {
get retain() {

This will throw an error on access, without waiting for invoking the function.

throw new Error(`Not in a native screen stack`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw new Error(`Not in a native screen stack`);
throw new Error(MISSING_CONTEXT_ERROR);

and then:

const MISSING_CONTEXT_ERROR = `Couldn't find a native screen context.`

},
release() {
throw new Error(`Not in a native screen stack`);
},
restore() {
throw new Error(`Not in a native screen stack`);
},
supported: false,
});

type RetainedScenes = {
[key: string]: {
route: Route<any>;
descriptor: NativeStackDescriptorMap['key'];
};
};

export function useRetainContext(
state: StackNavigationState<ParamListBase>,
navigation: NativeStackNavigationHelpers,
descriptors: NativeStackDescriptorMap
) {
const [retainedScenes, setRetainedScenes] = React.useState<RetainedScenes>(
{}
);
const latestValues = React.useRef({ state, retainedScenes, descriptors });
React.useEffect(() => {
latestValues.current = { state, retainedScenes, descriptors };
});
Comment on lines +58 to +61
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm rather against patters like that. The new state should be the result of props and the previous state as react may batch those updates, we should not rely on what is currently in useRef.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is not very needed here and it's possible to refactor the code without that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume the intention is to memoize the retain, release, restore functions. In that case define those functions separately with useLatestCallback and then they can be safely added to dependency array for useMemo.

With that it's unnecessary to do such a ref.


const retainContext = React.useMemo<RetainContextT>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const retainContext = React.useMemo<RetainContextT>(
const retainContext = React.useMemo(

I thnk TS should be smart enough. Maybe not :)

() => ({
retain() {
const { state, descriptors } = latestValues.current;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not state? Also, please avoid shadowing variables as it gets harder to understand the code.

const route = state.routes[state.routes.length - 1];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of assuming the last screen, this hook should use the useRoute hook to get the correct route object

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As currently written, the retain function isn't a hook. I'll look into changing the API to a useRetain and see whether that can meet all our needs

setRetainedScenes((screens) => ({
...screens,
[route.key]: {
route,
descriptor: descriptors[route.key],
},
Comment on lines +70 to +73
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should only store the route key. Duplicating the route and descriptor is problematic as those could change after storing here making them out of sync. They should always be read from state.routes and descriptors.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm it can't be read from state.routes if the screen has been popped off the stack; that's why I'm storing it here. In the case where the route hasn't been popped, I use the one from state.routes.

}));
return route.key;
},
release(key) {
setRetainedScenes((screens) => {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const { [key]: _, ...rest } = screens;
return rest;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check and warn if not present

});
return latestValues.current.retainedScenes[key] != null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should not rely on the ref here as concurrent mode can make it flaky. Why can't you rely on the previous state?

},
restore(key) {
const { retainedScenes, state } = latestValues.current;

const route = retainedScenes[key]?.route;
if (!route) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throw error

return false;
}

// Remove from retained
setRetainedScenes((screens) => {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const { [key]: _, ...rest } = screens;
return rest;
});

const index = state.routes.findIndex((r) => r.key === key);
const routes =
index !== -1 ? state.routes.slice(0, index) : state.routes.slice();
routes.push(route);
navigation.dispatch({
type: 'RESET',
payload: { ...state, index: routes.length - 1, routes },
});
Comment on lines +100 to +107
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't refer to actions by string. Use the CommonActions.reset action creator instead. In this case it should be possible to just use navigation.reset.

Also, I assume it's moving the screen to the end. Is this necessary/desirable? If the screen wasn't the last one, seems strange to change the order after pip restore. I'd remove this, consumers can do this themselves if they want to.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the screen wasn't the last one, seems strange to change the order after pip restore.

I thought that in order for a screen to be visible on a screen stack, it needed to be the last one. Is that not the case? restore() is supposed to bring the screen back to visibility; either adding it to the end if it had been popped off, or popping off anything on top of it if the screen still exists in the stack.


return true;
},
supported: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This boolean seems unnecessary if it's always true.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's true here, but the default value for the RetainContext has supported: false

}),
[navigation]
);

const routeKeys = state.routes.reduce((map, route) => {
map[route.key] = true;
return map;
}, {} as { [key: string]: true });

// Routes that were marked as 'retain' and have been removed from the stack,
// for which we still want to keep the memory allocated.
const hiddenRoutes: Route<any>[] = [];
const hiddenDescriptors: NativeStackDescriptorMap = {};
Object.keys(retainedScenes).forEach((key) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer

const {
  hiddenDescriptors: descriptors,
  hiddenRoutes: routes 
} = Object.values(retainedScreen).reduce(() => { ... }, { routes: [], descriptors: {} })

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code will be much simpler if there was just a hiddenRouteKeys array. Seems like the only place that needs it is when we set hidden={true}, and with such an array, it could just check hidden={hiddenRoutes.includes(route.key)} without changing descriptor and routes related logic.

if (!routeKeys[key]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just check the state.routes here directly, even though it's a higher theoretical complexity. This is not a big deal as this array should be small anyway.

hiddenRoutes.push(retainedScenes[key].route);
hiddenDescriptors[key] = retainedScenes[key].descriptor;
}
});

return {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is that supposed to be a public API?

retainContext,
hiddenRoutes,
hiddenDescriptors,
retainedKeys: Object.keys(retainedScenes),
};
}