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

refactor(web-components): refactor menu to popover #31674

Merged

Conversation

davatron5000
Copy link
Contributor

@davatron5000 davatron5000 commented Jun 11, 2024

Previous Behavior

The current Menu component has a hard dependency on @floating-ui/dom, this is an attempt to slim that down and leverage HTML popover and CSS anchor positioning.

Also discovered:

  • Drop-shadow bug on nested menu items
  • Nested menu items not working in Safari

New Behavior

This component has been refactored to use HTML popover and CSS anchor positioning. HTML popoveris polyfillable and we're able to gracefully degrade CSS anchor, so that's promising.

  • Refactor Menu to use the HTML [popover] and CSS anchor
  • Allow for setting max-height of dropdown (per request from Commerce UI Kit)
  • Refactor MenuList and MenuItem to use [popover] as well.
  • Add default fallback position for browsers that don't support CSS anchor
  • Remove @floating-ui/dom dependency from Menu and MenuItem

Due to platform limitations with popover, we're unable to apply popovertarget to fluent-button (because it's not a real <button>) which would automate some of the show/hiding focus management and ARIA-expanded work, but have included workarounds.

Known issues and TODOs

  • :focus-visible not visible on first and last items in Safari
  • contextmenu is a little futzy
  • Document HTML popover polyfill
  • Run yarn change
  • Update API Extractor
  • Update package.json

Related Issue(s)

  • Fixes #

@davatron5000 davatron5000 changed the title wip: refactor menu to popover wip(web-components): refactor menu to popover Jun 11, 2024
@fabricteam
Copy link
Collaborator

fabricteam commented Jun 11, 2024

📊 Bundle size report

✅ No changes found

@davatron5000 davatron5000 force-pushed the davatron5000/refactor-menu-to-popover branch 2 times, most recently from 6b8beb2 to 951b615 Compare June 19, 2024 18:55
@davatron5000 davatron5000 changed the title wip(web-components): refactor menu to popover refactor(web-components): refactor menu to popover Jun 19, 2024
@davatron5000 davatron5000 marked this pull request as ready for review June 19, 2024 18:56
@davatron5000 davatron5000 requested a review from a team as a code owner June 19, 2024 18:56
@davatron5000 davatron5000 force-pushed the davatron5000/refactor-menu-to-popover branch from 49e9093 to 113ae51 Compare June 19, 2024 20:07
Copy link
Member

@chrisdholt chrisdholt left a comment

Choose a reason for hiding this comment

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

This looks great! One place where we'll want to remove the @floating-ui/dom dependency as well is the tensile config for the benchmarks. There's a reference in there we can likely remove.

@davatron5000 davatron5000 force-pushed the davatron5000/refactor-menu-to-popover branch from 48b73f9 to 38adb86 Compare June 21, 2024 18:45
@davatron5000 davatron5000 enabled auto-merge (squash) June 21, 2024 18:53
@mlijanto
Copy link
Member

Looks like there's a couple of color bugs in menu-item when disabled. Here's what I see:

Disabled, rest - looks good!
image

Disabled, active - icon color should not change color?
image

Disabled, high contrast - no color difference, maybe needs a system color override
image

@radium-v
Copy link
Contributor

A small interaction nit with menu: When clicking a menu to open it, keyboard navigation doesn't work. You have to redirect focus away from the control then back into it to use the keyboard. This isn't an issue with menu-list.

@davatron5000
Copy link
Contributor Author

@mlijanto Thanks for finding that. 🙌

  • Fixed the [disabled]:active issue.
  • Added a disabled menuitem to the story as well.
  • Fixed disabled state in forced-colors mode.

@radium-v

  • Rolled in your nits
  • Fixed the focus situation on Menu when you click and then want to use arrow keys.

@davatron5000 davatron5000 merged commit 8cfadd0 into microsoft:master Jun 21, 2024
18 checks passed
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.

6 participants