Skip to content
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

Xcode 14 modernization #3

Closed
wants to merge 4 commits into from
Closed

Xcode 14 modernization #3

wants to merge 4 commits into from

Conversation

ksuther
Copy link
Collaborator

@ksuther ksuther commented Oct 18, 2022

Can you double-check my accessibility modernization in SRRecorderControl.m?

@vendruscolo
Copy link

It provides a worse UX, previously, it would read the current shortcut, now it only says it's a button. And it would be nice to post a notification to confirm it was recorded.

Do you want some help in sorting this out?

@ksuther
Copy link
Collaborator Author

ksuther commented Oct 21, 2022

Yeah, if you don't mind you'd probably be much faster at it :)

In this way VO reads 'type shortcut' when entering recording mode. When
recording is done, it will read the newly recorded shortcut.
@vendruscolo
Copy link

I pushed some changes. There's only one thing that I can't get to work, but I feel like it's not possible:

  1. Highlight the recorder
  2. Start recording
  3. -← to move focus to the label before it

I'd like to cancel recording, but I can't get any accessibility hook to work. The closest one is accessibilityFocused but it seems is never called when the control loses focus (so I only see YES values).

Other than that VO now reads the button as before, and it speaks the recorded shortcut.

@ksuther
Copy link
Collaborator Author

ksuther commented Oct 25, 2022

Thanks, that's still better than before. @rustle any opinions?

@@ -82,6 +82,8 @@ typedef NS_ENUM(NSUInteger, _SRRecorderControlButtonTag)
_SRRecorderControlMainButtonTag = 2
};

@interface SRRecorderControl () <NSAccessibility, NSViewToolTipOwner>
Copy link

Choose a reason for hiding this comment

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

It's unusual to add NSAccessibility conformance like this. What are you indicating?

Copy link

Choose a reason for hiding this comment

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

You might want NSAccessibilityButton which will populate the role for you

@@ -422,6 +430,11 @@ - (NSString *)accessibilityLabel
return label;
}

- (NSString *)accessibilityTitle
Copy link

Choose a reason for hiding this comment

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

Does this duplication resolve a specific issue?

Copy link

Choose a reason for hiding this comment

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

(label is intended to be a clean replacement for title, but there have definitely been bugs historically)

Choose a reason for hiding this comment

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

That was done in 349fa68 - IIRC VoiceOver reads the title of the button but we were only providing the label.
It results in having VO doing a better job at reading the state of the button.

@ksuther
Copy link
Collaborator Author

ksuther commented Oct 27, 2022

I just noticed there's a bunch of upstream changes, taking a look at those and figuring out if we need any of this at all. I might have wasted a bunch of time by not looking there first, oops.

@vendruscolo
Copy link

After speaking with Doug, there's no need to stop recording mode when moving focus away from the button. This happens automatically if you navigate to a different (interactive) control.

@ksuther
Copy link
Collaborator Author

ksuther commented Oct 27, 2022

I'm going to close this in favor of Kentzo#150. Might as well get on the latest upstream version since they've modernized a lot in the last few years.

Sorry that I burned up a bunch of review cycles on this!

@ksuther ksuther closed this Oct 27, 2022
@ksuther ksuther deleted the xcode14-modernization branch October 27, 2022 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants