microsoft / fluentui

Fluent UI web represents a collection of utilities, React components, and web components for building web applications.
https://react.fluentui.dev
Other
18.26k stars 2.7k forks source link

[Bug]: imperative set target on context menu does not work #31727

Open YuanboXue-Amber opened 2 months ago

YuanboXue-Amber commented 2 months ago

Library

React Components / v9 (@fluentui/react-components)

System Info

v9.54.2

Are you reporting Accessibility issue?

None

Reproduction

https://stackblitz.com/edit/1xfnw4?file=src%2Fexample.tsx,src%2FApp.tsx

Bug Description

Actual/Expected Behavior

Open the reproduction in Edge and focus on red box, on pressing shift+f10, expect menu to be anchored to "Target" button. But in actual menu is anchored to red box.

This bug is discovered when trying to anchor a context menu on TreeItem with subtree. On mouse click context menu should follow the mouse, and on keyboard, context menu should anchor to TreeItemLayout.

Investigation and Proposed fix

Menu openOnContext sets target through state. It overrides the target set imperatively by user. (So there's a workaround to this issue by setting target imperatively in useEffect on menu open) After discussing with @ling1726, on way to fix it can be set target imperatively instead of setting state in useMenu.

Logs

No response

Requested priority

High

Products/sites affected

No response

Are you willing to submit a PR to fix?

no

Validations

ling1726 commented 2 months ago

The target for this feature was set when imperative setTarget did not exist https://github.com/microsoft/fluentui/blob/5139ad8299071bb70681a140ce52b12cc24763fc/packages/react-components/react-menu/library/src/components/Menu/useMenu.tsx#L63

The imperative target always loses to the target prop since the prop is updated in an effect https://github.com/microsoft/fluentui/blob/5139ad8299071bb70681a140ce52b12cc24763fc/packages/react-components/react-positioning/src/usePositioning.ts#L68-L85

For userland overrides to win here: