necolas / react-native-web

Cross-platform React UI packages
https://necolas.github.io/react-native-web
MIT License
21.58k stars 1.78k forks source link

Touchable issues #1219

Closed necolas closed 4 years ago

necolas commented 5 years ago

The problem

The overall issue appears to be that a) events don't always occurs in the same order, which means the responder is not transferred correctly, b) mouse move events during a touch can cause the responder to be terminated, and c) scroll events are not always fired in situations where they are expected in React Native.

There are problems with the responder event system setup that are covered by these issues:

693

https://glitch.com/edit/#!/statuesque-bedbug

locationX is relative to the element on which the native event occurred but not the element which the responder event is attached.

731

https://glitch.com/edit/#!/cultured-bracket

In React Native, ScrollView will trigger onScroll events if you're dragging down when at the top of the scroll view - this allows the responder system to transfer the responder grant from the touchable back to the scroll view. But the web stops dispatching scroll events when scrollTop === 0, so the responder system leaves the touchable as the responder. The reported issue goes away when ScrollView dispatches a DOM scroll event after it receives a touchmove that occurs when scrollTop === 0.

829

There are 2 significant issues with using Touchable* components within an app that relies on the document.body (rather than ScrollView) being the root scroller. I believe the root cause of both issues is that the body is not part of the responder system, which Touchable and ScrollView rely on to coordinate transfer of the responder grant.

1164

https://codesandbox.io/s/6njwzqojnr

TouchableOpacity will trigger another component at the same location when changing views (if second component is an "" or "")

1124

Across multiple browsers, if you hold the mouse very still onPress triggers, but if the mouse is in motion onPress does not work.

Context

Part of the solution may related to producing the correct events (as can be inferred from React Native). This might involve changes to the event filtering logic in the REP, and the scroll event logic in ScrollView.

Related incomplete patches

Attempts to patch these problems.

tafelito commented 5 years ago

@necolas what's missing on the 2 solutions you mentioned above?

I don't see this happening at twitter, is this an issue from the latest versions of RNW, are you doing anything different there?

paularmstrong commented 5 years ago

@tafelito: Twitter doesn't use ScrollView and has a bunch of hacky workarounds in order to avoid using it (because we rely on body-scrolling, for various reasons). In the long run, this isn't great and we'd like a path to move away from our workaround.

tafelito commented 5 years ago

For us this also happens outside a ScrollView. We are avoiding it in some places just using preventDefault but that just also a hack and it doesn't even work in every case


