Expose setAccessibilityFocus from Native-Common AccessibilityUtil and expose setting accessibility focus on RX.TextInput. #69

eulphean · 2017-04-24T21:09:04Z

This change exposes a way to set accessibility focus on RX.TextInput. This also changes the way we had AccessibilityUtil implementations individually for each platform. That becomes an overhead for each component that wants to leverage those platform helpers. We need to create a platform specific copy of each component and leverage the platform specific AccessibilityUtil.

With this change, we let Native-Common AccessibilityUtil do all the work for calling any platform specific methods.

msftclas · 2017-04-24T21:09:09Z

@eulphean,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

berickson1 · 2017-04-24T23:14:20Z

src/native-common/AccessibilityUtil.ts


 export class AccessibilityUtil extends CommonAccessibilityUtil {
+    // Native platform specific instance for AccessibilityUtil. 
+    private _instance: typeof AndroidAccessibilityUtil | typeof iOSAccessibilityUtil; 


These should both be the same type. You should define an interface (or abstract base class) that both iOS and Android need to implmenet

berickson1 · 2017-04-24T23:15:05Z

src/native-common/AccessibilityUtil.ts

+                break;
+
+            default: 
+                assert(false, 'Unknown platform.');


uwp isn't an unknown platform inside ReactXP

msftsvachon · 2017-04-25T06:44:25Z

src/native-common/AccessibilityUtil.ts

 };

