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.

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 accessibilityHint matcher #2313

Closed
Andarius opened this issue Sep 8, 2020 · 13 comments
Closed

Add accessibilityHint matcher #2313

Andarius opened this issue Sep 8, 2020 · 13 comments

Comments

@Andarius
Copy link
Contributor

Andarius commented Sep 8, 2020

Is your feature request related to a problem? Please describe.

As suggested by react-native-testing-library, I'm trying to move away from using testID when possible in favor of Accessibility options.

Describe the solution you'd like
Add a by.hint or by.a11yHint matcher.

@LeoNatan
Copy link
Contributor

LeoNatan commented Sep 8, 2020

Accessibility hint is not a throwaway string; it is supposed to be a localized string on iOS and is use in the accessibility system: https://developer.apple.com/documentation/objectivec/nsobject/1615093-accessibilityhint?language=objc

So I don't see how that is a good candidate for uniquely identifying elements. You might as well use by.text or by.label.

Since XCUIElementAttributes doesn't expose the hint, and there is no compelling reason to have this (it doesn't enable any safer matching than text or labels), I don't see as adding this.

testID is translated to accessibilityIdentifier, BTW.

@LeoNatan
Copy link
Contributor

LeoNatan commented Sep 8, 2020

The text on that link makes no sense. The accessibility identifier is intended to be a one to one way to match your UI elements. If you expect an element to be visible and then interact with it, I don't see how that gives you a "false sense of security", unless you have the same identifier for multiple UI elements.

On the other hand, abusing the accessibility system for things it is not intended to has actual real-world harm for disabled users.

@Andarius
Copy link
Contributor Author

Andarius commented Sep 8, 2020

Thank you for the feedback !

So I don't see how that is a good candidate for uniquely identifying elements. You might as well use by.text or by.label.

If I have an icon to press for instance, without text or label, or an uncontrolled input (but you know what the item does) I find it to make more sense to use the accessibility description of what the icon does rather than just adding a testID to it.
Also I don't see how having a selector that pushes you to add accessibility properties like what the field / button does to your code instead of just an ID causes "real-world harm".

That being said, I understand the technical limitation.

@LeoNatan
Copy link
Contributor

LeoNatan commented Sep 8, 2020

If you care about accessibility, any image view should have a label, as things like Voice Over use the label to read to the user. While labels and hints have different technical meaning, they are so close in their ability to distinct UI elements (e.g. not very good), I still don't see a point.

You should be setting hints and labels not because of tests, but because you care about accessibility and your disable users. If you add hints that are unique just for tests, and your users use Voice Over, the real-world harm is that they will hear strings that are not intended for the users to hear. This is why on iOS and Android, you have accessibility identifiers just for this purpose.

@Andarius
Copy link
Contributor Author

Andarius commented Sep 8, 2020

You should be setting hints and labels not because of tests, but because you care about accessibility and your disable users.

I'm confused, but where did you see that accessibility options are written only for tests ? They are written alongside text / label for disabled people. Sometimes they are even the only information about a component (for instance an icon button).
So why would it make sense to be able to access it with text / label but not accessibility description when it is the only descriptive information available ? With this line of thoughts, by.text and by.label are useless as we can use by.id everywhere.

@LeoNatan
Copy link
Contributor

LeoNatan commented Sep 8, 2020

If your hints are intended for users, that's fine, but why would you add a hint but not add a label? My point here is that label and hint are very analogous from a test's matching standpoint.

Text and label matching are useful when it is impossible to set an identifier, such as in system views. They are discouraged, but provided just in case. Hint would be equally discouraged, so I just don't see the point. Even Apple doesn't see the point, as it is not exposed in its own testing API. Apple also doesn't expose text, but we felt this was important enough to have (as text is different from label in some cases). I don't feel this is the case for hint.

@LeoNatan
Copy link
Contributor

LeoNatan commented Sep 8, 2020

By all means, you should be using by.id() as much as possible.

@thymikee
Copy link
Contributor

thymikee commented Sep 9, 2020

Hey, a RNTL maintainer here 👋. I think we were too harsh about this "recommendation" of avoiding using testIDs. Our intention was to force users a little to think about accessibility, which in our experience is often forgotten when developing a UI, unfortunately. Making test queries accessibility-oriented increases the chance for a developer to do so.

Using testID is fine in most cases, but we believe the textual matchers should be used first, because they resemble user behaviors more closely. Users can't see testIDs and while the ID may not change, a text it represents may – I can imagine a button that mistakenly changes "confirm" to "cancel" because some text/replace went too far. The semantics have changed, while keeping the same ID, so a test would happily pass.

