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

fix: Menu triggers no longer take focus when they are mounted #25016

Merged
merged 1 commit into from
Oct 3, 2022

Conversation

behowell
Copy link
Contributor

@behowell behowell commented Sep 29, 2022

Current Behavior

Menu triggers take focus when they're first mounted.

This is likely a regression from #24738. Specifically, the removal of shouldHandleKeyboardRef from useMenu.tsx is causing it to always call state.triggerRef.current?.focus() when mounted.

-    if (shouldHandleKeyboardRef.current && !open) {
+    if (!open) {
      if (shouldHandleTabRef.current && !state.isSubmenu) {
        pressedShiftRef.current ? focusBeforeMenuTrigger() : focusAfterMenuTrigger();
      } else {
        state.triggerRef.current?.focus();
      }
    }

New Behavior

Restore the shouldHandleKeyboardRef check to useMenu_unstable.

@bsunderhus I'm not familiar with the reason that it was removed in the first place. If this is not the correct fix, please assign the issue to yourself so you can make the correct fix. Thanks!

Related Issue(s)

@fabricteam
Copy link
Collaborator

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
189.156 kB
52.385 kB
189.209 kB
52.401 kB
53 B
16 B
react-menu
Menu (including children components)
116.718 kB
35.78 kB
116.771 kB
35.786 kB
53 B
6 B
react-menu
Menu (including selectable components)
119.787 kB
36.289 kB
119.84 kB
36.296 kB
53 B
7 B
Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-components
react-components: FluentProvider & webLightTheme
33.4 kB
11.008 kB
react-portal-compat
PortalCompatProvider
5.857 kB
1.978 kB
🤖 This report was generated against 0885025f43076ebdf3a395cfb6ae65b3304cc5b2

@size-auditor
Copy link

size-auditor bot commented Sep 29, 2022

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: 0885025f43076ebdf3a395cfb6ae65b3304cc5b2 (build)

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit c01761d:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration
suspicious-cloud-ir1l1b Issue #24981

@fabricteam
Copy link
Collaborator

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 1559 1565 5000
Button mount 1121 1127 5000
FluentProvider mount 1883 1923 5000
FluentProviderWithTheme mount 768 769 10
FluentProviderWithTheme virtual-rerender 703 694 10
FluentProviderWithTheme virtual-rerender-with-unmount 742 738 10
MakeStyles mount 2314 2293 50000
SpinButton mount 2993 3024 5000

@behowell behowell marked this pull request as ready for review September 29, 2022 20:29
@behowell behowell requested a review from a team as a code owner September 29, 2022 20:29
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.

The original issue filed by @smhigley was #24680

Let's discuss this first to make sure we know what our focus requirements are for mouse users

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.

It seems this bug is more serious than I thought. I think it makes more sense to merge this PR and reopen #24680

@behowell behowell merged commit b0b7f99 into microsoft:master Oct 3, 2022
@behowell behowell deleted the menu/fix-autofocus-on-trigger branch October 3, 2022 18:35
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request Oct 5, 2022
* master: (62 commits)
  chore(migrate-converged): add functionality to execute various v9 package file restructuring tasks (microsoft#24458)
  chore(react-dialog): updates stories (microsoft#25070)
  refactor Progress to remove label and description slots (microsoft#25067)
  fix(SplitButton): Remove borders from hover and pressed state in primary split buttons (microsoft#25059)
  chore(react-persona): Add vr, conformance, and unit tests (microsoft#25007)
  (docs): update Icon docs with examples (microsoft#24768)
  Split button/primary hc fix (microsoft#25066)
  chore(react-utilities): restricts trigger API types (microsoft#25044)
  Add utilities export @fluentui/style-utilities v8 (microsoft#25065)
  chore: migrate whole repo to webpack 5 (microsoft#24542)
  applying package updates
  BREAKING: refactor `useTable` to be composable (microsoft#24947)
  Added shims to shim packages (microsoft#25048)
  Add Progress SPEC (microsoft#24418)
  applying package updates
  chore(react-persona): Add bundle-size command for bundle-size fixtures (microsoft#25055)
  fix(Button): Remove margin added to buttons by Safari (microsoft#25052)
  fix: Menu triggers no longer take focus when they are mounted (microsoft#25016)
  chore: deletes react-trigger package (microsoft#25042)
  Fixed a minor typo: immmediately => immediately (microsoft#25036)
  ...
NotWoods pushed a commit to NotWoods/fluentui that referenced this pull request Nov 18, 2022
…oft#25016)

This is likely a regression from microsoft#24738. Specifically, the removal of `shouldHandleKeyboardRef` from useMenu.tsx is causing it to always call `state.triggerRef.current?.focus()` when mounted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Menu triggers autofocus on mount
4 participants