-
Notifications
You must be signed in to change notification settings - Fork 448
Allow dropdowns to be opened by MouseDown event #6610
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
base: master
Are you sure you want to change the base?
Allow dropdowns to be opened by MouseDown event #6610
Conversation
c57216e
to
68d6501
Compare
/// <summary> | ||
/// Whether parent dropdown <see cref="Dropdown{T}"/> should open/close on OnMouseDown event. | ||
/// </summary> | ||
public bool ToggleOnMouseDown { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need to be a toggleable flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your question is towards naming or the setter?
It's easier to setup for tests if it has a setter, same was done above for AlwaysShowSearchBar
. For real use, it is supposed that it will never be reassigned after init, but nothing will be broken if it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably needed to disable on dropdowns inside a scroll container, else you can't drag start scroll on a dropdown anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my question is towards the mere existence of the flag
Probably needed to disable on dropdowns inside a scroll container, else you can't drag start scroll on a dropdown anymore.
this sort of thing makes me question the entire premise of this change. are we really going to have dropdowns have different UX depending on whether they're inside a scroll or not? that's horrible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we really going to have dropdowns have different UX depending on whether they're inside a scroll or not? that's horrible
How about auto scrolling slowly, one item by one, when you get past the bottom?
Tried mimicking what I mean:
osu._tQzA2LnUcC_EDIT.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I'm talking about is dropdowns in settings. For example, on mobile, you drag to scroll. But if you were to touch a dropdown header before dragging, it won't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we really going to have dropdowns have different UX depending on whether they're inside a scroll or not? that's horrible
Elements inside scrollable containers already have different UX by definition. With a touch, you expect scroll to happen when you drag your finger. When you touch and lift your finger, you expect a click to occur. Fixed surfaces can't scroll, so user's intent will always be a click by default (and then maybe a long-press, etc.) Why don't we skip a step then if we already know user's intent?
You can find these click optimizations in a browser for example:
- on PC tabs list surface is fixed and non-scrollable, therefore tabs switch on mouse down,
- on mobile tabs are inside a scroll container, therefore they open on click.
On the functional side, "power" users will be happy as they can select items quicker (e.g. beatmap collection) while regular ones won't notice the difference (except that menus will open slightly faster). Also, this UX was in stable for years.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about auto scrolling slowly, one item by one, when you get past the bottom?
Probably could be lazily done by adding offset param to ScrollContainer.ScrollIntoView
, but better solution will be adding an area on top/bottom of the menu that will auto-scroll in a given direction by one item height. It should also have something like a shadow gradient or an arrow to show its interactive. overall, this requires design changes and is just not in the scope of this PR.
osu.Framework.Tests_DehYRpHzlm.webm
Collections dropdown max height can also just be bigger :p
// focus will be lost on `onClick` event -- therefore, closing dropdown. | ||
// We need to prevent that by manually focusing on the `SearchBar.textBox`. | ||
if (dropdown.MenuState == MenuState.Open) | ||
dropdown.ChangeFocus(SearchBar.Child); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this access of .Child()
is turbo dodgy, should be replaced by a DropdownSearchBar.TakeFocus()
method or something that ensures that the child in question here is actually the textbox that you want it to be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was looking on 0fed903 (#6286); I think we can reintroduce focus methods for DropdownSearchBar
and keep focus management inside it as well:
/// <summary>
/// Focuses the textbox and opens parent <see cref="Dropdown{T}"/> as a result.
/// </summary>
public void ObtainFocus()
{
dropdown.ChangeFocus(textBox);
}
/// <summary>
/// Clears current search and removes focus from the textbox,
/// closing parent <see cref="Dropdown{T}"/> as a result.
/// </summary>
public void ReleaseFocus()
{
// Reset states when the menu is closed by any means.
SearchTerm.Value = string.Empty;
if (!textBox.HasFocus)
return;
dropdown.ChangeFocus(null);
}
/// <summary>
/// Handles changes to the menu visibility.
/// </summary>
private void onMenuStateChanged(MenuState state)
{
if (state == MenuState.Closed)
ReleaseFocus();
else
ObtainFocus();
updateTextBoxVisibility();
}
Another solution I've tried is to let DropdownSearchBar manage focus on its own and invoke MenuStateChanged
by closing and opening the menu (it won't invoke if previous value matches current) which is very strange to see in a dropdown.MenuState == MenuState.Open
if statement. Also it makes textbox lose focus temporarily.
private bool onClick(ClickEvent e)
if (ToggleOnMouseDown) {
if (dropdown.MenuState == MenuState.Closed) return false
dropdown.CloseMenu();
dropdown.OpenMenu();
return false;
}
...
}
Obtain/ReleaseFocus works fine and provides public methods for the header, but maybe @smoogipoo has something to say for it.
// Without this check, when dropdown is opened by clicking outside `SearchBar`, | ||
// focus will be lost on `onClick` event -- therefore, closing dropdown. | ||
// We need to prevent that by manually focusing on the `SearchBar.textBox`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would like test coverage for this scenario
// Close dropdown when cursor is released outside the menu | ||
if (!Menu.IsHovered) | ||
{ | ||
// Do not close the menu if we are releasing on the DropdownHeader | ||
if (!Header.IsHovered) | ||
Menu.Close(); | ||
return; | ||
} | ||
|
||
// Cursor is inside the menu and possibly selecting an item, | ||
// commit that selection and close the menu | ||
((IDropdown)this).CommitPreselection(); | ||
Menu.Close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this logic is very tangled, would rather have
diff --git a/osu.Framework/Graphics/UserInterface/Dropdown.cs b/osu.Framework/Graphics/UserInterface/Dropdown.cs
index bdfa9b212..7bcb2eff3 100644
--- a/osu.Framework/Graphics/UserInterface/Dropdown.cs
+++ b/osu.Framework/Graphics/UserInterface/Dropdown.cs
@@ -355,18 +355,12 @@ protected override void OnMouseUp(MouseUpEvent e)
if (!ToggleOnMouseDown)
return;
- // Close dropdown when cursor is released outside the menu
- if (!Menu.IsHovered)
- {
- // Do not close the menu if we are releasing on the DropdownHeader
- if (!Header.IsHovered)
- Menu.Close();
+ if (Header.IsHovered)
return;
- }
- // Cursor is inside the menu and possibly selecting an item,
- // commit that selection and close the menu
- ((IDropdown)this).CommitPreselection();
+ if (Menu.IsHovered)
+ ((IDropdown)this).CommitPreselection();
+
Menu.Close();
}
For clarity I'm not willing to review this further until I get buy-in from @ppy/team-client that everyone is fine with dropdowns changing UX on the fly depending on whether they're inside a scroll or not (#6610 (comment)). |
I'm fine with this as a UX behaviour. As for implementation, we may want to consider making it automatic via inferring a parent scroll container? osu-framework/osu.Framework/Graphics/Containers/ScrollContainer.cs Lines 193 to 196 in 8166354
|
Adds `ToggleOnMouseDown` boolean property for Dropdown and DropdownHeader. The flag will use `MouseDownEvent` instead of `ClickEvent` for toggling the menu. Dropdown handles `MouseUpEvent`, looking for active hovers and selecting preselected items if the menu is hovered. Dropdown should close if cursor was released outside the menu and should stay open if it is released on the `DropdownHeader`.
Add test for padded dropdown with ToggleOnMouseDown=true
912b2b9
to
370233d
Compare
Add test for dropdown inside a scrollable parent Automatically resolve default value for DropdownHeader.ToggleOnMouseDown by checking if ScrollContainer is present somewhere in parent tree. If it is, we should disable OnMouseDown behaviour as it would mess up scroll UX.
370233d
to
364d4d1
Compare
osu.mp4Looks like I've checked most of the places where dropdown menus are used in osu! and all look great, although editor's top bar is scrollable and its menus will therefore open on click – I guess it's fine anyways. Added a test with dropdown in a scrollable parent as well. The only edge case for this behaviour is when dropdown header is moved somewhere from cursor. This is the unwanted UX change bdach was concerned about. In my defense, I would say this is rare (not present anywhere in osu! at least) and I would consider fixing the layout or just implicitly setting osu.Framework.Tests_pbJXEPPDFX.webm |
See #6584
Adds
ToggleOnMouseDown
boolean property for Dropdown and DropdownHeader.The flag will use
MouseDownEvent
instead ofClickEvent
for toggling the menu. Dropdown handlesMouseUpEvent
, looking for active hovers and selecting preselected items if the menu is hovered. Dropdown should close if cursor was released outside the menu and should stay open if it is released on the DropdownHeader.osu._0wpYGXF2M1.webm