[Android] label -> contentDescription #293

simonracz · 2017-09-26T12:53:25Z

I rewrote label handling on Android to make it search by contentDescription instead of text.

This showed that the detox test suite is actually tied to iOS. Therefore, I had to touch nearly all screens and tests to make it cross-platform.

iOS vs Android example
A TextView has no default contentDescription on Android. From RN's perspective this means that you have to set accessibilityLabel directly on a <Text>. On iOS this works without this.

I also updated the docs and detox init's default e2e file.

isnifer · 2017-09-26T13:01:55Z

detox/test/src/Screens/MatchersScreen.js

        </TouchableOpacity>

        <TouchableOpacity onPress={this.onButtonPress.bind(this, 'ID Working')}>
-          <Text testID='UniqueId345' style={{color: 'blue', marginBottom: 20}}>ID</Text>
+          <Text testID='UniqueId345' style={{color: 'blue', marginBottom: 20}} accessibilityLabel={'ID'}>ID</Text>


@simonracz explain, please, why testID and accessibilityLabel have different values? I'm using react-native-testid at Tipsi. And we have both parameters the same

AccessibilityLabel is user visible. TestId is not.

@isnifer Bad idea.
tipsi/react-native-testid#1

LeoNatan · 2017-09-26T18:44:35Z

detox/test/src/Screens/ActionsScreen.js

