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): pressable on Screen loses focus on pointer movement #2292

Merged
merged 22 commits into from
Aug 27, 2024

Conversation

kkafar
Copy link
Member

@kkafar kkafar commented Aug 8, 2024

Description

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 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:

statusBarHidden: true

statusBarTranslucent: true

statusBarTranslucent: false

Screenshot_1723212300

Screenshot_1723212331

Screenshot_1723212382

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, which was supposed to prevent content jumps, but doesn't work since RN 0.75 sadly.

Screenshot_1723220034

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

Changes

  • 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.

Test code and steps to reproduce

TestHeader, Test1975

Checklist

  • Included code example that can be used to test this change
  • Ensured that CI passes

@kkafar kkafar marked this pull request as draft August 8, 2024 09:59
@alduzy alduzy marked this pull request as ready for review August 22, 2024 10:07
@alduzy alduzy changed the title fix(Android,Fabric): WIP Pressable / Touchable press issues fix(Android,Fabric): pressable issues with native header Aug 22, 2024
@alduzy alduzy requested review from tboba and WoLewicki August 22, 2024 10:14
@alduzy alduzy force-pushed the @kkafar/fabric-pressables branch from 270134b to 867284c Compare August 22, 2024 10:20
@WoLewicki
Copy link
Member

I'd still wait for @kkafar to come back and elaborate why he wanted to leave the FrameOriginCorrection set.

## Description

This PR applies modifications to a previous fix:
#2169 for
fabric only, which has stopped working since RN `0.75`.

In RN `0.74` the `adopt` in `RNSScreenComponentDescriptior.h` was once
called without `stateData` but with children and we could then check if
the `ScreenStackHeaderConfig` is present among them and make adjustments
based on it.

When working on
#2292 it
became clear that the fix does not work anymore. Now the `adopt` is
called either with no children and no `stateData` or with both.
The solution is to move the code to `appendChild` in
`RNSScreenShadowNode.cpp` so we can perform the adjustments as soon as
the children append.

## Changes

- moved code from `adopt` in `RNSScreenComponentDescriptior.h` to newly
added `appendChild` override in `RNSScreenShadowNode.cpp`

## Screenshots / GIFs

### Before


https://github.com/user-attachments/assets/6b76864b-58bb-4c6e-9f5b-a01bb0c88d2a

### After


https://github.com/user-attachments/assets/98931e77-3877-4f67-8b28-f49d2e0f42ff


## Test code and steps to reproduce

- Use `TestHeader` to test this change

## Checklist

- [ ] Included code example that can be used to test this change
- [ ] Updated TS types
- [ ] Updated documentation: <!-- For adding new props to native-stack
-->
- [ ]
https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md
- [ ]
https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md
- [ ]
https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx
- [ ]
https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx
- [ ] Ensured that CI passes

---------

Co-authored-by: Kacper Kafara <[email protected]>
@kkafar
Copy link
Member Author

kkafar commented Aug 27, 2024

@WoLewicki I've left FrameOriginCorrection set after receiving initial update from JVM side, "to ensure correct values in layout metrics of shadownode" <- without this, the Screen's origin position would be (0, 0) - which would not take the header into account, however this was not used by our code anywhere.

@kkafar kkafar changed the title fix(Android,Fabric): pressable issues with native header fix(Android,Fabric): pressable on Screen loses focus on pointer movement Aug 27, 2024
@WoLewicki
Copy link
Member

So it was only for libs like reanimated to correctly read those values from layoutMetrics?

Copy link
Member Author

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

Okay, this fixes the issue with pressables on screen (but not in header), thus I'm for merging it.

The issue with pressables in header will be handled separately.

@kkafar
Copy link
Member Author

kkafar commented Aug 27, 2024

@WoLewicki Exactly, but right now we should prioritise making it work & defer integration with reanimated.

@kkafar kkafar merged commit 34c1ba8 into main Aug 27, 2024
8 checks passed
@kkafar kkafar deleted the @kkafar/fabric-pressables branch August 27, 2024 12:31
@hosseinmd
Copy link

hosseinmd commented Aug 31, 2024

Please release these changes.

@Kaveh-ap
Copy link

Kaveh-ap commented Sep 1, 2024

I'm facing the same issue!
Are there any plans to release these changes?

ja1ns pushed a commit to WiseOwlTech/react-native-screens that referenced this pull request Oct 9, 2024
…ement (software-mansion#2292)

## Description

> [!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](software-mansion#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 software-mansion#1975 

## Changes

- 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`.


## Test code and steps to reproduce

`TestHeader`, `Test1975`

## Checklist

- [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]>
@WKampel
Copy link

WKampel commented Oct 12, 2024

The onPress event does not work for me (Android), but only in a specific situation: Inside a scrollview inside a drawer's content, AND scrolled down more than a few pixels. But onPressIn does work. I'm assuming this has the same root cause.

And I'm on the latest beta version (4.0.0-beta.9) so I'm guessing this won't be fixed anytime soon.

@kkafar
Copy link
Member Author

kkafar commented Oct 14, 2024

@WKampel, if you mind providing a reproduction we might be quicker to handle this, thanks!

kkafar added a commit that referenced this pull request Oct 25, 2024
…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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Taps often lost on Android real device + Fabric with header enabled
6 participants