-
-
Notifications
You must be signed in to change notification settings - Fork 587
feat(Android, Tabs): safe area component for Android #3215
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
Conversation
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.
I had some issues with testing runtime locally, but verified with @kligarski that it's working well.
android/src/main/java/com/swmansion/rnscreens/gamma/tabs/TabsHost.kt
Outdated
Show resolved
Hide resolved
cd722ac
to
575c00b
Compare
Currently, Let me know what you think. |
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.
Okay. The PR looks really nice. I have few requests, please answer them.
android/src/main/java/com/swmansion/rnscreens/safearea/EdgeInsets.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/swmansion/rnscreens/gamma/tabs/TabsHost.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/swmansion/rnscreens/gamma/tabs/TabsHost.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/swmansion/rnscreens/safearea/SafeAreaProvider.kt
Show resolved
Hide resolved
} | ||
|
||
init { | ||
ViewCompat.setOnApplyWindowInsetsListener(this, this) |
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.
Why are we using insets listener here, instead of regular onApplyWindowInsets method?
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.
I used it to get WindowInsetsCompat
instance instead of getting WindowInsets
and converting it manually.
android/src/main/java/com/swmansion/rnscreens/safearea/SafeAreaView.kt
Outdated
Show resolved
Hide resolved
// Block the main thread until the native module thread is finished with | ||
// its current tasks. To do this we use the done boolean as a lock and enqueue | ||
// a task on the native modules thread. When the task runs we can unblock the | ||
// main thread. This should be safe as long as the native modules thread | ||
// does not block waiting on the main thread. | ||
var done = false | ||
val lock = ReentrantLock() | ||
val condition = lock.newCondition() | ||
val startTime = System.nanoTime() | ||
var waitTime = 0L | ||
getReactContext(this).runOnNativeModulesQueueThread { | ||
lock.withLock { | ||
if (!done) { | ||
done = true | ||
condition.signal() | ||
} | ||
} | ||
} |
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.
hate this. Let's keep our fingers crossed we won't run into problems with this.
getReactContext(this).runOnNativeModulesQueueThread { | ||
lock.withLock { | ||
if (!done) { | ||
done = true |
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.
We're modifying here non-volatile value possible across thread boundaries 🙈
I won't demand changes here, since this code is already a bit battle tested, but man...
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.
The PR looks really good now. I have singl remark. Please answer it. I'll check the runtime behaviour in meantime.
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.
Realised it is not used yet. The app builds fine. I think we can proceed after cleaning up what I've indicated in preceding review.
Description
Adds implementation for handling safe area on Android for bottom tabs.
Implementation has been adapted from
react-native-safe-area-context
.TODO:
SafeAreaView
component (system and/or interface bars)SafeAreaView
component (system and/or interface bars) (https://github.com/software-mansion/react-native-screens-labs/issues/434)CustomToolbar
) (https://github.com/software-mansion/react-native-screens-labs/issues/435)Screen.Recording.2025-09-15.at.15.11.21.mov
Transparent tab bar
top: false, bottom: false
top: true, bottom: false
top: true, bottom: true
Changes to TabsHost's layout
SafeAreaView
After internal discussion about approach to
SafeAreaView
, we had following conclusions:Screen
s of our navigation containers (e.g.StackScreen
,BottomTabsScreen
) to have full dimensions of their parents, even if it means that they will be laid out behind navigation bars (header in Stack, tab bar),SafeAreaView
will provide unified way to handle the safe area,onApplyWindowInsets
), e.g.systemBars
,displayCutout
bottomNavigationView
Before this PR
Prior to this PR, we were using
LinearLayout
forTabsHost
:This approach had following problems:
contentView
did not have dimensions of its parent, which is not what we wanted,contentView
's height - all content insideTabScreen
was laid out as if the screen had full height of its parent -> this meant thatcontentView
's dimensions and actual content dimensions were not in sync,contentView
's bounds.Approach in this PR
In this PR, we change
TabsHost
's layout toFrameLayout
which allows multiple views placed on top of each other - this is what we want to achieve (tab bar floating over content, attached to the bottom).To attach
bottomNavigationView
to the bottom, we useGravity.BOTTOM
.Now, we can:
bottomNavigationView
,SafeAreaView
to control how much space can the actual content take (do we allow it to render underbottomNavigationView
).Support for older Android versions
On Android versions prior to R, insets dispatch is broken (children of ViewGroup receive insets from previous child; they should all receive the same insets). That's why we need to override
dispatchApplyWindowInsets
implementation inTabsHost
. InViewGroup
's implementation of this method,View
's implementation is used (viasuper
) - we can't access this directly. Unfortunately,View
's implementation sets some private flags which are used by defaultonApplyWindowInsets
implementation. If we try to useonApplyWindowInsets
on API 28 without setting the private flag, application goes into infinite loop (fitSystemWindows
callsdispatchApplyWindowInsets
-> we might want to investigate this in more detail). As we don't use insets inTabsHost
, I decided not to callonApplyWindowInsets
inTabsHost
at all. I haven't found any problems with it yet.Changes
SafeAreaView
and related classes for both architecturesSafeAreaProvider
interface and implement it forTabsHost
TabsHost
to useFrameLayout
:TabsScreen
layout behind tab bar (take full available height to match JS screen)dispatchApplyWindowInsets
inTabsHost
in order to fix insets for older Android versionsinsetType
prop to control what kind of insets should SafeAreaView respect (all, only system, only interface)Test code and steps to reproduce
Run
TestBottomTabs
with uncommented SafeAreaView. You can change which edges are enabled and addinsetType
prop.Checklist