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

Keyboard accel fixes and Home key support #1294

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

toolstack
Copy link
Contributor

This PR does a few things:

  • re-implements the home key support in the main view (see PR Add Home shortcut for scroll to top of page. #1251 and revert PR revert: "feat: scroll to top on Home (#1251)" #1259)
  • Disable appropriate global accelerators in text boxes and dialogs
  • Fixes the duplicate dialog boxes that can triggered with multiple global accelerator hits (aka you can open multiple about boxes by hitting F1 multiple times, as well as other dialogs that can be triggered in this way).
  • Fixes opening dialog boxes over top of each other (aka opening About and then hitting Ctl+, to open preferences)

@GeopJr
Copy link
Owner

GeopJr commented Dec 23, 2024

Thanks!

I hate to stretch the discussion even further on this, but I still am not 100% comfortable with the Home shortcut. I don't know the extend of how much it messes with accessibility and keyboard navigation. For example, it won't be possible to move to the first item of any list.

From the sidebar to the hashtags in explore. The previous behavior would focus on the first item, the current behavior moves the scroll position to the top.

Disable appropriate global accelerators in text boxes and dialogs

I understand that this is the best solutions of the ones we had, but... it increases the amount of testing that needs to be done on pretty much any new widgets which I 100% will forget to do.

I mean, for dialogs, we can subclass them into "AccelSafeDialog" that toggles accels automatically. But it's messy on GtkEntry, GtkText and any other widgets we'd have to manually capture events on.

@toolstack
Copy link
Contributor Author

I hate to stretch the discussion even further on this, but I still am not 100% comfortable with the Home shortcut.

Np, and happy to continue discussing until it's right (or as right as it can be 😉).

I don't know the extend of how much it messes with accessibility and keyboard navigation. For example, it won't be possible to move to the first item of any list.

I understand the concern, however the same concern should be with the pgup/dn keys as well, as they have the same kind (if not necessarily the same magnitude) of accessibility impact. You can't currently scroll any list up and down with the keyboard at the moment either.

Perhaps having a preference to enable/disable both of these separately? Leave pgup/dn enabled by default and home disabled by default to keep the existing behaviour?

Or if accessibility is the overriding concern, set them both as disabled?

Not having the home key scroll the same content as pgup/dn seems fundamentally broken to me as an end-user. And I don't think I'd be the only one with that opinion 😉

From the sidebar to the hashtags in explore. The previous behavior would focus on the first item, the current behavior moves the scroll position to the top.

I think the current behaviour is very counter intuitive, it should be fixed one way or the other to be consistent as to what the keyboard navigation does. This might even include mapping the end key as well for consistency, on a page like the hashtags it would take you to the bottom of the list, but it might not do anything in the main home page as there is no real end 🤷.

The existing behaviour is also very dependent on what you have focused and what is visible. For example, if you have the main content focused, then home doesn't move the left hand list... or if it isn't displayed due to size restrictions.

Disable appropriate global accelerators in text boxes and dialogs

I understand that this is the best solutions of the ones we had, but... it increases the amount of testing that needs to be done on pretty much any new widgets which I 100% will forget to do.

Yes and no, even if home isn't implemented, the testing will need to be done to ensure pgup/dn or the other accels don't break things (like opening multiple of the same dialog).

And completely understand the desire to reduce the long term effort.

I mean, for dialogs, we can subclass them into "AccelSafeDialog" that toggles accels automatically. But it's messy on GtkEntry, GtkText and any other widgets we'd have to manually capture events on.

Agreed, it's not a great solution, but I don't see a better one at the moment. I have to go through and make these same changes to Folio as well as was hoping someone might see a better solution 😉

I think GTK/Adwita is a little (maybe a lot?) broken here, and looking at some other third party GTK apps I use they all appear to be broken in the same way. First party stuff seems to be a mixed bag, it looks like Gnome Web seems to block the accels when the preferences dialog is open, but Gnome Files seems to "reopen" the preferences dialog by closing the first instance before letting the second be created.

@toolstack toolstack force-pushed the keyboard-accel-fixes branch from 9646eea to 2995763 Compare December 31, 2024 02:30
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.

2 participants