Menu closing without selecting element on touch inside a Popper

[izyb]

• downshift : 2.1.0
• node : 8.11.2
• npm : 5.6.0

Relevant code or config

// input container
<Popper anchorEl={this.popperNode} {...getMenuProps()}>
  <div>
    // items with getItemProps
  </div>
</Popper>

What you did:
Tapping items in chrome/firefox devtools device mode, using Material UI's Popper components in the above configuratoin.

Problem description:
Tapping items does not select them, and the menu instantly closes on touchstart.

Suggested solution:
After reading #485, I tried setting the checkActiveElement flag to true on touchstart validation, and solved the problem (no doubt opening up other problems): https://github.com/paypal/downshift/blob/19732576d4f5e72b792bbc555c3c5ef7336883b1/src/downshift.js#L973-L976. I'm not super familiar with debugging this sort of thing; any suggestions would be greatly appreciated!

*Relevant Issue: #485

[izyb]

This is also demonstrated on a Material UI demonstration page https://material-ui.com/demos/autocomplete/#downshift.

[kentcdodds]

Yup, this is an issue for sure. Anyone wanna track down what's going on?

[bresson]

@izyb Can you confirm the same behavior on Downshift's Storybook ( "react-popper"):
https://downshift.netlify.com/

In Downshift's Storybook, I ran a cursory test on my iPad, iPhone and with desktop Chrome/FF ( versions ... ) in devtools and responsive mode and "react-popper" tested OK.

In the Material-UI Popper at https://material-ui.com/demos/autocomplete/#downshift, I can absolutely duplicate the issue on my iPad, iPhone and with desktop Chrome/FF. However, it appears Material-UI is using Popper straight from popper.js whereas Downshift leverages react-popper.js

/* [from] */ material-ui/docs/src/pages/demos/autocomplete/IntegrationDownshift.js
import Popper from '@material-ui/core/Popper';

/* [from] */ material-ui/packages/material-ui/package.json
 "dependencies": {
      ...
     "popper.js": "^1.14.1",
      ...
}

/* [from] */ downshift/stories/package.json
  "dependencies": {
      ...
      "react-popper": "^0.7.2",
      ...
  }

FYI, According to its repo page, react-popper is a react wrapper around popper.js. Both libraries stem from the same author. https://github.com/FezVrasta/react-popper

I haven't dug deep to understand why Material-UI chose popper.js instead of react-popper but perhaps this is part of the explanation:
floating-ui/react-popper#172

At this point, based solely on the different libraries used and not on any actual debugging, it appears the difference in libraries is the culprit. It also appears Material-UI's is a newly refreshed component:
mui/material-ui#9893 (comment)

[izyb]

@bresson I also can't reproduce the issue on Downshift's storyboard, and I think you're right that it's an implementation inconsistency between the popper libraries.

I'll make an issue on Material-UI about this, and see what needs to be done to resolve the problem.

[oliviertassinari]

I'm looking into it.

[oliviertassinari]

The issue can easily be solved by using the disablePortal option of the Popper. However, it defeats a large part of the use case for the Popper component.
Looking at #485 and the closing PR, I haven't found any way to improve the support of the Portal component with Downshift. Any tips?

[oliviertassinari]

I have removed the Popper component out from the equation. Using a Portal component is enough to notice the issue. Here is a reproduction example: https://codesandbox.io/s/2xz252nwq0.

[kentcdodds]

Yep, that reproduces for me. Any ideas what's going on?

[kentcdodds]

This is still an issue with the latest from everything. Anyone want to spend some time debugging what's going on here?

[oliviertassinari]

I believe it's still an issue mui/material-ui#12679, as soon as it's fixed, we can revert our portal hack and close mui/material-ui#13014 on Material-UI side.

[izyb]

@kentcdodds @oliviertassinari sorry I've been busy this past month, I'm willing to look at it more in depth this weekend, on both ends

[kentcdodds]

Thanks!

[izyb]

@oliviertassinari @kentcdodds I've made a branch here* in which I added an example to the downshift storybook demonstrating the problem.

Okay so here's what I've gathered so far:

Why is this happening?

This is a bug with the passing of the ref prop down from downshift's getMenuProps to the target menu node. When paired with Popper, Portal, and/or Paper from Material UI (I should mention that my original project used Paper instead of div as shown above), this prop gets lost somewhere, even though the role, aria-labelledby, and id props are passed fine.

Where are the refs going?

You may already be familiar with this, but after doing some digging I learned that this is due to the way React passes the ref prop to components. React doesn't consider ref a prop, it's a reserved keyword almost, and when passed down is immediately applied to the component. However, the ref is passed to the top level component, and not the top level DOM Node (necessarily). This is a problem for when Downshift validates that the touched item is inside the menu. When passing to native React components like div, these two are essentially one and the same, so setting getMenuProps() on the nested div solves my original issue, since ref is being passed to a valid DOM node.

Yay?

However, this reveals a more pressing issue, namely that any React component that is not a native element cannot be set as a menu node with getMenuProps() without some sort of refs forwarding, meaning no portal/popper/DOM teleporting will work unless the root of the menu is a native element. Material UI sets the standard of using innerRef as the defacto ref key and then when wrapping the component with withTheme() assigns that prop to the ref prop when passed to the wrapped component. ref is still passed to the component, however, so it essentially moves the ref one layer deeper, but not deep enough to the DOM level (I tried setting getMenuProps({ refKey: 'innerRef' })). So neither Popper (which does create a native element but doesn't have ref forwarded to it), nor Paper nor List nor any other component wrapped with withTheme will work as root menu nodes. Portal, while it is not wrapped with withTheme, ignores all props except disablePortal and children. Also, react throws this error when stateless functions are passed ref's, but that is out of the scope of this issue.

Where to go from here

I may be completely off base with this assessment, and I hope you verify my claims. I imagine this might create a lot of work for one or both of these projects (both fantastic, by the way :)). I see that MUI is already considering switching to React.forwardRef, which is React's built in way of passing refs down, and already recognizes the scope of the task. Fixes on Downshift's end probably need more thought, since it is relying on the ref being forwarded to the menu node.

Recap + loose ends

• ref is not a regular prop, so it should maybe be documented in Downshift the importance of choosing which component to put getMenuProps() on.
• native elements work fine with getMenuProps() and portals.
  • Material UI can fix the autocomplete demo by setting a root div with getMenuProps() wrapped around the Paper component.
• this isn't a problem with clicking because Downshift checks the document active element on click events, but not touch events.
• I haven't looked much into why this works with react-popper, but I can see that in the storybook the Target is still inside Downshift's root node, so it is likely that targetWithinDownshift is returning true for _rootNode, but not _menuNode.

* for my test on the storybook the ref node is not being set correctly on initial component load so just double click one of the checkboxes to get that set (otherwise the options will just be at the bottom of the screen, its the same outcome though since we just want the target to be outside of rootRef)

[kentcdodds]

Thank you so much for that research @izyb! What if instead of applying getMenuProps to the portal element, we apply it to an element within the portal element? Like this: https://codesandbox.io/s/40z34mk989

Seems to have fixed it for me.

I was going to say that we could have some validation that things are being used properly, but I don't think we can reliably do that and handle all use cases... Hmmm... Maybe... Especially considering the getMenuProps() should be called on every render for accessibility purposes. So we can have some validation that it is called and we could also validate that the ref callback gets a DOM node. Thoughts?

Anyone wanna make that contribution to the next branch? I'm hoping to get this fixed up and released on Monday.

[izyb]

@kentcdodds Yes putting getMenuProps on the inside component like that will work, assuming the ref is properly forwarded/applied to the DOM (either via a native ul as you've done or with a component that has some way of forwarding the ref to a native element)!

I agree that the validation would be difficult to implement and handle all cases. I think adding a isDOMElement() call in validateGetMenuPropsCalledCorrectly is a good place to start, but when I was tinkering earlier it would throw errors even when the component seemed to be working, but that may have been due to a ton of other things. I can certainly look into adding that because it could resolve a lot of confusion while we think about a more permanent solution.

[kentcdodds]

think about a more permanent solution.

I'd prefer to come up with this more permanent solution in time for the 3.0.0 release on Monday (in case the solution involves breaking changes).

[izyb]

I hear ya, it would be nice to have it done by Monday. I'll look into other solutions for now

[kentcdodds]

I went ahead and pushed the release. I think we're probably good for now. If you come up with something though that'd be super :)

[kentcdodds]

I'll actually go ahead and close this issue for now.

[oliviertassinari]

@kentcdodds Awesome, thank you, I'm continuing that on Material-UI side :).