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

Fix clicking on padded dropdown search bars not opening the menu #6315

Merged
merged 2 commits into from
Jun 26, 2024

Conversation

smoogipoo
Copy link
Contributor

@smoogipoo smoogipoo commented Jun 26, 2024

Fixes ppy/osu#28594

In hindsight, it's probably not a good idea to handle click-to-open input in something like a "search bar".

In osu!, the search bar is padded to add space for the down-arrow icon: https://github.com/ppy/osu/blob/9039ec34bae03a9b20e43b2161cc5c9e767c3889/osu.Game/Graphics/UserInterface/OsuDropdown.cs#L405-L408

I also explored the option of moving the padding to the textbox proper, but textboxes don't seem to properly handle padding at all.

Comment on lines +149 to +150
// And importantly, when the menu is closed as a result of the above toggle, block the search bar from receiving input.
return dropdown.MenuState == MenuState.Closed;
Copy link
Contributor Author

@smoogipoo smoogipoo Jun 26, 2024

Choose a reason for hiding this comment

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

I was just about to remove this, and ended up adding it back. I want to explain this a little bit for future reference.

"Why isn't this just return true;?"

Originally, this condition was required because of an interaction with the code changing the menu state when the textbox is focused/unfocused (keep in mind the focus of this entire component is handled by the textbox alone):

/// <summary>
/// Opens or closes the menu depending on whether the textbox is focused.
/// </summary>
private void updateMenuState()
{
if (textBox.HasFocus)
dropdown.OpenMenu();
else
dropdown.CloseMenu();
}

Basically, we need to make sure the textbox receives focus at some point, hence input needs to pass through when the dropdown is supposed to be visible, and not pass through when it's supposed to be invisible.

By sheer luck, and as a result of a separate bugfix, this is now also handled by:

else
{
// This exists because the menu is _sometimes_ opened via external means rather than a direct click.
// _Sometimes_, this occurs via a click on an external button (such as a test scene step button), and so it needs to be scheduled for the next frame.
Schedule(() => dropdown.ChangeFocus(textBox));
}

That specifically handles the case where the dropdown is opened externally, i.e. by calling Dropdown.Menu.Open().

Given that the above is a very roundabout way of handling this, I prefer to be explicit about intentions here.

@peppy peppy self-requested a review June 26, 2024 05:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dropdown-menu not triggering when klicking on the arrow of the respective menu
2 participants