Skip to content

Fix dropdown menu keyboard navigation traversal order potentially not matching display order #6616

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.

Sign up for GitHub

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Fix dropdown menu keyboard navigation traversal order potentially not matching display order #6616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
da681b8
Add failing test for dropdown keyboard selection order
Seercat3160 Jul 21, 2025
22035db
Fix dropdown keyboard selection order
Seercat3160 Jul 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions osu.Framework.Tests/Visual/UserInterface/TestSceneDropdown.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,42 @@ void checkOrder(int index, string item) => AddAssert($"item #{index + 1} is '{it
() => Is.EqualTo(item));
}

[Test]
public void TestKeyboardSelectionOrder()
{
TestDropdown testDropdown = null!;
BindableList<TestModel?> bindableList = null!;

AddStep("setup dropdown", () => testDropdown = createDropdown());

AddStep("bind source", () => testDropdown.ItemSource = bindableList = new BindableList<TestModel?>());

AddStep("add items to bindable", () => bindableList.AddRange(new[] { "one", "two", "three" }.Select(s => new TestModel(s))));

AddStep("hover dropdown", () => InputManager.MoveMouseTo(testDropdown.Header));

AddStep("select first item", () => InputManager.Keys(PlatformAction.MoveToListStart));
AddAssert("'one' is selected", () => testDropdown.Current.Value?.Identifier == "one");

AddStep("select next item", () =>
{
InputManager.Key(Key.Down);
});
AddAssert("'two' is selected", () => testDropdown.Current.Value?.Identifier == "two");

AddStep("add 'three-halves'", () => bindableList.Add("three-halves"));
AddStep("move 'three-halves'", () => bindableList.Move(3, 1));

AddStep("select previous item", () =>
{
InputManager.Key(Key.Up);
});
AddAssert("'three-halves' is selected", () => testDropdown.Current.Value?.Identifier == "three-halves");

AddStep("select last item", () => InputManager.Keys(PlatformAction.MoveToListEnd));
AddAssert("'three' is selected", () => testDropdown.Current.Value?.Identifier == "three");
}

[Test]
public void TestExternalManagement()
{
Expand Down
4 changes: 2 additions & 2 deletions osu.Framework/Graphics/UserInterface/Dropdown.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ private void selectionKeyPressed(DropdownHeader.DropdownSelectionAction action)
if (!MenuItems.Any())
return;

var dropdownMenuItems = MenuItems.ToList();
var dropdownMenuItems = Menu.VisibleMenuItems.Select(item => (DropdownMenuItem<T>)item.Item).ToList();
Copy link
Collaborator

@bdach bdach Jul 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I don't deny that there's likely a real problem here but this is a really shallow (and therefore likely incorrect) way of handling the problem.

If there's a concern that MenuItems may be in incorrect order after some sequence of operations, then what does it say about the correctness of the public Items property given the getter is implemented as follows:

osu-framework/osu.Framework/Graphics/UserInterface/Dropdown.cs

Lines 65 to 78 in f91f679

/// <summary>
/// Enumerate all values in the dropdown.
/// </summary>
public IEnumerable<T> Items
{
get => MenuItems.Select(i => i.Value);
set
{
if (boundItemSource != null)
throw new InvalidOperationException($"Cannot manually set {nameof(Items)} when an {nameof(ItemSource)} is bound.");
setItems(value);
}
}

Would it not follow that this property also can not follow the visual ordering?

If that is the case, what does that say about the safety of the following logic?:

osu-framework/osu.Framework/Graphics/UserInterface/Dropdown.cs

Lines 363 to 369 in f91f679

case NotifyCollectionChangedAction.Move:
{
var item = Items.ElementAt(e.OldStartingIndex);
removeDropdownItem(item);
addDropdownItem(item, e.NewStartingIndex);
break;
}

Is it not reasonable to expect to find this doing completely ridiculous things if you arrange a few move operations?

All of this is to say that this is a half-fix at best to me. Consumers clearly expect MenuItems and Items to be consistent with the visual order of items. And they are right to do so, by the principle of least surprise.

How one sets out to do this is up in the air, but if it were up to me, I would say that relying on the drawables to store data here is bad juju. It usually doesn't end well, especially down the line where performance starts to matter (e.g. in a year or two we suddenly want to virtualise the dropdown items at which point you can no longer rely on layout positions being there because some of the drawable items are not in the hierarchy anymore).

I would say that ideally the ordering information should be a part of the backing data model of this class, i.e. somewhere in itemMap or something. Or maybe you would have a separate list tracking the correct item order. Or maybe you even ditch itemMap altogether and just have an ordered collection instead, eating the overhead from all of the ContainsKey() calls this class currently does (maybe fine, needs benchmarking on how bad it gets in real world usages).

Copy link
Author

Choose a reason for hiding this comment

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

When you put it like that, I agree that my changes here are a half-fix at most. I was focused on the particular thing that I had noticed and didn't see the larger cause. I was originally planning to just open an issue and link my test case, but I decided to try and see if I could fix it.

That being said, your comments here are less a review of my changes and more directed towards the existing state of this part of the codebase. Fixing that is above my level, so it might be better to migrate this to an issue.


switch (action)
{
Expand Down Expand Up @@ -505,7 +505,7 @@ private void clearPreselection(MenuState obj)
PreselectItem(null);
}

protected internal IEnumerable<DrawableDropdownMenuItem> VisibleMenuItems => Children.OfType<DrawableDropdownMenuItem>().Where(i => i.MatchingFilter);
protected internal IEnumerable<DrawableDropdownMenuItem> VisibleMenuItems => Children.OrderBy(itemsFlow.GetLayoutPosition).OfType<DrawableDropdownMenuItem>().Where(i => i.MatchingFilter);
protected internal IEnumerable<DrawableDropdownMenuItem> MenuItemsInView => VisibleMenuItems.Where(item => !item.IsMaskedAway);

public DrawableDropdownMenuItem PreselectedItem => VisibleMenuItems.FirstOrDefault(c => c.IsPreSelected)
Expand Down
Loading