Additional accessibilityLabels for VoiceOver users (take #3) #1228
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR enhances accessibility for VoiceOver users by adding and refining accessibility labels, elements, and values across various UI views, while also addressing an infinite recursion issue related to the BLESignalStrengthIndicator.
- Added accessibility modifiers (.accessibilityElement, .accessibilityLabel, .accessibilityValue) to improve VoiceOver navigation
- Introduced computed accessibility descriptions in views like NodeListItem and SignalStrengthIndicator, plus a dedicated PluggedInIndicator for battery status
- Adjusted accessibility grouping for more consistent VoiceOver behavior in device connectivity and battery display
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Meshtastic/Views/Nodes/NodeList.swift | Added accessibility element modifiers to improve VoiceOver focus on ZStack content and sidebar control |
| Meshtastic/Views/Nodes/Helpers/NodeListItem.swift | Implemented a computed accessibility description for node items and grouped the row for VoiceOver |
| Meshtastic/Views/Nodes/Helpers/NodeInfoItem.swift | Grouped accessibility elements to present a concise description of node info |
| Meshtastic/Views/Nodes/Helpers/NodeDetail.swift | Enhanced accessibility grouping for sections and tap gestures throughout node details |
| Meshtastic/Views/Nodes/Helpers/Actions/IgnoreNodeButton.swift | Added an accessibility comment for improved VoiceOver labeling |
| Meshtastic/Views/Messages/TextMessageField/TextMessageSize.swift | Added accessibility label and value for message size display |
| Meshtastic/Views/Messages/TextMessageField/RequestPositionButton.swift | Introduced an accessibility label for the request position button |
| Meshtastic/Views/Helpers/ConnectedDevice.swift | Adjusted accessibility for representing connected/disconnected Bluetooth states |
| Meshtastic/Views/Helpers/BatteryGauge.swift | Split battery gauge display between normal and plugged-in states with dedicated accessibility support |
| Meshtastic/Views/Helpers/BatteryCompact.swift | Grouped battery icon and text into an accessible container, adding localized accessibility labels and values |
| Meshtastic/Views/Helpers/BLESignalStrengthIndicator.swift | Added a computed accessibility description and grouped accessibility children for the signal strength display |
| Meshtastic/Extensions/String.swift | Added a helper to format node names for VoiceOver with a prefixed descriptor |
Comments suppressed due to low confidence (1)
Meshtastic/Views/Helpers/BatteryCompact.swift:69
- [nitpick] Consider localizing or replacing the abbreviation 'PWD' with a more descriptive text for better clarity with VoiceOver.
Text("PWD")
| } else if let longName = node.user?.longName { | ||
| desc = longName | ||
| } else { | ||
| desc = "unknown node" |
There was a problem hiding this comment.
Consider using NSLocalizedString to localize the default description 'unknown node' for VoiceOver users.
Copilot uses AI. Check for mistakes.
| with: "$1 $2", | ||
| options: .regularExpression | ||
| ) | ||
| return "Node " + spaced |
There was a problem hiding this comment.
[nitpick] Consider localizing the 'Node ' prefix to support multiple languages in the VoiceOver output.
Copilot uses AI. Check for mistakes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changed?
Redo of previous pull-request with fix for infinite recursion that was happening on Mac.
Added accessibility tags to a lot of the sections that were hard/impossible to navigate without sight.
Why did it change?
After sitting with a Visually impaired user understood the lack of enthusiasm with the Meshtastic radio.
How is this tested?
Tested on both iphone and macbook pro.
Ensured there are no visual changes and with VoiceOver my visually impaired tester got it all going now.
Trying to stay out of the hair of the ongoing localization efforts.
@garthvh believe I finally got to the bottom of it. After some lengthy debugging found the code was crashing with a very deep stack (more 40 thousand deep). i.e. some unwanted recursion going on.
Culprit is the Divided wrapper extension in BLESignalStrengthIndicator.swift being applied over and over again by SwiftUI’s accessibility machinery until we blow the stack.
I had an accessibilityHidden(true) on the RoundedRectangles there and problem goes away without that.
The .accessibilityElement(children: .ignore) above it makes in unnecessary to hide the individual parts so the VoiceOver works fine too.
Checklist