-
Notifications
You must be signed in to change notification settings - Fork 172
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
[macOS] Announce DatePickerController
changes in VoiceOver
#2134
base: main
Are you sure you want to change the base?
Conversation
|
||
datePickerController?.hasEdgePadding = true | ||
menuDatePickerController?.hasEdgePadding = true | ||
DatePickerLocation.allCases.forEach { location in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally ignorable - I've mostly stopped using .forEach
in favor of old-fashioned for...in
. I personally pretty strongly agree with https://forums.swift.org/t/do-we-want-foreach/56929/2m, plus it makes debugging easier to just be able to step through a for loop and not to have to set a separate breakpoint inside because it's fired as a closure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link is broken, but I think I found the conversation you were referencing. It's interesting, but there doesn't seem to be a single consensus.
In this case I think it's fine to switch to a traditional for...in
loop, but I'd be loathe to replace my later uses of .forEach
in this change.
Clean:
datePickerControllers.values.forEach { $0.hasEdgePadding = enabled }
Overly verbose:
for datePickerController in datePickerControllers.values {
datePickerController.hasEdgePadding = enabled
}
What are your thoughts on those?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's still a matter of preference and not a proven guideline so I'm good either way
NSAccessibility.NotificationUserInfoKey.announcement: newMonthYearValue, | ||
NSAccessibility.NotificationUserInfoKey.priority: NSAccessibilityPriorityLevel.medium.rawValue | ||
]) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this usually happen because of direct user interaction? In which case we shouldn't need an announcement? Or is there a case where it's happening indirectly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole control is a bit of an accessibility mess. The bandaid here is because the header title is never announced anywhere, in any context. A VoiceOver user can still infer what's happening by reading the individual dates in the calendar, but there's no way to know what's happening when the calendar changes months.
There are two longer-term fixes possible here that are outside the scope of this targeted fix:
- Rethink the entire accessibility hierarchy and behavior of this control, along with a variety of minor visibility and usability problems.
- Deprecate the control and work with partners to remove and replace it with the standard macOS date picker.
I have a preference, and it's not #1 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about just making the header title accessible so it's announced it a user moves focus to it? Or is that not trivial?
Platforms Impacted
Description of changes
NSAccessibility
announcement when the visible month in theDatePickerController
changes.TestDatePickerController
so popover-hosted picker works as expected.Binary change
n/a, no Mac size testing
Verification
Pull request checklist
This PR has considered:
Microsoft Reviewers: Open in CodeFlow