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 trigger should be focused when menu is closed with keyboard #25165

Merged
merged 2 commits into from
Oct 11, 2022

Conversation

ling1726
Copy link
Member

#25016 missed out on a crucial condition.

keyboard handling flag was only set when the key was Tab this is incorrect and the keyboard handling flag should be set every time the event is a keyboard event

keyboard handling flag was only set when the key was `Tab` this is
incorrect and the keyboard handling flag should be set every time the
event is a keyboard event
.type('{esc}')
.get(menuSelector)
.should('not.exist')
.get(menuTriggerSelector)
Copy link
Member Author

@ling1726 ling1726 Oct 11, 2022

Choose a reason for hiding this comment

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

These situations should now be caught with tests

@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.273 kB
52.499 kB
189.275 kB
52.503 kB
2 B
4 B
react-menu
Menu (including children components)
116.604 kB
35.753 kB
116.606 kB
35.755 kB
2 B
2 B
react-menu
Menu (including selectable components)
119.673 kB
36.273 kB
119.675 kB
36.276 kB
2 B
3 B
Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-components
react-components: Button, FluentProvider & webLightTheme
62.827 kB
17.599 kB
react-components
react-components: FluentProvider & webLightTheme
33.446 kB
11.033 kB
react-portal-compat
PortalCompatProvider
5.857 kB
1.978 kB
🤖 This report was generated against 8954cd0e856c4fdda7745f40d3c30916a6e24f6b

@size-auditor
Copy link

size-auditor bot commented Oct 11, 2022

Asset size changes

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

Baseline commit: 8954cd0e856c4fdda7745f40d3c30916a6e24f6b (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 f199475:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration

@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 1266 1269 5000
Button mount 930 908 5000
FluentProvider mount 1509 1513 5000
FluentProviderWithTheme mount 579 578 10
FluentProviderWithTheme virtual-rerender 542 545 10
FluentProviderWithTheme virtual-rerender-with-unmount 577 577 10
MakeStyles mount 1962 1947 50000
SpinButton mount 2389 2310 5000

@ling1726 ling1726 merged commit 77e15da into microsoft:master Oct 11, 2022
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request Oct 11, 2022
* master:
  fix: Menu trigger should be focused when menu is closed with keyboard (microsoft#25165)
  fix(codemods): Do not fail on empty configMod config (microsoft#25148)
  fix: Field should have block layout, not inline (microsoft#25126)
  chore: Update Griffel to latest version (microsoft#25075)
NotWoods pushed a commit to NotWoods/fluentui that referenced this pull request Nov 18, 2022
…microsoft#25165)

* fix: Menu trigger should be focused when menu is closed with keyboard

keyboard handling flag was only set when the key was `Tab` this is
incorrect and the keyboard handling flag should be set every time the
event is a keyboard event

* changefile
@ling1726 ling1726 deleted the fix/menu-trigger-focus-2 branch January 4, 2023 15:28
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.

4 participants