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

[Seercat3160]

I noticed the following inconsistency: When items are inserted or moved within a Dropdown using a BindableList, the visible order of the items in the DropdownMenu correctly reflects this. However, changing the Dropdown's selection using the keyboard appears to jump around rather than follow that correct order, instead seemingly being based on the order the underlying items were added.

I added an (initially) failing test to illustrate the problem.

I think this happens because MenuItems -- used in selectionKeyPressed to interact with the menu items when doing keyboard navigation -- is defined in terms of itemMap, which is a Dictionary (unordered).

As a possible fix, I've changed that code to use VisibleMenuItems from the DropdownMenu (which is probably desirable anyway), which I've made to be ordered based on the items' layout positions. I don't think that has any unintended effects, and tests are passing.
That approach is too shallow and doesn't address the actual problem here.

[bdach]

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).

[Seercat3160]

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.