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

Touch event is canceled on small move because element height is 0 #157

Open
gorangajic opened this Issue Oct 11, 2016 · 24 comments

Comments

Projects
None yet
@gorangajic
Contributor

gorangajic commented Oct 11, 2016

On galaxy s6 touch is so sensitive that when you tap on the phone sometimes move event is fired. And because height in TouchableMixin._handleQueryLayout is 0 so because of that isTouchWithinActive will always be false and touch event will be canceled.

Now I am not sure how to fix because I am not sure how react-native is getting that value from the native component. Any help is appreciated

@chengyin

This comment has been minimized.

Contributor

chengyin commented Oct 24, 2016

Also getting the same conclusion here.

The Bug

Summary: All move related touch behavior functions incorrectly with react-native-svg.

I created a demo project to demo this:
https://github.com/chengyin/react-native-svg-press-bug

Here is a GIF:

You can see that in this sequence:

  1. Touch down
  2. Move within the Rect
  3. Touch up

The expected behavior is onPress event is filed. Unfortunately for react-native-svg, this is not happening.

Technical Details

This line in RN's Touchable determines whether the touch moved out of the view boundary or not. If the touch moved out of the boundary, onPressOut will be fired immediately. It also leads to that when the touch releases, onPress would not be fired.

To perform this calculate, Touchable mixin saves the size and position of the element it is attached to to compare them to the touch event position. The measurement is done through UIManager on the native side using frame (for iOS). But this measurement is wrong for react-native-svg components because they are UIView subclass with zero frames. This wrong measurement makes Touchable treat almost all touch moves as if the they are out of the component touch area.

This is particularly bad for 3D Touch enabled devices. When combining a Force Touch device (6s, 6s Plus, 7, 7 Plus) with an app that targets 9.0 and below.

Due to an iOS SDK 9.0 bug, Force change triggers touchesMoved event call back (although the touch actually didn't move), which leads to the touchableHandleResponderMove being called, and cancels out the press itself. Essentially, onPresss on react-native-svg components do not work at all on 6s, 6s Plus, 7, and 7 Plus. When the tap is very fast and light, it would work, but this is an unpractical expectation. This is reproducible in the Simulator with a 3D Touch enabled trackpad.

Suggestions

I would like to hear @magicismight's input on this, some options are:

  1. Fix frame on native SVG components. I am afraid that this bug here could lead to other unknown bugs.
  2. Override Touchable's handlers to accommodate this limitation, either by cripple the touch support, or through offsetting the measurements from the native side to make them accurate.

I'm more than happy to help here with the code.

@rgoldiez

This comment has been minimized.

rgoldiez commented Nov 8, 2016

Any update on this?

@chengyin

This comment has been minimized.

Contributor

chengyin commented Nov 8, 2016

Not that I know of. One workaround, which cripple's touch moved event, but makes onPress work is to disable the move responder:

onPress: [[your press handler]]
onResponderMove: function() { return; }
@maxg7

This comment has been minimized.

maxg7 commented Nov 15, 2016

Similar to the above comment, if you only care about the onPress event not touchMove events, you can explicitly use the onPressIn (or onPressOut). In testing it appeared to be slightly more responsive than having the onPanResponder just return.

@chengyin

This comment has been minimized.

Contributor

chengyin commented Nov 15, 2016

If you use the svg in a scroll view, it is very easy to trigger the touch out event if the user starts scrolling from the svg. So be extra careful with it.

@danielduner

This comment has been minimized.

danielduner commented Dec 2, 2016

Any update on this?

The reason I'm using svg is to get non-rectangular buttons, specifically in a scroll view.

left-right-buttons

Once one svg element has failed to register an onPress event, then no other svg element will register presses until the scroll view is reloaded.

The workaround of setting onResponderMove to NOPs on the SVG elements doesn't seem to work, maybe because of how the scroll view works? Is there any other workaround I can attempt?

@chengyin

This comment has been minimized.

Contributor

chengyin commented Dec 2, 2016

@danielduner

This comment has been minimized.

danielduner commented Dec 2, 2016

Just to be clear, I already have this working:

  • drawing the svg polygons
  • correctly registering presses on the individual polygons
  • changing the polygon opacity individually in response to presses

It works perfectly fine most of the time. But because of (presumably) this bug, the svg elements completely stop responding sometimes. Definitely after a pressIn -> tiny move -> pressOut, though maybe also at other times (it's difficult to tell)

So if this bug was fixed that would solve my problem, right?

@jhilden

This comment has been minimized.

jhilden commented Apr 20, 2017

Has there been any progress or new insights on this issue?

We are currently also facing the issue. We are working around it by using the onPressIn event instead of the onPress (as suggested by @maxg7 above). This works okish for simple use cases, but as soon as you have SVG inside an ScrollView it becomes problematic. Then touch/scroll gestures that start on an SVG shape will always trigger a press event, even though this is not what we want.

@GeeWee

This comment has been minimized.

GeeWee commented Jun 2, 2017

I also have this issue through victory-native which uses this library. Is anyone working on it? Seems reasonably breaking.

@18601673727

This comment has been minimized.

18601673727 commented Aug 31, 2017

My situation is like @GeeWee, this could be a critical problem.

@macrozone

This comment has been minimized.

macrozone commented Sep 1, 2017

I have also the same problem. onPressIn and onPressOut does not work, because I use svg inside a panresponder.

I probably need to do the hit-detection myself, as it seems to be broken on android

@macrozone

This comment has been minimized.

macrozone commented Sep 2, 2017

possible workaround: instead of the viewboundary, we could check the speed or distance of the touches like in a panresponder and tolerate small moves. This would fix the problem in many cases.

@macrozone

This comment has been minimized.

macrozone commented Sep 2, 2017

i debugged it a little and i am very confused now:

If i touch down and move the finger, onPressOut is called immediatly. But contrary to what @chengyin said, touchableHandleResponderMove is not called then and isTouchWithinActive is not called. That's why you can't even manipulate the hitbox with pressRetentionOffset or hitSlop.

Instead touchableHandleResponderTerminate is called immediatly. I could not find out yet, why this happens.

Maybe it happens when the svg is inside a panresponder or so.

@chengyin

This comment has been minimized.

Contributor

chengyin commented Sep 2, 2017

Hey @macrozone, I don't really know why touchableHandleResponderMove wasn't called in your particular case. I'm quite sure in my experiment it did. It could be related to the pan responder as you already pointed out.

@macrozone

This comment has been minimized.

macrozone commented Sep 3, 2017

@chengyin you are correct. I have overwritten onMoveShouldSetPanResponderCapture with true, then, touchableHandleResponderTerminate is called imediatly.

I tried to get around this issue here by playing with different threshold in onMoveShouldSetPanResponderCapture, but i realized that there are even more bugs in the android version of react-native-svg. If you have a panresponder attached on a view around an svg, you will get odd jumps in movements (dx and dy) and speeds (vx, vy) when you move your finger over the view. If you log the gestureState (second argument) in onMoveShouldSetPanResponderCapture you will even notice that dx and dy do not start with 0 as they should!

I am trying to work around this nasty issue for days now, but the more I found out, the more bugs arise 👎

Edit: this Issue here might lead to this "jumping" behaviour: #433

@SethHamilton

This comment has been minimized.

SethHamilton commented Jan 4, 2018

Has anyone found a solution to this. I'm drawing little graphs with SVG inside a horizontal ScrollView so they can be panned. My targets on bar graphs (for example) are huge, if my finger drifts at all or rolls even I don't get on onPress event. I can't use the onPressIn because that highlights a bar whenever some scrolls the graph.

I could probably figure out a fix using the onPressIn and onPressOut events using coordinates.

@SethHamilton

This comment has been minimized.

SethHamilton commented Jan 4, 2018

I posted a fix here:

FormidableLabs/victory-native#25

@msand

This comment has been minimized.

Collaborator

msand commented Sep 23, 2018

@chengyin I think I've fixed the underlying issue now, try with the latest commit in the master branch and this code:

import React, { Component } from 'react';
import {
  StyleSheet,
  Text,
  View,
  TouchableWithoutFeedback,
} from 'react-native';

import Svg, { Rect, Text as SvgText } from 'react-native-svg';

export default class touchesMove extends Component {
  constructor(props) {
    super(props);

    this.state = { event: 'Last press event will be shown here.' };
    this.handlers = {
      onPress: () => {
        this.setEvent('onPress');
      },
      onPressIn: () => {
        this.setEvent('onPressIn');
      },
      onPressOut: () => {
        this.setEvent('onPressOut');
      },
    };
  }

  setEvent(event) {
    this.setState({ event });
  }

  render() {
    return (
      <View style={styles.container}>
        <Svg width={200} height={200}>
          <Rect fill="navy" width={300} height={500} {...this.handlers} />
          <SvgText x={0} y={14} fill="#fff">
            {'react-native-svg <Rect>'}
          </SvgText>
        </Svg>
        <TouchableWithoutFeedback {...this.handlers}>
          <View style={styles.demo}>
            <Text style={styles.text}>{'react-native <View>'}</Text>
          </View>
        </TouchableWithoutFeedback>
        <Text>{this.state.event}</Text>
      </View>
    );
  }
}

const styles = StyleSheet.create({
  container: {
    flex: 1,
    justifyContent: 'center',
    alignItems: 'center',
    backgroundColor: 'white',
  },
  demo: {
    width: 200,
    height: 200,
    backgroundColor: 'orange',
  },
  text: { color: 'white' },
});
@chengyin

This comment has been minimized.

Contributor

chengyin commented Sep 24, 2018

@msand I haven’t done RN development for a while now and I no longer have the SDKs installed. So I can’t help with the testing here, unfortunately. Feel free to utilize the testing repo I posted. It should contain all code needed.

@jnicholls

This comment has been minimized.

jnicholls commented Oct 24, 2018

FWIW I installed the latest version 8.0.5 from npm and this issue is still present.

@msand

This comment has been minimized.

Collaborator

msand commented Oct 24, 2018

@jnicholls Oh, I wonder if I've introduced a regression after having fixed this, could you try with this commit (the latest one when I wrote the previous comment) ad99f97
Or if that still fails, then the parent commit to that one: 35597ed

@msand

This comment has been minimized.

Collaborator

msand commented Oct 24, 2018

@jnicholls I have a fix for android: e2e415c
It also adds support for the react-native inspector, so you can check the fill bounds of svg elements.

@jnicholls

This comment has been minimized.

jnicholls commented Oct 24, 2018

@msand Thank you, I will check these out for sure. In the meantime I implemented onResponderMove, onPressIn and onPressOut and am doing my own calculation to determine if the user is tapping or dragging their finger (in a scroll-like fashion) which is working quite well as a JS workaround.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment