Keyboard shortcuts for full text search

[jarrodmoldrich]

Hi maintainers,

I really wanted keyboard access to the find bar and the results, so I made some changes. I tried to make them compatible with the target MacOS 13.x, but a couple of things could use newer functions. It fine for my purposes, but thought I'd send it upstream in case it's worthy of a merge

[kelson42]

@jarrodmoldrich Thank you very much for your PR, we will review it and share a feedback within a few days.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 15.78947% with 48 lines in your changes missing coverage. Please review.

Project coverage is 38.18%. Comparing base (1a673e5) to head (f9e46f4).
Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
Views/SearchResults.swift	0.00%	43 Missing ⚠️
App/App_macOS.swift	33.33%	4 Missing ⚠️
Views/BrowserTab.swift	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1079      +/-   ##
==========================================
- Coverage   38.31%   38.18%   -0.14%     
==========================================
  Files         120      120              
  Lines        6511     6555      +44     
==========================================
+ Hits         2495     2503       +8     
- Misses       4016     4052      +36     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kelson42]

@jarrodmoldrich Converting to draft as it seems you are still working on rhe PR. Move back to ready to review when you will have finished.

[BPerlakiH]

Hi @jarrodmoldrich, Thank you for your PR.
Initially it looks good, there was one out of bounds issue when navigating downwards, from the last cell it was possible to run out of the array bounds. I pushed a commit for that.

So my assumption is that it's more like a power user feature, for navigating without a mouse.
In that case, there's another UX issue with this implementation, is that the focus movement on the search results is not triggering a scroll (down):

[BPerlakiH]

We need a way to trigger scroll for the search results as we move the focus up/down.

[jarrodmoldrich]

@BPerlakiH Thanks for taking the time to review!

@kelson42 I apologise for all the noise. You're right to make it a draft, as it's more a proposal than contribution at this stage.

It looks like I've opened a can of worms with this PR 😅 Essentially this was me just 'scratching my own itch' as a keyboard user, but also is an accessibility feature which could open usage of the app to the disability community. That said, the way I've implemented it is non-standard. Using the enter key to navigate to the first search result isn't intuitive or easily discoverable, and could confuse users. There's a couple of options I can think of to solve it:

Option a) I could try and make it so that the down arrow goes to the first result, and an up arrow from the first result goes back to the search bar.

Option b) Use a dropdown and remove the search results page. This would also improve the UX a bit on desktop by not hiding the currently viewed page in order to search for something – which means the user is free to refer to something on the page they are viewing while searching.

A way to do this might be to use the .searchSuggestions modifier. Clicking on a suggestion replaces the search text with the value, so we might need to intercept any value starting with zim:// to clear the search and actually navigate to the result. Haven't tried, but it could work. It takes up less screen width, and I could see existing users being upset by this.

I'm thinking I'll do option a) for now, and see if I can make the search results scroll along with focus. It's up to you if you want to merge it, or perhaps you might think there's a better approach towards accessibility. I gleaned a lot of insights from this video about how focus works on the various Apple OSes. I's a non-trivial amount of work to implement it across the app, but it could be a big win for the disability community and impatient keyboard users like me.

[BPerlakiH]

Thank you @jarrodmoldrich. Option A sounds good to me, as it's similar to TVOS focus behaviour.

[jarrodmoldrich]

@BPerlakiH No worries. That’s already done. Seems fine on MacOS and iOS simulator, but could do with some extra checks, preferably on a real phone. If everything checks out I’ll fix up the tests.

[BPerlakiH]

@jarrodmoldrich Thank you for the changes. My suggestion is the following, based on your PR, I have created the following changes:
#1084

Which allows us to keep the scope of these changes only for macOS (iOS is not affected).

For the contextual reasons of the search bar (which can be or cannot be seen on the screen, depending on where you are in the app) it won't contain the keyboard short-cut suggested by you in a global form.

On the other hand, it's possible to navigate to the search bar with a single TAB button, which I think is equally good, plus you can close the search results with the ESC button,
and using the native focus + scroll solution go up and down the list of results, and use RETURN to select one.

[jarrodmoldrich]

HI @BPerlakiH,

All that makes sense!

it's possible to navigate to the search bar with a single TAB button

For me, I initially tried using tab to navigate to the search bar, but each time I would Cmd-Tab to the application it would sometimes go to a different focusable item when I tabbed. As I use this in concert with another application, this solution doesn't work for me and exactly this was the catalyst for me starting this yak shave 😂 Also for me I much prefer to use the cursor keys to navigate to and from search results, as this imitates how the native searchSuggestions works, even if it requires a bit of hackery. So in the end I will probably just use my fork.

I'm happy this PR has had a positive effect on the project! Your code is much much cleaner and idiomatic, so feel free to close this PR in preference to yours. Let me know if there's anything else you want me to do.

Cheers!

[BPerlakiH]

Thank you for your feed back and PR @jarrodmoldrich. I will keep the focus navigation on my radar. Maybe it will be possible to have what you require implemented without the custom work-around, as the search results and the search bar are really "in-line" (one above the other). For now, I am closing this PR. There's a new issue opened for the focus movement part, that you can follow: #1090