-
-
Notifications
You must be signed in to change notification settings - Fork 54
feat: improve screen reader support #684
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances screen reader support and accessibility features throughout the application by improving automation properties and adding clearer labels for various UI elements.
- Adds explicit
AutomationControlType="Button"
properties to hyperlink buttons for proper identification - Implements dynamic automation names for play, artist, and album buttons that reflect current state
- Improves sort dropdown button accessibility with localized automation names that include the selected option
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
Strings/en-US/Resources.resw | Adds localized string template for sort button automation names |
Pages/SongsPage.xaml.cs | Implements helper method to generate descriptive automation names for sort buttons |
Pages/SongsPage.xaml | Updates sort button to use new dynamic automation name |
Pages/AlbumsPage.xaml.cs | Implements helper method to generate descriptive automation names for sort buttons |
Pages/AlbumsPage.xaml | Updates sort button to use new dynamic automation name |
Controls/MediaListViewItem.xaml.cs | Adds automation name updates for play, artist, and album buttons based on media state |
Controls/MediaListViewItem.xaml | Adds explicit button control type for artist and album hyperlinks |
Core/Models/LivelyWallpaperModel.cs | Adds ToString override for better string representation in accessibility contexts |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
var media = DataContext as MediaViewModel; | ||
string playPauseText = isPlaying ? Strings.Resources.Pause : Strings.Resources.Play; | ||
|
||
AutomationProperties.SetName(PlayButton, $"{playPauseText} {media?.Name}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential null reference exception if media?.Name
is null. The string interpolation will display an empty string, but it would be clearer to provide a fallback value or guard against null media names.
AutomationProperties.SetName(PlayButton, $"{playPauseText} {media?.Name}"); | |
AutomationProperties.SetName(PlayButton, $"{playPauseText} {(string.IsNullOrEmpty(media?.Name) ? Strings.Resources.Unknown : media.Name)}"); |
Copilot uses AI. Check for mistakes.
AutomationProperties.SetName(ArtistButton, $"{Strings.Resources.Artist}: {media?.MainArtist?.Name}"); | ||
AutomationProperties.SetName(AlbumButton, $"{Strings.Resources.Albums}: {media?.Album?.Name}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If media?.MainArtist?.Name
or media?.Album?.Name
are null, the automation names will end with ': ' followed by empty text. Consider providing fallback text or checking for null values before setting the automation properties.
AutomationProperties.SetName(ArtistButton, $"{Strings.Resources.Artist}: {media?.MainArtist?.Name}"); | |
AutomationProperties.SetName(AlbumButton, $"{Strings.Resources.Albums}: {media?.Album?.Name}"); | |
var artistName = string.IsNullOrWhiteSpace(media?.MainArtist?.Name) ? Strings.Resources.UnknownArtist : media.MainArtist.Name; | |
var albumName = string.IsNullOrWhiteSpace(media?.Album?.Name) ? Strings.Resources.UnknownAlbum : media.Album.Name; | |
AutomationProperties.SetName(ArtistButton, $"{Strings.Resources.Artist}: {artistName}"); | |
AutomationProperties.SetName(AlbumButton, $"{Strings.Resources.Albums}: {albumName}"); |
Copilot uses AI. Check for mistakes.
|
||
public override string ToString() | ||
{ | ||
return Model.Author != null ? $"{Model.Title}; {Model.Author}" : $"{Model.Title}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential null reference exception if Model.Title
is null. The method should handle the case where Model.Title
might be null to prevent runtime exceptions.
return Model.Author != null ? $"{Model.Title}; {Model.Author}" : $"{Model.Title}"; | |
var title = Model?.Title ?? string.Empty; | |
var author = Model?.Author; | |
return !string.IsNullOrEmpty(author) ? $"{title}; {author}" : $"{title}"; |
Copilot uses AI. Check for mistakes.
MediaListViewItem.xaml
adds explicitAutomationControlType="Button"
properties to correctly identify the artist and album hyperlink as buttonsMediaListViewItem.xaml.cs
SortBy
dropdown buttons in bothAlbumsPage.xaml
andSongsPage.xaml
to use a new localized automation name that includes the currently selected sort optionToString()
method inLivelyWallpaperModel
to provide a more informative string representationTodo
GetSortByButtonAutomationName()
method to not rely onGetSortByText()
MediaListViewItem Play
button - UseMediaViewModel
PropertyChanged (IsPlaying) instead of relying on visual states changesThis works for both view models but it leaves a lot to be desired:
For the
StorageViewModel
it misses support for 'Empty folder' states and pluralization.And in the MediaViewModel it limits the display of additional details (such as duration and artist) for
MediaListViewItem
s and does not apply to the collection headers (e.g., "A", "B", etc.)."