From: Paul Armstrong notifications@github.com Sent: Wednesday, January 16, 2019 18:24 To: necolas/react-native-web Cc: tafelito; Mention Subject: Re: [necolas/react-native-web] Responder Event System issues (#1219)

@tafelitohttps://github.com/tafelito: Twitter doesn't use ScrollView and has a bunch of hacky workarounds in order to avoid using it (because we rely on body-scrolling, for various reasons). In the long run, this isn't great and we'd like a path to move away from our workaround.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/necolas/react-native-web/issues/1219#issuecomment-454982665, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AFsmlCJS0V7tKkrZCZxIwIKrZVcNS2b0ks5vD7SVgaJpZM4ZoyPr.

viggyfresh commented 5 years ago

Hi @necolas,

Just wanted to follow up on @aligg's comment in #1124 (see https://github.com/necolas/react-native-web/issues/1124#issuecomment-450954401) - we ended up solving our issue! For posterity: we had inadvertently mixed our metaphors and wrapped a bunch of RNW components inside a div with overflow-y set to scroll; when we replaced this with a RNW ScrollView, all of our problems magically went away! I'm not at all familiar with the responder event system, but I presume this particular blend of web and RNW components isn't supported (or wise).

RichardLindhout commented 5 years ago

@necolas Maybe we can listen to a normal click event of the browser instead of onTouchStart etc.

EyMaddis commented 5 years ago

@RichardLindhout I use custom touchables right now and it works by using something like this:

<TouchableOpacity
          ...
          {...(Platform.OS === 'web'
            ? {
                // When scrolling the document body, the touchables might be triggered
                // see  https://github.com/necolas/react-native-web/issues/1219
                onClick: this.onPress,
              }
            : {
                onPress: this.onPress,
              })}
/>

Edit: This currently might not work if you use accessibilityRole="link", see #1348.

loudwinston commented 5 years ago

@EyMaddis good tip! We applied this as a monkey-patch for Touchable components by using webpack NormalModuleReplacementPlugin so we don't have to apply this ternary all over our app

webpack.config.js

plugins: [
  new webpack.NormalModuleReplacementPlugin(
    /^react-native$/,
    path.resolve(__dirname, './src/web/react-native-web.tsx')
  )
]

react-native-web.tsx

import React from 'react'
import * as ReactNativeWeb from 'react-native-web'

const createTouchable = (Touchable) => (props: any) => {
  const { onPress, ...restProps } = props
  return (
    <Touchable {...restProps} onClick={onPress} />
  )
}

export * from 'react-native-web'

export const TouchableOpacity = createTouchable(ReactNativeWeb.TouchableOpacity)
export const TouchableWithoutFeedback = createTouchable(ReactNativeWeb.TouchableWithoutFeedback)
// etc
tafelito commented 5 years ago

one problem I see with that solution is that you manually need to handle propagation. If you have a Touchable inside another touchable, the onPress only gets called once. Using onClick you have to stop propagation depending on your use case it might just work adding in to the onClick event

EyMaddis commented 5 years ago

The touchable problem with touchables triggering on scrollY=0 boils down to this line:

https://github.com/necolas/react-native-web/blob/master/packages/react-native-web/src/modules/ScrollResponder/index.js#L336 (scrollResponderIsAnimating) as it controls scrollResponderHandleStartShouldSetResponderCapture.

If I understand it correctly the problem is that the touchstart event fires first and then scroll. But touchstart was already triggered and grants the handling to the touchable... Once the browser triggers scroll, the touchable handling should be terminated and the scrollview should seize control.

Edit: I cannot find a way to send signals to the responder system. I would like to trigger a termination to other touchables once a browser scroll was registered. Where is the responder system actually implemented? Does it occur in RNW, in vendor code or from react, @necolas ? If so, do you have any hints?

stoicskyline commented 5 years ago

Noticed that on certain mobile browsers like iOS Safari, scrolling will accidentally click when gesture is released. Is this the same issue that you're describing @EyMaddis ?

EyMaddis commented 5 years ago

@jorgechen Yes, see https://github.com/necolas/react-native-web/issues/731

andreassavva commented 5 years ago

About #1124 it only happens when I have scrolled down below approximately 50 pixels. If I am at the top of the page, the onPress event is being called as it should. If I first scroll below 50 pixels or more, click down inside the TouchableOpacity and then move the mouse (still inside the TouchableOpacity component), then the press event is being released and the onPress event is not called when I release the mouse click.

Anyone that can shed some light on where in react-native-web this is happening? It would help me search for a way to fix this

andreassavva commented 5 years ago

I fixed #1124 . In Touchable/index.js the touch from the native event is extracted in the touchableHandleResponderMove function and pageX and pageY are received from it. These two attributes give the x and y positions in the whole page, which means that the scrolled pixels are added to them.

Getting clientX and clientY from touch instead of pageX and pageY fixes this issue for me.

brunolemos commented 4 years ago

Is there any workaround for the touchable press getting triggered if I scroll and release the finger on top of the touchable? This is hurting the ux of my app.

cc @MoOx

EDIT: I just realized (after reading this comment) that the issue only happens because my TouchableOpacity is inside react-window. If I use the built-in FlatList the issue doesn't happen. Any workaround for that?

stoicskyline commented 4 years ago

@brunolemos Have you tried the method suggested by @EyMaddis ? That solution has worked for me until an official fix.

By the way, good work on DevHub and hope you continue writing dev articles. Obrigado.

brunolemos commented 4 years ago

I ended up using a different workaround. I have a Touchable.tsx component and I check the pageX and pageY values from the press events, and if the user moved the finger I assume it was a scroll and cancel the press event. Seems to work great, I'm happy with the result.

const pressInPagePointRef = useRef({ x: 0, y: 0 })

const onPressIn = useCallback<NonNullable<TouchableProps['onPressIn']>>(
  e => {
    pressInPagePointRef.current = {
      x: e.nativeEvent.pageX,
      y: e.nativeEvent.pageY,
    }

    if (_onPressIn) _onPressIn(e)
  },
  [_onPressIn],
)

const onPressOut = useCallback<NonNullable<TouchableProps['onPressOut']>>(
  e => {
    if (_onPressOut) _onPressOut(e)

    const [x, y] = [e.nativeEvent.pageX, e.nativeEvent.pageY]
    if (
      Math.abs(pressInPagePointRef.current.x - x) > 1 ||
      Math.abs(pressInPagePointRef.current.y - y) > 1
    ) {
      e.preventDefault()
    }
  },
  [_onPressOut],
)

const onPress = useCallback<NonNullable<TouchableProps['onPress']>>(
  e => {
    if (e.isDefaultPrevented()) return
    if (_onPress) _onPress(e)
  },
  [_onPress],
)
necolas commented 4 years ago

I'm working on a user-space replacement for the responder system that aims to resolve these issues and drop the dependency on ReactDOM's unstable-native-dependencies module.

brunolemos commented 4 years ago

drop the dependency on ReactDOM's unstable-native-dependencies module.

Nice. Would that unlock preact support?

Ref: #330 Ref: https://github.com/preactjs/preact/issues/1830

necolas commented 4 years ago

Maybe!

mschipperheyn commented 4 years ago

@necolas achievement unlocked: Guru!

necolas commented 4 years ago

FYI I tried PreactX in my hooks branch (which has the in-progress responder replacement). Preact is rendering the benchmarks 50% slower than React. And Preact has a bug that prevents the responder hook's event handlers from ever being run.

marvinhagemeister commented 4 years ago

@necolas Can you file an issue for us over at the Preact repo? We'd love to get to the bottom of this :+1:

jfrolich commented 4 years ago

@necolas: Do you have a recommendation how to handle the current issue with window scrolling being broken? Are you open to have a temporary workaround in the stable RNW, before the responder system rewrite that fixes the scrolling issue has landed. (Perhaps a fix that does not handle all cases, but makes Touchables usable in a window scroll environment). Or do you foresee the new responder system landing soon-ish? Or would you recommend to do a patch-package for now?

xiaods commented 4 years ago

Bug:

$ npm ls react-native-web em@93.0.0 /Users/xiaods/Documents/Code/em └── react-native-web@0.12.0

xiaods at xiaodeshi in ~/Documents/code/em on (dev) $ npm ls react-scripts em@93.0.0 /Users/xiaods/Documents/Code/em └── react-scripts@1.1.5

in a react-native-web project.

1、create multigesture component from react. use react-native-web packge // requires installation of react-dom and react-native-web import { PanResponder } from 'react-native'

when came with react-scripts@1.1.5 , it works like a charm.

when upgrade the react-script > 2, the PanResponder will can't work in iOS mobile.

EyMaddis commented 4 years ago

@jfrolich Could you try this PR? https://github.com/necolas/react-native-web/pull/1472

jfrolich commented 4 years ago

@EyMaddis: Will try! Looks like a good solution! I see that it's conflicting now with master?

necolas commented 4 years ago

Please try version 0.0.0-33f8505d6 (a canary release) and let me know if the problem is resolved. See #1538

rxb commented 4 years ago

I had the same / similar problem as @jfrolich which brought me to this issue. Version 0.0.0-33f8505d6 totally fixed it for me. Thanks for all your work on this!

jfrolich commented 4 years ago

I tried different solutions, and they didn't really solve the problem. The new fix by @necolas works perfectly however! Only now reanimated/react-native-gesture-handler are breaking, probably because this is such a big change in the internals.

necolas commented 4 years ago

Are they breaking on findNodeHandle calls? That API is going away in React Native/DOM as it doesn't work with functional components or concurrent mode

jfrolich commented 4 years ago

Are they breaking on findNodeHandle calls? That API is going away in React Native/DOM as it doesn't work with functional components or concurrent mode

Yes precisely that. What API is replacing that?

necolas commented 4 years ago

React refs to host nodes should be used. Facebook and React Native itself have mostly migrated off those APIs already. https://reactjs.org/docs/react-dom.html#finddomnode

jfrolich commented 4 years ago

React refs to host nodes should be used. Facebook and React Native itself have mostly migrated off those APIs already. https://reactjs.org/docs/react-dom.html#finddomnode

Gotcha, that's what I thought as well 👍

Maker-Mark commented 3 years ago

Still seeing this issue!