Skip to content

Add implementations for NSAccessibilityCustomRotor/Action and changes to related classes#362

Open
gcasa wants to merge 40 commits intomasterfrom
NSAccessibilityCustomRotor_branch
Open

Add implementations for NSAccessibilityCustomRotor/Action and changes to related classes#362
gcasa wants to merge 40 commits intomasterfrom
NSAccessibilityCustomRotor_branch

Conversation

@gcasa
Copy link
Copy Markdown
Member

@gcasa gcasa commented Sep 12, 2025

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

@gcasa gcasa requested a review from fredkiefer as a code owner September 12, 2025 18:58
@gcasa gcasa marked this pull request as draft September 12, 2025 19:19
@gcasa gcasa requested a review from rfm September 12, 2025 19:19
@gcasa gcasa marked this pull request as ready for review September 21, 2025 23:06
@gcasa gcasa requested a review from Copilot September 21, 2025 23:06
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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.

Comment thread Source/NSAccessibilityCustomAction.m Outdated
Comment thread Source/NSAccessibilityElement.m Outdated
Comment thread Source/NSAccessibilityElement.m
Comment thread Source/NSAccessibilityCustomRotor.m Outdated
Comment thread Source/NSAccessibilityCustomRotor.m Outdated
Comment thread Source/NSAccessibilityCustomRotor.m Outdated
Comment thread Source/NSAccessibility.m
Comment thread Source/NSAccessibility.m
Comment thread Source/NSAccessibility.m
Comment thread Headers/AppKit/NSAccessibilityCustomAction.h Outdated
gcasa and others added 8 commits September 21, 2025 19:11
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.
@gcasa
Copy link
Copy Markdown
Member Author

gcasa commented Sep 24, 2025

I think this PR is ready as well, @fredkiefer / @rfm

Copy link
Copy Markdown
Contributor

@rfm rfm left a comment

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member

@fredkiefer fredkiefer left a comment

Choose a reason for hiding this comment

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

I would prefer to see these few changes, but otherwise the code looks fine.

Comment thread Headers/AppKit/NSAccessibilityCustomAction.h Outdated
Comment thread Source/NSAccessibilityCustomAction.m Outdated
Comment thread Source/NSAccessibilityCustomRotor.m Outdated
Comment thread Source/NSAccessibilityCustomRotor.m Outdated
@gcasa
Copy link
Copy Markdown
Member Author

gcasa commented Oct 8, 2025

This branch is not ready yet. I have so much more I didn't realize I have to do on this.

Copy link
Copy Markdown
Contributor

Copilot AI commented Feb 17, 2026

@gcasa I've opened a new pull request, #398, to work on those changes. Once the pull request is ready, I'll request review from you.

Comment thread Headers/AppKit/NSView.h Outdated
@gcasa
Copy link
Copy Markdown
Member Author

gcasa commented Feb 20, 2026

@fredkiefer, I have made the changes you suggested after approval.

Comment thread Source/NSAccessibilityCustomAction.m Outdated
Comment thread Source/NSAccessibilityCustomAction.m Outdated
Comment thread Source/NSButton.m Outdated
@gcasa
Copy link
Copy Markdown
Member Author

gcasa commented Feb 21, 2026

@fredkiefer are you okay with merging this?

Comment thread Source/NSColorWell.m Outdated
Comment thread Source/NSColorWell.m Outdated
Comment thread Source/NSView.m Outdated
@rfm
Copy link
Copy Markdown
Contributor

rfm commented Feb 22, 2026 via email

Comment thread Source/NSView.m
// For now, calculated dynamically based on children
}

- (NSRect) accessibilityFrame
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@fredkiefer fredkiefer self-requested a review February 22, 2026 14:29
@fredkiefer
Copy link
Copy Markdown
Member

I would suggest to break up this PR into multiple. That way it would be easier to review.

  • NSAccessabilty classes
  • NSView plus GSAccessability
  • NSSwitch extensions, which are mostly unrelated
  • override of NSView methods for specific controls, plus extra methods. Maybe even split this up

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.

@gcasa
Copy link
Copy Markdown
Member Author

gcasa commented Feb 22, 2026

I would suggest to break up this PR into multiple. That way it would be easier to review.

  • NSAccessabilty classes
  • NSView plus GSAccessability
  • NSSwitch extensions, which are mostly unrelated
  • override of NSView methods for specific controls, plus extra methods. Maybe even split this up

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.

@fredkiefer
Copy link
Copy Markdown
Member

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.

@gcasa
Copy link
Copy Markdown
Member Author

gcasa commented Feb 23, 2026

The test for the NSColor stuff is here https://github.com/gcasa/NSColor_test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants