Skip to content

Fix onPress event accidentally triggered while scrolling the page #997

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

Closed
wants to merge 1 commit into from

Conversation

huangzuizui
Copy link

The touchable components are likely to be miss triggered while scrolling the page. We can fix this issues by judging by touch velocity to make a distinction between press or scroll; I created a PR before #901 that soluction is not good enough. I think this one might be better.

The touchable components are likely to be miss triggered while scrolling the page. We can fix this issues by judging by touch velocity to make a distinction between press or scroll;
@necolas
Copy link
Owner

necolas commented Jun 14, 2018

As I asked last time, what is this for? Are you talking about body scrolling?

@huangzuizui
Copy link
Author

Sorry, I did not make it clear. Yes, it is about body scrolling. When we scroll the page in which filled with touchable components, It is likely to trigger the onPress event of the touchable components. Which is not intended to happen. We just want to scroll the page but got an onPress event triggered.

@necolas
Copy link
Owner

necolas commented Jun 28, 2018

Thanks but I don't think this is a good approach either. It's a hack, only applies to the Touchable, and it isn't releasing the responder. I think what is needed is a body scroller that interacts with the responder system as normal scroll views do.

@necolas necolas closed this Jun 28, 2018
@huangzuizui
Copy link
Author

huangzuizui commented Jun 29, 2018 via email

@necolas
Copy link
Owner

necolas commented Jun 29, 2018

The problem isn't the touchable, it's that the body scroller is not connected to the responder system like a ScrollView is

@hushicai
Copy link
Contributor

hushicai commented Aug 27, 2018

I think not the body scroller's problem, In my case, touchable sometims be pressed in a full-page ScrollView.

when touchable was pressed, the scrollview's onStartShouldSetResponderCapture is false, because the scroll event is fired so delay, and can not terminate the touchable in time.

maybe it is because the poor support of the momentum scroll event.

@hushicai
Copy link
Contributor

@huangzuizui Thanks for this. it worked for me.

@hushicai
Copy link
Contributor

I had merge this pr to my fork. hushicai@38530af

@hushicai
Copy link
Contributor

hushicai commented Aug 27, 2018

It will actually release the responder.

When the velocity is matched, we actually want to scroll the view, and after ScrollView is firing onscroll, the touchable responder should be cancelled.

when the velocity is not matched, touchable should be the active responder, and onPress fired.

@hushicai
Copy link
Contributor

hushicai commented Aug 27, 2018

I think this pr may also solve #731.

But if the scrollview is not scrolling then, touchable may keep the responder.

In my case, it seems nothing bad.

@necolas
Copy link
Owner

necolas commented Aug 30, 2018

Ok sounds like it's worth looking at this again at some point

@necolas necolas reopened this Aug 30, 2018
@hushicai
Copy link
Contributor

hushicai commented Sep 3, 2018

I have test on a real iphone.

when Touchable was pressed, the log :

image

Otherwise, the log:

image

the source code:

image

As we can see, the scrollResponderScrollShoultSetResponder sometimes can not terminate the Touchable responder in time when I quickly touch the screen to scroll, because onscroll is fired so delay after ontouchend.

@hushicai
Copy link
Contributor

hushicai commented Sep 3, 2018

i think we should extend rn's touchable state machine.

how about release the touchable responder when we move on the touchable HitRect? treat touchable as tappable?

in a raw react app, if we move on a clickable element, click will never be called...

@necolas
Copy link
Owner

necolas commented Nov 10, 2018

I think we could also look at triggering scroll events based on touch; that seems to be one way to fix #731 as well

@MoOx
Copy link
Contributor

MoOx commented Dec 21, 2018

I am facing this issue with tons of touchable on my body (scrollable). It’s almost impossible to scroll without triggering onPress of touchables as they are really big and take almost the entire screen (cards).
I decided to keep body scroll to keep some native safari features on iOS.
How can I help to move forward?

@necolas
Copy link
Owner

necolas commented Dec 22, 2018

How can I help to move forward?

You can look into hacks and/or whether we can integrate the body into the responder system.

When the velocity is matched, we actually want to scroll the view, and after ScrollView is firing onscroll, the touchable responder should be cancelled.

That may help when ScrollView is being used, but is it true if the body scrolls? I don't think so.

@RichardLindhout
Copy link
Contributor

@necolas What if we listen to a normal click event instead of onTouchStart etc.? That way the browser probably fix this issue itself.

@vladp
Copy link

vladp commented Aug 4, 2019

I am also struggling with figuring out how to solve this.
I am using react-bootstrap cards with body scroll.
And on an Android browser, when scrolling through the cards, it immediately invokes onPress.

I temporarily switched to onLongPress -- but test users are complaining that they think the app is not working, because they have to hold 'pressed' for too long.

If I switch back to onPress, they are rightly complaining that scrolling on Mobile browser (android) does not work


    return (
      <View
             style={{ marginTop: 40 }}
      >
        <TouchableHighlight
          activeOpacity={0.63} //change transparency on click
          underlayColor="#b9f9d4" 
             onPress /*onLongPress*/={() => this._onPressTopic(value)}
        >
          <View>
            <Card className="bg-dark" border="primary" text="danger"  >
              <Card.Img src={imageURL} alt={title} />
              <Card.ImgOverlay>
                <Card.Title>{title}</Card.Title>
                <Card.Text />
                <Card.Text />
              </Card.ImgOverlay>
            </Card>
          </View>
        </TouchableHighlight>
      </View>
    );
  }

this is related to an already ongoing discussion
#1219

@RichardLindhout
Copy link
Contributor

@vladp Probably the best workaround for that is this code: #1219 (comment)

@necolas necolas closed this Oct 4, 2019
@MoOx
Copy link
Contributor

MoOx commented Oct 5, 2019

@necolas has this issue been fixed somehow? Something coming from react itself directly?

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.

6 participants