Skip to content
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

useMouseAndTouchTracker will not work with React Native #1315

Closed
DaveWelling opened this issue Sep 8, 2021 · 16 comments · Fixed by #1472
Closed

useMouseAndTouchTracker will not work with React Native #1315

DaveWelling opened this issue Sep 8, 2021 · 16 comments · Fixed by #1472

Comments

@DaveWelling
Copy link

utils.useMouseAndTouchTracker has some code like this:

    environment.addEventListener('mousedown', onMouseDown)
    environment.addEventListener('mouseup', onMouseUp)
    environment.addEventListener('touchstart', onTouchStart)
    environment.addEventListener('touchmove', onTouchMove)
    environment.addEventListener('touchend', onTouchEnd)

When the component is run in production mode with react-native this will fail because environment is defined like so:

const defaultProps = {
  itemToString,
  stateReducer,
  getA11ySelectionMessage,
  scrollIntoView,
  circularNavigation: false,
  environment:
    /* istanbul ignore next (ssr) */
    typeof window === 'undefined' ? {} : window,
}

As you can see, if window does not exist, the environment.addEventListener is going to crash every time.
I looked into the actual use of this hook (utils.useMouseAndTouchTracker) and it seems as though it is only used in two places, but both for the same purpose (it creates a ref (mouseAndTouchTrackersRef) that is used in the two code snippets below):
useCombobox:

      const inputHandleBlur = () => {
        /* istanbul ignore else */
        if (
          latestState.isOpen &&
          !mouseAndTouchTrackersRef.current.isMouseDown
        ) {
          dispatch({
            type: stateChangeTypes.InputBlur,
            selectItem: true,
          })
        }
      }

and useSelect:

      const menuHandleBlur = () => {
        // if the blur was a result of selection, we don't trigger this action.
        if (shouldBlurRef.current === false) {
          shouldBlurRef.current = true
          return
        }

        const shouldBlur = !mouseAndTouchTrackersRef.current.isMouseDown
        /* istanbul ignore else */
        if (shouldBlur) {
          dispatch({type: stateChangeTypes.MenuBlur})
        }
      }
      const menuHandleMouseLeave = () => {
        dispatch({
          type: stateChangeTypes.MenuMouseLeave,
        })
      }

It looks like it is used to determine if the component should blur based on whether the user clicked off of the component vs clicking on the dropdown portion of the component. Would it be better to check if the blur target is the dropdown portion? This would avoid the problem with react native I think? I don't think there is an equivalent to window.addEventListener('mousedown') in react native.

  • downshift version: latest (looking at current code in master)
  • node version: 16.6
  • npm (or yarn) version: 7.19.1

Relevant code or config
See above description.

What you did:
Render the component in react native production mode (vs dev mode).

What happened:

image

Reproduction repository:
This can be reproduced easily by just running the useCombobox component in any react native production mode code.

Problem description:
See above.

Suggested solution:
use the target of the onBlur handler to determine if the component should be blurred instead of looking for a click outside the component.

@silviuaavram
Copy link
Collaborator

Thanks for reporting this! I believe that the logic we use in this hook is used in the Downshift component as well. If Downshift works in React native then the hooks should too. Is there a fix we could add here?

@DaveWelling
Copy link
Author

DaveWelling commented Sep 9, 2021

Hi @silviuaavram If I perform a search, I cannot find any other usage:
image
Maybe there is something tricking going on though, and I am not seeing it?

A simple fix to prevent the component from completely crashing would be to not create the listener at all if the environment cannot. For instance (in utils.js at line 367):

if (environment.addEventListener != null) {
        environment.addEventListener('mousedown', onMouseDown)
        environment.addEventListener('mouseup', onMouseUp)
        environment.addEventListener('touchstart', onTouchStart)
        environment.addEventListener('touchmove', onTouchMove)
        environment.addEventListener('touchend', onTouchEnd)

        return function cleanup() {
            environment.removeEventListener('mousedown', onMouseDown)
            environment.removeEventListener('mouseup', onMouseUp)
            environment.removeEventListener('touchstart', onTouchStart)
            environment.removeEventListener('touchmove', onTouchMove)
            environment.removeEventListener('touchend', onTouchEnd)
        }
}

