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 mod search textbox having focus while settings are visible #25857

Merged
merged 3 commits into from
Dec 19, 2023

Conversation

peppy
Copy link
Member

@peppy peppy commented Dec 18, 2023

Stopped arrow key adjust on slider bars from working.

Also just felt wrong that you could type into an off-screen textbox.

Closes #25842

Stopped arrow key adjust on slider bars from working.

Also just felt wrong that you could type into an off-screen textbox.
@bdach
Copy link
Collaborator

bdach commented Dec 18, 2023

The way this is implemented is pretty hacky and directly fails one of the tests (TestSceneModSelectOverlay.TestSearchFocusChangeViaClick).

  • If the search textbox starts focused, or gains focus via a Tab key press, it cannot be unfocused by mouse.
  • If the search textbox does not start focused and is focused via mouse instead, it can be unfocused by mouse.
2023-12-18.20-38-48.mp4

I'm not sure what to make of that. Honestly seems like weird behaviour.

@peppy
Copy link
Member Author

peppy commented Dec 18, 2023

Hmm, I guess? Depends on how you see it working (HoldFocus makes it behave like the song select one while in focus mode).

Alternative would be making a tracking bool and handling focus more manually as per before. Should only require a few lines of change.

@bdach
Copy link
Collaborator

bdach commented Dec 18, 2023

I'd also accept having both mouse and keyboard hold focus and not give it back (plus removing that failing test; rest should start passing with that change I believe). So long as you can't drop the focus one way but not the other.

@bdach
Copy link
Collaborator

bdach commented Dec 18, 2023

#25880 also needs investigation here (potentially a regression from this PR as it was cherrypicked into the hotfix release)

@bdach bdach added the next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! label Dec 19, 2023
@peppy
Copy link
Member Author

peppy commented Dec 19, 2023

I've reverted to a safer approach to handling this, let me know what you think. Not 100% sure on the private method taking an argument, can refactor that if required.

@pull-request-size pull-request-size bot added size/M and removed size/S labels Dec 19, 2023
Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

Seems okay. Have added appropriate test coverage.

@bdach bdach merged commit 64b0534 into ppy:master Dec 19, 2023
15 of 17 checks passed
@peppy peppy deleted the fix-mod-settings-keyboard-adjust branch December 22, 2023 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:song-select next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The input form for mod search is active when opening mod customization
2 participants