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

Allow slider bar to take focus and accept keyboard input while not hovered #6390

Merged
merged 8 commits into from
Oct 24, 2024

Conversation

OliBomby
Copy link
Contributor

This change would allow you to click the slider bar, have it gain focus, and then adjust the slider bar with the keyboard while the cursor is not hovering the slider bar.

This matches the user expectation because you can do the same thing in osu! stable.
In current lazer the only way to do this is click away to unfocus everything, then hover the slider bar while using keyboard. This flow is apparently so confusing that someone in Discord thought it wasn't possible at all.

Discord_XhhdbulGlk

osu.Game.Rulesets.Osu.Tests_asCoVNwym2.mp4

Discord_5hnRVUNu9n

Copy link
Member

@Joehuu Joehuu left a comment

Choose a reason for hiding this comment

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

UX-wise this is the same in OSes (hover wasn't a factor anywhere I looked). But it was expected UX for others in lazer so not sure of the reaction. Can probably be a fallback (if it's not focused)?

That said, it should have focus feedback. Not many UIs do this, but I feel like it's needed for lazer. In Windows 11, the volume control shows an outline with the number always visible when using keyboard:
eVaSk66Shg

protected override bool OnKeyDown(KeyDownEvent e)
{
if (currentNumberInstantaneous.Disabled)
return false;

if (!IsHovered)
Copy link
Member

Choose a reason for hiding this comment

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

Should probably early return if not focused. Pressing left/right randomly in settings can unintentionally change a slider.

@bdach
Copy link
Collaborator

bdach commented Oct 17, 2024

Also test failures

@OliBomby
Copy link
Contributor Author

That said, it should have focus feedback. Not many UIs do this, but I feel like it's needed for lazer. In Windows 11, the volume control shows an outline with the number always visible when using keyboard:

Correct me if I'm wrong, but I think the visual feedback is to be fixed on lazer's side, not framework. I can follow up with a PR on lazer for focus feedback after this is merged.

@Joehuu
Copy link
Member

Joehuu commented Oct 17, 2024

Yes, but the visual can be basic, to demonstrate the behavior and implementation here (in BasicSliderBar). It's mostly for framework consumers, and it'll not be as subjective as lazer's side.

@pull-request-size pull-request-size bot added size/M and removed size/S labels Oct 17, 2024
@Joehuu Joehuu self-requested a review October 19, 2024 06:02
Copy link
Member

@Joehuu Joehuu left a comment

Choose a reason for hiding this comment

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

One more thing I noticed is that if you drag, it won't focus:

Kapture.2024-10-18.at.23.28.28.mp4

Comparing again to OSes and other UI, they handle this:

Kapture.2024-10-18.at.23.32.04.mp4
dlkXw0c9Bf.mp4

@OliBomby
Copy link
Contributor Author

You're so right. How didnt i notice this

@OliBomby OliBomby changed the title Allow slider bar to use keyboard input while not hovered Allow slider bar to take focus and accept keyboard input while not hovered Oct 19, 2024
Copy link
Member

@Joehuu Joehuu left a comment

Choose a reason for hiding this comment

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

Behavior looks good to me otherwise.

@@ -160,6 +168,20 @@ public void TestKeyboardInput()
InputManager.ReleaseKey(Key.Right);
});
checkValue(1);

AddStep("Focus slider", () => sliderBar.GetContainingFocusManager()!.ChangeFocus(sliderBar));
Copy link
Member

Choose a reason for hiding this comment

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

This should test click by not using programmed focus:

diff --git a/osu.Framework.Tests/Visual/UserInterface/TestSceneSliderBar.cs b/osu.Framework.Tests/Visual/UserInterface/TestSceneSliderBar.cs
index 797cba1c4..e63e8bbec 100644
--- a/osu.Framework.Tests/Visual/UserInterface/TestSceneSliderBar.cs
+++ b/osu.Framework.Tests/Visual/UserInterface/TestSceneSliderBar.cs
@@ -169,7 +169,9 @@ public void TestKeyboardInput()
             });
             checkValue(1);
 
-            AddStep("Focus slider", () => sliderBar.GetContainingFocusManager()!.ChangeFocus(sliderBar));
+            AddStep("Click slider", () => InputManager.Click(MouseButton.Left));
+
+            AddAssert("Slider has focus", () => sliderBar.HasFocus);
 
             AddStep("move mouse outside", () =>
             {
@@ -181,7 +183,7 @@ public void TestKeyboardInput()
                 InputManager.PressKey(Key.Right);
                 InputManager.ReleaseKey(Key.Right);
             });
-            checkValue(2);
+            checkValue(-4);
         }
 
         [TestCase(false)]

@bdach bdach added the area:UI label Oct 21, 2024
@bdach bdach requested a review from peppy October 21, 2024 05:54
@peppy
Copy link
Member

peppy commented Oct 21, 2024

The Basic display's active state is quite shoddy. Haven't dived into the code yet.

@peppy peppy added the next-release Pull requests which are almost there label Oct 21, 2024
@peppy
Copy link
Member

peppy commented Oct 21, 2024

There's no osu! side PR to see how this will look yet is there?

@bdach
Copy link
Collaborator

bdach commented Oct 21, 2024

ppy/osu#30315 was vaguely linked but does not actually use anything from this PR or include any visual changes to sliders to indicate focus.

@peppy
Copy link
Member

peppy commented Oct 21, 2024

I think I'd want to see that, just to confirm everything is as it should be, before going ahead with this.

Also arguably, this is changing the UX of this control and other framework consumers may prefer the old behaviour. I'm relatively neutral on it, but maybe we want focus to be given when hovering and arrow keys are used to keep the old behaviour also working?

@OliBomby
Copy link
Contributor Author

maybe we want focus to be given when hovering and arrow keys are used to keep the old behaviour also working?

This suggestion confuses me. The way I have it implemented now does allow you to use the old behaviour, that is adjusting the slider bar with keyboard while hovering without focusing. To allow keyboard input it needs either focus or hover state.
To give (persisting) focus when hovering is not necessary and its not how it works in OSes, which I aim to mimic.

@peppy
Copy link
Member

peppy commented Oct 22, 2024

Right, it has been fixed in the latest commits.

@peppy
Copy link
Member

peppy commented Oct 22, 2024

I'm tentatively okay with this, but would still want to see the osu!-side change before merging.

@peppy peppy self-requested a review October 24, 2024 10:09
@peppy peppy merged commit 451f8b2 into ppy:master Oct 24, 2024
19 of 21 checks passed
@OliBomby OliBomby deleted the focus-sliderbar branch October 24, 2024 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:UI next-release Pull requests which are almost there size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants