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
fix(react-listbox): Fixing click/tap focus scroll bug introduced with recent Listbox standalone changes #31235
base: master
Are you sure you want to change the base?
fix(react-listbox): Fixing click/tap focus scroll bug introduced with recent Listbox standalone changes #31235
Conversation
…use the listbox scroll container to shoot to the top do to unrestrained focus logic
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. |
@smhigley here are some recordings of the before and after: ListboxWithoutFocusFix.mp4ListboxWithFocusFix.mp4 |
@smhigley + @ling1726 + @bsunderhus Thanks for review/approval :) |
🤜🤛 bump |
Bump again 🥲 |
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 |
@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 |
@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 😓 |
@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? |
@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 |
@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: #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 😅 |
@smhigley Awesome! I'll check that out now :) |
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