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.

Sign up for GitHub

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

Jump to bottom

Fix/ Improve accessibility of playlist icon buttons #4943

Merged
merged 5 commits into from
Apr 14, 2024
Merged

Fix/ Improve accessibility of playlist icon buttons #4943

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bab0dad
Add aria-pressed attribute to playlist icon button
sossost Apr 13, 2024
f30a042
Improve accessibility by adjusting stroke thickness of playlist icon
sossost Apr 13, 2024
e72b5b7
Fix to keep the inactive hover state the same
sossost Apr 13, 2024
445f039
Update playlist icon active hover style
sossost Apr 13, 2024
fb1422a
Update playlist icon active hover color to --accent-color-hover
sossost Apr 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/renderer/components/watch-video-playlist/watch-video-playlist.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,16 +51,22 @@
border-radius: 50%;
color: var(--tertiary-text-color);
transition: background 0.2s ease-out;
stroke-width: 20;
stroke: var(--bg-color);
}

.playlistIcon:hover {
background-color: var(--side-nav-hover-color);
color: var(--side-nav-hover-text-color);
stroke-width: 10;
stroke: var(--side-nav-hover-text-color);
transition: background 0.2s ease-in;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
.playlistIcon:hover {
background-color: var(--side-nav-hover-color);
color: var(--side-nav-hover-text-color);
stroke-width: 10;
stroke: var(--side-nav-hover-text-color);
transition: background 0.2s ease-in;
}
.playlistIcon:hover {
background-color: var(--side-nav-hover-color);
color: var(--side-nav-hover-text-color);
transition: background 0.2s ease-in;
}

This looks great @sossost, thanks for doing this so quickly! My only suggestion is that we keep the inactive hover state the same, as otherwise this makes it seem like it's active.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback, @jasonhenriquez! I appreciate your quick review. I agree with keeping the inactive hover state unchanged to avoid confusion about its activity status.
In this case, need to change the stroke color to the hover background color without deleting the style from the hover properties.
I used stroke color to make the icons thin without changing them.
스크린샷 2024-04-14 오전 1 24 13
스크린샷 2024-04-14 오전 1 23 57

I'll make the necessary adjustments and update the pull request shortly.

Copy link
Collaborator

@kommunarr kommunarr Apr 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the update! However, It's now not keeping the wider stoke width on active hover. You can add an additional rule for .playlistIconActive:hover. I think it's a mistake in the existing implementation that the active color is lost on hover.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing that out! I missed updating the stroke width for the active hover state. I'll add a rule for .playlistIconActive:hover to ensure the wider stroke width and active color are maintained during hover. I'll make this change and update the pull request shortly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @sossost! This is working perfectly now. My last thought is that while we're here, can you make the .playlistIconActive:hover's stroke & color be --accent-color-hover? I think this was another pre-existing bug I didn't even notice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @ jasonhenriquez! I'm glad to hear it's working well now. I agree with your suggestion to update the .playlistIconActive:hover stroke and color to --accent-color-hover. It sounds like a good catch on a pre-existing issue. I'll make these changes and push the update soon.


.playlistIconActive {
color: var(--accent-color)
color: var(--accent-color);
stroke-width: 10;
stroke: var(--accent-color);
}

.playlistItems {
Expand Down
4 changes: 4 additions & 0 deletions src/renderer/components/watch-video-playlist/watch-video-playlist.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
:title="$t('Video.Loop Playlist')"
role="button"
tabindex="0"
:aria-pressed="loopEnabled"
@click="toggleLoop"
@keydown.enter.prevent="toggleLoop"
@keydown.space.prevent="toggleLoop"
Expand All @@ -67,6 +68,7 @@
:title="$t('Video.Shuffle Playlist')"
role="button"
tabindex="0"
:aria-pressed="shuffleEnabled"
@click="toggleShuffle"
@keydown.enter.prevent="toggleShuffle"
@keydown.space.prevent="toggleShuffle"
Expand All @@ -78,6 +80,7 @@
:title="$t('Video.Reverse Playlist')"
role="button"
tabindex="0"
:aria-pressed="reversePlaylist"
@click="toggleReversePlaylist"
@keydown.enter.prevent="toggleReversePlaylist"
@keydown.space.prevent="toggleReversePlaylist"
Expand Down Expand Up @@ -109,6 +112,7 @@
:title="$t('Video.Pause on Current Video')"
role="button"
tabindex="0"
:aria-pressed="pauseOnCurrentVideo"
@click="togglePauseOnCurrentVideo"
@keydown.enter.prevent="togglePauseOnCurrentVideo"
@keydown.space.prevent="togglePauseOnCurrentVideo"
Expand Down