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

Responsive Dropdown activates links below #45

Open
evengene opened this issue Dec 9, 2020 · 20 comments
Open

Responsive Dropdown activates links below #45

evengene opened this issue Dec 9, 2020 · 20 comments
Labels

Comments

@evengene
Copy link

evengene commented Dec 9, 2020

Hello,

As you can see on a video below, when I click About > Our Team - it clicks the other menu below which is Help Center. When clicking About > Media - it clicks on a button below the dropdown opening a link.
It seems as if the dropdown has a lower priority or as if it is not visible.

If there are no links/content below the dropdown - it work perfect and goes to according links.

Do you think you can help me with that?
Thank you so much!

ezgif com-gif-maker (2)

@jedwards1211
Copy link
Member

These are hover menus right? I'll try to add a case to the demos like this to debug it

@evengene
Copy link
Author

evengene commented Dec 9, 2020 via email

@jedwards1211
Copy link
Member

Hmmm, I tried to reproduce with the code below but I can't yet, at least not macOS or my Android phone. I have an old iPhone I can pull out and charge up and try with. Can you post your code that's rendering the menus?

import * as React from 'react'
import Menu from 'material-ui-popup-state/HoverMenu'
import MenuItem from '@material-ui/core/MenuItem'
import Button from '@material-ui/core/Button'
import Box from '@material-ui/core/Box'
import {
  usePopupState,
  bindHover,
  bindMenu,
} from 'material-ui-popup-state/hooks'

type HoverMenuProps = {
  id: string,
  title: React.Node,
  children: React.Element<typeof MenuItem>[],
}

function HoverMenu({ id, title, children }: HoverMenuProps): React.Node {
  const popupState = usePopupState({ variant: 'popover', popupId: id })
  return (
    <React.Fragment>
      <Button variant="contained" {...bindHover(popupState)}>
        {title}
      </Button>
      <Menu
        {...bindMenu(popupState)}
        getContentAnchorEl={null}
        anchorOrigin={{ vertical: 'bottom', horizontal: 'left' }}
        transformOrigin={{ vertical: 'top', horizontal: 'left' }}
      >
        {React.Children.map(children, child =>
          React.cloneElement(child, { onClick: popupState.close })
        )}
      </Menu>
    </React.Fragment>
  )
}

const MenuPopupState = () => {
  return (
    <Box
      display="flex"
      flexDirection="column"
      alignItems="center"
      justifyContent="center"
    >
      <HoverMenu title="About" id="about-menu">
        <MenuItem>Our Team</MenuItem>
        <MenuItem>Media</MenuItem>
        <MenuItem>Careers</MenuItem>
      </HoverMenu>
      <HoverMenu title="Help Center" id="help-center-menu">
        <MenuItem>Contact Support</MenuItem>
        <MenuItem>Your Tickets</MenuItem>
        <MenuItem>Knowledge Base</MenuItem>
      </HoverMenu>
    </Box>
  )
}

export default MenuPopupState

@evengene
Copy link
Author

evengene commented Dec 9, 2020

this is a small chunk of code for a particular button and it's dropdown.
here is the link to the test environment so you can test it on 414px
if a part of a dropdown is positioned over another button - the priority goes to the latter.
Let me know if you need anything else.
Thanks!

                     <Grid item>
                            <Button className="dropdown-button"  {...bindHover(draweraboutpopupState)}>
                                <div className={`btn-link`}>
                                    About
                                    <AboutArrowDrawer/>
                                </div>
                            </Button>
                            <Menu
                                {...bindMenu(draweraboutpopupState)}
                                getContentAnchorEl={null}
                                anchorOrigin={{vertical: 'center', horizontal: 'right'}}
                                // anchorPosition={{vertical: 'bottom', horizontal: 'center'}}
                                transformOrigin={{vertical: 'top', horizontal: 'bottom'}}
                                id="about">
                                <MenuItem onClick={draweraboutpopupState.close}>
                                    <Link className="inside-link" to='/about/OurTeam/'>
                                        Our Team
                                    </Link>
                                </MenuItem>
                                <MenuItem onClick={draweraboutpopupState.close}>
                                    <Link className="inside-link" to='/about/media/Media/'>
                                        Media
                                    </Link>
                                </MenuItem>
                                <MenuItem onClick={draweraboutpopupState.close}>
                                    <Link className="inside-link"
                                          to='https://angel.co/company/superworld'
                                          target="_blank"
                                          rel="noopener noreferrer">
                                        Careers
                                    </Link>
                                </MenuItem>
                            </Menu>
                        </Grid>

@jedwards1211
Copy link
Member

jedwards1211 commented Dec 10, 2020

And are you using import Menu from 'material-ui-popup-state/HoverMenu'? (looks like it in your test page)

What browser/platform did you take the video on? It seems like the pointer event handling might be buggy on that browser. (HoverMenu has to use pointer-events: none on the popover backdrop, and then turn them back on with pointer-events: auto on the menu popover element). It seems like as you said the menu element isn't even receiving the click.

If this prevents the issue it would at least give me some idea how to solve it.

@jedwards1211
Copy link
Member

Oh sorry just saw your test link. Let me look

@jedwards1211
Copy link
Member

Hmmm that issue doesn't seem to happen in Chrome on my macOS or Android phone at least. Also when you're tapping on the menu items, they're not taking you to the linked pages I take it?

@jedwards1211
Copy link
Member

Just FYI since the MenuItems have padding, if you tap within the padding it's outside the Links and just closes the menu without following the link. Not 100% sure if it will work but you could try <MenuItem component={Link} to='/about/OurTeam/'> to make the entire menu item a link.

@evengene
Copy link
Author

evengene commented Dec 10, 2020 via email

@evengene
Copy link
Author

ok, seems to be working :) thank you so much

@jedwards1211
Copy link
Member

Okay, I still want to try to prevent this from happening, without the workaround -- can you let me know what browser/platform you recorded that video on? Thanks!

@evengene
Copy link
Author

evengene commented Dec 12, 2020 via email

@jpodpro
Copy link

jpodpro commented Apr 5, 2021

we are experiencing what appears to be a similar version of this too. on mobile in our cascading submenus, if you click on the chevron or towards the right of a menu item that is meant to open a sub-menu it selects an item from the sub-menu that is never shown.

trying your fix from above doesn't change anything. i also noticed that this happens when the menus are against the edge of the screen. i think it has to do with the sub-menus running out of space and being positioned over top the current menu.

i have made a sandbox that shows our problem. note that the sub-menus wouldn't open at all in firefox device mode. however in windows chrome device mode it shows the problem:

@jedwards1211
Copy link
Member

Sorry for the delay...man this is proving difficult to figure out :( I think I'm going to have to implement touch handlers instead of relying on click emulation

@jedwards1211
Copy link
Member

🎉 This issue has been resolved in version 1.8.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@jedwards1211
Copy link
Member

jedwards1211 commented Apr 17, 2021

Okay let me know if version 1.8.1 fixes this for y'all. Sorry for the long delay...it's mostly that I got sidetracked from this, but this situation is mess and I'm miffed at the Google Chrome team, and I kinda hate my workaround, which is to delay opening/closing with setTimeout(..., 0) when a touch event is detected.

If you want details of what the underlying problem is, read on...

For some reason, this is what was happening when you tap a hover trigger on a touch device:

  1. trigger element gets a simulated mouseover event
  2. my mouseover handler opens the menu
  3. if the newly-opened menu contains the tap location, it gets a simulated click event (wtf? why should an element that didn't exist when the tap occurred get any event?)

And in the case where you tap a menu item that closes the menu on a touch device:

  1. menu item gets a simulated click event
  2. my click handler closes the menu
  3. whatever is underneath the tap location gets a simulated click event (wtf? why did it pass through the menu?)

Real mouse events don't behave this way, so I think it's either a bug in the browser or React event handling.

I wish I could event.preventDefault() in the onTouchStart handler to prevent the simulated mouse events, but this is why I'm mad at the Google Chrome team -- ever since they made touch listeners passive by default, it's impossible to event.preventDefault() inside a React onTouchStart handler (see facebook/react#9809). I would have to get a ref to the DOM element to register an active touchstart listener, but this wouldn't be problematic because 1) Then it would clobber myRef in <Button ref={myRef} {...bindHover(popupState)} /> (EDIT: I forgot, same danger exists with event listeners and I advise about that in the readme, so I could try refs) and 2) I can't guarantee your component will forward the ref to a DOM element anyway.

@jedwards1211
Copy link
Member

jedwards1211 commented Apr 17, 2021

I guess this API is not flexible enough though. Maybe I need to redesign the API to look like

<BindHover popupState={popupState} component={Button} />`

(typing components like this properly for TS/Flow is hard)

Or perhaps

<BindHover popupState={popupState}>
  <Button />
</BindHover>

In this case, I would have to clone the Button element, which is a can of worms if you want any of the injected properties to be required in the Button prop type defs.

@jedwards1211
Copy link
Member

By the way, cascading menus are under development in Material UI itself: mui/material-ui#20591 (comment)

@brianedelman
Copy link

This issue is fixed for me on my android phone in chrome but is still broken on my ios device in chrome and ios

@jedwards1211
Copy link
Member

@brianedelman sorry for the delay, I'll look into solving this by injecting refs into the hover and popover elements...

@jedwards1211 jedwards1211 reopened this Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants