-
-
Notifications
You must be signed in to change notification settings - Fork 661
fix(Android, Tabs): Apply bottom inset immediately when the container is attached #4098
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -15,6 +15,7 @@ import TestTabsStaleStateUpdateRejection from './test-tabs-stale-update-rejectio | |||||
| import TestTabsTabBarMinimizeBehavior from './test-tabs-tab-bar-minimize-behavior-ios'; | ||||||
| import TestTabsTabBarControllerMode from './test-tabs-tab-bar-controller-mode-ios'; | ||||||
| import TestTabsSpecialEffectsScrollToTop from './test-tabs-special-effects-scroll-to-top'; | ||||||
| import TestTabsSynchronousInsetsAndroid from './test-tabs-synchronous-insets-android'; | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there is no corresponding iOS screen with the same name, we can omit the platform from the scenario name (similar to how it is done for TestTabsTabBarMinimizeBehavior). The fact that a scenario is Android-only is already indicated by the icon on the scenario list.
Suggested change
|
||||||
| import TestTabsTabBarExperimentalUserInterfaceStyle from './test-tabs-tab-bar-experimental-user-interface-style-ios'; | ||||||
| import TestTabsLifecycleEvents from './test-tabs-lifecycle-events'; | ||||||
| import TestTabsItemTitle from './test-tabs-item-title-ios'; | ||||||
|
|
@@ -35,6 +36,7 @@ const scenarios = { | |||||
| TestTabsTabBarMinimizeBehavior, | ||||||
| TestTabsTabBarControllerMode, | ||||||
| TestTabsSpecialEffectsScrollToTop, | ||||||
| TestTabsSynchronousInsetsAndroid, | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| TestTabsTabBarExperimentalUserInterfaceStyle, | ||||||
| TestTabsLifecycleEvents, | ||||||
| TestTabsItemTitle, | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,153 @@ | ||
| import React, { createContext, useContext, useState, useEffect } from 'react'; | ||
| import { Button, StyleSheet, Text, View } from 'react-native'; | ||
|
|
||
|
t0maboro marked this conversation as resolved.
|
||
| import { scenarioDescription } from './scenario-description'; | ||
| import { createScenario } from '@apps/tests/shared/helpers'; | ||
| import { SettingsSwitch } from '@apps/shared'; | ||
| import { | ||
| TabsContainerWithHostConfigContext, | ||
| type TabRouteConfig, | ||
| useTabsHostConfig, | ||
| DEFAULT_TAB_ROUTE_OPTIONS, | ||
| } from '@apps/shared/gamma/containers/tabs'; | ||
| import { | ||
| StackContainer, | ||
| useStackNavigationContext, | ||
| } from '@apps/shared/gamma/containers/stack'; | ||
| import { Colors } from '@apps/shared/styling'; | ||
|
|
||
| const TestConfigContext = createContext({ | ||
| syncInsets: true, | ||
| setSyncInsets: (_: boolean) => {}, | ||
| }); | ||
|
|
||
| export function SetupScreen() { | ||
| const { syncInsets, setSyncInsets } = useContext(TestConfigContext); | ||
| const { push } = useStackNavigationContext(); | ||
|
|
||
| return ( | ||
| <View style={styles.centerContainer}> | ||
| <Text style={styles.heading}>Test Configuration</Text> | ||
|
|
||
| <View style={styles.section}> | ||
| <SettingsSwitch | ||
| label="shouldApplyInsetsSynchronously" | ||
| value={syncInsets} | ||
| onValueChange={setSyncInsets} | ||
| /> | ||
| </View> | ||
|
|
||
| <Button | ||
| title="Push Tabs Screen" | ||
| color={Colors.primary} | ||
| onPress={() => push('TabsScreen')} | ||
| /> | ||
| </View> | ||
| ); | ||
| } | ||
|
|
||
| export function DummyTabScreen() { | ||
| const { syncInsets } = useContext(TestConfigContext); | ||
| const { updateHostConfig } = useTabsHostConfig(); | ||
|
|
||
| useEffect(() => { | ||
| updateHostConfig({ | ||
| android: { tabBarShouldApplyInsetsSynchronously: syncInsets }, | ||
| }); | ||
| }, [syncInsets, updateHostConfig]); | ||
|
|
||
| return ( | ||
| <View style={styles.centerContainer}> | ||
| <Text style={styles.heading}>Inside Tabs</Text> | ||
| <Text style={styles.info}> | ||
| Synchronous Insets: {syncInsets ? 'ENABLED' : 'DISABLED'} | ||
| </Text> | ||
| </View> | ||
| ); | ||
| } | ||
|
|
||
| const ROUTE_CONFIGS: TabRouteConfig[] = [ | ||
| { | ||
| name: 'MainTab', | ||
| Component: DummyTabScreen, | ||
| options: { | ||
| ...DEFAULT_TAB_ROUTE_OPTIONS, | ||
| title: 'Main', | ||
| }, | ||
| }, | ||
| { | ||
| name: 'SecondaryTab', | ||
| Component: () => ( | ||
| <View style={styles.centerContainer}> | ||
| <Text style={styles.text}>Another tab</Text> | ||
| </View> | ||
| ), | ||
| options: { | ||
| ...DEFAULT_TAB_ROUTE_OPTIONS, | ||
| title: 'Secondary', | ||
| }, | ||
| }, | ||
| ]; | ||
|
|
||
| export function TabsScreen() { | ||
| return <TabsContainerWithHostConfigContext routeConfigs={ROUTE_CONFIGS} />; | ||
| } | ||
|
|
||
| export function App() { | ||
| const [syncInsets, setSyncInsets] = useState(true); | ||
|
|
||
| return ( | ||
| <TestConfigContext.Provider value={{ syncInsets, setSyncInsets }}> | ||
| <StackContainer | ||
| routeConfigs={[ | ||
| { | ||
| name: 'SetupScreen', | ||
| Component: SetupScreen, | ||
| options: {}, | ||
| }, | ||
| { | ||
| name: 'TabsScreen', | ||
| Component: TabsScreen, | ||
| options: {}, | ||
| }, | ||
| ]} | ||
| /> | ||
| </TestConfigContext.Provider> | ||
| ); | ||
| } | ||
|
|
||
| const styles = StyleSheet.create({ | ||
| centerContainer: { | ||
| flex: 1, | ||
| justifyContent: 'center', | ||
| alignItems: 'center', | ||
| padding: 24, | ||
| backgroundColor: Colors.background, | ||
| }, | ||
| heading: { | ||
| fontSize: 24, | ||
| fontWeight: 'bold', | ||
| marginBottom: 20, | ||
| color: Colors.text, | ||
| }, | ||
| info: { | ||
| fontSize: 16, | ||
| color: Colors.NavyLight60, | ||
| marginTop: 10, | ||
| }, | ||
| text: { | ||
| fontSize: 16, | ||
| color: Colors.text, | ||
| }, | ||
| section: { | ||
| marginBottom: 30, | ||
| width: '100%', | ||
| backgroundColor: Colors.cardBackground, | ||
| borderColor: Colors.cardBorder, | ||
| borderWidth: 1, | ||
| borderRadius: 8, | ||
| padding: 16, | ||
| }, | ||
| }); | ||
|
|
||
| export default createScenario(App, scenarioDescription); | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,10 @@ | ||||||
| import type { ScenarioDescription } from '@apps/tests/shared/helpers'; | ||||||
|
|
||||||
| export const scenarioDescription: ScenarioDescription = { | ||||||
| name: 'Synchronous insets application', | ||||||
| key: 'test-tabs-synchronous-insets-android', | ||||||
| details: 'Test synchronous application of window insets on Android', | ||||||
| platforms: ['android'], | ||||||
| e2eCoverage: 'tbd', | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the scenario E2E test section it's already decided that this test won't be automated so e2eCoverage value update should be made:
Suggested change
|
||||||
| smokeTest: false, | ||||||
| }; | ||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,42 @@ | ||||||
| # Test Scenario: Synchronous insets application | ||||||
|
|
||||||
| ## Details | ||||||
|
|
||||||
| **Description:** Verifies the `tabBarShouldApplyInsetsSynchronously` prop on the Android. When set to `true`, window insets are retrieved from the `DecorView` and applied synchronously during the `onAttachedToWindow` lifecycle phase. This prevents visible layout jumps of the bottom tab bar that occur when insets are applied asynchronously by the system during or after a screen transition. | ||||||
|
|
||||||
| **OS test creation version:** Android: API Level 36. | ||||||
|
|
||||||
| ## E2E test | ||||||
|
|
||||||
| No. Visual layout jumps during screen transitions are difficult to capture reliably in automated UI tests like Detox without precise frame-by-frame visual regression tools. Manual visual verification is required. | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Following new naming (described in RFC), if we are not going to implement e2e test I would change this line to:
Suggested change
|
||||||
|
|
||||||
| ## Prerequisites | ||||||
|
|
||||||
| - Android emulator or physical device. | ||||||
|
|
||||||
|
|
||||||
| ## Steps | ||||||
|
|
||||||
| ### Synchronous insets | ||||||
|
|
||||||
| 1. Launch the app and navigate to **Synchronous insets application**. | ||||||
|
|
||||||
| - [ ] Expected: The **Setup** screen is visible. The `shouldApplyInsetsSynchronously` button is toggled `true`. | ||||||
|
|
||||||
| 2. Tap the **Push Tabs Screen** button. | ||||||
|
|
||||||
| - [ ] Expected: A transition to the tabs screen begins. The bottom tab bar renders with the correct height and bottom padding respecting system navigation bars. There is no visible vertical layout jump or resize of the tab bar after the transition completes. | ||||||
|
|
||||||
| 3. Press the back button to return to the **Setup** screen. | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
|
|
||||||
| --- | ||||||
|
|
||||||
| ### Asynchronous insets | ||||||
|
|
||||||
| 4. On the **Setup** screen, toggle the `shouldApplyInsetsSynchronously` button to `false`. | ||||||
|
|
||||||
| - [ ] Expected: The switch state updates. | ||||||
|
|
||||||
| 5. Tap the **Push Tabs Screen** button. | ||||||
|
|
||||||
| - [ ] Expected: A transition to the tabs screen begins. The bottom tab bar may initially render with incorrect height (lacking proper bottom padding). Shortly after or right at the end of the transition, the tab bar visibly "jumps" or resizes as the system asynchronously dispatches the insets. | ||||||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -15,4 +15,24 @@ export interface TabsHostPropsAndroid { | |||
| * @supported API 30 or higher | ||||
| */ | ||||
| tabBarRespectsIMEInsets?: boolean | undefined; | ||||
|
|
||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we've been using no new line between props. Recently in FormSheet we started using them but it would be nice to keep one convention. Not sure which one. |
||||
| /** | ||||
| * @summary Determines whether window insets should be applied synchronously from the DecorView. | ||||
| * | ||||
| * By default, the Android system applies window insets with some delay | ||||
| * (React Native may calculate the initial layout before the insets are dispatched). | ||||
| * This can cause sudden height changes of the bottom tab bar during screen transitions. | ||||
| * | ||||
| * Setting this prop to `true` fetches and applies the insets manually during the view attachment | ||||
| * phase, effectively preventing these visual glitches. | ||||
| * | ||||
| * Setting this to `false` disables this workaround and falls back to the default asynchronous | ||||
| * behavior. This serves as an opt-out mechanism if the synchronous application causes | ||||
| * conflicts with other inset-handling configurations or unexpected UI side-effects. | ||||
| * | ||||
| * @default true | ||||
| * | ||||
| * @platform android | ||||
| */ | ||||
| tabBarShouldApplyInsetsSynchronously?: boolean; | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm thinking about how we want to expose controlling the inset in tabs here. But should we also expose an option to disable the inset completely? E.g. is somebody uses a footer of some kind that handles the bottom inset visually but doesn't consume it natively (e.g. a react native view)? If so, do we need an option to control synchronous read? I guess it won't hurt even if we add a prop to control inset application later. |
||||
| } | ||||

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.
First call to this method comes from us in
onAttachedToWindow-> can we call it "by system" then?