This would certainly be an improvement, but it would not fulfill the original goal of the code.

That being said, real estate is usually different on mobile devices that use react native. Putting the list portion for a dropdown inside a form (below the input) does not always make very much sense because it tends to require scrolling to see it and then you have contending scrolling areas (scrolling for the form itself and scrolling for the dropdown list).
For this reason, we render the list portion full screen as a modal and we are unaffected by this code (except that it is crashing). So the fix above would work for us.
How we render the open, mobile dropdown as a modal:
image

@JeroenSchrader
Copy link

I am currently working on implementing downshift in my React Native app.
I started of using useCombobox, but ended up with errors (see image below).
It works fine using the Downshift component, but I would love to use hooks instead.

image

@silviuaavram
Copy link
Collaborator

I think the fix from @DaveWelling makes sense. I looked into the Downshift component code and this part is ignored when in react native. Will apply the same fix for hooks, so that they work in the same way as Downshift for this part.

Thank you for you help!

@silviuaavram
Copy link
Collaborator

silviuaavram commented Feb 27, 2023

@DaveWelling @JeroenSchrader I need your help.

I want to create hooks usage examples for React Native.

For example, this one with useSelect: https://snack.expo.dev/@silviuaavram/downshift-useselect-in-react-native?platform=web. It uses this build that should allow useSelect to work with React Native #1476.

The snack above works well on mobile, but fails on web. On web, isReactNative value is turned to false, but apparently the components still need onPress passed to them, and useSelect is passing onClick and onKeyDown when isReactNative is false, in order for web React usage to work.

However, React Native web equivalent does not work, and I don't know how to properly fix this. I can grab onClick and make it as onPress in the React Native example, but that kind of defeats the whole purpose of isReactNative and making the getter props work automatically with any scenario: React, React Native web, React Native mobile.

I am not very good at React Native, I am hoping you can help me with some advice. Thanks!

@JeroenSchrader
Copy link

JeroenSchrader commented Mar 1, 2023

@silviuaavram
Hmm why is isReactNative false on React Native web? React Native web should have the exact same API as for Android/iOS, so yes the component does expect onPress. I am not sure how the package release flow works, but maybe something is wrong there? Or maybe the isReactNative macro does not work correctly on React Native web?

@DaveWelling
Copy link
Author

@silviuaavram @JeroenSchrader Sorry I am slow to reply. You might need to distinguish isReactNative and a new boolean for isReactNativeWeb. I realize that further complicates things, but you really do have a different set of APIs available for all three. It's probably obvious, but the entire DOM is available with React Native Web, and is absent with React Native using native code. IMO React Native Web is sort of a misnomer caused by the dev being between a rock and a hard place. I think he was trying to provide a common API for both web and native to enable one of the true original objective of React Native (shared code). Sorry if this sounds a little whiney, but the Meta team doesn't really seem to care at all about sharing code between web and native. I don't think they have any code that is shared with a web app that they need to support. In any case, I think that's what the original guy who started React Native Web was trying to rectify.

@silviuaavram
Copy link
Collaborator

silviuaavram commented Mar 7, 2023

I have no idea how isReactNative works, or if isReactNativeWeb can be created, but it's something we may need to look into.

Also, FYI, https://downshift-js.com now contains RN examples as well, for both the hooks and the component.

@JeroenSchrader
Copy link

Ah I didn't know, I expected react-native web to be the same as android/ios, probably because I have never worked with react-native web before.

In that case it's indeed a good idea to create a isReactNativeWeb macro and use web api's whenever that's the case.

@silviuaavram
Copy link
Collaborator

Any idea on how to implement this? I spent quite some time on this trying to figure it out, but no luck so far. No idea how to get those environment variables from is.macro.js or some other way. Really, I tried!

Can you provide some help in getting a isReactNativeWeb value that will work? It will help the library a lot to fix this use case. @DaveWelling @JeroenSchrader @Andarist . Thank you!

@DaveWelling
Copy link
Author

@silviuaavram I'll try to take a look on Friday if nobody beats me to it. 👍

@DaveWelling
Copy link
Author

DaveWelling commented Mar 17, 2023

@silviuaavram I think I have it figured out for you.

The is.macro.js is used to exclude/include certain code. It outputs variables you can use to identify code specific to an environment and toggle the appearance of that code at build time.
If you wrap code like this:
if (!isReactNative) { /*non react-native code here*/} it will be excluded from the react native build. For example:
This code:

  import {isPreact, isReactNative} from './is.macro'
  scrollHighlightedItemIntoView() {
    /* istanbul ignore else (react-native) */
    if (!isReactNative) {
      const node = this.getItemNodeFromIndex(this.getState().highlightedIndex)
      this.props.scrollIntoView(node, this._menuNode)
    }
  }

Results in this outputted code in the react native build (downshift.native.cjs.js):

_proto.scrollHighlightedItemIntoView = function scrollHighlightedItemIntoView() {
    };

You can verify this for yourself by just opening /dist/downshift.native.cjs.js and performing a find on scrollHighlightedItemIntoView and comparing that to what you find in /dist/downshift.cjs.js.

So to create code that is included/excluded depending on if it is react native web is relatively simple. You need to change is.macro.js like this:

const importToEnvVar = {
    isPreact: 'BUILD_PREACT',
    isReactNative: 'BUILD_REACT_NATIVE',
    isReactNativeWeb: 'BUILD_REACT_NATIVE_WEB'
}

and package.json like this:

"scripts": {
        "build": "npm run build:web --silent && npm run build:native --silent && npm run build:nativeWeb --silent",
        "build:web": "kcd-scripts build --bundle --p-react --no-clean --size-snapshot",
        "build:native": "cross-env BUILD_REACT_NATIVE=true BUILD_FILENAME_SUFFIX=.native kcd-scripts build --bundle cjs --no-clean",
        "build:nativeWeb": "cross-env BUILD_REACT_NATIVE_WEB=true BUILD_FILENAME_SUFFIX=.nativeweb kcd-scripts build --bundle cjs --no-clean",
         ...
}

Now builds will output a new file downshift.nativeweb.cjs.js which can be used by react native web users.
For instance, this code:

    import { isPreact, isReactNative, isReactNativeWeb } from './is.macro'
    scrollHighlightedItemIntoView() {
        /* istanbul ignore else (react-native) */
        if (!isReactNative) {
            const node = this.getItemNodeFromIndex(this.getState().highlightedIndex)
            this.props.scrollIntoView(node, this._menuNode)
        }
        if (isReactNativeWeb) {
            console.log('I\'m in react native web!');
        }
    }

Yields this code inside of downshift.nativeweb.cjs.js:

_proto.scrollHighlightedItemIntoView = function scrollHighlightedItemIntoView() {
      /* istanbul ignore else (react-native) */
      {
        var node = this.getItemNodeFromIndex(this.getState().highlightedIndex);
        this.props.scrollIntoView(node, this._menuNode);
      }
      {
        console.log('I\'m in react native web!');
      }
    };

@silviuaavram
Copy link
Collaborator

This is amazing, @DaveWelling , I love it! I am planning to write an article about using downshift & react native and this post will definitely be covered in there, the explanation is top notch!

Will go ahead and implement the changes in the meantime. Thanks a lot!

@silviuaavram
Copy link
Collaborator

@all-contributors please add @DaveWelling for code, ideas, research.

@allcontributors
Copy link
Contributor

@silviuaavram

I've put up a pull request to add @DaveWelling! 🎉

@silviuaavram
Copy link
Collaborator

Created #1489, feel free to take a quick look, I think it should be good to go. @DaveWelling @JeroenSchrader

Thanks again everyone, I'm happy to be able to support React native web in downshift with your help!

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

Successfully merging a pull request may close this issue.

3 participants