[material-ui][Popover] Allow null in anchorEl function return type

[eduter]

I'm using a ref for a button as the anchorEl, but this forces me to, either:

1. Access ref.current during the rendering phase, breaking one of the Rules of React, about idempotency and making the React Compiler bail on my component.
2. Use a function that returns the anchor element, giving me a TypeScript error.

Looking at the implementation of resolveAnchorEl, I can see that, in practice, it doesn't matter if the null was passed directly or if it's the return value of a function. The type of the prop is inconsistent with the implementation.

This is how I ran into this problem:

export default function PopoverButton({ isOpen, open, close }: PopoverButtonProps) {
  const buttonRef = useRef<HTMLButtonElement>(null);

  return (
    <>
      <Button
        ref={buttonRef}
        onClick={open}
      >
        Open Popover
      </Button>
      <Popover
        open={isOpen}
        onClose={close}
        // Causes React rule violation
        // anchorEl={buttonRef.current}
        // Causes TypeScript error because buttonRef.current can be null
        anchorEl={() => buttonRef.current}
      >
        <div>Popover Content</div>
      </Popover>
    </>
  );
}

interface PopoverButtonProps {
  isOpen: boolean;
  open: () => void;
  close: () => void;
}

[mui-bot]

Netlify deploy preview

https://deploy-preview-45045--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against a8ccc89

[ZeeshanTamboli]

@eduter Why not use state to set the anchor element when the button is clicked, as shown in the Popover docs: https://mui.com/material-ui/react-popover/#basic-popover? This avoids React rule violations since state updates are idempotent.

[DiegoAndai]

Thanks for the contribution @eduter!

---

Why not use state to set the anchor element when the button is clicked, as shown in the Popover docs: https://mui.com/material-ui/react-popover/#basic-popover?

I agree that following the docs is the way to go.

That said, this change does make sense to me. The function's returning null would be valid as well.

The only section of the code in which this type change might have unintended consequences is:

material-ui/packages/mui-material/src/Popover/Popover.js

Line 359 in cb2a191

const containerWindow = ownerWindow(anchorEl);

As anchorEl is used directly, and not through resolveAnchorEl. I think this line might be incorrect. @eduter may I ask you to investigate the history a bit and see if there's a reason not to use the resolved anchor element there?

---

@eduter may I ask you to add a spec test for this updated type?

[ZeeshanTamboli]

@DiegoAndai I took over this PR since the author hasn’t responded.

The only section of the code in which this type change might have unintended consequences is:

material-ui/packages/mui-material/src/Popover/Popover.js

Line 359 in cb2a191

const containerWindow = ownerWindow(anchorEl);

As anchorEl is used directly, and not through resolveAnchorEl. I think this line might be incorrect. @eduter may I ask you to investigate the history a bit and see if there's a reason not to use the resolved anchor element there?

It was added in #22159. Resolving the anchor element function was already introduced until then. I think this was just missed. A separate PR would be better for this fix. What do you think?

@eduter may I ask you to add a spec test for this updated type?

I added it.

---

I looked into why anchorEl supports a function type. It prevents unnecessary re-renders when using ref for anchor element (see #10411). Without it, the popover wouldn't attach correctly. Specifically, #10411 (comment) describes the same issue @eduter mentioned in the PR.

[ZeeshanTamboli]

I updated the code to resolve anchorElement when it's a function on the missed line mentioned here.

Base UI also supports null as the function return type for anchorEl: https://base-ui.com/react/components/popover

Looks good to merge.

[github-actions]

Cherry-pick PRs will be created targeting branches: v6.x