nativescript-community / ui-collectionview

Allows you to easily add a collection view (grid list view) to your projects. Supports vertical and horizontal modes, templating, and more.
Apache License 2.0
58 stars 18 forks source link

Touch event handler on items not triggered on Android (when interpreted as vertical scroll) #29

Closed cjohn001 closed 3 years ago

cjohn001 commented 3 years ago

Hello together, I am trying to implement a kind of RadListView with items holding a menu which can be shown or hidden by swiping left and right. I have things working on IOS, but I cannot get the Android version of it working. The problem is, that on android the gestures need to be 100% horizontal in order for touch events to be passed through to nested views. Touch events interpreted as vertical scrolling are not passed through from the uicollection view to nested views. Is there a solution to this problem already? Maybe hooking up an additional touch event handler? I am not in Android development, hence if would be great if someone could provide a direction. Alternatively, may I ask for it as a feature request here? Thanks for your help!

Ok, looks like it is definitely a bug in the android RecyclerView. A fix is described here. I will try to provide a PR. Unfortunately, I am not sure yet in how far the fix is compatible with the reordering feature of the collectionview. I do not understand the code in this context completely. So if someone with more background in Android development could have a look this would likely be the preferred option.

https://bladecoder.medium.com/fixing-recyclerview-nested-scrolling-in-opposite-direction-f587be5c1a04

farfromrefug commented 3 years ago

@cjohn001 could you share an example. I am not sure i see where the issue really is. I very often use horizontal collectionview inside a vertical one and it works great. So not sure.

cjohn001 commented 3 years ago

@farfromrefug : Thanks for looking into it. Attached you find a demo. Please check the touch coordinates in the top box of the demo. On Android (best try also with real device) you can see, that the collectionview intercepts the touch events and that they do not bubble down to the child views. On IOS, they are correctly forwarded. This results in errors when, for example, using a nativescript swipe gesture handler. In the example, I tried handling touches myself in order to better understand and control whats going on. The problem with the issue is, that the user has to swipe perfectly in a horizontal direction in order for swipes being deteted. The medium article I referenced also describes the problem. Their solution is to check if a swipe gesture exists on the RecyclerView, and in case it is more horizontal than vertical, they pass the touch events to the children, instead of scrolling. This is the fix I mentioned.

Note: You can copy the attached demo directly to the other demos in the ui-collectionview sources.

demo-ng-swiping.zip

farfromrefug commented 3 years ago

@cjohn001 ok i am starting to understand. You should not do it with a touch event. I would advise you to use this https://github.com/nativescript-community/gesturehandler. you have a lot of options on the pan gesture https://github.com/nativescript-community/gesturehandler/blob/master/src/gesturehandler.d.ts#L89 which would allow you do do what you want. This is what i did on the drawer plugin https://github.com/nativescript-community/ui-drawer. And it is what i intended to do when i add time to do swipable cell plugin ;) (would do it almost exactly the same way as i did it with the drawer (except the behavior is simply reversed)

cjohn001 commented 3 years ago

@farfromrefug : Actually my first approach was to use a gesture handler. (swipe)="onSwipe($event)". They are not reliably detected! In the example I just detect a swipe and start an animation. Nevertheless, I connect to touch events, as I would like to be able to interactively let the user control the amount of opening (like in RadListView item swiping example) therefore I need the higher update frequency and touch coordinates. But as mentioned above, the problem is that a gesture handler also cannot work as expected, as the touch events are not received by the child view in case they are interpreted as vertical scroll from the ui-collectionview (they are not passed through to child views on Android). Do you understand the source of the problem now? I am currently trying to implement the referenced approach and would try to provide a PR

farfromrefug commented 3 years ago

@cjohn001 I would like you to try first with gesturehandler. I dont "trust" N event gesture recognition.

cjohn001 commented 3 years ago

@farfromrefug : Not sure why you are not trusting the gesture recognition in N. For me it worked well so far. If you look into the source code of the gesture handler you referenced:

function onGestureTouch(args: GestureTouchEventData) { const { state, extraData, view } = args.data; view.translateX = extraData.translationX; view.translateY = extraData.translationY; } gestureHandler.on(GestureHandlerTouchEvent, onGestureTouch, this); gestureHandler.on(GestureHandlerStateEvent, onGestureState, this); gestureHandler.attachToView(view);

You can see that it does exactely the same like the N gesture detectors and also the touch listener I provided in my demo. They all register on the touch events of the view on which they should detect gestures/touches. But again to the source of the problem: As touch events that the recycler view consumes for vertical scroll are not forwarded to its child views, neither your referenced touch handler nor the N or my touch listener can work correctly.

The article, I referenced, describes it very well. There is a bug in the Android RecyclerView which is used by the plugin. The Recycler View does not forward the intercepted touch events, it consumes for scrolling, to its child views. One can simply proof that this is the problem, by changing the touch listener. If the listener returns true instead of falls, the RecyclerView does not process the touch events and they are forwarded correctly to the child views, gesture handlers are then working fine as well. I tried this already. However, this of course breaks the RecyclerView navigation. The touch event propagation needs to treated by the onTouchListener of the current RecyclerView. The correct way would be to change the recycler view itself, but this is Android code. I have understood now how it works and were the problem is. The article is very good and also provides Kotlin source code which shows how to do it.

Here the code to show that the Recycler View is the problem:

public collectionViewLoaded(args: EventData) {
        if (global.isAndroid) {
            const collectionView = (args.object as View).android as androidx.recyclerview.widget.RecyclerView;
    collectionView.android.setOnTouchListener(
                new android.view.View.OnTouchListener({
                    onTouch: function(view, motionEvent) {
                        return true;
                    }
                })
cjohn001 commented 3 years ago

@farfromrefug: One question, before I start coding the fix. I have seen here:

https://github.com/nativescript-community/ui-collectionview/blob/c7e5ce9f5f6ef31beeffb87e771838dafd1db276/src/collectionview/collectionview.android.ts#L616

that the reordering feature possibly need to be considered for the fix, as it sets a custom touch listener. How can I activate this reordering?

Seems it is not enabled by default and I have not got it working in my example yet.

farfromrefug commented 3 years ago

@cjohn001 simply set reorderLongPressEnabled="true" on your collectionview.

farfromrefug commented 3 years ago

@cjohn001 i confirm what i said gesturehandler works as expected on collectionview listitems. I can have a pan gesture on the cells without any issue and any change on the recyclerview.

EDIT: attaching this on list item load gives you almost exactly what you should want:

import {
    GestureHandlerStateEvent,
    GestureHandlerTouchEvent,
    HandlerType,
    Manager,
    install as installGestures
} from '@nativescript-community/gesturehandler';
const manager = Manager.getInstance();
        const gestureHandler = manager.createGestureHandler(HandlerType.PAN, 16173, {
            activeOffsetXStart:-20,
            activeOffsetXEnd:20,
            failOffsetYStart:-10,
            failOffsetYEnd:10,
        });
        const view = e.object;
        gestureHandler.on(GestureHandlerTouchEvent, e=>{
            console.log('GestureHandlerTouchEvent', e.data.extraData);
        }, view);
        gestureHandler.on(GestureHandlerStateEvent, e=>{
            console.log('GestureHandlerStateEvent', e.data.state);
        }, view);
        gestureHandler.attachToView(view);
cjohn001 commented 3 years ago

@farfromrefug: Interesting plugin, I red about it a little more. Could you tell me for what the second parameter (16173) is good for? Could not find it in the docs.

manager.createGestureHandler(HandlerType.PAN, 16173,

However, I just provided a PR to resolve the issue. I think as the nativescript people would like to get rid of the proUI listview, it makes sense that this bug gets fixed. I am moreover very happy with the solution, because this does not force me to add additional plugins which replicate functionality that already exists in N. N gesture handlers on child views of ui-collection view now work as expected.

farfromrefug commented 3 years ago

It is not to be disappointing. You cant see only your usecase and think that code should be there for everyone. If we did that we would end with huge mess of something dead slow, buggy and impossible to maintain. This repo is a mono repo. So it publishes 2 plugins (as of now) with lerna. The sources are in src and the packages (the shell of the packages) are in packages (like most lerna repos). What you want is create a new plugin like the waterfall which allows you to "plug" your new behavior in the base collectionview plugin. Create a PR and i ll release it.

cjohn001 commented 3 years ago

Could you give me a hint, how I could plug this in? As mentioned, I tried to extend the UICollection view class, But in my angular app, that did not work. The documentation on how to do such things is unfortunately very thin and I have not a lot experience in plugin development.

farfromrefug commented 3 years ago

@cjohn001 well if you just say it did not work i cant really help you.Now as i said the easiest way for you is to look at how the waterfall plugin is done and try to go that way. I dont know what hint you are looking for.

cjohn001 commented 3 years ago

@farfromrefug Ok, we than stay here :)

https://github.com/nativescript-community/ui-collectionview/pull/31#issuecomment-891366438

I did a export myclass extends CollectionView

and I have read that I need to use registerElement like described here https://v6.docs.nativescript.org/angular/plugins/angular-third-party

But as mentioned that did not work. I do not understand how the waterfall stuff is working. Do you know about a description somewhere how this can be done? Or a plugin where I can check sources that does something close to what I need.

As a matter of principle, I assume I just need to extend the CollectionView class and overwrite initNativeView and disposeNativeView that call install and uninstall on my class from the bugfix and call the methods of the super class

cjohn001 commented 3 years ago

I will close the issue than, nothing to add here