Skip to content

Fix Sidebar Menu Focus #7829

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

regulus79
Copy link
Contributor

This PR makes it so that the spacebar will not keep triggering open/closing the sidebar menus (like the instruments tab, and the file browser tabs).

All that was required was to set the focus policy of the close button to Qt::NoFocus which prevents the spacebar from triggering it after you click on it.

@regulus79 regulus79 mentioned this pull request Apr 4, 2025
@regulus79
Copy link
Contributor Author

Wait, Do not merge this yet! I just realized that my strategy of disabling focus for buttons in order to prevent the spacebar from triggering them could be a bad idea if we ever want to implement better keyboard accessibility support in the future. Setting Qt::NoFocus disables not only the spacebar, but also the enter key, which may not be desirable for keyboard users. I would have to find some way to only disable the spacebar without touching any other focus keys. Perhaps this could be done by doing a kind of global override for every widget's keyPressEvent? I will have to look into it.

Of course, we don't really have good keyboard support anyways.... but still, I kind of want to be on the road to adding more accessibility features to lmms in the future.

@regulus79 regulus79 marked this pull request as draft April 9, 2025 00:09
@tresf
Copy link
Member

tresf commented Apr 9, 2025

Wait, Do not merge this yet! I just realized that my strategy of disabling focus for buttons in order to prevent the spacebar from triggering them could be a bad idea if we ever want to implement better keyboard accessibility support in the future.

Agreed, however if you search the source code, we use this trick quite liberally.

the spacebar will not keep triggering open/closing the sidebar menus (like the instruments tab, and the file browser tabs).

I can't reproduce this issue on macOS although I did take a minute to try to write a patch, I just have no easy way of testing this.

Furthemore, even with your patch, if a plugin is clicked in the plugin listing, the spacebar stops working again.

@regulus79
Copy link
Contributor Author

I can't reproduce this issue on macOS

For me, it only occurs when you first press the small button to close the tab; normally closing/opening the tabs via the icons doesn't cause the issue.

I did take a minute to try to write a patch

Wow, that actually sounds like a great idea, making a dedicated class with the event filter would definitely save a lot of redundant code!

Furthemore, even with your patch, if a plugin is clicked in the plugin listing, the spacebar stops working again.

I will have to test this.

@tresf
Copy link
Member

tresf commented Apr 10, 2025

For me, it only occurs when you first press the small button to close the tab; normally closing/opening the tabs via the icons doesn't cause the issue.

Ok, thanks. I can reproduce this, but the PR says the following:

keep triggering open/closing the sidebar menus

The only thing I can reproduce locally is by clicking the "X" to close the sidebar and the moving my mouse away, I can then hit Space to close it. Enter does nothing and I can't find a way to "... open the sidebar menus..." (In this case, when you say "menu", do you mean the sidebar itself or something else?)

@tresf
Copy link
Member

tresf commented Apr 10, 2025

sidebar.mov

@regulus79
Copy link
Contributor Author

The only thing I can reproduce locally is by clicking the "X" to close the sidebar and the moving my mouse away, I can then hit Space to close it.

That is odd, on my device running Arch Linux + GNOME after pressing the X button, I can keep pressing the space bar to open/close the sidebar.

Edit: Wait, somehow it's only happening for the isntrument tab for me? I only just realized this when recording the video... that is even more odd.

2025-04-10.16-22-39.mp4

Enter does nothing

WHOOPS you're totally right, I just assume that enter and space did the same thing, when it appears that only space bar triggers it.

@regulus79
Copy link
Contributor Author

I'm wondering whether I should just apply the Qt::NoFocus for now, just so that we can merge #7762, and then maybe later work on improving keyboard navigation. I think that might be the better move.

@regulus79 regulus79 changed the title Fix Sidebar Menu Spacebar Fix Sidebar Menu Focus Apr 20, 2025
@tresf
Copy link
Member

tresf commented Apr 21, 2025

I'm wondering whether I should just apply the Qt::NoFocus for now, just so that we can merge #7762, and then maybe later work on improving keyboard navigation. I think that might be the better move.

That's fine with me -- since we've done this in the past -- as long as we remain open to reverting it all in the future in favor of a proper solution.

@regulus79
Copy link
Contributor Author

This PR may have to be rethought, since it appears there is more than just the close button which is getting focus. If I have an item selected in one of the file browsers, the issue occurs.

Or, if we want to convert this PR/make a new one to implement your patch to replace all the places where Qt::NoFocus is used, maybe we can do that sometime. For now though, #7762 should not be blocked by this.

@szeli1
Copy link
Contributor

szeli1 commented Apr 22, 2025

improving keyboard navigation

I would like to mention that I have a plan for keyboard navigation that is in the scope of #7734
PRs will come shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants