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

MenuList's MenuItem steals focus when hovered, cannot override as a user #33836

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

Conversation

lekevin-microsoft
Copy link

Please see #33818 for full detailed description.

Current Behavior

I'm seeing an issue where when a user has focus within the input box, if they hover over any of the <MenuItem>s then focus will immediately jump to the menu item. This is particularly unwanted behavior as input boxes can have lots of ongoing text, and when focus is reset to a menu item, pressing keys like "r" will trigger certain hotkeys (e.g. "Reload the window") which is particularly unwanted.

Image

Of note, this is the exact same issue that was fixed back in 2021 with ContextualMenu via this ticket: Ticket #20552. The fix here was that ContextualMenu has a delayUpdateFocusOnHover prop added that wasn't quite working, which was fixed from that ticket (PR here).

I am looking for an ask to have it implemented in or a similar mechanism such that a user can opt in or out of the functionality that on-hover for MenuItems will automatically focus.

The code that is causing this auto-focus is here:

Image

From useMenuItem.tsx on line 77. I've tested locally that when this block of code is commented out, the behavior is resolved and focus is no longer stolen away.

Proposed PR Fix

The expected behavior is that hovering over a menu item does not steal away focus from another active menu item. In this case, a user should be able to continue typing in the Input component while moving the mouse around their screen, without worry that focus would be stolen and any subsequent button presses would trigger browser hotkeys.

Two options here: First, we could do something similar to Ticket #20552., where a property delayUpdateFocusOnHover is added. The user would then say delayUpdateFocusOnHover={true} in their creation of MenuItem and it would enforce that hover does not take focus.

Another option (that this PR is starting with) is a bit lighter-weight. It simply rearranges the call such that a user calling e.preventDefault() will work in preventing what FluentUI is doing. Before, it was calling focus() first and then the user's props.onMouseMove?.(event). Now, it would call onMouseMove first, and then focus() if it isn't prevented.

As it stands before the PR, the code is like so:

          onMouseMove: useEventCallback(event => {
            if (event.currentTarget.ownerDocument.activeElement !== event.currentTarget) {
              innerRef.current?.focus();
            }

            props.onMouseMove?.(event);
          }),

props.onMouseMove() is called after the focus() call, so it's too late to do anything. A way to fix this would be:

          onMouseMove: useEventCallback(event => {
            props.onMouseMove?.(event);  // Move this to be the first call

            if (!event.isDefaultPrevented() && // First call `isDefaultPrevented()` check
                 event.currentTarget.ownerDocument.activeElement !== event.currentTarget) {
              innerRef.current?.focus();
            }
          }),

Reproduction

https://stackblitz.com/edit/vprprnxa?file=src%2Fexample.tsx

Steps to reproduce

  1. Click on "Toggle Menu" in the Stackblitz
  2. Click into the Input component (first one) in the dropdown
  3. Begin typing the word "test"
  4. After the letter 's', move your mouse to any other menu item within the dropdown like "New" or "New Window"
  5. Observe that focus is lost, and that if you press that final letter 't' in "test", you will invoke the browser's default behavior which is opening a new tab.

Copy link

github-actions bot commented Feb 13, 2025

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
224.582 kB
64.913 kB
224.608 kB
64.921 kB
26 B
8 B
react-components
react-components: entire library
1.171 MB
293.127 kB
1.171 MB
293.131 kB
26 B
4 B
react-menu
Menu (including children components)
155.616 kB
46.882 kB
155.642 kB
46.885 kB
26 B
3 B
react-menu
Menu (including selectable components)
158.598 kB
47.478 kB
158.624 kB
47.483 kB
26 B
5 B
Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-avatar
Avatar
49.329 kB
15.824 kB
react-avatar
AvatarGroup
20.132 kB
7.976 kB
react-avatar
AvatarGroupItem
63.473 kB
20.043 kB
react-breadcrumb
@fluentui/react-breadcrumb - package
114.719 kB
31.752 kB
react-components
react-components: Button, FluentProvider & webLightTheme
69.638 kB
20.24 kB
react-components
react-components: FluentProvider & webLightTheme
44.473 kB
14.597 kB
react-datepicker-compat
DatePicker Compat
225.344 kB
63.793 kB
react-dialog
Dialog (including children components)
100.513 kB
30.131 kB
react-overflow
hooks only
12.808 kB
4.819 kB
react-persona
Persona
56.22 kB
17.704 kB
react-popover
Popover
130.32 kB
40.699 kB
react-portal-compat
PortalCompatProvider
8.39 kB
2.64 kB
react-table
DataGrid
161.06 kB
45.718 kB
react-table
Table (Primitives only)
42.692 kB
13.862 kB
react-table
Table as DataGrid
131.895 kB
36.579 kB
react-table
Table (Selection only)
70.562 kB
20.007 kB
react-table
Table (Sort only)
69.205 kB
19.618 kB
react-tag-picker
@fluentui/react-tag-picker - package
185.663 kB
55.782 kB
react-tags
InteractionTag
15.225 kB
6.165 kB
react-tags
Tag
29.098 kB
9.559 kB
react-tags
TagGroup
82.745 kB
24.532 kB
react-teaching-popover
TeachingPopover
91.711 kB
27.921 kB
react-timepicker-compat
TimePicker
108.551 kB
36.094 kB
react-tree
FlatTree
147.793 kB
42.393 kB
react-tree
PersonaFlatTree
148.538 kB
42.536 kB
react-tree
PersonaTree
144.749 kB
41.391 kB
react-tree
Tree
144.01 kB
41.265 kB
🤖 This report was generated against 91b288be817f46c003a65e5808043fe6655a9dca

Copy link

Pull request demo site: URL

@lekevin-microsoft
Copy link
Author

@smhigley / @microsoft/teams as heads up. I believe @microsoft/teams owns this code -- will need an approval from them regarding it. Thank you!

Copy link
Member

@ling1726 ling1726 left a comment

Choose a reason for hiding this comment

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

Nothing major wrong with this PR - As I mentioned offline, just would like to first discuss the input/menu pattern more deeply with @smhigley first to understand if we can do anything better about this

This pull request has been automatically marked as stale because it was marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 5 days of this comment. Thank you for your contributions to Fluent UI!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants