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

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

eduter
Copy link

@eduter eduter commented Jan 17, 2025

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;
}

Signed-off-by: eduter <[email protected]>
@mui-bot
Copy link

mui-bot commented Jan 17, 2025

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 0660030

@eduter eduter changed the title Fix oversight in PopoverProps.anchorEl type [material-ui][Popover] Fix oversight in PopoverProps.anchorEl type Jan 17, 2025
@zannager zannager added the component: Popover The React component. label Jan 17, 2025
@zannager zannager requested a review from DiegoAndai January 17, 2025 14:18
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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 ZeeshanTamboli changed the title [material-ui][Popover] Fix oversight in PopoverProps.anchorEl type [material-ui][Popover] Allow null in anchorEl function return type Jan 29, 2025
@ZeeshanTamboli ZeeshanTamboli added the package: material-ui Specific to @mui/material label Jan 29, 2025
@ZeeshanTamboli ZeeshanTamboli added v6.x needs cherry-pick The PR should be cherry-picked to master after merge labels Jan 29, 2025
@ZeeshanTamboli
Copy link
Member

@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:

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.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Popover The React component. needs cherry-pick The PR should be cherry-picked to master after merge package: material-ui Specific to @mui/material PR: out-of-date The pull request has merge conflicts and can't be merged typescript v6.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants