Skip to content
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

Refactor dropdown search bar and fix many issues with it #6097

Closed

Conversation

frenzibyte
Copy link
Member

@frenzibyte frenzibyte commented Dec 29, 2023

Using the concept of "multiple focus targets", now Dropdown gains focus when in an open state, and in addition sends focus events to Menu and indirectly to the search bar text box using the chain "DropdownHeader -> DropdownSearchBar -> TextBox".

This eliminates the necessity of an input manager inside DropdownSearchBar, which in turn resolves many issues that arose from it (such as blocking key events from propagating past the search bar text box, or the search bar text box receiving focus despite being blocked by another component in front of it).

A necessary component in this implementation is to stop both Menu and TextBox from managing the focus state completely, using an internal boolean property. I'm not quite fond of the approach, but I did not want to overthink this more than I had done already (one idea I had that I scraped off was defining an IUIControl interface, and let Menu/TextBox avoid handling focus if there's a parent with the IUIControl interface, via DI or parent traversal).

As a side detail in this change, positional input on the search bar is now possible (i.e. you can drag the search bar text box to change selection).

Comment on lines +27 to +30
// On mobile platforms, let's not make the keyboard popup unless the dropdown is intentionally searchable.
// todo: however, this causes such dropdowns to not be searchable at all when using hardware keyboard on mobile.
if (host.OnScreenKeyboardOverlapsGameWindow && !AlwaysDisplayOnFocus)
return Array.Empty<Drawable>();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change in #6080 has been moved here. Once a dropdown is focused, if AlwaysDisplayOnFocus is false and the platform shows a software keyboard, then the TextBox is excluded from being focused, therefore not showing any software keyboard when the dropdown is open.

@frenzibyte frenzibyte force-pushed the refactor-dropdown-search-bar branch from 0c023e4 to 5f2acdd Compare December 29, 2023 02:06
@peppy
Copy link
Member

peppy commented Apr 23, 2024

@frenzibyte I don't suppose you ever made test coverage of the actual issues this is fixing (ie the issues being closed by this PR)?

I'd like to give this a shot from scratch because I can't really get behind the multiple focus path that you've proposed here.

@pull-request-size pull-request-size bot added size/XL and removed size/L labels May 6, 2024
@frenzibyte
Copy link
Member Author

Sorry about that, I've included test coverage for ppy/osu#25769 and ppy/osu#26079, which are the issues fixed by the "multiple focus target" approach. Should be good enough to explore alternative options with. I've also pushed a separate branch for the tests in case needed: master...frenzibyte:osu-framework:dropdown-search-bar-issues

@smoogipoo smoogipoo mentioned this pull request May 17, 2024
@peppy peppy closed this in #6286 May 19, 2024
@frenzibyte frenzibyte deleted the refactor-dropdown-search-bar branch May 19, 2024 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants