Material-ui: Warn when Popper/Popover anchorRef is not visible
If I put menu with button inside a component and use it with <Component /> syntax, popOver has wrong position.
With {Component()} syntax everything is ok.
Could somebody explain my why is that?
- [x] This is not a v0.x issue.
- [x] I have searched the issues of this repository and believe that this is not a duplicate.
Steps to Reproduce 馃暪
https://codesandbox.io/s/n3ny4r5qvm
Your Environment 馃寧
| Tech | Version |
|--------------|---------|
| Material-UI | v3.9.2 |
| React | 16.8.4 |
| Browser | Brave 0.61.52 Chromium: 73.0.3683.86 (Official Build) (64-bit) |
| TypeScript | 3.3.3333 |
| etc. | |
kifirkin
All 18 comments
You're defining a new component with every render call which will remount that component on every render call. It's pretty tricky to wrap my head around this. I would recommend you move the component definition outside of the render call like so:
import React from "react";
import Button from "@material-ui/core/Button";
import Menu from "@material-ui/core/Menu";
import MenuItem from "@material-ui/core/MenuItem";
const Head = ({ anchorEl, onClick, onClose, title }) => {
return (
<div>
<Button
aria-owns={anchorEl ? "simple-menu" : undefined}
aria-haspopup="true"
onClick={onClick}
>
{title}
</Button>
<Menu
id="simple-menu"
anchorEl={anchorEl}
open={Boolean(anchorEl)}
onClose={onClose}
>
<MenuItem onClick={onClose}>Profile</MenuItem>
<MenuItem onClick={onClose}>My account</MenuItem>
<MenuItem onClick={onClose}>Logout</MenuItem>
</Menu>
</div>
);
};
function SimpleMenu() {
const [anchorEl, setAnchorEl] = React.useState(null);
function handleClick(event) {
setAnchorEl(event.currentTarget);
}
function handleClose() {
setAnchorEl(null);
}
return (
<div style={{ float: "right" }}>
<Head
anchorEl={anchorEl}
onClick={handleClick}
onClose={handleClose}
title="All good"
/>
</div>
);
}
export default SimpleMenu;
eps1lon
on 22 Mar 2019
@eps1lon This issue remembers me a discussing we had with an invisible tooltip. If we console log the output of anchorEl.getBoundingClientRect() we get a none visible node:
I'm in favor of raising a warning.
For reference: https://github.com/twbs/bootstrap/blob/v4.3.1/js/src/tooltip.js#L251.
@alexmironof So what happens? You are creating a new component. React unmount the old one and mount the new component. React removes the DOM node, and insert a new one. So the anchorEl you set from event.currentTarget is no longer in the DOM. The menu can't use the prop to correctly position itself.
oliviertassinari
on 22 Mar 2019
The workaround:
import React from "react";
import Button from "@material-ui/core/Button";
import Menu from "@material-ui/core/Menu";
import MenuItem from "@material-ui/core/MenuItem";
function SimpleMenu() {
+ const buttonRef = React.useRef();
const [anchorEl, setAnchorEl] = React.useState(null);
function handleClick(event) {
- setAnchorEl(event.currentTarget);
+ setAnchorEl(true);
}
function handleClose() {
setAnchorEl(null);
}
const Head = ({ title }) => {
return (
<div>
<Button
aria-owns={anchorEl ? "simple-menu" : undefined}
aria-haspopup="true"
onClick={handleClick}
+ ref={buttonRef}
>
{title}
</Button>
<Menu
id="simple-menu"
- anchorEl={anchorEl}
+ anchorEl={() => buttonRef.current}
open={Boolean(anchorEl)}
onClose={handleClose}
>
<MenuItem onClick={handleClose}>Profile</MenuItem>
<MenuItem onClick={handleClose}>My account</MenuItem>
<MenuItem onClick={handleClose}>Logout</MenuItem>
</Menu>
</div>
);
};
return (
<div style={{ float: "right" }}>
{Head({ title: "Niceee 馃憣" })}
{/* Whats wrong here? */}
<Head title="Ooops! 馃槺" />
</div>
);
}
export default SimpleMenu;
This is a very obscure pattern. I don't think we should document how one would work with components defined in render. It's a pretty wasteful pattern. I couldn't find it in our docs and wouldn't like to see this introduced.
eps1lon
on 22 Mar 2019
Yeah, we might not need a documentation section for it. What do you think of the warning part?
oliviertassinari
on 22 Mar 2019
Yeah, we might not need a documentation section for it. What do you think of the warning part?
Sounds useful for dev-only.
eps1lon
on 22 Mar 2019
We share the same vision then. If somebody wants to look into adding a warning for this problem, we would be happy to review it. I have personally face this issue on multiple occasions in the past. It was always an issue with my code
oliviertassinari
on 22 Mar 2019
@oliviertassinari @eps1lon Thanks for the explanation! It just flew out of my head :sweat_smile:
I'll make a PR with warning
kifirkin
on 25 Mar 2019
@alexmironof Awesome :)
oliviertassinari
on 25 Mar 2019
If i provide anchorOrigin={{ vertical: "bottom" }} prop to Menu component, I'm getting to many warnings :cry:
kifirkin
on 28 Mar 2019
@alexmironof The resolution is in the warning message.
oliviertassinari
on 28 Mar 2019
@oliviertassinari :sweat_smile: I want to say that every warning inside Popover component logs twice, and if I add new warning about unmounted anchorRef that looks little bit weird
kifirkin
on 28 Mar 2019
@alexmironof You are right, thank you for clarifying it.
oliviertassinari
on 28 Mar 2019
Couldn't agree with anchorEl at all, I can't imagine myself writing a createRef every time I want to use the component with a custom button. This should have been handled by the component itself, imagine how much less trouble and pain would that be, perhaps save you from writing a few lines of code
nelsieborja
on 11 Apr 2019
@nelsieborja
This should have been handled by the component itself, imagine how much less trouble and pain would that be, perhaps save you from writing a few lines of code
If you pass a ref when is the Popper or Popover supposed to know when the element is available/changes? By using a prop we can delegate the diffing to React and the developer is in full control when the element is available.
This decision will save us a lot of time once lazy components become more prevalent or Concurrent mode becomes stable. All components using an anchorEl prop would break if the element in the ref is not actually available after mounting or changes during the component lifetime.
If you can come up with a bullet proof approach that works with ref I'm happy to review this. Until then I don't see an issue with handling this yourself. If you feel like you're repeating logic very often you can always extract this into a hook or a component and share it with the community. It would be perfect to include this in the simple Popper/Popover demo if it really covers most of the use cases.
eps1lon
on 11 Apr 2019
I can't imagine myself writing a createRef every time I want to use the component
@nelsieborja It's why this project https://github.com/jcoreio/material-ui-popup-state exists. What do you think about it?
@jedwards1211 has added a new hook API, we should be able to update our demos.
oliviertassinari
on 11 Apr 2019
There are 2 approaches that I can think of:
Wrap the button and popover/menu inside a parent container. This way it is easier to track whether the click is triggered within or outside the container. But obviously this will cause some layout issue, because the container will override the styles attached to the button
Using
callback refconsidering you already have access on the ref of the button. Because most likely, you will be using the Popover or Menu component with a Button component.
Using Menu with a ButtonIcon
const MenuButton = (
<ButtonIcon>
<Home />
</ButtonIcon>
);
<Menu button={MenuButton} list={...} />
ButtonIcon component
function ButtonIcon({ forwardRef, ...rest }) {
return <ButtonIconStyled ref={forwardRef} {...rest} />
}
Menu component
function Menu({ button, ...rest }) {
const buttonEl = useRef();
...
return (
<>
{button && <button.type forwardRef={buttonEl} ... />}
<MenuStyled ... />
</>
)
}
Of course, this will require some extra checks
nelsieborja
on 14 Apr 2019
@nelsieborja It's usually not necessary to worry about refs because you can get anchorEl from the click event's currentTarget instead. The only exception to this is when you want to position the menu in a totally unrelated place, or when you want something besides a DOM event to trigger it.
I think the way my https://github.com/jcoreio/material-ui-popup-state works really will be to your liking. This is what the code looks like with the hooks API:
import * as React from 'react'
import Button from '@material-ui/core/Button'
import Menu from '@material-ui/core/Menu'
import MenuItem from '@material-ui/core/MenuItem'
import {
usePopupState,
bindTrigger,
bindMenu,
} from 'material-ui-popup-state/hooks'
const MenuPopupState = () => {
const popupState = usePopupState({ variant: 'popover', popupId: 'demoMenu' })
return (
<div>
<Button variant="contained" {...bindTrigger(popupState)}>
Open Menu
</Button>
<Menu {...bindMenu(popupState)}>
<MenuItem onClick={popupState.close}>Cake</MenuItem>
<MenuItem onClick={popupState.close}>Death</MenuItem>
</Menu>
</div>
)
}
export default MenuPopupState
jedwards1211
on 14 Apr 2019
Related issues
iamzhouyi
路
3Comments
newoga
路
3Comments
mattmiddlesworth
路
3Comments
pola88
路
3Comments
activatedgeek
路
3Comments
Most helpful comment
The workaround: