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

[Popover] On mobile Chrome while zoomed, opening popover causes page to jump #17636

Open
2 tasks done
ilyamerman opened this issue Sep 30, 2019 · 21 comments
Open
2 tasks done
Labels
bug 🐛 Something doesn't work component: Popover The React component. priority: important This change can make a difference

Comments

@ilyamerman
Copy link

  • The issue is present in the latest release.
  • * I have searched the issues of this repository and believe that this is not a duplicate.

* There's an open issue related to IconMenu for iOS devices, but this is not the same component, and also no longer exclusive to iOS devices but happens on Android also.

Current Behavior 😯

On mobile Chrome, while the page is slightly zoomed, opening the popover will cause it to open but will also cause the page to scroll itself up into a position in which the popover can no longer be visible.
Also, while being in that position, the scrolling/zooming of the page is disabled and the only interaction allowed is to tap the invisible backdrop in order for the popover to close.

The issue does not happen on the root component (Modal) which leads us to believe that this may be an issue with the calculation of the anchor position, but we're not 100% sure.

We have also tested all the combinations of disableAutoFocus, disableEnforceFocus and disableRestoreFocus for the root modal props, but to no effect.

The issue only happens on the actual mobile device, using the desktop Chrome Devtools to emulate the issue was unsuccessful.

Expected Behavior 🤔

Expected the popover to open and for the page to stay in the same scroll position regardless of the current zoom level in order for the popover content to be visible, also expected to be able to at least drag the viewport in order to reach the relevant popover after the jump (solved it by overriding the touch-action: none css for the backdrop, but was confused by its default value)

Steps to Reproduce 🕹

The issue can be reproduced on the material-ui popover component demo website: https://material-ui.com/components/popover/

Including video of the interaction described in the steps below: https://drive.google.com/file/d/12MLICM7nngPuthRaVhXurLLkKq1212PK/view

Steps:

  1. Browse on mobile Chrome to https://material-ui.com/components/popover/
    popover1

  2. Detect the "Simple Popover" demo section, slightly zoom in (device pinch) so that the "Open Popover" button is still visible.
    popover2

  3. Click the "Open Popover" button
    popover3

  4. At this point, the popover is open but the page already scrolled itself up

Context 🔦

We've been using material-ui for our project for quite some time and are very pleased by it. Due to the nature of our project, mobile support has been minimal but users that wished to browse through mobile generally had no difficulties to pan/zoom in order to move around the page.
We made heavy usage of modals, popovers and dialogs, but the current issue prevents users from completing simple actions.
We believe that the issue is quite new and that the new Chrome mobile version might also have something to do with it.

Your Environment 🌎

Our local environment is a bit outdated (React v16.7 & material-ui v3.7.0) but the issue occurs even on the demo website, which leads us to believe that the issue is consistent even on the latest version.

Tech Version
Material-UI v4.4.3 (latest)
React v16.8.0+ (latest)
Browser Google Chrome (mobile) 77.0.3865.92
Device OnePlus 6 (ONEPLUS A6003), but happens on all of the Android and iOS devices we had a chance to test
@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: Popover The React component. labels Sep 30, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 30, 2019

@ilyamerman Thank you for reporting the issue, I can reproduce it. I might have ready seen it a few months ago.

I'm not sure this issue is impacting enough users to justify Material-UI investing a lot of resources in it (given the iOS side had few interactions). However, I think that we have an opportunity to see bigger and solve #17353 and #17636 at the same time. The Material Design specification seems to have given up on the idea of content anchor (getContentAnchorEl), it accounts for a large part of the Popover sources. If we also give up on it, we might be able to use the Popper component to position the Popover and allow scrolling. So we should be able to allow scrolling with the Menu and Select component too. Also, if we are willing to support 12 instead of 81 placements, we can further simplify the logic (antd has 9 placements).

@mui-org/core-contributors What do you think of the idea?

@mbrookes
Copy link
Member

@ilyamerman Thanks for taking the time to submit a thorough issue report!

The Material Design specification seems to have given up on the idea of content anchor

@oliviertassinari Any context for that? I didn't know where to look for what changed.

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 30, 2019

Any context for that? I didn't know where to look for what changed.

@mbrookes Sure!

The current implementation of the Popover inherits the specification from https://material.io/archive/guidelines/components/menus.html#menus-behavior
Capture d’écran 2019-09-30 à 21 24 41
Capture d’écran 2019-09-30 à 21 25 26

While the new specification says the opposite:

https://material.io/components/menus/#exposed-dropdown-menu
Capture d’écran 2019-09-30 à 21 23 45

The "appears over" behavior requires the getContentAnchorEl prop. My hope is that we can remove this requirement (Is this a breaking change 🤔?). Without, it starts to make sense to use a single solution to relatively position two distinct elements. (Popper.js doesn't have this zooming issue)

By the way, the "selected item appears on top of the emitting element" is pretty unique to Material Design (legacy?), I haven't seen it implemented in any of the other Design systems I could encounter.

cc @nathanmarks who implemented it.

@mbrookes
Copy link
Member

The position section adds some nuance:

"They typically appear next to (or in front of) the element that generates them. If they are in a position to be cut off by the browser or screen’s edge, the menu can instead appear to the left, right, or above the element that generates it."

@oliviertassinari
Copy link
Member

This sounds like a behavior that we can support with Popper.
As long as the "selected item appears on top of the emitting element" requirement is gone, we should be able to move forward.

@Bouncue
Copy link

Bouncue commented Oct 1, 2019

By the way, the "appears over" is pretty unique to Material Design (legacy?), I haven't seen it implemented in any of the other Design systems I could encounter.

@oliviertassinari I agree with that, please keep the "appears over" available.

As long as the "selected item appears on top of the emitting element" requirement is gone, we should be able to move forward.

@oliviertassinari Did you mean you want to remove the "appears over" behaviour?

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 1, 2019

@Bouncue sorry, I have updated my comment to be more precise. I think that we have two different concepts. From my understanding, "appears over", or "in front of" is still in the specification, it still wildly used in Android. On the other hand, the "selected item appears on top of the emitting element" behavior is very rare.

@mbrookes
Copy link
Member

mbrookes commented Oct 1, 2019

@Bouncue This would, for example, remove support for: https://material-ui.com/components/menus/#selected-menus

@Bouncue
Copy link

Bouncue commented Oct 1, 2019

oh I get it. I was worried the "appears over", or "in front of" will no longger present in MUI. As for this behaviour https://material-ui.com/components/menus/#selected-menus I didn't realise it was exist. Yes you should good to go... just chase it out of MUI.

@NicholasTuck
Copy link

In terms of "impact" this has a huge impact in our ability to utilize material-ui menus in our transition to a mobile responsive website. We do not have the entire site mobile-responsive, but we get closer by introducing more material/react components to different pages. However until the majority of the page is responsive, the mobile experience will be a zoomed out experience. Thus interacting with any menu will ultimately feel like the site is "broken" because they are panned away and dragging/zooming doesn't work until you think to just "tap" somewhere to unknowingly close the open menu. Considering the prevalence of menu use headers / mobile header experiences, it seems likely this will have high impact.

This really makes it hard for us to get to a mobile responsive experience if our toolset introduces new problems while replacing old UI.

Just wanted to highlight how important this can be for that goal and it is one of our biggest initiatives at our company right now, so it's a big impact.

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 15, 2019

@NicholasTuck Thanks for sharing your story. Material-UI doesn't have the resources to solve all the problems faced by our users, but we try to bet on the topics that have the most potential. Right now, we have a significant focus on building new components and features. I can't give you an ETA on when this problem will be solved.

Notice that your company could support this effort with: https://material-ui.com/getting-started/support/#custom-work

@ffurkk
Copy link

ffurkk commented Oct 21, 2019

Just for notice... Also me have similar problem.

stackoverflow link..

@NicholasTuck
Copy link

Note: This can be replicated on chrome desktop mobile emulator, you do need to set the "zoom" to "auto-adjust zoom" (the drop down of the "100%" has that as the last option, to kick in the foe mobile zooming experience.

@NicholasTuck
Copy link

NicholasTuck commented Oct 22, 2019

I've tried to identify what is causing the actual pan to the corner and I can't pin point it, I've spent a lot of time debugging through. I am suspicious it's not dom manipulation side effects, and that it's specific js calls, but it's hard to determine that with how react implements dom changes and the effect loops.

I was able to programatically implement the hack @ilyamerman mentioned in the original ticket to allow scrolling back over after opening a menu. I did this by adding the following class to the <Menu> element:

const useStyles = makeStyles({
    popover: {
        '& *': {
            touchAction: 'auto !important',     // used to override material adding touch-action to none on the popover
        }
    },
});

...

<Menu
                    id="simple-menu"
                    anchorEl={anchorEl}
                    keepMounted
                    open={Boolean(anchorEl)}
                    onClose={handleClose}
                    className={classes.popover}
                >

@et-nat1995
Copy link

Im not sure anyone else has noticed this and i'm not sure yet if its from the Popover component. Im following what was done in the example thats on the docs. (Docs) When the popover is open and the screen size changes the popover prevents any of the page mobile responsiveness.

i'll look into the issue more, might be something i did. Will follow up once I find out more

@sakekasi
Copy link

Just want to add that I have the same issue. It occurs on desktop chrome when you zoom in with a 2-finger pinch. I think that chrome is recognizing the popover root or the backdrop as the 'main element' that you're looking at and centering that. Or, the popover element itself is scrolling the body. Any thoughts on how to disable if the latter is the case?

@NicholasTuck
Copy link

NicholasTuck commented Nov 20, 2019

We ended up re-making the menu component to look the same and behave the same (or as close as we could). Here is the code we made for our Menu component to work around this problem:

import ClickAwayListener from '@material-ui/core/ClickAwayListener';
import {Box} from '@material-ui/core';
import Typography from '@material-ui/core/Typography';
import Popper from '@material-ui/core/Popper';
import Paper from '@material-ui/core/Paper';
import * as PropTypes from 'prop-types';
import React from 'react';

export function Menu({id, children, onClose, launchElement}) {

    const handleClose = (event) => {
        event.stopPropagation();
        setAnchorEl(null);
        if (onClose) {
            onClose(event);
        }
    };

    const [anchorEl, setAnchorEl] = React.useState(null);

    const toggleMenu = event => {
        setAnchorEl(anchorEl ? null : event.currentTarget);
    };

    const open = Boolean(anchorEl);

    return <Box my={4}>
        <ClickAwayListener onClickAway={handleClose} touchEvent={false} >
            <Box display='flex' flexDirection='column' justifyContent='center' alignItems='center'>
                {React.cloneElement(launchElement, {onClick: toggleMenu})}
                <Popper id={id} open={open} anchorEl={anchorEl} popperOptions={{positionFixed: true}} style={{zIndex: 100}}>
                    <Paper elevation={8}>
                        {children}
                    </Paper>
                </Popper>
            </Box>
        </ClickAwayListener>
    </Box>;
}

Menu.propTypes = {
    children: PropTypes.arrayOf(PropTypes.element).isRequired,
    launchElement: PropTypes.element.isRequired,
    id: PropTypes.string,
    onClose: PropTypes.func
};

LaunchElement is the button or link or wtv that you want to cause the menu to open.

To use the Menu:


const button =
        <Button aria-controls={menuId} aria-haspopup="true" >
           Open Menu
        </Button>;

<Menu id={menuId} launchElement={button}>
            <MenuItem onClick={()=>{console.log('here');}}>Profile</MenuItem>
            <MenuItem onClick={()=> {alert('Clicked My Account');}}>My account</MenuItem>
            <MenuItem >Logout</MenuItem>
        </Menu>

@thanmaic
Copy link

@oliviertassinari Any update on this issue? When i set my browser zoom/display setting on my laptop to any value less than 100%, the position of popover is wrong.
Material UI: ^4.0.0
React: ^17.0.1
Browser: On chrome, version 91

My document.body.style.zoom is at 75%. It works as expected when the value is 100%, but once it is decreased or increased the position of popover is wrong.
Tried both Menu and Popover components for the same, but it doesn't work.
In case i tried to position the anchor through anchorPosition prop, calculating the left % is very difficult for all laptop resolutions.

<Popover
        id="menu-appbar"
        anchorEl={anchorEl}
        getContentAnchorEl={null}
        anchorOrigin={{ vertical: "bottom", horizontal: "center" }}
        transformOrigin={{ vertical: "top", horizontal: "center" }}
        open={openMenu}
        onClose={menuClose}
        // anchorReference="anchorPosition"
        // anchorPosition={{ top: "5%", left: "75%" }}
      >
        {menus &&
          menus.map((item, index) => (
            <div
              onClick={item.onClick}
              key={index}
              className={classes.menuItemNew}
            >
              <span className={classes.mFontNew}>{item.displayText}</span>
            </div>
          ))}
 </Popover>

Any work around or alternate to make it working as of now?

@andylacko
Copy link

still have this issue today, pinch zoom on mac desktop

chrome
safari same behaviour

this is pretty annoying bug, does anyone have a workaround?

or do you suggest that I should use popper?

image

@JahnoelRondon
Copy link

I love mui until I run into such trivial issues like this...something that should be an easy fix but isnt. They have it working in their docs but it doesnt explain how.
https://mui.com/material-ui/react-menu/ in the section (MenuList Composition) with Popover.

@samirp91
Copy link

@oliviertassinari Any ETA when this will be fixed or any workaround?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: Popover The React component. priority: important This change can make a difference
Projects
None yet
Development

No branches or pull requests