Add high contrast API to AccessibilityInfo #742

appden · 2021-03-06T19:39:26Z

Please select one of the following

I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
I am making a fix / change for the macOS implementation of react-native
I am making a change required for Microsoft usage of react-native

Summary

This adds the isHighContrastEnabled() method and highContrastChanged event to the AccessibilityInfo module. This matches the existing API conventions we have in React Native macOS for other accessibility features. React Native Windows has a similar API with its AppTheme module, but that module doesn't exist for other React Native platforms and this is more appropriate in AccessibilityInfo.

Changelog

[macOS] [Added] - Add high contrast API to AccessibilityInfo

Test Plan

Tested in RNTester. Observed live updates when changing the Increase contrast setting under System Preferences.

Microsoft Reviewers: Open in CodeFlow

harrieshin · 2021-03-09T02:13:42Z

Libraries/Components/AccessibilityInfo/AccessibilityInfo.ios.js

+  /**
+   * macOS only
+   */
+  isHighContrastEnabled: function(): Promise<boolean> {


should we call it hightcontrast when for apple platform it is increased contrast? high contrast is pretty Window's term right?

Not really. Apple also uses the term "high contrast", such as with the appearance names (i.e. NSAppearanceNameAccessibilityHighContrastAqua), and I think the industry in general refers to this accessibility mode as "high contrast". 😀

I'm happy to be shown I'm wrong here but I get the impression from Apple's support docs that Increase Contrast is the overarching feature encapsulating many different contrasts whereas HighContrast is one of many contrast options within it (e.g. NSAppearanceNameAqua, NSAppearanceNameDarkAqua, NSAppearanceNameAccessibilityHighContrastDarkAqua, NSAppearanceNameVibrantDark , NSAppearanceNameAccessibilityHighContrastVibrantDark).

If other platforms call it "High Contrast" then I think we just have to choose the one that makes the most sense to go by because I get the impression it won't match up perfectly for all platforms.

Since we're writing this feature primarily for macOS, I'd also lean towards IncreaseContrast, but again am happy to see how I'm misunderstanding Apple's docs :)

We also plan to implement this API on iOS, Android, and Windows, which already use "high contrast" terminology in their APIs (i.e. UIAccessibilityContrastHigh on iOS, isHighTextContrastEnabled() on Android, HighContrast() on Windows and the -ms-high-contrast media query in Edge). I really think "high contrast" is right choice for this API.

Yea the main argument against using the macOS terminology is to keep it consistent cross-plat. I can see why that's important enough to keep it

harrieshin · 2021-03-09T02:16:16Z

React/Modules/MacOS/RCTAccessibilityManager.m

@@ -53,7 +53,7 @@ - (void)dealloc
  [[NSWorkspace sharedWorkspace] removeObserver:self
                                     forKeyPath:@"voiceOverEnabled"
                                        context:AccessibilityVoiceOverChangeContext];
-  [[[NSWorkspace sharedWorkspace] notificationCenter] removeObserver:self]; 
+  [[[NSWorkspace sharedWorkspace] notificationCenter] removeObserver:self];


do we need this line? "If your app targets iOS 9.0 and later or macOS 10.11 and later, and you used addObserver:selector:name:object:, you do not need to unregister the observer. If you forget or are unable to remove the observer, the system cleans up the next time it would have posted to it." https://developer.apple.com/documentation/foundation/nsnotificationcenter/1413994-removeobserver

True, but it's certainly more explicit to just leave this because otherwise somebody needs to know this behavior when reading this code.

Don't we not support iOS 8.0 anymore? I'm always in favor of deleting code that has no functional benefit :)

harrieshin · 2021-03-09T02:17:21Z

React/CoreModules/RCTAccessibilityManager.h

@@ -21,6 +21,7 @@ extern NSString *const RCTAccessibilityManagerDidUpdateMultiplierNotification; /

 @property (nonatomic, assign) BOOL isBoldTextEnabled;
 @property (nonatomic, assign) BOOL isGrayscaleEnabled;
+@property (nonatomic, assign) BOOL isHighContrastEnabled; // TODO(macOS ISS#2323203)


even if we leave the JS prop to be high contrast, I still think it is more natural that Apple has boolean property named isIncreasedContrastEnabled. what do you think?

What if we leave as is but add a comment "maps to IncreaseContrast for macOS" or something like that?

Anyone whose written macOS code before may wonder how these two relate to each other so I think it should be documented somewhere

harrieshin · 2021-03-09T02:19:22Z

React/Modules/MacOS/RCTAccessibilityManager.m

@@ -23,12 +23,12 @@ @implementation RCTAccessibilityManager

 static void *AccessibilityVoiceOverChangeContext = &AccessibilityVoiceOverChangeContext;

-+ (BOOL)requiresMainQueueSetup 
+ (BOOL)requiresMainQueueSetup
 {
  return NO;


for my own understanding, is this correct? anything we ask for NSWorkspace, we should be on the UIthread right?

Great point! I'll change this.

HeyImChris · 2021-03-09T22:49:13Z

React/Modules/MacOS/RCTAccessibilityManager.m

@@ -39,11 +39,11 @@ - (instancetype)init
                                             selector:@selector(accessibilityDisplayOptionsChange:)
                                                 name:NSWorkspaceAccessibilityDisplayOptionsDidChangeNotification
                                               object:nil];
+    _isHighContrastEnabled = [[NSWorkspace sharedWorkspace] accessibilityDisplayShouldIncreaseContrast];


Not your fault but while we're touching these can we cache this to save all the extra time calling into the same method over and over again?

NSWorkspace *sharedWorkspace = [NSWorkspace sharedWorkspace and just use that var throughout the method

That would be a micro-optimization since calling these singleton methods take microseconds and I think it's fine to continue following the convention in React Native of calling them for each use (see [NSNotificationCenter defaultCenter] calls in other native modules).

I'd argue those should get fixed too. Functionally speaking it will only improve performance (albeit a small amount) with no downsides if we fix up these calls everywhere.

HeyImChris · 2021-03-09T22:49:49Z

React/Modules/MacOS/RCTAccessibilityManager.m

@@ -124,9 +130,16 @@ - (void)observeValueForKeyPath:(NSString *)keyPath

 - (void)accessibilityDisplayOptionsChange:(NSNotification *)notification
 {
+  BOOL newHighContrastEnabled = [[NSWorkspace sharedWorkspace] accessibilityDisplayShouldIncreaseContrast];


Same here, might as well be efficient and cache the sharedWorkspace

HeyImChris · 2021-03-18T18:03:11Z

React/Modules/MacOS/RCTAccessibilityManager.m

@@ -39,11 +39,11 @@ - (instancetype)init
                                             selector:@selector(accessibilityDisplayOptionsChange:)
                                                 name:NSWorkspaceAccessibilityDisplayOptionsDidChangeNotification
                                               object:nil];
+    _isHighContrastEnabled = [[NSWorkspace sharedWorkspace] accessibilityDisplayShouldIncreaseContrast];


I'd argue those should get fixed too. Functionally speaking it will only improve performance (albeit a small amount) with no downsides if we fix up these calls everywhere.

HeyImChris · 2021-03-18T18:05:42Z

React/CoreModules/RCTAccessibilityManager.h

@@ -21,6 +21,7 @@ extern NSString *const RCTAccessibilityManagerDidUpdateMultiplierNotification; /

 @property (nonatomic, assign) BOOL isBoldTextEnabled;
 @property (nonatomic, assign) BOOL isGrayscaleEnabled;
+@property (nonatomic, assign) BOOL isHighContrastEnabled; // TODO(macOS ISS#2323203)


What if we leave as is but add a comment "maps to IncreaseContrast for macOS" or something like that?

Anyone whose written macOS code before may wonder how these two relate to each other so I think it should be documented somewhere

This reverts commit e9e4980.

appden requested a review from alloy as a code owner March 6, 2021 19:39

appden force-pushed the high-contrast branch 3 times, most recently from e5978d3 to 3ec1253 Compare March 8, 2021 22:57

Add high contrast API to AccessibilityInfo

9ee15f9

appden force-pushed the high-contrast branch from 3ec1253 to 9ee15f9 Compare March 9, 2021 00:32

harrieshin reviewed Mar 9, 2021

View reviewed changes

HeyImChris reviewed Mar 9, 2021

View reviewed changes

appden added 2 commits March 11, 2021 20:16

Make RCTAccessibilityManager require main queue setup

9fa3fc3

Remove unnecessary removeObserver: call

ad7c337

HeyImChris approved these changes Mar 18, 2021

View reviewed changes

Add comment for macOS "high contrast" = "increase contrast"

5fc6d93

HeyImChris merged commit e9e4980 into microsoft:master Mar 18, 2021

Saadnajmi added a commit to Saadnajmi/react-native-macos that referenced this pull request Mar 22, 2021

Revert "Add high contrast API to AccessibilityInfo (microsoft#742)"

eeb03e8

This reverts commit e9e4980.

Saadnajmi added a commit to Saadnajmi/react-native-macos that referenced this pull request Mar 22, 2021

Revert "Add high contrast API to AccessibilityInfo (microsoft#742)"

026a799

This reverts commit e9e4980.

chiuam mentioned this pull request Mar 6, 2023

[Button] HC fixes button microsoft/fluentui-react-native#2679

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add high contrast API to AccessibilityInfo #742

Add high contrast API to AccessibilityInfo #742

appden commented Mar 6, 2021 •

edited by ghost

Loading

harrieshin Mar 9, 2021

appden Mar 9, 2021

HeyImChris Mar 12, 2021

HeyImChris Mar 12, 2021

appden Mar 12, 2021

HeyImChris Mar 18, 2021

harrieshin Mar 9, 2021

appden Mar 9, 2021

HeyImChris Mar 12, 2021

harrieshin Mar 9, 2021

HeyImChris Mar 18, 2021

harrieshin Mar 9, 2021

appden Mar 9, 2021

HeyImChris Mar 9, 2021

appden Mar 18, 2021

HeyImChris Mar 18, 2021

HeyImChris Mar 9, 2021

HeyImChris Mar 18, 2021

HeyImChris Mar 18, 2021

Add high contrast API to AccessibilityInfo #742

Add high contrast API to AccessibilityInfo #742

Conversation

appden commented Mar 6, 2021 • edited by ghost Loading

Please select one of the following

Summary

Changelog

Test Plan

Microsoft Reviewers: Open in CodeFlow

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

appden commented Mar 6, 2021 •

edited by ghost

Loading