Skip to content

fix: Popover toggle #35746

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Sep 11, 2025
+49 −37
Merged

fix: Popover toggle #35746

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0a498bd
WIP
n3ps Sep 7, 2025
b9b1329
fix: Popover toggle via ref element
n3ps Sep 8, 2025
61f81d5
Fixed failing test
hjetpoluru Sep 9, 2025
b5bdf6e
Update ui/components/multichain/app-header/app-header-unlocked-conten…
n3ps Sep 9, 2025
0dc8661
Change return value to isMenuOpen in state update
n3ps Sep 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions test/e2e/page-objects/pages/header-navbar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,11 @@ class HeaderNavbar {
}
}

async mouseClickOnThreeDotMenu(): Promise<void> {
console.log('Clicking three dot menu using mouse move');
await this.driver.clickElementUsingMouseMove(this.threeDotMenuButton);
}

async openPermissionsPage(): Promise<void> {
console.log('Open permissions page in header navbar');
await this.openThreeDotMenu();
Expand Down Expand Up @@ -140,12 +145,12 @@ class HeaderNavbar {

async clickNotificationsOptions(): Promise<void> {
console.log('Click notifications options');
await this.openThreeDotMenu();
await this.mouseClickOnThreeDotMenu();
await this.driver.clickElement(this.notificationsButton);
}

async checkNotificationCountInMenuOption(count: number): Promise<void> {
await this.openThreeDotMenu();
await this.mouseClickOnThreeDotMenu();
await this.driver.findElement({
css: this.notificationCountOption,
text: count.toString(),
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/snaps/test-snap-cronjob-duration.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ describe('Test Snap Cronjob Duration', function () {
await headerNavbar.checkNotificationCountInMenuOption(1);

// This click will close the menu.
await headerNavbar.openThreeDotMenu();
await headerNavbar.mouseClickOnThreeDotMenu();
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like openThreeDotMenu accounts for differences on Firefox vs. Chrome. Do we not need to do that anymore?

Copy link
Contributor Author

@n3ps n3ps Sep 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The original test had a flaw itself, it called both methods when only 1 click really mattered because of the popover bug


// Click the notification options and validate the message in the
// notification list.
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/snaps/test-snap-notification.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ describe('Test Snap Notification', function () {
await headerNavbar.checkNotificationCountInMenuOption(1);

// this click will close the menu
await headerNavbar.openThreeDotMenu();
await headerNavbar.mouseClickOnThreeDotMenu();

// click the notification options
await headerNavbar.clickNotificationsOptions();
Expand Down
41 changes: 21 additions & 20 deletions ui/components/component-library/popover/popover.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,29 +86,30 @@ export const Popover: PopoverComponent = React.forwardRef(
width: matchWidth ? referenceElement?.clientWidth : 'auto',
};

// Esc key press
const handleEscKey = (event: KeyboardEvent) => {
if (event.key === 'Escape') {
// Close the popover when the "Esc" key is pressed
if (onPressEscKey) {
onPressEscKey();
useEffect(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. The event handlers where used in the useEffect but not in the deps array

// Esc key press
const handleEscKey = (event: KeyboardEvent) => {
if (event.key === 'Escape') {
// Close the popover when the "Esc" key is pressed
if (onPressEscKey) {
onPressEscKey();
}
}
}
};
};

const handleClickOutside = (event: MouseEvent) => {
if (
isOpen &&
popoverRef.current &&
!popoverRef.current.contains(event.target as Node)
) {
if (onClickOutside) {
onClickOutside();
const handleClickOutside = (event: MouseEvent) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. So we move them inside

if (
isOpen &&
popoverRef.current &&
!popoverRef.current.contains(event.target as Node) &&
!referenceElement?.contains(event.target as Node)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. The trigger (reference) element was not being checked

) {
if (onClickOutside) {
onClickOutside();
}
}
}
};
};

useEffect(() => {
document.addEventListener('keydown', handleEscKey, { capture: true });
if (isOpen) {
document.addEventListener('click', handleClickOutside, {
Expand All @@ -122,7 +123,7 @@ export const Popover: PopoverComponent = React.forwardRef(
document.removeEventListener('keydown', handleEscKey);
document.removeEventListener('click', handleClickOutside);
};
}, [onPressEscKey, isOpen, onClickOutside]);
}, [onPressEscKey, isOpen, onClickOutside, referenceElement]);

const PopoverContent = (
<Box
Expand Down
32 changes: 19 additions & 13 deletions ui/components/multichain/app-header/app-header-unlocked-content.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,15 +139,21 @@ export const AppHeaderUnlockedContent = ({
origin &&
origin !== browser.runtime.id;

const handleMainMenuOpened = () => {
trackEvent({
event: MetaMetricsEventName.NavMainMenuOpened,
category: MetaMetricsEventCategory.Navigation,
properties: {
location: 'Home',
},
const handleMainMenuToggle = () => {
setAccountOptionsMenuOpen((previous) => {
const isMenuOpen = !previous;
if (isMenuOpen) {
trackEvent({
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we creating a MetaMetrics event in a React state change handler? Shouldn't we do that afterward?

Copy link
Contributor Author

@n3ps n3ps Sep 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@mcmire That is existing prod code - see line 143, tracking whenever the menu opens

Was trying to keep the focus of this PR about fixing the toggle

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha. Does this not work?

setAccountOptionsMenuOpen((previous) => !previous);
if (accountOptionsMenuOpen) {
  trackEvent({
    event: MetaMetricsEventName.NavMainMenuOpened,
    category: MetaMetricsEventCategory.Navigation,
    properties: {
      location: 'Home',
    },
  });
}

Or does it make sense to move the tracking into a useEffect?

Copy link
Contributor

Choose a reason for hiding this comment

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

(This is non-blocking, I will let you decide, but just wanted to point it out in case there is a better way to do this)

Copy link
Contributor Author

@n3ps n3ps Sep 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Calling

if (accountOptionsMenuOpen) {
  trackEvent({
    event: MetaMetricsEventName.NavMainMenuOpened,
    category: MetaMetricsEventCategory.Navigation,
    properties: {
      location: 'Home',
    },
  });
}

directly like that will skew the tracking especially knowing the extension has existing excessive re-renders

useEffect might work but again susceptible to re-renders

IMO safest option without a full rewrite of this component and popover is the setState and logically close to the button click event

I mean we could also put events on the Menu component itself when it renders but I think these are for another PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay no problem!

event: MetaMetricsEventName.NavMainMenuOpened,
category: MetaMetricsEventCategory.Navigation,
properties: {
location: 'Home',
},
});
}

return isMenuOpen;
});
setAccountOptionsMenuOpen(true);
};

const handleConnectionsRoute = () => {
Expand Down Expand Up @@ -287,25 +293,25 @@ export const AppHeaderUnlockedContent = ({
style={{ position: 'relative' }}
>
{!accountOptionsMenuOpen && (
<Box onClick={() => handleMainMenuOpened()}>
<Box onClick={handleMainMenuToggle}>
<NotificationsTagCounter noLabel />
</Box>
)}
<ButtonIcon
iconName={IconName.Menu}
data-testid="account-options-menu-button"
ariaLabel={t('accountOptions')}
onClick={() => {
handleMainMenuOpened();
}}
onClick={handleMainMenuToggle}
size={ButtonIconSize.Lg}
/>
</Box>
</Box>
<GlobalMenu
anchorElement={menuRef.current}
isOpen={accountOptionsMenuOpen}
closeMenu={() => setAccountOptionsMenuOpen(false)}
closeMenu={() => {
setAccountOptionsMenuOpen(false);
}}
/>
<VisitSupportDataConsentModal
isOpen={showSupportDataConsentModal}
Expand Down
Loading