fluentui icon indicating copy to clipboard operation
fluentui copied to clipboard

[Bug]: Fluent V9 Tree : onOpenChange is not correctly sending/recieving the Enter keydown event to TreeItemOpenChangeEvent

Open dennymicrosoft opened this issue 1 year ago • 4 comments

Library

~React Northstar / v0 (@fluentui/react-northstar)~ v9 (@fluentui/react-components)

System Info

System:
    OS: Windows 11 10.0.22631
    CPU: (16) x64 AMD EPYC 7763 64-Core Processor
    Memory: 32.04 GB / 63.95 GB
  Browsers:
    Edge: Chromium (126.0.2592.61)
    Internet Explorer: 11.0.22621.3527

Are you reporting Accessibility issue?

None

Reproduction

https://stackblitz.com/edit/xxnjkp?file=src%2Fexample.tsx

Bug Description

Actual Behavior

Enter keydown on TreeItem is triggering Click data.type instead of the Enter type for TreeItemOpenChangeEvent

Expected Behavior

Pressing Enter on focused treeitem will send Enter data.type to onOpenChange. Would like to add custom functionality when ctrl + Enter for onOpenChange.

Logs

No response

Requested priority

Blocking

Products/sites affected

No response

Are you willing to submit a PR to fix?

no

Validations

  • [X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
  • [X] The provided reproduction is a minimal reproducible example of the bug.

dennymicrosoft avatar Jun 19 '24 02:06 dennymicrosoft

Confirming, data.type in TreeItemOpenChangeData is correctly set to ArrowLeft or ArrowRight when arrows are used, but to Click when pressing Enter.

miroslavstastny avatar Jun 19 '24 05:06 miroslavstastny

Enter has been deprecated on Tree in #29390.

miroslavstastny avatar Jun 24 '24 14:06 miroslavstastny

We should perhaps add support for handling modifier keys better in the library. @ling1726 to create a tracking issue and link it here. @bsunderhus we should align Tree and TreeItem types to match.

miroslavstastny avatar Jun 24 '24 14:06 miroslavstastny

@ling1726 @bsunderhus @miroslavstastny

I am trying to add behavior for ctrl + enter, but if enter events fire a click event does that mean i lose ctrlKey data from the original keydown event? Any workarounds for now?

I can override the onKeyDown but then the click event is also still fired to onOpenChange I believe, so this is causing an issue where enter will have keydown behavior but also trigger click in onOpenChange which I do not want.

dennymicrosoft avatar Jun 24 '24 17:06 dennymicrosoft

@ling1726 @bsunderhus @miroslavstastny

I am trying to add behavior for ctrl + enter, but if enter events fire a click event does that mean i lose ctrlKey data from the original keydown event? Any workarounds for now?

I can override the onKeyDown but then the click event is also still fired to onOpenChange I believe, so this is causing an issue where enter will have keydown behavior but also trigger click in onOpenChange which I do not want.

Hey @dennymicrosoft . Yeah, we do not have a proper mapping of meta keys on this behavior 🥲.

I believe as a quick fix what I would recommend is using an onKeyDown handle to customize your behaviour and to also invoke event.preventDefault the event to ensure it will be ignored internally (that way it will not trigger the onOpenChange, here's an example

I believe we'd need some discussion for this. Maybe returning the Enter event type?! But this might introduce other problems (as it was deprecated to avoid a11y problems)

bsunderhus avatar Jul 15 '24 09:07 bsunderhus

Ok @dennymicrosoft , we've decided to go with the example's approach for your use case, here's the reasons:

  1. you want a custom behavior that is not associated with controlling open state
  2. we do provide a mechanism for you to stop the onOpenChange invocation down to the keydown event handler (by invoking event.preventDefault), which allows you to prevent open state changes and add your custom behavior
  3. adding anything that supports both click and Enter scenarios discerning them apart is troublesome as people might forget to support both cases and this will introduce a11y issues.

Let me know if the example provided is enough to ensure achieve what you want.

bsunderhus avatar Jul 18 '24 14:07 bsunderhus