+// Platform specific helpers exposed through Native-Common AccessibilityUtil. 
+export abstract class NativeHelpers {


Consider giving a more specific name to this class to be derived by each platform. Maybe someone like CommonNativeAccessibilityUtil which would be in line with the pattern used currently or NativeAccessibilityHelpers. NativeHelpers is very generic and makes it's difficult to the reader to understand what it's about on its own.

I agree with Seb. NativeHelpers is too generic a name to export.

berickson1 · 2017-04-25T06:52:15Z

src/android/AccessibilityUtil.ts


 import Accessibility from '../native-common/Accessibility';
-import { AccessibilityUtil as CommonAccessibilityUtil } from '../native-common/AccessibilityUtil';
+import { NativeHelpers } from '../native-common/AccessibilityUtil';


I think you have a potential circular dependency here. native-common/AccessibilityUtil -> android/AccessibilityUtil -> native-common/AccessibilityUtil

erictraut · 2017-04-25T16:29:13Z

src/ios/AccessibilityUtil.ts

@@ -0,0 +1,22 @@
+/**
+* iOSAccessibilityUtil.ts


The comment should match the file name.

erictraut · 2017-04-25T16:30:53Z

src/native-common/AccessibilityUtil.ts

+import RN = require('react-native');

 import { AccessibilityUtil as CommonAccessibilityUtil } from '../common/AccessibilityUtil';
+import AndroidAccessibilityUtil from '../android/AccessibilityUtil';


Why are you importing platform-specific files within a common file? This is an anti-pattern and will create dependency cycles. Platform-specific files should import native-common, not the other way around.

While this is an anti-pattern, it's beneficial to us. The idea behind this is that pretty much every UI component needs to talk to AccessibilityUtils in some way, shape or form. That leaves a few options:

Extend every UI component (that needs accessibility) for every platform so that it imports the platform specific AccessibiilityUtils and calls the function

Provide some sort of override/shim at the RX platform creation (something AccessibilityUtils.setPlatformHelperFunctions that pipes to this._impl that's in this PR)

Have native-common/AccessibilityUtils decide which impl to use on init and import platform specific implementations

#1 adds a bunch of extra code and another layer of abstract classes
#2 is somewhat obscure and non-obvious to someone working in the codebase
#3 isn't ideal but seems somewhat better than the other options. (Also can be done without cycles)

Do you have any alternative solutions?

erictraut · 2017-04-25T16:31:44Z

src/native-common/AccessibilityUtil.ts

 };

+// Platform specific helpers exposed through Native-Common AccessibilityUtil. 
+export abstract class NativeHelpers {


I agree with Seb. NativeHelpers is too generic a name to export.

… components can leverage native-common instance directly to set focus on iOS and Android platforms.

eulphean · 2017-04-27T01:31:02Z

@erictraut @berickson1

I have a new iteration addressing the comments.

erictraut · 2017-04-27T03:36:44Z

src/android/AndroidAccessibilityUtil.ts

@@ -1,5 +1,5 @@
 /**
-* AccessibilityUtil.ts
+* AndroidAccessibilityUtil.ts


Why did you rename this file to start with "Android"? It's already in the Android directory, so adding this is redundant. It's also inconsistent with all of the other code in ReactXP. I'd prefer if you went back to the previous name for consistency.

erictraut · 2017-04-27T03:39:09Z

src/ios/iOSAccessibilityUtil.ts

@@ -0,0 +1,22 @@
+/**
+* iOSAccessibilityUtil.ts


Once again, the file name shouldn't contain iOS since that's redundant. It's already in a folder called iOS. Same with the class name it exports.

erictraut · 2017-04-27T03:40:54Z

src/native-common/AccessibilityUtil.ts

+import RN = require('react-native');
+
+import { AccessibilityUtil as CommonAccessibilityUtil, PlatformAccessibilityHelpers } from '../common/AccessibilityUtil';
+import AndroidAccessibilityUtil from '../android/AndroidAccessibilityUtil';


This is still inverted. The "native common" implementation should not be importing the platform-specific implementations. It should be the other way around. The platform-specific implementations should be importing the common and overriding the parts that are necessary. Doing it the other way is inconsistent with all other parts of ReactXP and has the potential for creating dependency cycles.

erictraut · 2017-04-27T03:41:40Z

src/native-common/AccessibilityUtil.ts

+        // Set AccessibilityUtil instance based on the platform. 
+        switch (RN.Platform.OS) {
+            case 'android':
+                this._instance = AndroidAccessibilityUtil;


Instantiation of platform-specific modules should happen only in the ReactXP.ts files that are specific to each platform. You shouldn't be doing it here in this native-common module. By doing so, you're pulling in all platform implementation on all platforms. The iOS version of ReactXP shouldn't contain any Android-specific code and vice versa. By instantiating the platform-specific code in ReactXP.ts, we are able to strip out the code for other platforms. By doing it here, you defeat that mechanism.

erictraut · 2017-04-27T03:43:46Z

src/windows/WindowsAccessibilityUtil.ts

@@ -0,0 +1,20 @@
+/**
+* WindowsAccessibilityUtil.ts


See note above. This file and its exported class shouldn't start with "Windows".

eulphean · 2017-04-27T09:08:21Z

Thanks for the feedback, guys. I just pushed a latest iteration that should address the concerns around this. The problem with using a flat inheritance model how RX.Linking component does is that platform specific implementations need to be initialized inside Native-Common AccessibilityUtil in order for it to consume it. This brings up the concern of importing platform specific code into Native-Common instance that defeats the purpose of ReactXp's message.

By letting initialization path in ReactXp poke the platform specific instance inside native-common AccessibilityUtil, we solve that problem.

I used Seb's suggestion for naming the platform specific accessibility helper. Goal was to keep the suffix 'Util' to bear resemblance and use the term 'Native' to drive the difference between the two classes.

erictraut · 2017-04-27T11:36:45Z

Looking better. I realize this approach is still a bit convoluted, but I think it's much better than the previous approach. Once @berickson1 reviews and approves the change, he or I can merge it in.

…ame AccessibilityNativeUtil to AccessibilityPlatformUtil as that is more in sync with all the comments in the codebase.

… expose setting accessibility focus on RX.TextInput. (microsoft#69) * Expose setAccessibilityFocus from Native-Common AccessibilityUtil, so components can leverage native-common instance directly to set focus on iOS and Android platforms. * Code review feedback. * PR feedback. * PR feedback. * Nit: Space * PR feedback. Rename platform specific files to AccessibilityUtil. Rename AccessibilityNativeUtil to AccessibilityPlatformUtil as that is more in sync with all the comments in the codebase.

msftclas added the cla-not-required label Apr 24, 2017

berickson1 reviewed Apr 24, 2017

View reviewed changes

msftsvachon reviewed Apr 25, 2017

View reviewed changes

msftsvachon approved these changes Apr 25, 2017

View reviewed changes

berickson1 requested changes Apr 25, 2017

View reviewed changes

erictraut suggested changes Apr 25, 2017

View reviewed changes

Amay Kataria added 3 commits April 26, 2017 15:28

Expose setAccessibilityFocus from Native-Common AccessibilityUtil, so…

df18cb5

… components can leverage native-common instance directly to set focus on iOS and Android platforms.

Code review feedback.

e15e034

PR feedback.

e904f2e

eulphean force-pushed the ak/a11yTextInput branch from 4f32743 to e904f2e Compare April 27, 2017 01:29

erictraut suggested changes Apr 27, 2017

View reviewed changes

Amay Kataria added 2 commits April 27, 2017 01:57

PR feedback.

f662f57

Nit: Space

7d16f97

erictraut approved these changes Apr 27, 2017

View reviewed changes

PR feedback. Rename platform specific files to AccessibilityUtil. Ren…

14a81d5

…ame AccessibilityNativeUtil to AccessibilityPlatformUtil as that is more in sync with all the comments in the codebase.

berickson1 approved these changes Apr 27, 2017

View reviewed changes

berickson1 merged commit cff4b51 into microsoft:master Apr 27, 2017

eulphean deleted the ak/a11yTextInput branch May 3, 2017 05:37

Expose setAccessibilityFocus from Native-Common AccessibilityUtil and expose setting accessibility focus on RX.TextInput. #69

Expose setAccessibilityFocus from Native-Common AccessibilityUtil and expose setting accessibility focus on RX.TextInput. #69

Uh oh!

Conversation

eulphean commented Apr 24, 2017

Uh oh!

msftclas commented Apr 24, 2017

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!

berickson1 Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eulphean commented Apr 27, 2017

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!

eulphean commented Apr 27, 2017

Uh oh!

erictraut commented Apr 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

berickson1 Apr 25, 2017 •

edited

Loading