Skip to content

feat(react-menu): replace keydown handlers by useARIAButtonShorthand on MenuItem#24738

Merged
bsunderhus merged 4 commits intomicrosoft:masterfrom
bsunderhus:react-menu/menu-item-useARIAButton
Sep 15, 2022
Merged

feat(react-menu): replace keydown handlers by useARIAButtonShorthand on MenuItem#24738
bsunderhus merged 4 commits intomicrosoft:masterfrom
bsunderhus:react-menu/menu-item-useARIAButton

Conversation

@bsunderhus
Copy link
Contributor

@bsunderhus bsunderhus commented Sep 9, 2022 •
edited
Loading

Current Behavior

MenuItem currently provides its own custom keydown handlers to ensure button functionality.

New Behavior

  1. Uses useARIAButtonShorthand instead of implementing it's own handlers
  2. Stops spreading props over MenuItem, MenuItemCheckbox and MenuItemRadio state.

Related Issue(s)

While pressing Space the oficial functionality of a native button is to emit a click event on keyup. take the example bellow:

export const Default = () => (
  <Menu>
    <MenuTrigger>
      <Button>Toggle menu</Button>
    </MenuTrigger>

    <MenuPopover>
      <MenuList>
        <MenuItem>New </MenuItem>
        <MenuItem>New Window</MenuItem>
        <MenuItem disabled>Open File</MenuItem>
        <MenuItem>Open Folder</MenuItem>
      </MenuList>
    </MenuPopover>
  </Menu>
);

Meanwhile Button behaves as a proper native button and emits click on keyup, MenuItem will close on Space keydown, meaning that as soon as you press Space on MenuItem the Menu will close on keydown and immediately open up again as soon as Space is release emitting a keyup event.

Screen Recording 2022-09-09 at 11 28 49

Fixes #24680

@bsunderhus bsunderhus self-assigned this Sep 9, 2022
@github-actions github-actions bot added this to the July Project Cycle Q3 2022 milestone Sep 9, 2022
@fabricteam
Copy link
Collaborator

fabricteam commented Sep 9, 2022 •
edited
Loading

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover		 187.939 kB
52.05 kB		 188.61 kB
52.323 kB		 671 B
273 B
react-menu
Menu (including children components)		 115.735 kB
35.419 kB		 116.529 kB
35.768 kB		 794 B
349 B
react-menu
Menu (including selectable components)		 118.934 kB
35.916 kB		 119.598 kB
36.287 kB		 664 B
371 B
Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-components
react-components: FluentProvider & webLightTheme		 33.359 kB
11.004 kB
react-overflow
hooks only		 10.685 kB
4.104 kB
react-portal-compat
PortalCompatProvider		 5.851 kB
1.964 kB
🤖 This report was generated against e0a0ab1481d0fb64f2a9804376ad17e4f4f8ea01

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 9, 2022 •
edited
Loading

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 44ab882:

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

@size-auditor
Copy link

size-auditor bot commented Sep 9, 2022 •
edited
Loading

Asset size changes

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

Baseline commit: e0a0ab1481d0fb64f2a9804376ad17e4f4f8ea01 (build)

@fabricteam
Copy link
Collaborator

fabricteam commented Sep 9, 2022 •
edited
Loading

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 1308 1303 5000
Button mount 954 946 5000
FluentProvider mount 1579 1573 5000
FluentProviderWithTheme mount 641 640 10
FluentProviderWithTheme virtual-rerender 591 592 10
FluentProviderWithTheme virtual-rerender-with-unmount 633 639 10
MakeStyles mount 1895 1892 50000
SpinButton mount 2563 2526 5000

@bsunderhus bsunderhus force-pushed the react-menu/menu-item-useARIAButton branch 3 times, most recently from 3435766 to 59ddb12 Compare September 9, 2022 12:01
@bsunderhus
Copy link
Contributor Author

bsunderhus commented Sep 9, 2022

Blocked by #24742

@bsunderhus bsunderhus added the Status: Blocked Resolution blocked by another issue label Sep 9, 2022
@bsunderhus bsunderhus force-pushed the react-menu/menu-item-useARIAButton branch 3 times, most recently from cc9036d to 8f00133 Compare September 9, 2022 18:48
@bsunderhus bsunderhus removed the Status: Blocked Resolution blocked by another issue label Sep 10, 2022
@bsunderhus bsunderhus marked this pull request as ready for review September 10, 2022 07:32
ling1726
ling1726 reviewed Sep 13, 2022
View reviewed changes
packages/react-components/react-overflow/src/stories/Overflow/utils.stories.tsx
}

return <MenuItem {...rest}>Item {id}</MenuItem>;
// As an union between button props and div props may be conflicting, casting is required
Copy link
Contributor

@ling1726 ling1726 Sep 13, 2022

Choose a reason for hiding this comment

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

does this mean that users that wrap the MenuItem component and spread props also need to cast, even if the type extends from MenuItemProps ?

Copy link
Contributor Author

@bsunderhus bsunderhus Sep 13, 2022

Choose a reason for hiding this comment

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

Yes! that is true for Button also, upper casting is required before spreading.

@bsunderhus bsunderhus force-pushed the react-menu/menu-item-useARIAButton branch 2 times, most recently from c5996e4 to b60a745 Compare September 13, 2022 09:08
@bsunderhus bsunderhus requested a review from ling1726 September 13, 2022 09:59
@ling1726
Copy link
Contributor

ling1726 commented Sep 13, 2022 •
edited
Loading

This PR also fixes #24680 @bsunderhus can you add it to the description ?

khmakoto
khmakoto reviewed Sep 13, 2022
View reviewed changes
@bsunderhus bsunderhus force-pushed the react-menu/menu-item-useARIAButton branch from 0c21fd0 to 9964ffd Compare September 14, 2022 05:52
@bsunderhus bsunderhus force-pushed the react-menu/menu-item-useARIAButton branch from b115e33 to 44ab882 Compare September 14, 2022 06:09
@bsunderhus bsunderhus requested a review from khmakoto September 14, 2022 08:29
GeoffCoxMSFT
GeoffCoxMSFT reviewed Sep 15, 2022
View reviewed changes
apps/public-docsite-v9/src/shims/MenuShim.tsx
if (props.itemType === ContextualMenuItemType.Divider) {
const shimProps = shimMenuItemProps(props);
return <MenuDivider {...shimProps} />;
return <MenuDivider {...(shimProps as MenuDividerProps)} />;
Copy link
Member

@GeoffCoxMSFT GeoffCoxMSFT Sep 15, 2022

Choose a reason for hiding this comment

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

I don't understand how the changes you made caused this cast to be required, but it won't hurt :)

Copy link
Contributor Author

@bsunderhus bsunderhus Sep 15, 2022

Choose a reason for hiding this comment

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

The casting is required since spreading of props can be conflicting when using discriminated unions. Basically there's no way to know if we're spreading button props or a props, so we need to explicitly upper cast to ensure it's a generic that represents both.

@bsunderhus bsunderhus merged commit e8f84e8 into microsoft:master Sep 15, 2022
@bsunderhus bsunderhus deleted the react-menu/menu-item-useARIAButton branch September 15, 2022 17:32
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request Sep 19, 2022
@marcosmoura
Merge branch 'master' into marcosmoura/fix/card-codesandbox-examples
3db2b04 
* master: (28 commits)
  fix: use trigger prop for aria-haspopup (microsoft#24794)
  chore(react-dialog): scaffold DialogContent component (microsoft#24844)
  chore: Northstar screener should read from screenerStates.json (microsoft#24848)
  applying package updates
  (web components) Standardize focus treatment (microsoft#24771)
  Divider - allow default prop override (microsoft#24840)
  GroupedList: fix virtualization (unstable preview) (microsoft#24460)
  fix: Add explicit children prop to TeachingBubble to support React 18 (microsoft#24823)
  feat: Adds `visible` prop to `TableCellActions` (microsoft#24831)
  [Northstar][Dropdown] Fix styling mutation when merging themes (microsoft#24787)
  fix: export `tableCellActionsClassNames` from unstable (microsoft#24830)
  bugfix(react-dialog): Adds color style to DialogSurface (microsoft#24832)
  applying package updates
  Prevent group toggling from selecting the whole group (microsoft#24822)
  feat(react-textarea): Add shadow variant of filled appearance (microsoft#24512)
  applying package updates
  Adding lib-commonjs top-level entries to exports map (microsoft#24792)
  Created shim packages (microsoft#24780)
  feat(react-menu): replace keydown handlers by useARIAButtonShorthand on MenuItem (microsoft#24738)
  fix: update version mismatches triggered by v9 release (microsoft#24812)
  ...
This was referenced Sep 29, 2022
Merged
Closed
behowell added a commit that referenced this pull request Oct 3, 2022
@behowell
fix: Menu triggers no longer take focus when they are mounted (#25016)
b0b7f99 
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.
NotWoods pushed a commit to NotWoods/fluentui that referenced this pull request Nov 18, 2022
@behowell @NotWoods
fix: Menu triggers no longer take focus when they are mounted (micros…
da262e8 
…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.
@ling1726 ling1726 mentioned this pull request Dec 5, 2022
Closed
@ling1726 ling1726 mentioned this pull request Jan 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@khmakoto khmakoto khmakoto approved these changes

@GeoffCoxMSFT GeoffCoxMSFT GeoffCoxMSFT approved these changes

@chpalac chpalac Awaiting requested review from chpalac

+1 more reviewer

@ling1726 ling1726 ling1726 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@bsunderhus bsunderhus

Labels

Area: Accessibility Component: Menu Fluent UI react-components (v9)

Projects

None yet

Milestone

July Project Cycle Q3 2022

Development

Successfully merging this pull request may close these issues.

[Bug]: react-menu doesn't restore focus to the trigger when clicking a menu item

7 participants

@bsunderhus @fabricteam @ling1726 @khmakoto @GeoffCoxMSFT @Hotell @msft-fluent-ui-bot