feat(core): a11y support by m-abs · Pull Request #8909 · NativeScript/NativeScript

m-abs · 2020-09-27T20:53:22Z

PR Checklist

The PR title follows our guidelines: https://github.com/NativeScript/NativeScript/blob/master/CONTRIBUTING.md#commit-messages.
There is an issue for the bug/feature this PR is for. To avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it.
You have signed the CLA.
All existing tests are passing: https://github.com/NativeScript/NativeScript/blob/master/DevelopmentWorkflow.md#running-unit-tests.
Tests for the changes are included - https://github.com/NativeScript/NativeScript/blob/master/WritingUnitTests.md.

What is the current behavior?

NativeScript doesn't support native accessibility features without our plugin @nota/nativescript-accessibility-ext

Unfortunately that plugin will soon go unmaintained.
I'd hate for the community to loose this feature, so here I'll contribute the feature to upstream NativeScript.

What is the new behavior?

Adds new properties to View-class.
CSS-helpers.
Regain accessibility focus on back navigation.
etc.

TODO:

Font-size scaling for iOS
GridLayout sizing. (current implementation was angular specific: https://github.com/Notalib/nativescript-accessibility-ext/blob/master/packages/nativescript-accessibility-ext/angular/directives/a11y-grid-layout.directive.ts)
Add helper classes to theme (help needed) (current: https://github.com/Notalib/nativescript-accessibility-ext/tree/master/packages/nativescript-accessibility-ext/scss) - some of this can be done programmatically in core now.
ListView scroll to item on swipe to item. (user will loose focus if trying to swipe to an item out of view in the list-view).
Testing.
Update API Documentation (help needed)
Magic Tap (iOS) (Must be registered on AppDelegate: https://developer.apple.com/documentation/objectivec/nsobject/1615137-accessibilityperformmagictap). Provide a Application.accessibilityMagicTapEvent which users can wire into.
Fix escape gesture issue see: Bug(iOS): Escape-gesture doesn't work as expected for VoiceOver users. #6123 (accessibilityPerformEscape https://developer.apple.com/documentation/objectivec/nsobject/1615091-accessibilityperformescape). Allow default to dismiss the active dialog however also emit a new Application.accessibilityPerformEscapeEvent which allows an override for custom behavior - most likely via args.cancel = true from the event emission.

Design document here:
https://docs.google.com/document/d/1gybrT1IQh8Fb5r0v_tZTMS06yP6xYmr9pDQIBRbokZc/edit

Implements #6993
Fixes: #6125

BREAKING CHANGES:
Remove the property automationText as it conflicts with the new property accessibilityId the implements the behavior correctly.

[Describe the impact of the changes here.]

Migration steps:
[Provide a migration path for existing applications.]

cla-bot · 2020-09-27T20:53:27Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign the CLA at https://www.nativescript.org/cla.
CLA has not been signed by users: @m-abs.
After signing the CLA, you can ask me to recheck this PR by posting @cla-bot check as a comment to the PR.

m-abs · 2020-09-27T20:58:32Z

@cla-bot check

cla-bot · 2020-09-27T20:58:35Z

The cla-bot has been summoned, and re-checked this pull request!

m-abs · 2020-09-28T18:17:27Z

Can you create a property automationText that points to automationId so that it is no longer a breaking change?

I can but it won't be exactly the same as the current behavior.

automationId sets automationIndentifier on iOS and doesn't do anything on android.

Currently automationText sets both accessibilityLabel and automationIndentifier on iOS and overrides contentDescription on android.

Hmm... I should probably rename automationId to automationIndentifier, to make it more consistent with the native platform.

rigor789 · 2020-09-28T20:02:12Z

automationId sets automationIndentifier on iOS and doesn't do anything on android.

@m-abs I think for android, we can do something like this:

const id = Utils.android.resources.getId(':id/nativescript_accessibility_id')
this.nativeViewProtected.setTag(id, value)
this.nativeViewProtected.setTag(value)

Tools like wix/Detox use that when getting views by id on android, and accessibilityIdentifier on ios.

I don't 100% recall why I had to set the tag with the id and without, I took that from a sample app I made with detox:
https://github.com/rigor789/ui-tested-nsvue-app/blob/fecd9ed0392b8903d20a0967d87d60201082bc32/app/app.js#L18-L25

In my case I named the property testID as that's the convention in react-native, but whatever is the most consistent should be fine!

m-abs · 2020-09-28T20:19:40Z

@rigor789

Thanks for working that out. 👍 I'll add it right away.

farfromrefug · 2020-09-29T06:33:03Z

A few comments. The accessibility/index.android.ts is not snapshot compatible. It accesses native namespace at its root which will fail in snapshot generation.
I am a bit worried about the whole initA11YView. Storing all views from an app seems "big". Can't we find another way to get the N view from a native view ?
Can we quantify everything which is added to an app not using accessibility?

m-abs · 2020-09-29T19:13:02Z

I am a bit worried about the whole initA11YView. Storing all views from an app seems "big". Can't we find another way to get the N view from a native view ?

We initial made this as a trade off.
In earlier versions of our plugin, we added a NotificationObserver per view, this was as you an imagine very expensive.
The WeakMap from a UIView to a WeakRef was the least expensive and safe way we found to do it. In our experience it doesn't cause too much of a memory overhead.

I've considered binding the View-instance to the native UIView, but I'm not sure if that reference survives when we get it inside the NotificationObserver callback.

m-abs · 2020-09-29T19:14:08Z

(Responding to this just so others seeing this issue understand, since you already know this) Snapshots are depreciated and unsupported in 7.x; we are not going to continue to mangle the code because of something that is not going to be supported anymore, code cache is the future.

Does this mean, I can ship the ensureNativeClasses-functions? They tend to make the code less readable.

farfromrefug · 2020-09-29T20:01:31Z

Please I know you deprecate this but people still use it ! If this PRs gets in as it is it will break snapshots. I would gladly update the code to make it snapshot compatible. It does not represent much work.
And I should point out that the way we write thing for snapshot is actually better performance wise! No no need to Marshal classes if they are not used at runtime !!! (Even If the file is actually imported).
I think N should continue to write android classes that way. Not so much drawback code wise.

As an example here all namespace / classes you added at the top of the file for easy access means they are marshalled (which can be dead slow, look at android R namespace as an example) even though they might never be used.

@NathanaelA I meant I would like to know the consequence of that PR (perfs, men, but mostly perfs) on an app which actually does not "use" or implement accessibility.

m-abs · 2020-09-29T20:30:43Z

@farfromrefug
Is it better now?

farfromrefug · 2020-09-29T20:43:40Z

@m-abs thanks a lot it is perfect now!

m-abs · 2020-09-29T23:06:15Z

I think, I found a better way to get from the UIView to the nativescript View-instance.

I use the UIView.tag and view._domId.
This adds a overhead for looking up the focused view, but removes the need for the WeakMap.

facetious

There are a few files that are misspelled as acessibility with one c; most notably the folder that they all live within.

facetious · 2020-09-30T02:43:54Z

packages/core/acessibility/accessibility-common.ts

+ * @param {boolean} receivedFocus
+ * @param {boolean} lostFocus
+ */
+export function notifyAccessibilityFocusState(view: View, receivedFocus: boolean, lostFocus: boolean): void {


One boolean would be sufficient.

~~This method is only used by iOS, but is in a -common. Is that intentional?~~ Apologies; the android file was collapsed by default due to its length.

facetious · 2020-09-30T02:50:56Z

packages/core/acessibility/accessibility-css-helper.ts

+
+	const oldActiveFontScaleCategory = currentFontScaleCategory;
+	if (global.isAndroid) {
+		currentFontScaleCategory = fontExtraMediumClass;


I think it would be better to define these font classes as identical on Android, rather than remove this functionality. It could be useful for developers to be able to target specific font size css classes in their on cascades. Unless I'm misunderstanding the reasoning behind this neutralizing?

Unfortunately there is a huge difference between the font-size the user can set on the two platforms, I'll try to explain:

Android:

The font-size is an integer between 0.85 and 1.3 (with some rounding errors).

iOS:

We have the setting preferredContentSizeCategory that maps to a global string constant, they are mapped here:

NativeScript/packages/core/acessibility/fontscale-observable.ios.ts

Lines 14 to 27 in 6b7cbe6

const sizeMap = new Map<string, number>([

[UIContentSizeCategoryExtraSmall, 0.5],

[UIContentSizeCategorySmall, 0.7],

[UIContentSizeCategoryMedium, 0.85],

[UIContentSizeCategoryLarge, 1],

[UIContentSizeCategoryExtraLarge, 1.15],

[UIContentSizeCategoryExtraExtraLarge, 1.3],

[UIContentSizeCategoryExtraExtraExtraLarge, 1.5],

[UIContentSizeCategoryAccessibilityMedium, 2],

[UIContentSizeCategoryAccessibilityLarge, 2.5],

[UIContentSizeCategoryAccessibilityExtraLarge, 3],

[UIContentSizeCategoryAccessibilityExtraExtraLarge, 3.5],

[UIContentSizeCategoryAccessibilityExtraExtraExtraLarge, 4],

]);

Range 0.5, 0.7, 0.85, 1, 1.15, 1.3, 1.5, 2, 2.5, 3, 3.5, 4

Values <0.85 we call extra small
Values >1.5 we call extra small

The (almost) shared range on iOS and Android 0.85 to 1.5 we call medium size.

We introduced this, because large increases in the font-sizes like 400% breaks most layouts.

I hope that makes sense.

facetious · 2020-09-30T03:05:32Z

packages/core/acessibility/accessibility-service-common.ts

+
+export const AccessibilityServiceEnabledPropName = 'accessibilityServiceEnabled';
+
+export class CommonA11YServiceEnabledObservable extends SharedA11YObservable {


I've noticed a number of places where you've used A11Y where I would have normally expected A11y. I'm open to discussion here, but my preference is the lowercase y.

I'm considering just writing it out as Accessibility :)

facetious · 2020-09-30T03:08:24Z

packages/core/acessibility/accessibility-service.android.ts

+	[touchExplorationStateEnabledPropName]: boolean;
+
+	get accessibilityServiceEnabled(): boolean {
+		return !!this.accessibilityServiceEnabled && !!this.touchExplorationStateEnabled;


Looks like an infinite recursion to me. Did you mean to use accessibilityStateEnabled instead of accessibilityServiceEnabled?

Thanks, that is clearly an error.

facetious · 2020-09-30T03:12:55Z

packages/core/acessibility/accessibility-service.d.ts

+/**
+ * Get the Android platform's AccessibilityManager.
+ */
+export function getAndroidAccessibilityManager(): android.view.accessibility.AccessibilityManager | null;


Android-specific types should be left out of common .d.ts files. In the remainder of the repo, this is done with : any /* <class name> */.

facetious · 2020-09-30T04:04:32Z

packages/core/ui/core/view/index.android.ts

 	}
+
+	public androidSendAccessibilityEvent(eventName: AndroidAccessibilityEvent, msg?: string): void {
+		if (!isAccessibilityServiceEnabled()) {


Checking the value here (and in the following two functions) seems superfluous to me. The final destination of all three ends up within sendAccessibilityEvent, which checks this condition anyway.

facetious · 2020-09-30T04:06:14Z

packages/core/ui/core/view/index.d.ts

+	/**
+	 * Internal use only. This is used to limit the number of updates to android.view.View.setContentDescription()
+	 */
+	_androidContentDescriptionUpdated?: boolean;


If it's internal use only, it could be left out of the .d.ts and just locally cast in order for TypeScript to continue validating the type.

I get a typing error in packages/core/acessibility/index.android.ts if I remove it.

facetious · 2020-09-30T04:10:30Z

packages/core/ui/core/view/index.d.ts

+	 * type = 'layout' used when the layout of a screen changes.
+	 * type = 'screen' large change made to the screen.
+	 */
+	public iosPostAccessibilityNotification(notificationType: IOSPostAccessibilityNotificationType, msg?: string): void;


I think these two methods would be best combined together.

interface AccessibilityEventOptions { androidAccessibilityEvent: AndroidAccessibilityEvent; iosNotificationType: IOSPostAccessibilityNotificationType; message: string; } public sendAccessibilityEvent(options: Partial<AccessibilityEventOptions>): void;

facetious · 2020-09-30T04:15:33Z

packages/core/ui/page/index.d.ts

+	/**
+	 * Announce screen changed
+	 */
+	public accessibilityScreenChanged(refocus?: boolean): void;


I think you mean for this to be within the //@endprivate. It should also follow the pattern: onAccessibilityScreenChanged.

The developer should be able to call this function for large updates on the page.

facetious · 2020-09-30T04:18:22Z

packages/core/ui/page/page-common.ts

 	public enableSwipeBackNavigation: boolean;
 	public backgroundSpanUnderStatusBar: boolean;
 	public hasActionBar: boolean;
+	public accessibilityAnnouncePageEnabled = true;


This property should mirror the definition of the other boolean properties, including specifying the default value elsewhere.

I'm not sure I understand.

m-abs · 2020-10-10T19:06:10Z

Hi @NathanaelA

Sorry your reply got lost in all the other replies.

@m-abs - Pardon what might be silly (& stupid) questions, trying to understand the changes as I have no real experience with A11y.

I don't see anything that calls the setContentDescription https://developer.android.com/reference/android/view/View.html#setContentDescription
I was under the impression that was needed for accessibility support for screen readers.

It is set here: https://github.com/m-abs/NativeScript/blob/feat/accessibility/packages/core/accessibility/index.android.ts#L563-L667

I attempt to recreate the behavior from iOS by concatenating accessibilityLabel, accessibilityValue and accessibilityHint here.

Are you aware that Appium for android uses https://github.com/NativeScript/nativescript-dev-appium (findElementByAccessibilityId) Find by Accessibility Id -- which is the primary method used by most test and why automationText exists in android. This uses the setContentDescription value. It is possible that Igor's fix will fix that??? or perhaps now Appium can actually use FindById via Igor's fix, which has never worked before... Don't know on this, just asking anyone who might now... Before we can merge this; we need to make sure Appium will continue to work and still has a easy to use "byid" element retrieval even if we have to change the internals of our Appium driver ..

We definitely need to have Appium working with this, but my experience with it is very limited.

@rigor789 Do you have some input here? Can FindById find views tagged via setTag?

…ervice

…r the rootView

vigdora · 2021-06-08T19:24:25Z

Hi
@NathanaelA, @rigor789 @m-abs
the change has broken our automation e2e test using Appium,

on android, we used to benefit from using automationText prop
since it added the necessary content-desc attribute to android's elements.
this way Appium managed to find those elements using the accessibility id selector.

for reference - look at the NS7 code, where setContentDescription is used:

and in NS8 it looks now like this: (probably for solving the issue for Detox platform):
@nativescript/core/ui/core/view/index.android.js

please notice that in NS8, automationText now points to accessibilityIdentifierProperty:

to summerise:
as you can see, setContentDescription was removed, thus all our automation tests on android are broken.
can the previous (NS7) solution can be combined to the new one?

m-abs · 2021-06-08T20:40:48Z

hi @vigdora,

I'm sorry this causes you problems.

can the previous (NS7) solution can be combined to the new one?

No, that would make the app inaccessible for accessibility users on android and defeat the purpose of this PR.

The old implementation was wrong, Android's setContentDescription" is not equivalent of iOS's accessibilityIdentifier`, they serve very different purposes.

Unfortunately you have to update your tests to make them work with this change.

I haven't worked with appium myself, but maybe this can help you:
https://github.com/rigor789/ui-tested-nsvue-app/blob/fecd9ed0392b8903d20a0967d87d60201082bc32/e2e/firstTest.spec.js#L24

Using setTag was suggested by @rigor789 who linked to that repo.

vigdora · 2021-06-09T11:45:10Z

@m-abs
this is tough...
as far as I know, Appium doesn't let us find elements easily cross app except using the Accessibility ID selector.
without this selector, I will need to use xpath or other strategies which will make testing way inconvenient.

The link you referred to doesn't help much since it uses Detox or still uses the Accessibility ID with Appium.

farfromrefug · 2021-06-09T12:43:40Z

@vigdora there is a quick workaround. Create a plugin which adds a mixin N. In that mixin override the [accessibilityIdentifierProperty.setNative] function. In there call setContentDescription as well as the original method.
I could even be part of the nativescript appium plugin.

vigdora · 2021-06-09T14:40:01Z

@farfromrefug
I already tried it - but it doesnt work since the function applyContentDescription (found in @nativescript/core/accessibility/index.android.js)
is clearing the contentdescription if no accessibility attribute (i.e. accessibilityLabel) is presented on the element:

@m-abs
may be we can remove the contentdescription clearance as depicted in the snapshot I shared (red arrow).
than I can apply @farfromrefug workaround

m-abs · 2021-06-09T16:41:31Z

@farfromrefug and @vigdora
I don't think this discussion belongs here on a merged PR.

You should probably create a new issue about it and maybe take the discussion on slack.

farfromrefug · 2021-06-09T17:29:16Z

@m-abs thanks we all take it along from here.
@vigdora I think we might be able to remove it. could you try ?

vigdora · 2021-06-13T10:41:30Z

I gave it another thought and removing androidView.setContentDescription(null) as I mentioned in my previous comment still wont help as text attribute (textValue) will populate content-description on android any way.

so I honestly have no quick solution for our automation tests,
I would have liked to open a PR for fixing it but the only solution I see right now is to allow disabling the accessibility feature on android once a developer choses to use the automationText attribute.
what are the chances for it to be approved?

cla-bot bot added the cla: yes label Sep 27, 2020

This comment was marked as abuse.

Sign in to view

facetious reviewed Sep 30, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

NathanWalker added this to the 8.0 milestone Oct 5, 2020

This was referenced Oct 21, 2020

feat(core): added testID for Detox e2e testing (#8943) #8944

Closed

Add Detox plugin NativeScript/plugins#23

Merged

m-abs added 6 commits January 12, 2021 09:40

feat(Slider<ios>): add a11y step size and increase/decrease events

9e3c08b

feat(A11Y): adds FontScaleObservable

2c0ea28

fix(Slider): Forgot to register accessibilityStepProperty

7c9703d

feat(A11Y): Make a11y properties

58acbce

feat(A11Y): Adds css-helper, accessibility-helper and accessibility-s…

58be72e

…ervice

refactor(A11Y): put helper functions into accessibility/index

dbcdad3

m-abs added 7 commits January 12, 2021 10:58

feat(A11Y): Lookup the focues UIView in modal views if not found unde…

d245ba4

…r the rootView

chore(A11Y): Refactor and better naming

b063c2e

chore(A11Y): Follow up on review suggestions

15ee076

fix: note spelling: acessibility => accessibility

35d3ac1

refactor(FontScale): Removed FontScaleObservable, resize fonts on iOS

046937a

fix spelling

c7a5d67

fix(a11y): FontScaleCategory not exported

d4fce71

NathanWalker force-pushed the feat/accessibility branch from 885f354 to d4fce71 Compare January 12, 2021 19:06

SylvatiCodes mentioned this pull request Jan 13, 2021

Feature(iOS): Fonts are not scaled for a11y in nativescript. #6125

Closed

NathanWalker changed the base branch from master to release/8.0.0 January 21, 2021 17:33

NathanWalker changed the title ~~Implements #6993 - Adds upstream accessibility support to nativescript (WIP)~~ feat(core): a11y support Jan 21, 2021

Merge branch 'release/8.0.0' into feat/accessibility

35d6f66

NathanWalker merged commit bbc8aad into NativeScript:release/8.0.0 Jan 29, 2021

m-abs deleted the feat/accessibility branch February 1, 2021 17:34

NathanWalker pushed a commit that referenced this pull request Feb 12, 2021

feat(core): first class a11y support (#8909)

c46da3f

NathanWalker pushed a commit that referenced this pull request Feb 27, 2021

feat(core): first class a11y support (#8909)

d038188

NathanWalker pushed a commit that referenced this pull request Feb 27, 2021

feat(core): first class a11y support (#8909)

f2e21a5

NathanWalker pushed a commit that referenced this pull request Mar 3, 2021

feat(core): first class a11y support (#8909)

76f1b4d

NathanWalker pushed a commit that referenced this pull request Mar 5, 2021

feat(core): first class a11y support (#8909)

39971a6

NathanWalker pushed a commit that referenced this pull request Apr 2, 2021

feat(core): first class a11y support (#8909)

cc99d97

NathanWalker pushed a commit that referenced this pull request Apr 6, 2021

feat(core): first class a11y support (#8909)

d5a8a25

coolboy0961 mentioned this pull request Jun 20, 2021

accessibilityLabel doesn't override accessibility id of Android #9450

Closed

Jan	FEB	Mar
	14
2025	2026	2027

	const sizeMap = new Map<string, number>([
	[UIContentSizeCategoryExtraSmall, 0.5],
	[UIContentSizeCategorySmall, 0.7],
	[UIContentSizeCategoryMedium, 0.85],
	[UIContentSizeCategoryLarge, 1],
	[UIContentSizeCategoryExtraLarge, 1.15],
	[UIContentSizeCategoryExtraExtraLarge, 1.3],
	[UIContentSizeCategoryExtraExtraExtraLarge, 1.5],
	[UIContentSizeCategoryAccessibilityMedium, 2],
	[UIContentSizeCategoryAccessibilityLarge, 2.5],
	[UIContentSizeCategoryAccessibilityExtraLarge, 3],
	[UIContentSizeCategoryAccessibilityExtraExtraLarge, 3.5],
	[UIContentSizeCategoryAccessibilityExtraExtraExtraLarge, 4],
	]);


		export const AccessibilityServiceEnabledPropName = 'accessibilityServiceEnabled';

		export class CommonA11YServiceEnabledObservable extends SharedA11YObservable {

Uh oh!

Conversation

m-abs commented Sep 27, 2020 • edited by NathanWalker Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

What is the current behavior?

What is the new behavior?

TODO:

Uh oh!

cla-bot bot commented Sep 27, 2020

Uh oh!

m-abs commented Sep 27, 2020

Uh oh!

cla-bot bot commented Sep 27, 2020

Uh oh!

This comment was marked as abuse.

m-abs commented Sep 28, 2020

Uh oh!

rigor789 commented Sep 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

m-abs commented Sep 28, 2020

Uh oh!

farfromrefug commented Sep 29, 2020

Uh oh!

This comment was marked as abuse.

m-abs commented Sep 29, 2020

Uh oh!

m-abs commented Sep 29, 2020

Uh oh!

farfromrefug commented Sep 29, 2020

Uh oh!

m-abs commented Sep 29, 2020

Uh oh!

farfromrefug commented Sep 29, 2020

Uh oh!

m-abs commented Sep 29, 2020

Uh oh!

This comment was marked as abuse.

This comment was marked as abuse.

facetious left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

m-abs commented Oct 10, 2020

Uh oh!

m-abs commented Sep 27, 2020 •

edited by NathanWalker

Loading

rigor789 commented Sep 28, 2020 •

edited

Loading

vigdora commented Jun 8, 2021 •

edited

Loading

vigdora commented Jun 9, 2021 •

edited

Loading

vigdora commented Jun 9, 2021 •

edited

Loading

m-abs commented Jun 9, 2021 •

edited by NathanWalker

Loading

vigdora commented Jun 13, 2021 •

edited

Loading