From 39158e4c79c67a7ae42c7f46713aaed161669ca1 Mon Sep 17 00:00:00 2001 From: AK <144495202+AKnassa@users.noreply.github.com> Date: Sat, 1 Aug 2026 03:29:57 -0400 Subject: [PATCH] fix(react-menu): let setTarget win for context menus For openOnContext menus, useMenu stored the right-click mouse position in state and passed it as the positioning target option. usePositioning re-syncs that option into its override ref in a layout effect at commit time, so a user's imperative positioningRef.setTarget() (e.g. anchoring a Shift+F10 context menu to the focused row) was always clobbered. Set the context target imperatively instead, through an internal positioning ref merged with the user's, and stop passing the state-driven target option (the state stays in sync because MenuState.contextTarget is public). An explicit positioning.target from props still wins. onOpenChange now fires after the internal context target is set, so a setTarget call inside it takes precedence - the ordering endorsed in the issue discussion. Covered by new jsdom tests (imperative target wins; mouse anchoring preserved; trigger restored on close) and Cypress geometry tests that reproduced the bug in a real browser before the fix. Full react-menu (496) and react-positioning (199) suites green. The same latent pattern exists in react-popover and the headless Menu copy - follow-up candidates. Fixes #31727 --- ...-95ffb33e-1931-4cbb-a449-7449ddee609a.json | 7 ++ .../library/src/components/Menu/Menu.cy.tsx | 73 +++++++++++++ .../Menu/MenuContextTarget.test.tsx | 100 ++++++++++++++++++ .../library/src/components/Menu/useMenu.tsx | 34 +++++- 4 files changed, 211 insertions(+), 3 deletions(-) create mode 100644 change/@fluentui-react-menu-95ffb33e-1931-4cbb-a449-7449ddee609a.json create mode 100644 packages/react-components/react-menu/library/src/components/Menu/MenuContextTarget.test.tsx diff --git a/change/@fluentui-react-menu-95ffb33e-1931-4cbb-a449-7449ddee609a.json b/change/@fluentui-react-menu-95ffb33e-1931-4cbb-a449-7449ddee609a.json new file mode 100644 index 00000000000000..50103b5fc004a4 --- /dev/null +++ b/change/@fluentui-react-menu-95ffb33e-1931-4cbb-a449-7449ddee609a.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "fix: Menu with openOnContext no longer overrides an imperative positioningRef.setTarget() call made in onOpenChange; the internal context target is now set imperatively and onOpenChange fires after it", + "packageName": "@fluentui/react-menu", + "email": "144495202+AKnassa@users.noreply.github.com", + "dependentChangeType": "patch" +} diff --git a/packages/react-components/react-menu/library/src/components/Menu/Menu.cy.tsx b/packages/react-components/react-menu/library/src/components/Menu/Menu.cy.tsx index 96f3fe80d333bc..9450820de7c9c3 100644 --- a/packages/react-components/react-menu/library/src/components/Menu/Menu.cy.tsx +++ b/packages/react-components/react-menu/library/src/components/Menu/Menu.cy.tsx @@ -17,6 +17,7 @@ import { } from '@fluentui/react-menu'; import { FluentProvider } from '@fluentui/react-provider'; import { Portal } from '@fluentui/react-portal'; +import type { PositioningImperativeRef } from '@fluentui/react-positioning'; import { teamsLightTheme } from '@fluentui/react-theme'; import * as React from 'react'; @@ -1129,6 +1130,78 @@ describe('Context menu', () => { .get(menuTriggerSelector) .should('have.focus'); }); + + it('should anchor to the mouse position on right click', () => { + mount(); + + cy.get(menuTriggerSelector).then(([trigger]) => { + const triggerRect = trigger.getBoundingClientRect(); + + // The positioned element is the popover surface (the parent of [role="menu"]) + cy.get(menuTriggerSelector) + .rightclick(10, 10) + .get(menuSelector) + .parent() + .should(([popover]) => { + const popoverRect = popover.getBoundingClientRect(); + // The context target is a 1px virtual element at the click point, the menu opens below-start of it + expect(popoverRect.left).to.be.closeTo(triggerRect.left + 10, 4); + expect(popoverRect.top).to.be.closeTo(triggerRect.top + 10 + 1, 4); + }); + }); + }); + + // https://github.com/microsoft/fluentui/issues/31727 + it('should not override an imperative positioningRef.setTarget() called from onOpenChange', () => { + const ImperativeTargetContextMenuExample = () => { + const positioningRef = React.useRef(null); + + return ( + <> +
+ { + if (data.open) { + positioningRef.current?.setTarget(document.getElementById('custom-anchor')); + } + }} + > + + + + + + Item + + + + + ); + }; + + mount(); + + cy.get(menuTriggerSelector).rightclick(5, 5); + + cy.get('#custom-anchor').then(([anchor]) => { + const anchorRect = anchor.getBoundingClientRect(); + + // The positioned element is the popover surface (the parent of [role="menu"]) + cy.get(menuSelector) + .parent() + .should(([popover]) => { + const popoverRect = popover.getBoundingClientRect(); + // The menu should be anchored below-start of the imperatively set target, not at the click point + expect(popoverRect.left).to.be.closeTo(anchorRect.left, 4); + expect(popoverRect.top).to.be.closeTo(anchorRect.bottom, 4); + }); + }); + }); }); /** diff --git a/packages/react-components/react-menu/library/src/components/Menu/MenuContextTarget.test.tsx b/packages/react-components/react-menu/library/src/components/Menu/MenuContextTarget.test.tsx new file mode 100644 index 00000000000000..7362d210dc9889 --- /dev/null +++ b/packages/react-components/react-menu/library/src/components/Menu/MenuContextTarget.test.tsx @@ -0,0 +1,100 @@ +import * as React from 'react'; +import { render, fireEvent } from '@testing-library/react'; +import type { PositioningImperativeRef, PositioningVirtualElement } from '@fluentui/react-positioning'; +import { Menu } from './Menu'; +import { MenuTrigger } from '../MenuTrigger/index'; +import { MenuList } from '../MenuList/index'; +import { MenuItem } from '../MenuItem/index'; +import { MenuPopover } from '../MenuPopover/index'; + +// The internal position manager is mocked at its resolved source path so that the effective +// positioning target can be asserted in jsdom. usePositioning imports './createPositionManager' +// relatively, which resolves to the same module registry entry as this deep relative path. +import { createPositionManager } from '../../../../../react-positioning/library/src/createPositionManager'; + +jest.mock('../../../../../react-positioning/library/src/createPositionManager', () => ({ + createPositionManager: jest.fn(() => ({ updatePosition: jest.fn(), dispose: jest.fn() })), +})); + +const lastTarget = () => { + const calls = (createPositionManager as jest.Mock).mock.calls; + return calls[calls.length - 1][0].target; +}; + +describe('Menu openOnContext positioning target', () => { + // Regression test for https://github.com/microsoft/fluentui/issues/31727 + it('does not clobber an imperative positioningRef.setTarget() called from onOpenChange', () => { + const anchor = document.createElement('div'); + document.body.appendChild(anchor); + const positioningRef = React.createRef(); + + const { getByRole } = render( + { + if (data.open) { + positioningRef.current?.setTarget(anchor); + } + }} + > + + + + + + Item + + + , + ); + + fireEvent.contextMenu(getByRole('button'), { clientX: 42, clientY: 24 }); + + expect(lastTarget()).toBe(anchor); + + anchor.remove(); + }); + + it('anchors to the mouse position on right click by default', () => { + const { getByRole } = render( + + + + + + + Item + + + , + ); + + fireEvent.contextMenu(getByRole('button'), { clientX: 42, clientY: 24 }); + + const target = lastTarget() as PositioningVirtualElement; + expect(target.getBoundingClientRect().x).toBe(42); + expect(target.getBoundingClientRect().y).toBe(24); + }); + + it('restores the trigger as positioning target when the context menu closes', () => { + const { getByRole } = render( + + + + + + + Item + + + , + ); + + const trigger = getByRole('button'); + fireEvent.contextMenu(trigger, { clientX: 42, clientY: 24 }); + fireEvent.click(getByRole('menuitem')); + + expect(lastTarget()).toBe(trigger); + }); +}); diff --git a/packages/react-components/react-menu/library/src/components/Menu/useMenu.tsx b/packages/react-components/react-menu/library/src/components/Menu/useMenu.tsx index 72123147238176..1e2898b6d2c0a4 100644 --- a/packages/react-components/react-menu/library/src/components/Menu/useMenu.tsx +++ b/packages/react-components/react-menu/library/src/components/Menu/useMenu.tsx @@ -7,7 +7,10 @@ import { usePositioning, useSafeZoneArea, usePositioningSlideDirection, + createVirtualElementFromClick, type PositioningShorthandValue, + type PositioningImperativeRef, + type SetVirtualMouseTarget, } from '@fluentui/react-positioning'; import { presenceMotionSlot } from '@fluentui/react-motion'; import { @@ -102,9 +105,29 @@ export const useMenuBase_unstable = ( const { targetDocument } = useFluent(); const triggerId = useId('menu'); - const [contextTarget, setContextTarget] = usePositioningMouseTarget(); + const [contextTarget, setContextTargetState] = usePositioningMouseTarget(); const resolvedPositioning = resolvePositioningShorthand(props.positioning); + const internalPositioningRef = React.useRef(null); + const positioningRef = useMergedRefs(internalPositioningRef, resolvedPositioning.positioningRef); + + // Sets the context target imperatively so that a later user positioningRef.setTarget() call is not + // clobbered by a state-driven `target` option re-sync (https://github.com/microsoft/fluentui/issues/31727). + // The legacy contextTarget state is kept in sync for consumers of MenuState.contextTarget. + const setContextTarget: SetVirtualMouseTarget = useEventCallback(event => { + setContextTargetState(event); + + // Preserve existing precedence: an explicit `positioning.target` from the user always wins + if (openOnContext && !('target' in resolvedPositioning)) { + if (event === undefined || event === null) { + internalPositioningRef.current?.setTarget(null); + } else { + const nativeEvent = event instanceof MouseEvent ? event : event.nativeEvent; + internalPositioningRef.current?.setTarget(createVirtualElementFromClick(nativeEvent)); + } + } + }); + const handlePositionEnd = usePositioningSlideDirection({ targetDocument, onPositioningEnd: resolvedPositioning.onPositioningEnd, @@ -113,10 +136,10 @@ export const useMenuBase_unstable = ( const positioningOptions = { position: isSubmenu ? 'after' : 'below', align: isSubmenu ? 'top' : 'start', - target: props.openOnContext ? contextTarget : undefined, fallbackPositions: isSubmenu ? submenuFallbackPositions : undefined, ...resolvedPositioning, onPositioningEnd: handlePositionEnd, + positioningRef, } as const; const children = React.Children.toArray(props.children) as React.ReactElement[]; @@ -286,7 +309,7 @@ const useMenuOpenState = ( const trySetOpen = useEventCallback((e: MenuOpenEvent, data: MenuOpenChangeData) => { const event = e instanceof CustomEvent && e.type === MENU_ENTER_EVENT ? e.detail.nativeEvent : e; - onOpenChange?.(event, { ...data }); + if (data.open && e.type === 'contextmenu') { state.setContextTarget(e as React.MouseEvent); } @@ -295,6 +318,11 @@ const useMenuOpenState = ( state.setContextTarget(undefined); } + // Fire the user's onOpenChange only after the internal context target is set so that an + // imperative positioningRef.setTarget() call inside onOpenChange takes precedence + // (https://github.com/microsoft/fluentui/issues/31727). + onOpenChange?.(event, { ...data }); + if (data.bubble) { parentSetOpen(e, { ...data }); }