software-mansion / react-native-gesture-handler

Declarative API exposing platform native touch and gesture system to React Native.
https://docs.swmansion.com/react-native-gesture-handler/
MIT License
6.14k stars 982 forks source link

Create gesturized pressable component #2942

Closed latekvo closed 4 months ago

latekvo commented 5 months ago

This PR adds our own Pressable component

Closes https://github.com/software-mansion/react-native-gesture-handler/issues/1221

latekvo commented 5 months ago

Original onBlur and onFocus: Pressable defaults to blurred state. When in blurred state, the first click on the pressable calls onFocus. When in focused state, when the user clicks outside of Pressable, onBlur is called and blurred state is set. Original onBlur and onFocus is only implemented on Web, and doesn't work on Android

latekvo commented 5 months ago

touchWithinBounds is still up for further investigation and i think i'll remove it all together in favour of dynamically changing hitSlop, if that'll be possible. onFocus and onBlur still have to be implemented, it may be tricky as the pointer would have to be tracked outside of Pressable, it's likely I'll have to remove it all together, which may not be a big deal, as previously it was a web specific feature anyways.

Other Pressable features seem to all be implemented, but I'll be testing their alignment with the original some more.

What's up for discussion is compatibility with legacy events. None of them are possible to fully recreate within rngh, and they weren't too consistent across web / mobile, but they could be partially imitated to better reflect those returned by the original callbacks.

latekvo commented 5 months ago

Pressable's onPress supposedly returns GestureResponderEvent, which looks something like this: image

And yet, when you actually inspect that event, it looks like this: image

latekvo commented 5 months ago

Last blocker: setting hitSlop doesn't have any effect on the pressable area.

latekvo commented 5 months ago

~~Could someone please confirm there is a crash on android with the standalone new_api example? It appears fontWeight style is causing it, but I still haven't ruled out that my particular android installation is somehow corrupted, which happened to me before :P~~

Edit: the android installation was corrupted 👍 + other issues

latekvo commented 5 months ago

State of things

latekvo commented 5 months ago

I see no further issues, and all bahaviours align with the original. I'll work on further testing, reduction of code, and polishing the adapted event, but as a whole, it's ready for review.

latekvo commented 5 months ago

for the web, we cannot use RNButton, and have to replace it with RectButton instead, but that doesn't infer on any functionality as far as I see.

latekvo commented 5 months ago

I am aware that the tests have strange styling on web, but I will take care of that after I finish writing them.

latekvo commented 5 months ago
Screenshot 2024-06-21 at 15 09 16

This is up for investigation, first of all, kotlin/native side interface doesn't align with the react typing, secondly the number I am providing doesn't seem to align with any kind of rgb standard, and is likely something else. Converting from hex 0xRRGGBB to int won't get the desired result, neither will 0xRGB. The random number that is present there is just a one that works at all, to showcase what is happening.

latekvo commented 5 months ago

I still need to fix styling for examples on web, and fix pressDelay on web, it's not being cancelled on touch up.

latekvo commented 5 months ago

For targetId, we could use a hidden variable _nativeTag located on View, which in our case is pressableRef. _nativeTag is not publicly exposed but could be used. It does seem to yield the correct target id, but it doesn't work on web and changes on every render. I'm not sure if it's an approach we want to be using.

latekvo commented 5 months ago

Regarding the border example, do we want to nest another View inside the RNButton to allow for some broken styles to be more aligned with the original? (c.c. @j-piasecki )

Screenshot 2024-06-26 at 09 31 41 Screenshot 2024-06-26 at 09 32 52
j-piasecki commented 5 months ago

Regarding the border example, do we want to nest another View inside the RNButton to allow for some broken styles to be more aligned with the original? (c.c. @j-piasecki )

Possibly, though It's something I'd like to avoid, given it's similar to Touchables from GH and there were many issues regarding that.

j-piasecki commented 5 months ago

I think we can try the approach with an additional view as a parent of the button. You would probably style the view and simply pass StyleSheet.absoluteFill as the style to the button. You may need to split the other props in some wacky way.

latekvo commented 5 months ago

I think we can try the approach with an additional view as a parent of the button. You would probably style the view and simply pass StyleSheet.absoluteFill as the style to the button. You may need to split the other props in some wacky way.

Hey just to clarify, in the attached screenshot, View was the inner most child, and it inherited all the styles, removing them from the button entirely. Everything else adjusted perfectly automatically.

Is there a reason why you'd prefer the View to be the topmost parent, or is it of no importance?

For now I'll commit the component with the additional View as it's child, but please let me know if this is what you wanted.

edit: Having the View as the inner child breaks ripple :P Will try around a bit more, but I think it's likely Your approach works better.

latekvo commented 5 months ago

Even if borderStyle causes the issue in Pressable (and I've mentioned it in bordersExample.tsx), I don't understand why do we need this comment here. In the separator styles work fine (though they look different when comparing platforms)

This comment refers to borderWidth, which is set to an arbitrary number. IOS does allow for dotted/slashed borders, just refuses to render the entire object, when any 1-side styles are set to any of the sides of the border, which is also dotted/slashed on any of the sides.

Because of this, we have to set the border width to a really small number, so that all the borders are squashed into a single line, while also making them thick enough, so that the slashed effect does not look like little dots, as it's look depends on border's width.

m-bert commented 5 months ago

Okay, after I've stumbled upon

The key here is borderStyle, it's the only thing causing issue on IOS.

I assumed that the problem was just borderStyle and borderWidth has nothing to do with it.

latekvo commented 4 months ago

When delay hover in/out is set, shouldn't it be canceled when moving outside of the view?

That's a good point. I have no idea how hover delay behaves on the legacy component, but I think that suggestion makes a bit more sense than the current implementation. I'll do that, shouldn't be a problem at all.

m-bert commented 4 months ago

When delay hover in/out is set, shouldn't it be canceled when moving outside of the view?

I think that if you hover on Pressable that has hoverDelayIn and then you move outside it before callback will be triggered, then it shouldn't be called at all (i.e. it should be cancelled).

latekvo commented 4 months ago

hover with delay tests:

https://github.com/software-mansion/react-native-gesture-handler/assets/74246391/da3c6f5a-9fdf-46d8-b79f-9f5b0dfbe5fa

m-bert commented 4 months ago

I've found 2 more problems:

Functional styling (all platforms)

If you drag out of Pressable and return to it, next press doesn't work

https://github.com/software-mansion/react-native-gesture-handler/assets/63123542/16fe1fb3-05fb-4902-84e3-a28d1aec11ae

Delay on press (web only)

Sometimes press stops working

https://github.com/software-mansion/react-native-gesture-handler/assets/63123542/b91046c1-bbef-4e2f-852c-7ec57466ad5f

Conclusion

I think we've done a lot in this PR and further issues may be addressed in following PRs. What's your stance on that, @j-piasecki?

m-bert commented 4 months ago

Also I've noticed that if you use stylus and you click on "delay hover" example, when you lift stylus you will get Hover out with delay registered, without hover in being logged.

And last thing for now, we still have problems with borders on iOS:

image

(...) I fixed this error since, so I think I can remove the borders example all together, as all the styles are now aligned with the legacy pressable.

Please, either remove the example or change borders to solid

latekvo commented 4 months ago

Functional styling (all platforms)

fixed, it was an issue with touch cancelling

Delay on press (web only)

couldn't replicate it, could you tell me which browser you're using?

Problems with borders on iOS

removed them

(c.c. @m-bert )

latekvo commented 4 months ago

Regarding the hover in/out bug on web browser with stylus, I think what you described is normal behaviour.

When just clicking (press in immediately followed by press out), well then before hoverIn's timer finishes (press in), hoverOut's is started (press out), thus only the latter will be displayed.

If you were to hold down the hover element, you'd also see Hovered in with delay.

This is in line with what you requested yesterday:

image

I hope this explains the issue you've found.

m-bert commented 4 months ago

couldn't replicate it, could you tell me which browser you're using?

I'm using Google Chrome (126.0.6478.63).

I hope this explains the issue you've found.

Makes sense 😅

m-bert commented 4 months ago

I think this is yet another bug 😅 But as I said earlier, I'm not sure if we want to fix it in this PR

https://github.com/software-mansion/react-native-gesture-handler/assets/63123542/e6bfbf42-6558-467b-a050-39e2627ef3cd

j-piasecki commented 4 months ago

I think we've done a lot in this PR and further issues may be addressed in the following PRs.

We can do that. Could you open issues for the problems you've found in that case?

m-bert commented 4 months ago

Could you open issues for the problems you've found in that case?

I will.