Add implementations for NSAccessibilityCustomRotor/Action and changes to related classes#362
Add implementations for NSAccessibilityCustomRotor/Action and changes to related classes#362
Conversation
There was a problem hiding this comment.
Pull Request Overview
Implements core NSAccessibility classes and C helpers to make accessibility elements, custom rotors, and custom actions functional under manual retain/release.
- Add concrete initializers, accessors, and simple role description to NSAccessibilityElement
- Implement NSAccessibilityCustomRotor and ItemResult, plus NSAccessibilityCustomAction (with block support, perform logic, and factories)
- Provide basic NSAccessibility* C helpers for notifications and role/action descriptions
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| Source/NSAccessibilityElement.m | Implements element initializers, accessors, dealloc, and a simple role description. |
| Source/NSAccessibilityCustomRotor.m | Fills in rotor and item result initializers, accessors, and dealloc. |
| Source/NSAccessibilityCustomAction.m | Adds safe block retention/release, perform logic, convenience factories, and description. |
| Source/NSAccessibility.m | Adds basic implementations for posting notifications and role/action description utilities. |
| Headers/AppKit/NSAccessibilityElement.h | Public API and ivars for NSAccessibilityElement, including initializers and accessors. |
| Headers/AppKit/NSAccessibilityCustomRotor.h | Public ivars for rotor and item result classes. |
| Headers/AppKit/NSAccessibilityCustomAction.h | Declares new factory methods and perform method for custom actions. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Updated accessibility property setters to use ASSIGNCOPY for better memory management.
|
I think this PR is ready as well, @fredkiefer / @rfm |
rfm
left a comment
There was a problem hiding this comment.
I'm not familiar with what this stuff is supposed to be doing, but as far as I can see this replaces unimplemented code so can't really be breaking anything even if it doesn't operate correctly.
One thing that confuses me is the use of
#pragma clang diagnostic ignored "-Warc-performSelector-leaks"
From what I read, that diagnostic should only be produced if you are building in ARC mode, but we don't make the core libraries depend on ARC and it doesn't look like you are even preparing to make this code depend on ARC (for instance, that would require the calls to [super dealloc]; to be replaced with the DEALLOC macro), so I wonder why the pragma to suppress a non-existent warning? Is it because a compiler bug generates an incorrect warning?
fredkiefer
left a comment
There was a problem hiding this comment.
I would prefer to see these few changes, but otherwise the code looks fine.
|
This branch is not ready yet. I have so much more I didn't realize I have to do on this. |
… suggestion by reviewer
|
@fredkiefer, I have made the changes you suggested after approval. |
|
@fredkiefer are you okay with merging this? |
|
Riccardo would know better, but I think there are still platforms where clang/libibjc try/catch fails. Certainly there have been.many issues with C++ integration breaking things (though to be fair C++ exceptions don't integrate with the macros either).
…
On 22 Feb 2026 at 08:29, Fred Kiefer ***@***.***> wrote:
@fredkiefer commented on this pull request.
In Source/NSColorWell.m:
> + +- (NSString *) accessibilityTitle +{ + return @"Color Well"; +} + +- (id) accessibilityValue +{ + NSColor *color = [self color]; + if (color) + { + // Use component methods directly for better compatibility + CGFloat red, green, blue, alpha; + + // Try to get RGB components using getRed:green:blue:alpha: + @Try
I am not sure here, but as far as I remember we still prefer to have all try/catch code written with the old macros. We could change that though, but I would prefer an official decision.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
| // For now, calculated dynamically based on children | ||
| } | ||
|
|
||
| - (NSRect) accessibilityFrame |
There was a problem hiding this comment.
Why is this in screen coordinates whereas accessibilityActivationPoint is in parent view coordinates?
Also why has the implementation of this method been duplicated in many subclasses?
There was a problem hiding this comment.
Because I felt as though, since this is for accessibility, it would be more useful for someone who is blind to hear absolute vs relative coordinates.
There was a problem hiding this comment.
But that still leaves my question unanswered. Some of these methods are in one coordinate system, some in the other. Sometimes the same method has another coordinate system when implemented in a different class. At least for me this is very confusing. We should define in advance for each method what to use and implement it consistently.
And you did not address the second question.
|
I would suggest to break up this PR into multiple. That way it would be easier to review.
That way we are sure that we get all this functionality without breaking anything and we can also reduce the amount of unnecessary code. The later is one of my biggest concerns with AI generated code. |
Okay, I will do this. Once I am done, I will create the separate PRs for each branch. My concern is that much of this needs to be tested together. |
|
You are correct about the tests. Most of these will belong into the last PR and only then will we know that everything works as expected. But at the moment we don't even know whether the tests are correct. Or did you run them on a Mac? That really would help to understand the concepts better. |
|
The test for the NSColor stuff is here https://github.com/gcasa/NSColor_test |
This PR adds support for NSAccessibilityCustomRotor and NSAccessibilityCustomAction to improve accessibility navigation and interaction for assistive technologies.
In compliance with policy: Parts of this PR, both code and documentation, were generated using AI