@@ -29,7 +29,7 @@ export default class ActionsScreen extends Component {
        <TouchableOpacity onPress={this.onButtonPress.bind(this, 'Tap Working')}
          onLongPress={this.onButtonPress.bind(this, 'Long Press Working')}
          >
-          <Text style={{ color: 'blue', marginBottom: 20, textAlign: 'center' }}>Tap Me</Text>
+          <Text style={{ color: 'blue', marginBottom: 20, textAlign: 'center' }} acccessibilityLabel={'Tap Me'}>Tap Me</Text>


I don’t like these changes.
Can you please explain why we need to require users to add accessibilityLabel on each element?

On iOS, accessibilityLabel is used by Voice Over, for example, and is not always linked 1:1 with the content. The developer can certainly change it if they feel they can provide a better label for the content.
How does this work on Android?

Perhaps we should change the tests on iOS to by.text(‘’), but requiring users to add accessibilityLabel on each element is not something I can live with.

Also please note you misspelled “accessibilityLabel” here (“ccc”).

LeoNatan · 2017-09-26T18:48:30Z

docs/APIRef.Expect.md

@@ -52,6 +53,13 @@ await expect(element(by.id('RandomJunk959'))).toNotExist();
 await expect(element(by.id('UniqueId204'))).toHaveText('I contain some text');
 ```

+### `toHaveLabel(label)`
+- Similar to `toHaveText(text)`, but searches by accessibilityLabel (iOS) or by contentDescription (Android)


The two shouldn’t be linked in such nonchalant manner. On iOS, text and label can have multiple meanings, and they wouldn’t be similar in cases where users care about the accessibility of their application. The documentation should split those completely.

You probably read much more out of the "similar" word that was meant there. But, I agree, I'll rewrite it.

My issue is that some users l, particularly in RN ecosystem, are not familiar with iOS concepts, and learn half-truths because of imprecise text they see here and there. I’m sure it’s the same for Android. This is why I try to push for accuracy, so everyone gets the correct picture.

LeoNatan · 2017-09-26T18:53:42Z

docs/APIRef.Matchers.md

@@ -42,14 +43,22 @@ Find an element by text, useful for text fields, buttons.
 ```js
 await element(by.text('Tap Me'));
 ```
+
+#### `by.label(label)`
+Find an element by accessibilityLabel(iOS) or contentDescription(Android), useful for text fields, buttons.


Accessibility label is useful for almost any UI element on iOS. As per documentation, this should be properly set to any element with custom appearance, such as images. The system helps by trying to set a value to this property be similar to the title of elements such as text fields or buttons, but also for other elements. The “useful” comment implies incorrect here.

Ok. I'll remove the useful part.

LeoNatan · 2017-09-26T18:54:41Z

detox/test/src/Screens/WaitForScreen.js

+            <Text style={{height: 30, backgroundColor: '#e8e8f8', padding: 5, margin: 10}} accessibilityLabel={'Text5'}>Text5</Text>
+            <Text style={{height: 30, backgroundColor: '#e8e8f8', padding: 5, margin: 10}} accessibilityLabel={'Text6'}>Text6</Text>
+            <Text style={{height: 30, backgroundColor: '#e8e8f8', padding: 5, margin: 10}} accessibilityLabel={'Text7'}>Text7</Text>
+            <Text style={{height: 30, backgroundColor: '#e8e8f8', padding: 5, margin: 10}} accessibilityLabel={'Text8'}>Text8</Text>
          </ScrollView>


I still don’t understand why, if tests are now by.text(), why this label was added to all views?

Not all tests search by text, even after this commit.

I followed the following pattern:
For fixed elements, I set accessibilityLabel. For dynamic elements I didn't. (The reason behind this was that blind people could read out the structure of the app from the fixed elements.)

I can remove nearly all accessibilityLabels and search only by text if you prefer. (I'll leave the accessibilityLabel for the Matcher tests, where we explicitly test for accessibilityLabel.)

I think this will be best. What do you think? It will also show best practice to users that wonder how to use the API.

I think detox users should almost always search by text. And only use label, if they really want to search by accessibilityLabel/contentDescription.

This is why I found it important to also change the docs, sample codes and detox init e2e file.

100% agree. That’s why I think all the labels in the test are unnecessary.

But the most accurate is to match by identifier. By text is problematic if you use localization.

erezrokah · 2017-09-26T19:29:00Z

Maybe you'll find the discussion(s) here helpful:

facebook/react-native#7135
facebook/react-native#9777

LeoNatan · 2017-09-26T19:33:06Z

People abusing accessibleLabel makes me sad. There is so little awareness of what accessibility is, and why it is important. That’s true on the web, too, so there is no excuse.

simonracz · 2017-09-27T19:51:36Z

I addressed the comments in the latest commit.

mfrachet · 2017-09-29T07:31:51Z

detox/test/android/app/src/main/AndroidManifest.xml

@@ -5,6 +5,7 @@

    <uses-permission android:name="android.permission.INTERNET" />
    <uses-permission android:name="android.permission.SYSTEM_ALERT_WINDOW"/>
+    <uses-permission android:name="android.permission.ACCESS_FINE_LOCATION"/>


Can't get the point of this line in this PR

It was missing for an Android test. (Which is not triggered by CI yet.)

The whole point of the PR was to make the test suite cross platform.

simonracz requested review from rotemmiz and LeoNatan September 26, 2017 12:53

isnifer reviewed Sep 26, 2017

View reviewed changes

LeoNatan suggested changes Sep 26, 2017

View reviewed changes

simonracz added 4 commits September 27, 2017 19:26

[Android] label -> contentDescription and test suite fixes

3cfbf54

[Android] label -> text rewrite finished where needed

5b130fe

Updated docs about label. Updated detox init script.

ad42950

label -> text nearly everywhere

37b5c40

simonracz force-pushed the contentDescription branch from 5a472f9 to 37b5c40 Compare September 27, 2017 19:48

simonracz merged commit 92a29ef into master Sep 27, 2017

simonracz deleted the contentDescription branch September 27, 2017 20:19

$mfrachet$

mfrachet reviewed Sep 29, 2017

View reviewed changes

wix locked and limited conversation to collaborators Jul 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Android] label -> contentDescription #293

[Android] label -> contentDescription #293

simonracz commented Sep 26, 2017

isnifer Sep 26, 2017 •

edited

Loading

simonracz Sep 26, 2017

LeoNatan Sep 26, 2017

LeoNatan Sep 26, 2017

LeoNatan Sep 26, 2017

simonracz Sep 26, 2017

LeoNatan Sep 26, 2017

LeoNatan Sep 26, 2017

simonracz Sep 26, 2017

LeoNatan Sep 26, 2017

simonracz Sep 26, 2017

LeoNatan Sep 26, 2017

simonracz Sep 26, 2017

LeoNatan Sep 26, 2017

erezrokah commented Sep 26, 2017

LeoNatan commented Sep 26, 2017

simonracz commented Sep 27, 2017

$@mfrachet$ mfrachet Sep 29, 2017

simonracz Sep 29, 2017

[Android] label -> contentDescription #293

[Android] label -> contentDescription #293

Conversation

simonracz commented Sep 26, 2017

isnifer Sep 26, 2017 • edited Loading

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

erezrokah commented Sep 26, 2017

LeoNatan commented Sep 26, 2017

simonracz commented Sep 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isnifer Sep 26, 2017 •

edited

Loading