If you add hints that are unique just for tests, and your users use Voice Over, the real-world harm is that they will hear strings that are not intended for the users to hear

I couldn't agree more. I've seen people blindly replacing testID with accessibilityHint because IDs are "not recommended" and it's just plain wrong in most cases. And if something should have been discouraged, accessibilityHint would be a better candidate now IMHO, since it accompanies accessibilityLabel which can uniquely identify a view in most cases. If not, testID is a way to go then.

@LeoNatan
Copy link
Contributor

LeoNatan commented Sep 9, 2020

Hello,

There are several problems with textual matching that we've seen people repeat over and over; 1. they are localized strings, so sometimes language and/or locale of the simulator on CI doesn't match the language and/or locale of the one writing tests (Detox also offers tools to change language and or locale to test different configurations—this would be impossible to test by text); 2. user facing text changes over time, and tests tend to break; 3. the chances of several elements having the same text or label (or hint for that matter) increases as the size of the application grows.
The last issue is especially prevalent with RN apps, where you have terrible libraries such as react-navigation keeping all views loaded into memory and in the view hierarchy. We've had countless users complain that a matcher finds multiple elements and they don't understand why.

I'm not sure why users not seeing the accessibility identifiers is important. We do offer expectations that test writers can execute to ensure their views have the right text and/or label. If a user is afraid the text of a button might change, they can assert that it is "confirm" instead of "cancel". I'd say that is quite the callous copypasta behavior if someone makes such an error. 😄

Internally, we push everyone to add identifiers. We've experienced many of the issues above multiple times over the years. Anyone should write tests as they see fit, of course, but my recommendations come from our extensive experience both internally and with the open-source community.

@thymikee
Copy link
Contributor

thymikee commented Sep 9, 2020

they are localized strings, so sometimes language and/or locale of the simulator on CI doesn't match

Very true for e2e tests, not a big deal for JS tests because we mock stuff as we like.

react-navigation keeping all views loaded into memory and in the view hierarchy. We've had countless users complain that a matcher finds multiple elements and they don't understand why

We bump into this as well. However as I've spoken to Satya, one of the maintainers, they implement a bunch of accessibility props to hide screens from the users, and we (RNTL) should probably respect that when querying. We have other means to do so as well.

I really appreciate the experience-based approach, good callouts 👍. I'm going to adjust the wording around byId queries, or maybe remove the warning entirely.

Thanks!

@LeoNatan
Copy link
Contributor

LeoNatan commented Sep 9, 2020

Thank you for taking the time to discuss! 🙂

I just want to say that Detox might be unique in that we operate within the confines of the native world. We have no access to the JS, nor operate at that level. Once an action or an expectation in Detox is run, it travels to the native world, where we observe the native view hierarchy. A JS testing framework might exhibit a different set of experiences.

With react-navigation, the smart thing to do for its users isn't to just hide the views, but to remove them altogether from the hierarchy. Even a hidden view gets laid out by the OS framework, and having several layers of views deep being laid out just doesn't make sense from a perspective standpoint. We have our own navigation library, react-native-navigation, which actually uses the OS native navigation controllers, so it behaves natively, looks natively and performs natively as much as possible.

@satya164
Copy link

satya164 commented Sep 9, 2020

The last issue is especially prevalent with RN apps, where you have terrible libraries such as react-navigation keeping all views loaded into memory and in the view hierarchy. We've had countless users complain that a matcher finds multiple elements and they don't understand why.

React Navigation integrates with React Native Screens which does remove views from hierarchy. The plan is to enable it by default soon.

Regardless, React Navigation also hides these inactive screens from accessibility tree by using relevant accessibility props. Accessibility tools like VoiceOver and TalkBack don't read contents of inactive screens. If the matcher would respect the accessibility tree, then it wouldn't be a problem. I believe tools like Appium work correctly in this case.

@LeoNatan
Copy link
Contributor

LeoNatan commented Sep 9, 2020

We operate on the view hierarchy level, not accessibility tree on purpose, unlike Apple's own testing framework, for example (which Appium can use as a driver). This gives us a much more accurate method of matching views. In some cases, you want to match a view and assert it isn't visible, for example. But as I said in another comment, even a hidden view gets laid out at the OS level. This, coupled with React Native and its userbase tendency to create layers upon layers of wrapper views, causes HUGE hierarchies to be laid out needlessly. This can cause serious performance issues and battery drain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants