Skip to content

Commit

Permalink
fix(Android,Fabric): pressable on Screen loses focus on pointer mov…
Browse files Browse the repository at this point in the history
…ement (#2292)

> [!important]
This PR aims to fix only pressables on screen components. This PR does
not fix similar pressable issue with pressables in native header. That
interaction will be fixed separately.

Pressable elements work just fine until there's a gesture involved. On
sensitive physical devices even a little movement during the press is
treated as a gesture.

When the `Pressable` element detects a gesture it calls
[onResponderMove](https://github.com/facebook/react-native/blob/82795715aefba07ae9d79278ce3fd4d2e9a928f2/packages/react-native/Libraries/Pressability/Pressability.js#L484)
which then checks wether the gesture happened within the element or went
outside by comparing the touch coordinates with coordinates of the
element using `_isTouchWithinResponderRegion`.

The `responderRegion` is obtained from `_responderID` and happens to
have unexpected values when the native header is present. It tuns out
that the Y origin is slightly off. After some further investigation and
comparison of coordinates it turned out that the height of the android
status bar is not well calculated in various scenarios:

<table>
<td>

`statusBarHidden: true`

</td>
<td>

`statusBarTranslucent: true`

</td>
<td>

`statusBarTranslucent: false`

</td>
</tr>
<tr>
<td>

![Screenshot_1723212300](https://github.com/user-attachments/assets/57e2f4a3-b002-4ca3-9519-45cfece860c4)

</td>
<td>

![Screenshot_1723212331](https://github.com/user-attachments/assets/bd46c8d1-8813-4fae-a8a9-0326193371d2)

</td>
<td>

![Screenshot_1723212382](https://github.com/user-attachments/assets/c7373437-524d-4a0f-951e-ce2689a4fe5c)

</td>
</tr>
</table>

The `calculateHeaderHeight` used for calculating the header and
statusBar height seems to be the problem. Luckily, we don't have to
calculate it by ourselves anymore, because the correct `t` value is
provided in the `onLayout` function of the `Screen`. Thus we can get rid
of the custom function.

Another issue found: after navigating to another screen the offset is
off again (exactly by 2x). It's caused by changes introduced in [this
PR](#2169),
which was supposed to prevent content jumps, but doesn't work since RN
`0.75` sadly.

![Screenshot_1723220034](https://github.com/user-attachments/assets/b0908c23-4667-4ccf-8e5e-5e7e11bca316)

I found out that `FrameOriginCorrection` is not being unset when
dimensions from JVM are received, while the `FrameHeightCorrection` is.
After adding the missing unset for `FrameOriginCorrection` I rolled back
to the commit with the mentioned PR merged and RN `0.74` and I can
confirm it works.

Fixes #1975

- removed `calculateHeaderHeight` function
- added unset for `FrameOriginCorrection` when dimensions from JVM are
received
- added `Test1975.tsx` repro
- moved code responsible for determining header height during the very
first render from component descriptor's `adopt` method to shadow node
`appendChild`.

`TestHeader`, `Test1975`

- [x] Included code example that can be used to test this change
- [x] Ensured that CI passes

---------

Co-authored-by: alduzy <[email protected]>
Co-authored-by: Alex Duży <[email protected]>
(cherry picked from commit 34c1ba8)
  • Loading branch information
kkafar committed Oct 25, 2024
1 parent eac554e commit 5a3b31a
Show file tree
Hide file tree
Showing 9 changed files with 228 additions and 138 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ abstract class FabricEnabledViewGroup(
protected fun updateScreenSizeFabric(
width: Int,
height: Int,
headerHeight: Double,
headerHeight: Int,
) {
updateState(width, height, headerHeight)
}
Expand All @@ -33,10 +33,11 @@ abstract class FabricEnabledViewGroup(
fun updateState(
width: Int,
height: Int,
headerHeight: Double,
headerHeight: Int,
) {
val realWidth: Float = PixelUtil.toDIPFromPixel(width.toFloat())
val realHeight: Float = PixelUtil.toDIPFromPixel(height.toFloat())
val realHeaderHeight: Float = PixelUtil.toDIPFromPixel(headerHeight.toFloat())

// Check incoming state values. If they're already the correct value, return early to prevent
// infinite UpdateState/SetState loop.
Expand All @@ -54,7 +55,7 @@ abstract class FabricEnabledViewGroup(
putDouble("frameWidth", realWidth.toDouble())
putDouble("frameHeight", realHeight.toDouble())
putDouble("contentOffsetX", 0.0)
putDouble("contentOffsetY", headerHeight)
putDouble("contentOffsetY", realHeaderHeight.toDouble())
}
mStateWrapper?.updateState(map)
}
Expand Down
36 changes: 3 additions & 33 deletions android/src/main/java/com/swmansion/rnscreens/Screen.kt
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import android.content.pm.ActivityInfo
import android.graphics.Paint
import android.os.Parcelable
import android.util.SparseArray
import android.util.TypedValue
import android.view.View
import android.view.ViewGroup
import android.view.WindowManager
Expand All @@ -14,7 +13,6 @@ import androidx.core.view.children
import androidx.fragment.app.Fragment
import com.facebook.react.bridge.GuardedRunnable
import com.facebook.react.bridge.ReactContext
import com.facebook.react.uimanager.PixelUtil
import com.facebook.react.uimanager.UIManagerHelper
import com.facebook.react.uimanager.UIManagerModule
import com.facebook.react.views.scroll.ReactScrollView
Expand Down Expand Up @@ -76,16 +74,13 @@ class Screen(
val width = r - l
val height = b - t

val headerHeight = calculateHeaderHeight()
val totalHeight =
headerHeight.first + headerHeight.second // action bar height + status bar height
if (BuildConfig.IS_NEW_ARCHITECTURE_ENABLED) {
updateScreenSizeFabric(width, height, totalHeight)
updateScreenSizeFabric(width, height, t)
} else {
updateScreenSizePaper(width, height)
}

notifyHeaderHeightChange(totalHeight)
notifyHeaderHeightChange(t)
}
}

Expand Down Expand Up @@ -328,32 +323,7 @@ class Screen(
}
}

private fun calculateHeaderHeight(): Pair<Double, Double> {
val actionBarTv = TypedValue()
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

return actionBarHeight to statusBarHeight
}

private fun notifyHeaderHeightChange(headerHeight: Double) {
private fun notifyHeaderHeightChange(headerHeight: Int) {
val screenContext = context as ReactContext
val surfaceId = UIManagerHelper.getSurfaceId(screenContext)
UIManagerHelper
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,16 @@ import com.facebook.react.uimanager.events.Event
class HeaderHeightChangeEvent(
surfaceId: Int,
viewId: Int,
private val headerHeight: Double,
private val headerHeight: Int,
) : Event<HeaderHeightChangeEvent>(surfaceId, viewId) {
override fun getEventName() = EVENT_NAME

// As the same header height could appear twice, use header height as a coalescing key.
override fun getCoalescingKey(): Short = headerHeight.toInt().toShort()
override fun getCoalescingKey(): Short = headerHeight.toShort()

override fun getEventData(): WritableMap? =
Arguments.createMap().apply {
putDouble("headerHeight", headerHeight)
putDouble("headerHeight", headerHeight.toDouble())
}

companion object {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ abstract class FabricEnabledViewGroup(
protected fun updateScreenSizeFabric(
width: Int,
height: Int,
headerHeight: Double,
headerHeight: Int,
) {
// do nothing
}
Expand Down
109 changes: 109 additions & 0 deletions apps/src/tests/Test1975.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
import * as React from 'react';
import { NavigationContainer } from '@react-navigation/native';
import { View, Text, StyleSheet, Pressable } from 'react-native';
import { createNativeStackNavigator } from '@react-navigation/native-stack';

const Stack = createNativeStackNavigator();

function App() {
const [count, setCount] = React.useState(0);

return (
<NavigationContainer>
<Stack.Navigator screenOptions={{ statusBarTranslucent: false }}>
<Stack.Screen
name="Screen"
component={Screen}
options={{
headerRight: () => (
<Pressable
onPress={() => setCount(prev => prev + 1)}
style={({ pressed }) => [
styles.pressable,
pressed && { backgroundColor: 'goldenrod' },
]}>
<Text>Press (+)</Text>
</Pressable>
),
title: count.toString(),
headerTitleAlign: 'center',
}}
/>
<Stack.Screen
name="Details"
component={DetailsScreen}
options={({ navigation }) => ({
title: 'Details',
headerRight: () => (
<Pressable
onPress={navigation.goBack}
style={({ pressed }) => [
styles.pressable,
pressed && { backgroundColor: 'goldenrod' },
]}>
<Text>Go Back</Text>
</Pressable>
),
})}
/>
</Stack.Navigator>
</NavigationContainer>
);
}

function Screen({ navigation }: any) {
return (
<View style={styles.container}>
<Pressable
onPress={() => navigation.navigate('Details')}
style={({ pressed }) => [
styles.pressable,
pressed && { backgroundColor: 'goldenrod' },
]}>
<Text>Go to Details</Text>
</Pressable>
</View>
);
}

function DetailsScreen() {
let counter = React.useRef(0);

return (
<View style={{ ...styles.container, backgroundColor: 'beige' }}>
<Pressable
onPressIn={() => {
counter.current += 1;
console.log(`[${counter.current}] Details: onPressIn`);
}}
onPress={() => {
console.log(`[${counter.current}] Details: onPress`);
}}
onPressOut={() => {
console.log(`[${counter.current}] Details: onPressOut`);
}}
style={({ pressed }) => [
styles.pressable,
pressed && { backgroundColor: 'goldenrod' },
]}>
<Text>Press me</Text>
</Pressable>
</View>
);
}

const styles = StyleSheet.create({
pressable: {
paddingVertical: 5,
paddingHorizontal: 10,
backgroundColor: 'red',
},
container: {
flex: 1,
alignItems: 'center',
justifyContent: 'center',
gap: 24,
},
});

export default App;
1 change: 1 addition & 0 deletions apps/src/tests/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ export { default as Test1829 } from './Test1829';
export { default as Test1844 } from './Test1844';
export { default as Test1864 } from './Test1864';
export { default as Test1970 } from './Test1970';
export { default as Test1975 } from './Test1975';
export { default as Test1981 } from './Test1981';
export { default as Test2002 } from './Test2002';
export { default as Test2008 } from './Test2008';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,6 @@ class RNSScreenComponentDescriptor final
public:
using ConcreteComponentDescriptor::ConcreteComponentDescriptor;

static constexpr const char *kScreenDummyLayoutHelperClass =
"com/swmansion/rnscreens/utils/ScreenDummyLayoutHelper";

void adopt(ShadowNode &shadowNode) const override {
react_native_assert(dynamic_cast<RNSScreenShadowNode *>(&shadowNode));
auto &screenShadowNode = static_cast<RNSScreenShadowNode &>(shadowNode);
Expand All @@ -39,8 +36,8 @@ class RNSScreenComponentDescriptor final
#ifdef ANDROID
if (stateData.frameSize.width != 0 && stateData.frameSize.height != 0) {
// When we receive dimensions from JVM side we can remove padding used for
// correction, and we can stop applying height correction for the frame.
// We want to leave top offset correction though intact.
// correction, and we can stop applying height and offset corrections for
// the frame.
// TODO: In future, when we have dynamic header height we might want to
// update Y offset correction here.

Expand All @@ -67,36 +64,11 @@ class RNSScreenComponentDescriptor final
screenShadowNode.setPadding({0, 0, 0, 0});
screenShadowNode.getFrameCorrectionModes().unset(
FrameCorrectionModes::Mode::FrameHeightCorrection);
screenShadowNode.getFrameCorrectionModes().unset(
FrameCorrectionModes::Mode::FrameOriginCorrection);

layoutableShadowNode.setSize(
Size{stateData.frameSize.width, stateData.frameSize.height});
} else {
// This code path should be executed only on the very first (few)
// layout(s), when we haven't received state update from JVM side yet.

auto headerConfigChildOpt = findHeaderConfigChild(layoutableShadowNode);

// During creation of the shadow node children are not attached yet.
// We also do not want to set any padding in case.
if (headerConfigChildOpt) {
const auto &headerConfigChild = headerConfigChildOpt->get();
const auto &headerProps =
*std::static_pointer_cast<const RNSScreenStackHeaderConfigProps>(
headerConfigChild->getProps());

const auto headerHeight = headerProps.hidden
? 0.f
: findHeaderHeight(
headerProps.titleFontSize, headerProps.title.empty())
.value_or(0.f);

screenShadowNode.setPadding({0, 0, 0, headerHeight});
screenShadowNode.setHeaderHeight(headerHeight);
screenShadowNode.getFrameCorrectionModes().set(
FrameCorrectionModes::Mode(
FrameCorrectionModes::Mode::FrameHeightCorrection |
FrameCorrectionModes::Mode::FrameOriginCorrection));
}
}
#else
if (stateData.frameSize.width != 0 && stateData.frameSize.height != 0) {
Expand All @@ -106,72 +78,6 @@ class RNSScreenComponentDescriptor final
#endif // ANDROID
ConcreteComponentDescriptor::adopt(shadowNode);
}

std::optional<std::reference_wrapper<const ShadowNode::Shared>>
findHeaderConfigChild(
const YogaLayoutableShadowNode &screenShadowNode) const {
for (const ShadowNode::Shared &child : screenShadowNode.getChildren()) {
if (std::strcmp(
child->getComponentName(), "RNSScreenStackHeaderConfig") == 0) {
return {std::cref(child)};
}
}
return {};
}

#ifdef ANDROID
std::optional<float> findHeaderHeight(
const int fontSize,
const bool isTitleEmpty) const {
JNIEnv *env = facebook::jni::Environment::current();

if (env == nullptr) {
LOG(ERROR) << "[RNScreens] Failed to retrieve env\n";
return {};
}

jclass layoutHelperClass = env->FindClass(kScreenDummyLayoutHelperClass);

if (layoutHelperClass == nullptr) {
LOG(ERROR) << "[RNScreens] Failed to find class with id "
<< kScreenDummyLayoutHelperClass;
return {};
}

jmethodID computeDummyLayoutID =
env->GetMethodID(layoutHelperClass, "computeDummyLayout", "(IZ)F");

if (computeDummyLayoutID == nullptr) {
LOG(ERROR)
<< "[RNScreens] Failed to retrieve computeDummyLayout method ID";
return {};
}

jmethodID getInstanceMethodID = env->GetStaticMethodID(
layoutHelperClass,
"getInstance",
"()Lcom/swmansion/rnscreens/utils/ScreenDummyLayoutHelper;");

if (getInstanceMethodID == nullptr) {
LOG(ERROR) << "[RNScreens] Failed to retrieve getInstanceMethodID";
return {};
}

jobject packageInstance =
env->CallStaticObjectMethod(layoutHelperClass, getInstanceMethodID);

if (packageInstance == nullptr) {
LOG(ERROR)
<< "[RNScreens] Failed to retrieve packageInstance or the package instance was null on JVM side";
return {};
}

jfloat headerHeight = env->CallFloatMethod(
packageInstance, computeDummyLayoutID, fontSize, isTitleEmpty);

return {headerHeight};
}
#endif // ANDROID
};

} // namespace react
Expand Down
Loading

0 comments on commit 5a3b31a

Please sign in to comment.