search

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.32k stars 2.72k forks source link

[Bug]: TreeItem click events firing navigation #32034

Open george-cz opened 2 months ago

george-cz commented 2 months ago

Library

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

Bernardo has more context. Creating this now so that I can link it in the TMP repo.

bsunderhus commented 2 months ago

Here's the context:

TL,DR: Jest does not support (at least not out-of-the-box) actual real click events 🤡, to solve this just emit a focus event after clicking

It seems like TreeItem click event handler is triggering navigations:

https://github.com/microsoft/fluentui/blob/3c9750e01f3e8b55a1e3b36744c2e855c864c78e/packages/react-components/react-tree/library/src/components/TreeItem/useTreeItem.tsx#L130-L136

This is to ensure the correct tabIndex of the focused element.

In browsers once you click on something and that something is focusable the click will propagate a bunch of mouse events and a focus event. So the assumption is that a click event is equivalent to a focus one, which is kind of true for browser environments (since once you click you also focus on a treeitem), but this is not true for the actual .click() method, this method is not equivalent to an actual click, as it does not invoke the same events, missing also the focus event.

So the bug here is:

  1. we're erroneously using a click instead of a focus

And the assumption is:

actions visibility on click. As the docs describes https://react.fluentui.dev/?path=/docs/components-tree--default#actions actions slot is only visible when the treeitem is active (by hovering or by navigating to it = focus)

steabert commented 1 month ago

Not sure this is related, but after #31766, there seems to be no longer a navigation event triggered when clicking on a leaf node. Navigating to a leaf node with the keyboard triggers onNavigation but not clicking on a leaf node. In #31766 the click handler seems to be completely bypassed for leaf nodes?

bsunderhus commented 1 month ago

Not sure this is related, but after #31766, there seems to be no longer a navigation event triggered when clicking on a leaf node. Navigating to a leaf node with the keyboard triggers onNavigation but not clicking on a leaf node. In #31766 the click handler seems to be completely bypassed for leaf nodes?

That is true @steabert. There's no longer invocation of onOpenChange on a leaf, as there's no change in the open state. This #31766 was a bugfix on this. Apparently this bugfix has some repercussions

george-cz commented 2 weeks ago

@bsunderhus should we close this?

bsunderhus commented 2 weeks ago

@bsunderhus should we close this?

No @george-cz , we still need to discuss moving navigation from click events to focus events, which is correlated to this issue https://github.com/microsoft/fluentui/pull/32042

At the moment this is not a priority though, as in real world scenarios click and focus are connected through a real click event.

The https://github.com/microsoft/fluentui/pull/32042 PR right now proposes the solution with a breaking change by adding a new navigation type on focus and deprecating the navigation on click

bsunderhus commented 2 weeks ago

I removed this from current iteration and moved it back to the backlog so that we can discuss this further 😁