fix(react-listbox): Fixing click/tap focus scroll bug introduced with recent Listbox standalone changes

[stevencomsft]

Previous Behavior

Initial focus logic added in a previous commit to help Listbox function better as a standalone component is triggered by any focus event, even if the focus event is from a mouse or touch. In a scrollable listbox (standalone), this means you can scroll down the list and then attempt to click and it will cause a focus event to trigger and scroll the listbox to the top and not select the list item.

New Behavior

Ignores focus events if not navigating with a keyboard

[codesandbox-ci[bot]]

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.

[stevencomsft]

@smhigley here are some recordings of the before and after:

https://github.com/microsoft/fluentui/assets/105799836/ea08980f-4e11-4dae-abec-f1be64241cb7

https://github.com/microsoft/fluentui/assets/105799836/b0e8dcbf-d5f9-48fb-9dc6-9889a68e0672

[stevencomsft]

@smhigley + @ling1726 would love to get this in before the next release

[stevencomsft]

@smhigley + @ling1726 + @bsunderhus

Thanks for review/approval :) I'm not powerful enough to do the merge on my own. Asking for help from the main team :)

[stevencomsft]

@smhigley @micahgodbolt

🤜🤛 bump

[stevencomsft]

@smhigley @micahgodbolt

Bump again 🥲

[smhigley]

So I've been poking at this, and the issue with the change is the same one as before -- detecting keyboard usage is super unreliable, and both misses a lot of real keyboard usage, and sometimes gets false positives on non-keyboard usage (under the hood there are some hit-or-miss heuristics to try to detect keyboard usage that doesn't send keyboard events through). For example, bluetooth keyboards on iOS, VoiceOver on mac or iOS, and Windows screen readers in virtual cursor/scan mode all use the keyboard but do not always trigger keyboard events.

There's no reason Listbox shouldn't have aria-activedescendant defined all the time, not only when focused, so I'm working on a different update to set the activedesc when it's first rendered. That should be ready today!

[stevencomsft]

@smhigley the issue this PR is trying to solve isn't really about whether or not the active descendent is set, but rather which item has initial focus. The current logic (evented off of 'focus') is sending focus to the first item in the list any time the list gets focus for the first time, even if that focus came from a click event on, say, the 5th item in the list. This PR is ensuring that that focus only happens for keyboards, because we don't need to apply custom focus in the event of a mouse event, as the mouse provides the focus itself

[smhigley]

@stevencomsft I think part of what's confusing this is that the listbox root always has focus -- individual options don't ever get it. There's a visual focus outline that is styled on the active descendant option, but that style can be conditional based on whether the listbox root has focus.

The issue with making it only occur for keyboard users is what I mentioned above -- it's essentially impossible to determine whether a user is actually using a keyboard. A number of keyboard users (e.g. bluetooth keyboard on iOS, VoiceOver on mac desktop, Windows screen reader users using virtual cursor) will present as if they were mouse users, and will fire mouse events but not keyboard events. They all still need the activedescendant logic to be present and work -- otherwise those experiences would break 😓

[stevencomsft]

I think part of what's confusing this is that the listbox root always has focus -- individual options don't ever get it.

@smhigley No I get that, but that's precisely what's causing the bug here.

So the current issue is that a mouse user can scroll in a listbox (standalone) and click an option 1-2 pages down the scroll viewport, and the current logic (because it fires on any and all focus events) intercepts that click and sends focus up to the first option in the listbox and doesn't fire a select event on the option that was clicked. Whereas keyboard users can't scroll that list until after they focus the listbox and then use arrow up/down to scroll, so they aren't affected by this bug.

Maybe what we need, rather than keying off of input type here, is to make sure the click of an option sets the active descendant before this logic ever fires? That way option selections take precedence over any active-descendant patching here in the Listbox?

[stevencomsft]

@smhigley bouncing off my last sentence, it seems that click events always fire after focus events, but things like PointerDown always fire first.

It's pretty straightforward to block the option focus logic (meant for keyboards) while, for example, a pointerDown event has been fired on an option directly. We could set a value isClickingOption = true while the user is pointerDown on a specific option, then set it to false when the pointerUp or pointerCancel fires. And while isClickingOption === true we skip any of the first-focus logic that is meant to help keyboards. I think this should be inclusive of the scenarios you mentioned (bluetooth keyboard on iOS, VoiceOver on mac desktop, Windows screen reader) because they would all be firing their first mouse events on the listbox itself and not on the options, right? So the pointerdown on the option wouldn't affect them during initial focus

[smhigley]

@stevencomsft I tweaked the PR I was working on earlier a bit to fully move logic from combobox/dropdown to listbox -- let me know what you think of this: https://github.com/microsoft/fluentui/pull/31415

It should fix your issue, since the Listbox sets the activedescendant when first rendered, and doesn't need to update it afterwards (so not on focus or on click or keyboard interaction).

The issue with the pointer events is that the cases I mentioned would actually focus the listbox in response to a pointer event on the option (so pretty much indistinguishable from mouse users, by design). There might be some heuristic we could use, but I'd rather sidestep that whole issue and handle the initial activedescendant on first render 😅

[stevencomsft]

@smhigley Awesome! I'll check that out now :)