microsoft / fluentui

Fluent UI web represents a collection of utilities, React components, and web components for building web applications.
https://react.fluentui.dev
Other
18.6k stars 2.75k forks source link

[Bug]: MenuPopover consumes the Esc keydown event but doesn't stop propagation #30590

Closed eridanurce closed 9 months ago

eridanurce commented 9 months ago

Library

React Components / v9 (@fluentui/react-components)

System Info

System:
    OS: Windows 10 10.0.22621
    CPU: (16) x64 Intel(R) Core(TM) i9-10885H CPU @ 2.40GHz
    Memory: 17.47 GB / 63.73 GB
  Browsers:
    Edge: Chromium (121.0.2277.98), ChromiumDev (122.0.2365.3)
    Internet Explorer: 11.0.22621.1

Are you reporting Accessibility issue?

no

Reproduction

https://codesandbox.io/p/sandbox/menu-propagates-esc-5fr3py

Bug Description

Actual Behavior

When the menu is opened and Esc is pressed on the keyboard, the menu is closed but Esc keydown event is still propagated.

Expected Behavior

After the Esc keydown is consumed to close the menu, the event should not be propagated.

Logs

No response

Requested priority

High

Products/sites affected

Dynamics 365 Business Central

Are you willing to submit a PR to fix?

no

Validations

eridanurce commented 9 months ago

This seems to be the root cause for the event being propagated: https://github.com/microsoft/fluentui/pull/29262 @bsunderhus can you provide some insight into why this change was made?

bsunderhus commented 9 months ago

Hey @eridanurce, few reasons:

  1. Stopping propagation of events might be problematic on the design system level, as we're assuming too much of how an application might consume the event.
    • it is quite simple to prevent the event on user side to avoid conflicting behaviour. Meanwhile if we stop propagation we're basically denying an event for the user, which might be cumbersome on some edge cases scenarios (there are some application level tools that consume keypress/click events from the body and then process what's happening based on that)
  2. To be more similar to what the native <dialog> element does. There's no stop propagation for the closing of a modal in the Escape press. Preventing the event is enough to stop the modal from closing, here's an example
    • By following the same pattern as dialog does we bridge for a more future proof solution where one day (🙏) we might be able to use native <dialog>
      1. The not official popover attribute also would work similar to what a native <dialog> does.

Note: I'm aware that a popover and a dialog are not equivalent, but the native <dialog> element is the closest we have on a native level to build a popover/modal

microsoft-github-policy-service[bot] commented 9 months ago

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