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

fix(Android, Fabric): jumping content with native header #2169

Merged
merged 66 commits into from
Jun 18, 2024
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
66 commits
Select commit Hold shift + click to select a range
6b2e6da
Stash current changes with jni setup
kkafar Jun 4, 2024
2a2e1bc
Play around with various ideas
kkafar Jun 5, 2024
5c8fd35
Working PoC
kkafar Jun 7, 2024
581cd7e
Get access to font size!
kkafar Jun 7, 2024
fd86a96
Add missing callsite parameter
kkafar Jun 7, 2024
fd9b32f
Improve PoC by adding font-size param
kkafar Jun 7, 2024
bdfc038
Take font size into consideration
kkafar Jun 7, 2024
1ff7f95
Cleanup custom toolbar
kkafar Jun 7, 2024
29a92f3
Remove native function from ScreenViewManager
kkafar Jun 7, 2024
8b445d8
Partially cleanup package file
kkafar Jun 7, 2024
ed0b6ca
Cleanup C++ code & move implementation back to header file
kkafar Jun 7, 2024
342c27e
Format common/ C++ code
kkafar Jun 7, 2024
7d2c5d8
Add ifdefs for Android implementation
kkafar Jun 7, 2024
5ed1933
Cleanup library entrypoint files
kkafar Jun 7, 2024
aadfe60
Move WEAK_THIS initialization to createViewManagers & ...
kkafar Jun 7, 2024
ecd33f6
Missing code comment & cleaup
kkafar Jun 7, 2024
14b0724
Cleanup shadow nodes
kkafar Jun 7, 2024
9df3744
Remove console log from Screen
kkafar Jun 7, 2024
b74618b
Restore in apps directory
kkafar Jun 7, 2024
1c1f8a0
More cleanup in java files
kkafar Jun 7, 2024
81805f0
Optimize imports
kkafar Jun 7, 2024
f8a251b
Improve signature of getInstance method
kkafar Jun 9, 2024
29f6b16
Improve error message in C++ layer when receiving null instance
kkafar Jun 9, 2024
6997c26
Whitespaces in C++ code
kkafar Jun 9, 2024
f67ba5e
Improve JVM code a bit more, nicer error messages
kkafar Jun 9, 2024
27c6ec9
Remove unnecessary line setting support action bar on Activity
kkafar Jun 9, 2024
35f41b7
Restore compiler warning flags
kkafar Jun 9, 2024
5d9210a
Add static variable for font-unset value & cleanup in loggs
kkafar Jun 9, 2024
5221637
Remove unnecessary library initialization code
kkafar Jun 9, 2024
73e533f
Include fbjni only on Android
kkafar Jun 9, 2024
55418d9
Cleanup imports
kkafar Jun 9, 2024
eb36e66
Setup custom component descriptor for screen stack
kkafar Jun 9, 2024
61c42d1
Add simple LRU cache for headerHeight computation
kkafar Jun 10, 2024
f76a0d9
Use HEADER_HEIGHT_CORRECTION global in custom header config descriptor
kkafar Jun 10, 2024
fcc5415
Handle headerShown: false
kkafar Jun 10, 2024
8ea06ac
Make HeaderHeightCacheEntry private
kkafar Jun 10, 2024
8e3362f
Better naming & rnscreens namespace
kkafar Jun 10, 2024
608a11c
Use initialized empty option instead of uninitialized option as retur…
kkafar Jun 13, 2024
9884e83
Use margin instead of setting header config size
kkafar Jun 13, 2024
699015e
Remove setup wiht screen stack header config component descriptor
kkafar Jun 13, 2024
f465673
Try out setup with setting margin
kkafar Jun 13, 2024
51ebbb7
Go back to padding solution
kkafar Jun 13, 2024
e90f57d
Update frame origin & height manually
kkafar Jun 13, 2024
d3da357
Separate JVM implementation to separate class
kkafar Jun 13, 2024
c3aab0e
Working PoC with padding & keeping header height in node state
kkafar Jun 14, 2024
175f49c
Some comments for JVM implementation part
kkafar Jun 14, 2024
02a58c6
Use std:cref
kkafar Jun 14, 2024
2c42d65
Cleanup example a bit
kkafar Jun 14, 2024
8a638c4
Restore Test42 as default
kkafar Jun 14, 2024
dc73577
Merge branch 'main' into @kkafar/attempt-to-fix-jumping-header
kkafar Jun 14, 2024
fc701ef
Add test header to new App.js
kkafar Jun 14, 2024
cc110c5
Remove leftover showcase code
kkafar Jun 14, 2024
3746958
Rename HeaderCorrectionModes to FrameCorrectionModes & put it behind …
kkafar Jun 16, 2024
e3c2fd8
Add verbose error message
kkafar Jun 16, 2024
8fb2709
Cleanup debug logging
kkafar Jun 16, 2024
5938d56
Make compareFrameSizes inline
kkafar Jun 16, 2024
359bc49
Merge branch 'main' into @kkafar/attempt-to-fix-jumping-header
kkafar Jun 17, 2024
54463b4
Use Float instead of raw float to accomodate for platform differences
kkafar Jun 17, 2024
d31dd00
Merge branch 'main' into @kkafar/attempt-to-fix-jumping-header
kkafar Jun 17, 2024
c5e3c6e
Remove merge artifact: App.js in old localisation
kkafar Jun 17, 2024
184a527
Make comparision equality weak
kkafar Jun 17, 2024
fbda22e
Set padding only if headerConfig child is present
kkafar Jun 17, 2024
cb6fe2b
Handle weird case when title is empty but there is custom font size
kkafar Jun 17, 2024
db138f9
Add comment why 0.01 was chosen as default value for parameter
kkafar Jun 17, 2024
a0d15bb
Revert building example from source
kkafar Jun 18, 2024
4f34895
Revert disabling of headerTopInset
kkafar Jun 18, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions FabricTestExample/android/settings.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,12 @@ rootProject.name = 'FabricTestExample'
apply from: file("../node_modules/@react-native-community/cli-platform-android/native_modules.gradle"); applyNativeModulesSettingsGradle(settings)
include ':app'
includeBuild('../node_modules/@react-native/gradle-plugin')

includeBuild('../node_modules/react-native') {
dependencySubstitution {
substitute(module("com.facebook.react:react-android")).using(project(":packages:react-native:ReactAndroid"))
substitute(module("com.facebook.react:react-native")).using(project(":packages:react-native:ReactAndroid"))
substitute(module("com.facebook.react:hermes-android")).using(project(":packages:react-native:ReactAndroid:hermes-engine"))
substitute(module("com.facebook.react:hermes-engine")).using(project(":packages:react-native:ReactAndroid:hermes-engine"))
}
}
kkafar marked this conversation as resolved.
Show resolved Hide resolved
145 changes: 143 additions & 2 deletions android/src/main/java/com/swmansion/rnscreens/RNScreensPackage.kt
Original file line number Diff line number Diff line change
@@ -1,21 +1,62 @@
package com.swmansion.rnscreens

import android.util.Log
import android.view.View
import android.view.View.MeasureSpec
import androidx.appcompat.app.AppCompatActivity
import androidx.appcompat.widget.Toolbar
import androidx.coordinatorlayout.widget.CoordinatorLayout
import com.facebook.react.TurboReactPackage
import com.facebook.react.bridge.NativeModule
import com.facebook.react.bridge.ReactApplicationContext
import com.facebook.react.module.annotations.ReactModuleList
import com.facebook.react.module.model.ReactModuleInfo
import com.facebook.react.module.model.ReactModuleInfoProvider
import com.facebook.react.uimanager.PixelUtil
import com.facebook.react.uimanager.ViewManager
import com.google.android.material.appbar.AppBarLayout
import java.lang.ref.WeakReference

@ReactModuleList(
nativeModules = [
ScreensModule::class
]
)
class RNScreensPackage : TurboReactPackage() {
override fun createViewManagers(reactContext: ReactApplicationContext) =
listOf<ViewManager<*, *>>(
// The state required to compute header dimensions. We want this on instance rather than on class
// for context access & being tied to instance lifetime.
private lateinit var coordinatorLayout: CoordinatorLayout
private lateinit var appBarLayout: AppBarLayout
private lateinit var dummyContentView: View
private lateinit var toolbar: Toolbar

// We do not want to be responsible for the context lifecycle. If it's null, we're fine.
// This same context is being passed down to our view components so it is destroyed
// only if our views also are.
private var reactContext: WeakReference<ReactApplicationContext> = WeakReference(null)

init {
// We load the library so that we are able to communicate with our C++ code (descriptor & shadow nodes).
// Basically we leak this object to C++, as its lifecycle should span throughout whole application
// lifecycle anyway.
try {
System.loadLibrary(LIBRARY_NAME)
} catch (e: UnsatisfiedLinkError) {
Log.w(TAG, "Failed to load $LIBRARY_NAME")
}

}

override fun createViewManagers(reactContext: ReactApplicationContext): List<ViewManager<*, *>> {
// This is the earliest we lay our hands on react context.
// Moreover this is called before FabricUIManger has finished initializing, not to mention
// installing its C++ bindings - so we are safe in terms of creating this hierarchy
// before RN starts creating shadow nodes.
kkafar marked this conversation as resolved.
Show resolved Hide resolved
WEAK_THIS = WeakReference(this)
this.reactContext = WeakReference(reactContext)
ensureDummyLayoutWithHeader(reactContext)
kkafar marked this conversation as resolved.
Show resolved Hide resolved

return listOf<ViewManager<*, *>>(
ScreenContainerViewManager(),
ScreenViewManager(),
ModalScreenViewManager(),
Expand All @@ -24,6 +65,7 @@ class RNScreensPackage : TurboReactPackage() {
ScreenStackHeaderSubviewManager(),
SearchBarManager()
)
}

override fun getModule(
s: String,
Expand Down Expand Up @@ -51,4 +93,103 @@ class RNScreensPackage : TurboReactPackage() {
moduleInfos
}
}

/**
* Initializes dummy view hierarchy with CoordinatorLayout, AppBarLayout and dummy View.
* We utilize this to compute header height (app bar layout height) from C++ layer when its needed.
*/
private fun ensureDummyLayoutWithHeader(reactContext: ReactApplicationContext) {
if (::coordinatorLayout.isInitialized) {
return
}
// We need to use activity here, as react context does not have theme attributes required by
// AppBarLayout attached leading to crash.
val contextWithTheme =
requireNotNull(reactContext.currentActivity) { "[RNScreens] Attempt to use context detached from activity" }
coordinatorLayout = CoordinatorLayout(contextWithTheme)

appBarLayout = AppBarLayout(contextWithTheme).apply {
layoutParams = CoordinatorLayout.LayoutParams(
CoordinatorLayout.LayoutParams.MATCH_PARENT,
CoordinatorLayout.LayoutParams.WRAP_CONTENT,
)
}

toolbar = Toolbar(contextWithTheme).apply {
kkafar marked this conversation as resolved.
Show resolved Hide resolved
title = "FontSize123!#$"
layoutParams = AppBarLayout.LayoutParams(
AppBarLayout.LayoutParams.MATCH_PARENT,
AppBarLayout.LayoutParams.WRAP_CONTENT
).apply { scrollFlags = 0 }
}

(contextWithTheme as AppCompatActivity).setSupportActionBar(toolbar)
appBarLayout.addView(toolbar)

dummyContentView = View(contextWithTheme).apply {
layoutParams = CoordinatorLayout.LayoutParams(
CoordinatorLayout.LayoutParams.MATCH_PARENT,
CoordinatorLayout.LayoutParams.MATCH_PARENT
)
}

coordinatorLayout.apply {
addView(appBarLayout)
addView(dummyContentView)
}
}

/**
* Triggers layout pass on dummy view hierarchy, taking into consideration selected
* ScreenStackHeaderConfig props that might have impact on final header height.
*
* @param fontSize font size value as passed from JS
* @return header height in dp as consumed by Yoga
*/
private fun computeDummyLayout(fontSize: Int): Float {
kkafar marked this conversation as resolved.
Show resolved Hide resolved
if (!::coordinatorLayout.isInitialized) {
Log.e(TAG, "[RNScreens] Attempt to access dummy view hierarchy before it is initialized")
return 0.0f
}

val topLevelDecorView = reactContext.get()!!.currentActivity!!.window.decorView

// These dimensions are not accurate, as they do include status bar & navigation bar, however
// it is ok for our purposes.
val decorViewWidth = topLevelDecorView.width
val decorViewHeight = topLevelDecorView.height

val widthMeasureSpec = MeasureSpec.makeMeasureSpec(decorViewWidth, MeasureSpec.EXACTLY)
val heightMeasureSpec = MeasureSpec.makeMeasureSpec(decorViewHeight, MeasureSpec.EXACTLY)

val textView = ScreenStackHeaderConfig.findTitleTextViewInToolbar(toolbar)
textView?.takeIf { fontSize != FONT_SIZE_UNSET }?.let { it.textSize = fontSize.toFloat() }

coordinatorLayout.measure(widthMeasureSpec, heightMeasureSpec)

// It seems that measure pass would be enough, however I'm not certain whether there are no
// scenarios when layout violates measured dimensions.
coordinatorLayout.layout(0, 0, decorViewWidth, decorViewHeight)

return PixelUtil.toDIPFromPixel(appBarLayout.height.toFloat())
}


companion object {
const val TAG = "RNScreensPackage"
const val LIBRARY_NAME = "react_codegen_rnscreens"

const val FONT_SIZE_UNSET = -1

// We access this field from C++ layer, through `getInstance` method.
// We don't care what instance we get access to as long as it has initialized
// dummy view hierarchy.
private var WEAK_THIS = WeakReference<RNScreensPackage>(null)

@JvmStatic
fun getInstance(): RNScreensPackage {
return WEAK_THIS.get()!!
}
}

}
51 changes: 36 additions & 15 deletions android/src/main/java/com/swmansion/rnscreens/Screen.kt
kkafar marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ class Screen(context: ReactContext?) : FabricEnabledViewGroup(context) {
val height = b - t

val headerHeight = calculateHeaderHeight()
val totalHeight = headerHeight.first + headerHeight.second // action bar height + status bar height
val totalHeight =
Copy link
Member

Choose a reason for hiding this comment

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

Are there any changes in this file? Please remove the formatting from the PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a comment there and I wouldn't mind if it has stayed. If you are strongly opinionated here I'm willing to open separate PR with that single line comment 😄

headerHeight.first + headerHeight.second // action bar height + status bar height
if (BuildConfig.IS_NEW_ARCHITECTURE_ENABLED) {
updateScreenSizeFabric(width, height, totalHeight)
} else {
Expand Down Expand Up @@ -171,7 +172,13 @@ class Screen(context: ReactContext?) : FabricEnabledViewGroup(context) {
ScreenWindowTraits.applyDidSetStatusBarAppearance()
}
field = statusBarStyle
fragmentWrapper?.let { ScreenWindowTraits.setStyle(this, it.tryGetActivity(), it.tryGetContext()) }
fragmentWrapper?.let {
ScreenWindowTraits.setStyle(
this,
it.tryGetActivity(),
it.tryGetContext()
)
}
}

var isStatusBarHidden: Boolean? = null
Expand Down Expand Up @@ -204,7 +211,13 @@ class Screen(context: ReactContext?) : FabricEnabledViewGroup(context) {
ScreenWindowTraits.applyDidSetStatusBarAppearance()
}
field = statusBarColor
fragmentWrapper?.let { ScreenWindowTraits.setColor(this, it.tryGetActivity(), it.tryGetContext()) }
fragmentWrapper?.let {
ScreenWindowTraits.setColor(
this,
it.tryGetActivity(),
it.tryGetContext()
)
}
}

var navigationBarColor: Int? = null
Expand All @@ -213,7 +226,12 @@ class Screen(context: ReactContext?) : FabricEnabledViewGroup(context) {
ScreenWindowTraits.applyDidSetNavigationBarAppearance()
}
field = navigationBarColor
fragmentWrapper?.let { ScreenWindowTraits.setNavigationBarColor(this, it.tryGetActivity()) }
fragmentWrapper?.let {
ScreenWindowTraits.setNavigationBarColor(
this,
it.tryGetActivity()
)
}
}

var isNavigationBarHidden: Boolean? = null
Expand All @@ -234,20 +252,23 @@ class Screen(context: ReactContext?) : FabricEnabledViewGroup(context) {

private fun calculateHeaderHeight(): Pair<Double, Double> {
val actionBarTv = TypedValue()
val resolvedActionBarSize = context.theme.resolveAttribute(android.R.attr.actionBarSize, actionBarTv, true)
val resolvedActionBarSize =
context.theme.resolveAttribute(android.R.attr.actionBarSize, actionBarTv, true)

// Check if it's possible to get an attribute from theme context and assign a value from it.
// Otherwise, the default value will be returned.
val actionBarHeight = TypedValue.complexToDimensionPixelSize(actionBarTv.data, resources.displayMetrics)
.takeIf { resolvedActionBarSize && headerConfig?.isHeaderHidden != true && headerConfig?.isHeaderTranslucent != true }
?.let { PixelUtil.toDIPFromPixel(it.toFloat()).toDouble() } ?: 0.0

val statusBarHeight = context.resources.getIdentifier("status_bar_height", "dimen", "android")
// Count only status bar when action bar is visible and status bar is not hidden
.takeIf { it > 0 && isStatusBarHidden != true && actionBarHeight > 0 }
?.let { (context.resources::getDimensionPixelSize)(it) }
?.let { PixelUtil.toDIPFromPixel(it.toFloat()).toDouble() }
?: 0.0
val actionBarHeight =
TypedValue.complexToDimensionPixelSize(actionBarTv.data, resources.displayMetrics)
.takeIf { resolvedActionBarSize && headerConfig?.isHeaderHidden != true && headerConfig?.isHeaderTranslucent != true }
?.let { PixelUtil.toDIPFromPixel(it.toFloat()).toDouble() } ?: 0.0

val statusBarHeight =
context.resources.getIdentifier("status_bar_height", "dimen", "android")
// Count only status bar when action bar is visible and status bar is not hidden
.takeIf { it > 0 && isStatusBarHidden != true && actionBarHeight > 0 }
?.let { (context.resources::getDimensionPixelSize)(it) }
?.let { PixelUtil.toDIPFromPixel(it.toFloat()).toDouble() }
?: 0.0

return actionBarHeight to statusBarHeight
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,6 @@ class ScreenStackFragment : ScreenFragment, ScreenStackFragmentWrapper {

override fun onAnimationRepeat(animation: Animation) {}
}

override fun startAnimation(animation: Animation) {
// For some reason View##onAnimationEnd doesn't get called for
// exit transitions so we explicitly attach animation listener.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ class ScreenStackHeaderConfig(context: Context) : ViewGroup(context) {
screenFragment?.setToolbar(toolbar)
}

if (isTopInsetEnabled) {
if (isTopInsetEnabled && false) {
kkafar marked this conversation as resolved.
Show resolved Hide resolved
headerTopInset.let {
toolbar.setPadding(0, it ?: 0, 0, 0)
}
Expand Down Expand Up @@ -210,7 +210,7 @@ class ScreenStackHeaderConfig(context: Context) : ViewGroup(context) {
toolbar.contentInsetStartWithNavigation = 0
}

val titleTextView = titleTextView
val titleTextView = findTitleTextViewInToolbar(toolbar)
if (titleColor != 0) {
toolbar.setTitleTextColor(titleColor)
}
Expand Down Expand Up @@ -309,19 +309,6 @@ class ScreenStackHeaderConfig(context: Context) : ViewGroup(context) {
maybeUpdate()
}

private val titleTextView: TextView?
get() {
for (i in 0 until toolbar.childCount) {
val view = toolbar.getChildAt(i)
if (view is TextView) {
if (view.text == toolbar.title) {
return view
}
}
}
return null
}

fun setTitle(title: String?) {
this.title = title
}
Expand Down Expand Up @@ -401,4 +388,18 @@ class ScreenStackHeaderConfig(context: Context) : ViewGroup(context) {
}
toolbar.clipChildren = false
}

companion object {
fun findTitleTextViewInToolbar(toolbar: Toolbar): TextView? {
for (i in 0 until toolbar.childCount) {
val view = toolbar.getChildAt(i)
if (view is TextView) {
if (view.text == toolbar.title) {
return view
}
}
}
return null
}
}
}
2 changes: 0 additions & 2 deletions android/src/main/jni/CMakeLists.txt
kkafar marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ add_compile_options(
-fexceptions
-frtti
-std=c++20
-Wall
-Wpedantic
-Wno-gnu-zero-variadic-macro-arguments
)

Expand Down
11 changes: 11 additions & 0 deletions android/src/main/jni/rnscreens.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,17 @@
*/
#include "rnscreens.h"

#include <iostream>

JNIEXPORT jint JNICALL JNI_OnLoad(JavaVM *vm, void *reserved) {
JNIEnv *env;
if (vm->GetEnv(reinterpret_cast<void **>(&env), JNI_VERSION_1_6) != JNI_OK) {
LOG(ERROR) << "[RNScreens] error while acquiring env";
return JNI_ERR;
}
return JNI_VERSION_1_6;
}

namespace facebook {
namespace react {

Expand Down
4 changes: 4 additions & 0 deletions android/src/main/jni/rnscreens.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@
#include <react/renderer/components/rnscreens/RNSScreenComponentDescriptor.h>
#include <react/renderer/components/rnscreens/RNSModalScreenComponentDescriptor.h>

#include <jni.h>

JNIEXPORT jint JNICALL JNI_OnLoad(JavaVM *vm, void *reserved);

namespace facebook {
namespace react {

Expand Down
1 change: 1 addition & 0 deletions apps/test-examples/src/App.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ import Test2048 from './Test2048';
import Test2069 from './Test2069';
import Test2118 from './Test2118';
import TestScreenAnimation from './TestScreenAnimation';
import TestHeader from './TestHeader';

enableFreeze(true);

Expand Down
Loading
Loading