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

Improve search filter & modal accessibility #3975

Merged

Conversation

kommunarr
Copy link
Collaborator

@kommunarr kommunarr commented Aug 30, 2023

Improve search filter accessibility

Pull Request Type

  • Bugfix

Related issue

resolves accessibility issue outlined here, and a handful of other accessibility issues with the filter icon

Description

  • Revamps ft-search-filters implementation such that:
    • Mobile view is actually usable
    • Focus is removed when the modal is clicked out of
    • A close button is present to exit the modal
  • ft-prompt accessibility improvements:
    • Implements proper focus management for all ft-prompts using portal-vue and inert (reference)
    • Implements ARIA role and properties on ft-prompt
    • Makes label property required on ft-prompt, and adds it to the prompts with missing properties
  • Adds title attribute to search filter icon, ensuring screenreaders will be aware of what it is
  • Updates radio button styling to show the --accent-active-color color on hover and focus, resolving issue where currently focused radio button is impossible to visually determine

Screenshots

Device Before After
Desktop localhost_9080_(Nest Hub Max)-1 localhost_9080_(Nest Hub Max)
Tablet localhost_9080_(iPad Mini)-1 localhost_9080_(iPad Mini)
Mobile localhost_9080_(iPhone SE)-1 localhost_9080_(iPhone SE)

Testing

  • Confirm that the title "Search Filters" appears on hover of the filter icon.
  • Test desktop, tablet, and mobile views of Search Filters.
  • Test clicking on "Close" or outside of the modal closes the Search Filters modal.
  • (REG) Test search filters are remembered when changed and reopened.
  • (REG) Test search filter functionality.
  • (REG) Test desktop, tablet, and mobile views of Create Playlist and Add Video to Playlist prompts.
  • Test that focus is now maintained in the modal for any ft-prompt.
  • Verify that the currently focused or hovered radio button changes color to the --accent-active-color.

Desktop

  • OS: OpenSUSE Tumbleweed

Additional info

I can change it such that the hover is the plainer color. I don't have a preference.

@absidue absidue added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Sep 4, 2023
@efb4f5ff-1298-471a-8973-3d47447115dc

Boosting this now that playlists PR is merged

Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@kommunarr kommunarr force-pushed the fix-filter-accessibility branch from 752b475 to f89ca1f Compare April 18, 2024 12:13
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@kommunarr kommunarr added the PR: waiting for review For PRs that are complete, tested, and ready for review label Apr 20, 2024
@kommunarr kommunarr mentioned this pull request Apr 20, 2024
4 tasks
@PikachuEXE
Copy link
Collaborator

z-index issue in delete playlist prompt
image

Add video to playlist prompt > create playlist = error + add to playlist prompt invisible during create playlist (which is fine in function but feels strange)
image
image

@kommunarr
Copy link
Collaborator Author

Yeah, you're right about the idea to make a file with these z-indexes, I'm losing my mind

@kommunarr kommunarr requested a review from PikachuEXE April 22, 2024 22:32
PikachuEXE
PikachuEXE previously approved these changes Apr 23, 2024
@PikachuEXE
Copy link
Collaborator

I'm losing my mind

image

Copy link
Member

Choose a reason for hiding this comment

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

LGTM!

@kommunarr
Copy link
Collaborator Author

@absidue @MarmadileManteater @ChunkyProgrammer

My suggested ordering for reviewing my open PRs:
this -> #4374 / #4970 -> #4949 -> #5013 / #5029

Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added PR: merge conflicts / rebase needed and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Apr 29, 2024
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@ChunkyProgrammer ChunkyProgrammer added the PR: waiting for review For PRs that are complete, tested, and ready for review label Apr 30, 2024
@FreeTubeBot FreeTubeBot merged commit 4bb53f7 into FreeTubeApp:development May 1, 2024
5 checks passed
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label May 1, 2024
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.

6 participants