Skip to content

[FIX] onTouchMove on mobile web does not trigger#1079

Merged
erictraut merged 2 commits intomicrosoft:masterfrom
dagatsoin:fix/mobile-ontouchmove
Apr 16, 2019
Merged

[FIX] onTouchMove on mobile web does not trigger#1079
erictraut merged 2 commits intomicrosoft:masterfrom
dagatsoin:fix/mobile-ontouchmove

Conversation

@dagatsoin
Copy link
Copy Markdown
Contributor

@dagatsoin dagatsoin commented Apr 15, 2019

Depends on #1078 for testing on mobile.

Issue:
View.onTouchMove handler is not available on mobile web.

This commit does the following:

  • bind props to div.onTouchMove handler
  • add test to ViewTouch

Note
Weird lint error forces me to force cast the TouchEvent handler

Full error:

Type '((e: TouchEvent) => void) | undefined' is not assignable to type '((event: TouchEvent<any>) => void) | undefined'.
  Type '(e: TouchEvent) => void' is not assignable to type '(event: TouchEvent<any>) => void'.
    Types of parameters 'e' and 'event' are incompatible.
      Type 'TouchEvent<any>' is not assignable to type 'TouchEvent'.
        Types of property 'changedTouches' are incompatible.
          Type 'React.TouchList' is not assignable to type 'import("/Users/***/dev/reactxp/src/common/Types").TouchList'.
            Types of property 'item' are incompatible.
              Type '(index: number) => React.Touch' is not assignable to type '(index: number) => import("/Users/***/dev/reactxp/src/common/Types").Touch'.
                Type 'Touch' is missing the following properties from type 'Touch': locationX, locationY

@erictraut
Copy link
Copy Markdown
Contributor

Adding onTouchMove to the public interface of RX.View isn't in keeping with the general philosophy of ReactXP because it's platform-specific (notably, the web). We should consider alternative approaches such as:

  1. Subscribing to onTouchMove internally to the web implementation but calling the general onMouseMove at the RX.View layer.
  2. Implementing onTouchMove for RX.View in all platforms.

@dagatsoin
Copy link
Copy Markdown
Contributor Author

Thanks for the precision of the general philosophy.

Regarding the alternative approches:
1- onMouseMove does not fit IMO because it misses important information like targetTouches
2- implementing onTouchMove could be redondant with the responder system. But that make think to another option

=> Bind onTouchMove to the onResponderMove:
1- the signature is the same (e: TouchEvent) => void
2- the API surface won't change.
3- we keep in the general philosophy

@dagatsoin
Copy link
Copy Markdown
Contributor Author

Just made the modification, waiting for your feedback.

@dagatsoin
Copy link
Copy Markdown
Contributor Author

dagatsoin commented Apr 16, 2019

To add to the onResponder cause: I am facing a new case where an other missing web touch handler (onTouchEnd) is missing.

Here is the current result. It is a Fast Action Button.

image

It works this way:

1- I long press the bottom right FAB
2- its options are displayed.
3- with the same touch I will over one of the options. All the options are included in a view which detects, on each touch move, which option is under the touch.
4- on the touch release, I will trigger the target option onPress handler.

This case works with onTouchMove and onTouchEnd handlers which are not currently supported.

So I already need to implement two new handlers. And I guess some others in near futur.
Regarding the implementation options, I think a good requirements is to keep the minimum surface API.
So I would argue that binding the web touch handlers to the responder system is a good way to take.

I would start with those handlers:
-onTouchStart => onResponderStart
-onTouchMove => onResponderMove
-onTouchEnd => onResponderEnd
-onTouchCancel => onResponderTerminate

There are also those handlers, but I don't find any doc on them.
-onTouchStartCapture => onStartShouldSetResponder ??
-onTouchMoveCapture => onMoveShouldSetResponderCapture ??
-onTouchEndCapture => ??
-onTouchCancelCapture => onResponderTerminationRequest ??

If you are agree I will update this PR this way.

@erictraut
Copy link
Copy Markdown
Contributor

Yeah, this solution looks good to me. Thanks.

@erictraut erictraut merged commit a57de2d into microsoft:master Apr 16, 2019
@erictraut
Copy link
Copy Markdown
Contributor

I jumped the gun on merging your initial PR for onTouchMove. Your plan above sounds good. Please submit a new PR with the other three responder callbacks bound as well.

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.

2 participants