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.

Sign up for GitHub

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

Add high contrast API to AccessibilityInfo #742

Merged
merged 4 commits into from
Mar 18, 2021
Merged

Add high contrast API to AccessibilityInfo #742

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9ee15f9
Add high contrast API to AccessibilityInfo
appden Mar 6, 2021
9fa3fc3
Make RCTAccessibilityManager require main queue setup
appden Mar 12, 2021
ad7c337
Remove unnecessary removeObserver: call
appden Mar 18, 2021
5fc6d93
Add comment for macOS "high contrast" = "increase contrast"
appden Mar 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Libraries/Components/AccessibilityInfo/AccessibilityInfo.android.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,13 @@ const AccessibilityInfo = {
return Promise.resolve(false);
},

/**
* macOS only
*/
isHighContrastEnabled: function(): Promise<boolean> {
return Promise.resolve(false);
},

/**
* iOS only
*/
Expand Down
7 changes: 7 additions & 0 deletions Libraries/Components/AccessibilityInfo/AccessibilityInfo.ios.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,13 @@ const AccessibilityInfo = {
});
},

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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". 😀

Copy link

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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 :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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

return Promise.resolve(false);
},

/**
* Query whether inverted colors are currently enabled.
*
Expand Down
18 changes: 18 additions & 0 deletions Libraries/Components/AccessibilityInfo/AccessibilityInfo.macos.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ const RCTDeviceEventEmitter = require('../../EventEmitter/RCTDeviceEventEmitter'
import NativeAccessibilityManager from './NativeAccessibilityManager';

const CHANGE_EVENT_NAME = {
highContrastChanged: 'highContrastChanged',
invertColorsChanged: 'invertColorsChanged',
reduceMotionChanged: 'reduceMotionChanged',
reduceTransparencyChanged: 'reduceTransparencyChanged',
Expand All @@ -26,6 +27,7 @@ const CHANGE_EVENT_NAME = {

type ChangeEventName = $Keys<{
change: string,
highContrastChanged: string,
invertColorsChanged: string,
reduceMotionChanged: string,
reduceTransparencyChanged: string,
Expand Down Expand Up @@ -58,6 +60,22 @@ const AccessibilityInfo = {
return Promise.resolve(false);
},

/**
* Query whether high contrast is currently enabled.
*
* Returns a promise which resolves to a boolean.
* The result is `true` when invert color is enabled and `false` otherwise.
*/
isHighContrastEnabled: function(): Promise<boolean> {
return new Promise((resolve, reject) => {
if (NativeAccessibilityManager) {
NativeAccessibilityManager.getCurrentHighContrastState(resolve, reject);
} else {
reject(reject);
}
});
},

/**
* Query whether inverted colors are currently enabled.
*
Expand Down
6 changes: 6 additions & 0 deletions Libraries/Components/AccessibilityInfo/NativeAccessibilityManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@ export interface Spec extends TurboModule {
onSuccess: (isGrayscaleEnabled: boolean) => void,
onError: (error: Object) => void,
) => void;
// [TODO(macOS ISS#2323203)
+getCurrentHighContrastState: (
onSuccess: (isHighContrastEnabled: boolean) => void,
onError: (error: Object) => void,
) => void;
// ]TODO(macOS ISS#2323203)
+getCurrentInvertColorsState: (
onSuccess: (isInvertColorsEnabled: boolean) => void,
onError: (error: Object) => void,
Expand Down
26 changes: 26 additions & 0 deletions RNTester/js/examples/Accessibility/AccessibilityExample.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -739,6 +739,16 @@ class DisplayOptionsStatusExample extends React.Component<{}> {
state = {};

componentDidMount() {
AccessibilityInfo.addEventListener(
'highContrastChanged',
this._handleHighContrastToggled,
);
AccessibilityInfo.isHighContrastEnabled().done(isEnabled => {
this.setState({
highContrastEnabled: isEnabled,
});
});

AccessibilityInfo.addEventListener(
'invertColorsChanged',
this._handleInvertColorsToggled,
Expand Down Expand Up @@ -771,6 +781,10 @@ class DisplayOptionsStatusExample extends React.Component<{}> {
}

componentWillUnmount() {
AccessibilityInfo.removeEventListener(
'highContrastChanged',
this._handleHighContrastToggled,
);
AccessibilityInfo.removeEventListener(
'invertColorsChanged',
this._handleInvertColorsToggled,
Expand All @@ -785,6 +799,12 @@ class DisplayOptionsStatusExample extends React.Component<{}> {
);
}

_handleHighContrastToggled = isEnabled => {
this.setState({
highContrastEnabled: isEnabled,
});
};

_handleInvertColorsToggled = isEnabled => {
this.setState({
invertColorsEnabled: isEnabled,
Expand All @@ -806,6 +826,12 @@ class DisplayOptionsStatusExample extends React.Component<{}> {
render() {
return (
<View>
<View>
<Text>
High contrast is{' '}
{this.state.highContrastEnabled ? 'enabled' : 'disabled'}.
</Text>
</View>
<View>
<Text>
Invert colors is{' '}
Expand Down
1 change: 1 addition & 0 deletions React/CoreModules/RCTAccessibilityManager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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

@property (nonatomic, assign) BOOL isInvertColorsEnabled;
@property (nonatomic, assign) BOOL isReduceMotionEnabled;
@property (nonatomic, assign) BOOL isReduceTransparencyEnabled;
Expand Down
21 changes: 17 additions & 4 deletions React/Modules/MacOS/RCTAccessibilityManager.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,12 @@ @implementation RCTAccessibilityManager

static void *AccessibilityVoiceOverChangeContext = &AccessibilityVoiceOverChangeContext;

+ (BOOL)requiresMainQueueSetup
+ (BOOL)requiresMainQueueSetup
{
return NO;
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great point! I'll change this.

}

- (instancetype)init
- (instancetype)init
{
if (self = [super init]) {
[[NSWorkspace sharedWorkspace] addObserver:self
Expand All @@ -39,11 +39,11 @@ - (instancetype)init
selector:@selector(accessibilityDisplayOptionsChange:)
name:NSWorkspaceAccessibilityDisplayOptionsDidChangeNotification
object:nil];
_isHighContrastEnabled = [[NSWorkspace sharedWorkspace] accessibilityDisplayShouldIncreaseContrast];
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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).

Copy link

Choose a reason for hiding this comment

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

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.

_isInvertColorsEnabled = [[NSWorkspace sharedWorkspace] accessibilityDisplayShouldInvertColors];
_isReduceMotionEnabled = [[NSWorkspace sharedWorkspace] accessibilityDisplayShouldReduceMotion];
_isReduceTransparencyEnabled = [[NSWorkspace sharedWorkspace] accessibilityDisplayShouldReduceTransparency];
_isVoiceOverEnabled = [[NSWorkspace sharedWorkspace] isVoiceOverEnabled];

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

}

RCT_EXPORT_METHOD(announceForAccessibility:(NSString *)announcement)
Expand All @@ -67,6 +67,12 @@ - (void)dealloc
);
}

RCT_EXPORT_METHOD(getCurrentHighContrastState:(RCTResponseSenderBlock)callback
error:(__unused RCTResponseSenderBlock)error)
{
callback(@[@(_isHighContrastEnabled)]);
}

RCT_EXPORT_METHOD(getCurrentInvertColorsState:(RCTResponseSenderBlock)callback
error:(__unused RCTResponseSenderBlock)error)
{
Expand Down Expand Up @@ -124,9 +130,16 @@ - (void)observeValueForKeyPath:(NSString *)keyPath

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

Choose a reason for hiding this comment

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

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

BOOL newInvertColorsEnabled = [[NSWorkspace sharedWorkspace] accessibilityDisplayShouldInvertColors];
BOOL newReduceMotionEnabled = [[NSWorkspace sharedWorkspace] accessibilityDisplayShouldReduceMotion];
BOOL newReduceTransparencyEnabled = [[NSWorkspace sharedWorkspace] accessibilityDisplayShouldReduceTransparency];

if (_isHighContrastEnabled != newHighContrastEnabled) {
_isHighContrastEnabled = newHighContrastEnabled;
[_bridge.eventDispatcher sendDeviceEventWithName:@"highContrastChanged"
body:@(_isHighContrastEnabled)];
}
if (_isInvertColorsEnabled != newInvertColorsEnabled) {
_isInvertColorsEnabled = newInvertColorsEnabled;
[_bridge.eventDispatcher sendDeviceEventWithName:@"invertColorsChanged"
Expand Down