Fix recent searches keyboard navigation

[obulat]

Fixes

Fixes #3195 by @fcoveram
Fixes #480 by @obulat

Description

This PR refactors the way the keyboard navigation in a search layout on a narrow screen works.

I extracted non-related changes to other PRs, and will un-draft this PR when those PRs are merged.
Some other related improvements in this PR are:

• handleClear and handleClearSingle functions were combined into one function with an optional entry argument. If entry is passed to the function, only that entry is removed, otherwise, all recent search entries are removed.
• the mobile modal now displays "No recent searches" text just like the desktop when recent searches list is blank.

Testing Instructions

Run the app using just frontend/run dev:only, and open localhost:8443/search?q=cat with a narrow screen.
Try navigating on the page using the keyboard. If you get to the search input, and press Tab again, you will navigate to the content settings button, and then - to the main contain. In production, you will not be able to navigate to the content settings button because when you get inside the search input, the recent searches modal opens, and then you can only close it using the Back button at the right side of the search input.

If you navigate using the keyboard to the search input, and then press a letter or an arrow key, the recent searches modal will open. Here, you can select one of the options using arrow keys. You can also clear recent searches by clicking "Clear all" or "Clear" cross button next to one of the recent searches.

When you click on the search input, the recent searches modal will open, and the cursur will be where you clicked (i.e, if you click in the middle of the search word, it will stay there and not move to the end of the search term as in prod"

Checklist

• My pull request has a descriptive title (not a vague title likeUpdate index.md).
• My pull request targets the default branch of the repository (main) or a parent feature branch.
• My commit messages follow best practices.
• My code follows the established code style of the repository.
• I added or updated tests for the changes I made (if applicable).
• I added or updated documentation (if applicable).
• I tried running the project locally and verified that there are no visible errors.
• I ran the DAG documentation generator (just catalog/generate-docs for catalog
  PRs) or the media properties generator (just catalog/generate-docs media-props
  for the catalog or just api/generate-docs for the API) where applicable.

Developer Certificate of Origin

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

[AetherUnbound]

This mostly makes sense to me! A few things I noticed when running this locally:

• I'm not able to navigate to the "Clear" button from the search bar with the recent searches modal open using the Tab key. Is that intentional?
• Similarly, when navigating through the recent searches, I'm not able to navigate to the X key using Tab (the Tab key only iterates over entries). Is that expected as well?

[AetherUnbound]

This comment seems incongruous with the logic below, is it describing something else?

[AetherUnbound]

Are there any other functionality tests we should add for the new behavior added to this file?

[obulat]

I added some tests for keyboard navigation.

[obulat]

• I'm not able to navigate to the "Clear" button from the search bar with the recent searches modal open using the Tab key. Is that intentional?

It wasn't intentional, and is fixed now. Use arrows for navigating between the search entries, and tabs to move to "Clear" button and "Clear " buttons.

[zackkrida]

@obulat please ping me when you think this is stable enough for final review! I don't want to review prematurely and miss any changes

[obulat]

@obulat please ping me when you think this is stable enough for final review! I don't want to review prematurely and miss any changes

I think it's ready for review now

[AetherUnbound]

I tried the testing steps locally and it doesn't seem like the current search shows up in the recent searches. If you also navigate to any of the recent searches, then tab out of the modal, the search text still remains as the recent search (rather than reverting to the current search). Are both of those intentional?

[zackkrida]

LGTM as it fixes the linked issues well. to @AetherUnbound's points, I believe those are existing bugs we need to rectify. To clarify what those bugs are, and list some more:

• Homepage searches are not added to the recent search list 😢
• Hovering over recent searches updates the search input text (great!) but removing focus from the final choice doesn't clear the input state back to the original search term
• The recent search popup can not be closed with the keyboard when there are no recent searches. It relies on a focus loss of the "clear" button to hide itself and this is impossible when the clear button isn't displayed
• Recent searches can not be navigated into via the keyboard to focus their individual removal buttons

[AetherUnbound]

Gotcha, thanks for that clarification Zack. LGTM otherwise!

[fcoveram]

I agree with @zackkrida's notes. A mockup/prototype showing the ideal flow could help align our expectations. And once we land on a solution, implement it throughout different PRs.

What do you think of this idea?

[AetherUnbound]

@fcoveram I think that's a great idea!
@obulat do you mind sharing what's changed functionality wise since my last review? I don't see that captured in a comment or the PR description but I might be missing something.

PR has undergone significant change

[obulat]

I wrote why I asked for a new review yesterday, but the comment didn't get posted.

I refactored the way the recent searches are saved in the store, and most of the points from @zackkrida's message are fixed.

LGTM as it fixes the linked issues well. to @AetherUnbound's points, I believe those are existing bugs we need to rectify. To clarify what those bugs are, and list some more:

• Homepage searches are not added to the recent search list 😢

The way we used local storage in the search store was persisting the state intermittently. I tried different options, but the only one that worked consistently was to create a function that persists the recent searches by both setting the recentSearches state property and the value of useStorage composable.

• Hovering over recent searches updates the search input text (great!) but removing focus from the final choice doesn't clear the input state back to the original search term

Hovering does not update the input text, but selecting the options using arrow keys does (Google search box has the same behavior). Then, if you close the recent searches popover/modal using the "Back" button (in the narrow view), the "Escape" or "Enter" keys, the search is executed with the selected recent search.
We could make it so that pressing "Escape" key or clicking "Back" button reset to the original search term, but I think keeping the term that is visible in the search bar is more intuitive.

• The recent search popup can not be closed with the keyboard when there are no recent searches. It relies on a focus loss of the "clear" button to hide itself and this is impossible when the clear button isn't displayed

The handler for hiding the recent searches when there are no recent searches wasn't working because the event handler wasn't falling through to the VButton component. Fixed in the most recent commit.

• Recent searches can not be navigated into via the keyboard to focus their individual removal buttons

You need to use the arrow keys to navigate to the recent search entries. But from there, you cannot navigate into the individual clear buttons. You can navigate into the individual removal buttons using "Tab" key on a narrow screen (they come after the main "Clear" button in the tabbing order), but not on desktop.
I tried searching for what the behavior should be in such cases when the combobox options have inner actions (other than searching when you press "Enter" on the option), but couldn't find any. I like @fcoveram's idea of creating a separate issue for finding the ideal flow for these individual actions.

@zackkrida, @AetherUnbound, could you re-review checking that the previous fixes are still working, and the 3 points here were also addressed.

• Clear all recent searches and search from the homepage. The recent searches should contain the latest search entry.
• Clear all recent searches and open an SSR-ed search page (http://localhost:8443/search?q=cat). The recent searches should have "cat" option.
• On desktop, navigate into the recent searches popover using "Tab" key, clear all the recent searches, and tab out of the popover. It should close.
• Select the recent searches using vertical arrow keys. The search bar text should be updated. If you press "Enter" or close the popover using "Back" button on narrow screen or "Escape" key, the search results for the updated search term should appear.

[AetherUnbound]

We could make it so that pressing "Escape" key or clicking "Back" button reset to the original search term, but I think keeping the term that is visible in the search bar is more intuitive.

I think this is the behavior that I'd expect, but if the current implementation is what makes the most sense than I won't block it!

Edit: That said, having the current search in the recent searches makes the existing behavior much easier to deal with!

[AetherUnbound]

Thanks for the update, the current feature set is working well for me!

[obulat]

Edit: That said, having the current search in the recent searches makes the existing behavior much easier to deal with!

Right, I forgot to update the long text in the description to say that I re-added the current search to the recent searches. I looked at other examples of recent searches, and realized that if someone selected a different option, but wants to go back to the original search term, we need to have it in the options list.

[openverse-bot]

Based on the medium urgency of this PR, the following reviewers are being gently reminded to review this PR:

@zackkrida
This reminder is being automatically generated due to the urgency configuration.

Excluding weekend¹ days, this PR was ready for review 5 day(s) ago. PRs labelled with medium urgency are expected to be reviewed within 4 weekday(s)².

@obulat, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings.

Footnotes

1. Specifically, Saturday and Sunday. ↩

2. For the purpose of these reminders we treat Monday - Friday as weekdays. Please note that the operation that generates these reminders runs at midnight UTC on Monday - Friday. This means that depending on your timezone, you may be pinged outside of the expected range. ↩

[zackkrida]

I would've sworn I approved this last week. It's much improved; lgtm