VoiceOver accessibility improvements for the clipboard history panel - #1448
Conversation
7cdb6b9 to
a836b2b
Compare
weisJ
left a comment
weisJ
left a comment
There was a problem hiding this comment.
Thank you for stepping up to implement this. I left few comments which should make the implementation cleaner and also make it easier to keep new code accessible.
|
Thank you so much for the detailed review — this is genuinely useful feedback and I appreciate you taking the time. I've pushed a commit addressing all six points:
On the Let me know if any of these choices don't fit how you'd rather see it done — happy to adjust. |
You mentioned that voice over seems to reach the popup for you. Is that reliable? If yes I think we can leave the version as is. Otherwise I think we could incorporate a compatibility mode for VoiceOver. I will also test this locally so see whether I can reproduce a possible issue here. |
|
On reliability: so far it's been consistent for me. I've tested opening the popup multiple times in a row, both via the keyboard shortcut and by clicking the menu bar icon, and VoiceOver has reached it every time — I haven't hit a silent/unreachable case yet. I've also just been using Maccy day-to-day since testing this PR (it's genuinely become one of my favorite tools), so this isn't a one-off test, it's real daily use. That said, I don't want to overstate my confidence from a sample size of one person on one machine — really glad to hear you're going to try reproducing it locally too, that's a much better way to settle it than my testing alone. And separately from the technical question — thank you, genuinely. This is one of the first proposals I've ever put up on GitHub, as a junior developer just starting to find his footing in open source contributions. Getting a review this thoughtful, from someone who clearly cares about getting accessibility right rather than just waving a PR through, means a lot more than I think you'd expect. Thank you for the corrections you suggested earlier too, and please forgive whatever mistakes I made along the way — I'm still learning how all of this works. |
90bf337 to
b649bab
Compare
- Expose history rows as real accessible elements (label, button trait, Pin/Unpin and Delete actions) instead of just a painted highlight - Announce the selected item (title, source app, pinned state) on arrow-key navigation, which was previously silent - Label the quick-action toolbar buttons (Pin, Delete, copy extracted text, remove from paste stack), which announced as raw system icon names with no indication of what they do - Label the status bar icon, the search field's clear button, and the preview toggle (with its current state) - Fix Clear/Clear all both existing in the accessibility tree at once when only one is visually shown - Label 8 settings controls that had no accessible label at all - Add Spanish translations for every new accessibility string Tested live with VoiceOver, not just by reading the code.
- announceForAccessibility now takes the announcement as an autoclosure, so callers building a non-trivial string (e.g. NavigationManager's selection announcement) don't pay for that work when VoiceOver isn't running - Extract the repeated "mark as button + wire the same closure to onTapGesture and accessibilityAction" pattern into a View.buttonAction extension, used by ConfirmationView and ListItemView - Extract the repeated "opacity + accessibilityHidden" pattern into a View.invisible extension; convert FooterView's clearOpacity/ clearAllOpacity to booleans (showClear/showClearAll) since the crossfade was never actually animated - Extend KeyboardShortcutHelpModifier to also set accessibilityLabel from the same resolved string used for the .help() tooltip, and let its `name` parameter be nil for buttons with no keyboard shortcut (CopyExtractedText). This replaces the one-off accessibilityLabel calls on the Pin/Delete/CopyExtractedText toolbar buttons and on the preview toggle, whose PreviewKey string is now split into OpenPreview/ClosePreview so the label reflects the current state - Extract the repeated "\(selectionIndex + 1)" into a computed property (selectionNumber) shared by the visible badge and accessibilityValue - Rewrite the FloatingPanel.swift comment to clarify it documents an investigated-and-rejected local change, not a revert of anything that was ever committed to this PR
b649bab to
7886df1
Compare
p0deje
left a comment
p0deje
left a comment
There was a problem hiding this comment.
First of all, let me thank you for taking the time and effort to work on this. This is a non-trivial piece of work, and I don't know how much time you spent on it. I'll do my best to make sure it's not in vain.
I've left a couple of minor comments, but once they are addressed, I'll pull this and give it a short test. I honestly don't know how to test the accessibility aspects, so I'll take your word for it, as you clearly have more experience in this area. From a code perspective, all changes make perfect sense. I just want to make sure nothing breaks with them.
On a side note, it would be great if there were a way to make sure accessibility is not an afterthought. I, for instance, will likely forget to add an accessibility label to a new UI element, so a linter or a shared view helper that propagates a visible text label to an accessibility label would be handy. I'm not sure it exists, and I think this PR should be merged without this, but I wish it were harder to miss these aspects.
| // Describe the complete item independently of its potentially truncated visual content. | ||
| private var accessibilityLabel: String { | ||
| var parts: [String] = [] | ||
| if item.hasImage, let image = item.item.image { | ||
| let size = image.pixelSize | ||
| parts.append(String(format: NSLocalizedString("history_item_image_accessibility_label_no_app", comment: ""), Int(size.width), Int(size.height))) | ||
| } else { | ||
| parts.append(item.title) | ||
| } | ||
| if let application = item.application { | ||
| parts.append(application) | ||
| } | ||
| if item.isPinned { | ||
| parts.append(NSLocalizedString("history_item_pinned_accessibility_value", comment: "")) | ||
| } | ||
| if let index = visualIndex { | ||
| parts.append(String(format: NSLocalizedString("history_item_selected_accessibility_value", comment: ""), index + 1, appState.navigator.selection.count)) | ||
| } | ||
| return parts.joined(separator: ", ") | ||
| } |
There was a problem hiding this comment.
It would be great to move this to a HistoryItemDecorator, so the view doesn't have to work both with item and item.item.
|
|
||
| // Use the same selection number for the visible badge and accessibility value. | ||
| private var selectionNumber: String? { | ||
| selectionIndex.map { "\($0 + 1)" } |
There was a problem hiding this comment.
The selectionIndex is an integer and I don't think the map function exists on the integers.
| let finalHeight = max(min(height, size.height), miniumHeight) | ||
| setContentSize(NSSize(width: finalWidth, height: finalHeight)) | ||
| setFrameOrigin(popupPosition.origin(size: frame.size, statusBarButton: statusBarButton)) | ||
|
|
There was a problem hiding this comment.
Please, remove as it's not needed.
|
|
||
| /// Posts an announcement when VoiceOver is enabled. | ||
| /// | ||
| /// The autoclosure avoids constructing announcements when they cannot be heard. |
There was a problem hiding this comment.
I think this comment is not valid anymore.
3 participants
VoiceOver accessibility improvements for the clipboard history panel
A bit about me
I'm a junior developer taking some of my first real steps into open source — and into
accessibility work specifically. I lost my sight in 2020, and VoiceOver has been how
I've done everything on a Mac since, including, now, contributing code. This is one of
the first accessibility PRs I've ever attempted on GitHub, so please be gentle 🙂
Why this PR
Maccy is genuinely great, and it's an app I use constantly. But using it with VoiceOver
today ranges from "confusing" to "basically not possible" in a few key spots: there's no
way to tell which history item is selected, several buttons just announce as generic
system icon names with no indication of what they actually do, and arrow-key navigation
through your clipboard history is completely silent.
I'll be honest about the scope of who this matters to: this PR is essentially invisible
to 99% of the people who'll ever look at this repo. But for the 100% of people who
navigate macOS with a screen reader, it's the difference between being able to use Maccy
at all, or not.
What this changes
row exposes its full content (title, source app, pinned status), a proper button
trait, and two actions — Pin/Unpin and Delete — reachable directly from VoiceOver.
from, and whether it's pinned). Before this, moving through your history with the
keyboard produced no feedback at all.
stack) now has real labels. These were announcing as raw system icon names (e.g.
the pin icon just read as "pin marker") instead of saying what the button does —
something I only caught by actually using it live with VoiceOver, not by reading
the code.
identifiable when scanning the menu bar instead of being an anonymous icon.
magnifying glass icon is hidden from the accessibility tree instead of being read
as clutter.
depending on its current state, instead of nothing.
accessibility when visually hidden — previously both existed in the accessibility
tree simultaneously (one invisible), which meant a screen reader user could
accidentally trigger "Clear all" while intending to trigger "Clear."
had no accessible label — VoiceOver would announce just the raw value with zero
context about what setting it belonged to.
originals.
How I tested it
I didn't just read the source and guess. I built a debug version of the app and
tested every one of these changes live, with real VoiceOver, on my own Mac — copying
text, navigating history, pinning items, opening Settings, all of it. A few of the
bugs above (especially the toolbar icon-labeling one) only became obvious once I was
actually navigating with a screen reader instead of reading Swift — the kind of thing
that's genuinely hard to catch from code review alone.
What's not in this PR
There's an open question I looked into but couldn't responsibly resolve here: whether
VoiceOver's cursor reliably reaches the history popup at all when it opens (it's a
non-activating panel, which is exactly the reason it doesn't steal focus from whatever
app you're about to paste into). I found and reverted an attempted fix for this after
realizing it would silently break pasting into the wrong app — not something I was
willing to ship. My own live test suggests VoiceOver does reach the panel in
practice, but I'd rather flag this as unresolved than claim more confidence than I
actually have.
Being upfront
I know a PR like this, from someone this new both to the project and to accessibility
work in general, most likely won't get merged exactly as-is — maybe some of these
choices don't match how you'd want to approach it, maybe the scope should be split up,
maybe there's context about the codebase I'm missing. All of that's completely fine.
I mostly just wanted Maccy — an app I already rely on — to be a little more usable for
people like me.
Thank you, genuinely, for taking the time to read this far. 🙏