-
Notifications
You must be signed in to change notification settings - Fork 29.7k
[WIP] Semantics integration test for Android #19005
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
Conversation
|
cc @yjbanov @goderbauer - I'm not quite sure what I need to do to correctly wire this up as an integration test. This also includes some additions to flutter_test which I think are related to overall testing improvements, but I can split that out into a separate PR if that would be easier to track |
goderbauer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for adding integrations tests. We should figure out a similar way for iOS.
Also, can we split this into at least 3 PRs for easier reviewing:
- PR for the changes to flutter_test
- PR for the changes to flutter_driver
- PR for adding the actual integration tests
| Widget build(BuildContext context) { | ||
| return new MaterialApp( | ||
| routes: <String, WidgetBuilder>{ | ||
| 'SelectionControls': (BuildContext context) => new SelectionControlsPage(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these maybe be static consts on SelectionControlPage and TextFieldsPage? e.g. SelectionControlPage.routeName and TextFieldsPage.routeName.
| result.put("flags", flags); | ||
| Rect nodeRect = new Rect(); | ||
| node.getBoundsInScreen(nodeRect); | ||
| rect.put("left", nodeRect.left / mScreenDensity); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to correct these values with the density?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just so the rects will be in the same space as the rects on the SemanticsData. So you could check that a checkbox is 48 by 48
| /// A checkbox widget. | ||
| static const String checkBox = 'android.widget.CheckBox'; | ||
|
|
||
| /// The default className if none is provided by flutter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: flutter -> Flutter
| static const String textView = 'android.widget.TextView'; | ||
| } | ||
|
|
||
| /// Action constants Taken from [AccessibilityAction]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is Android's android.view.accessibility.AccessibilityNodeInfo.AccessibilityAction? Maybe make that clearer? Also, I am not sure if dart doc can link to that, so maybe use enclose it in `` instead of []?
| return id == typedOther.id; | ||
| } | ||
|
|
||
| /// Taken from [AccessibilityAction.ACTION_FOCUS]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also here and below.
| testWidgets('BackButton semantics', (WidgetTester tester) async { | ||
| final SemanticsHandle handle = tester.ensureSemantics(); | ||
| await tester.pumpWidget( | ||
| new MaterialApp( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add trailing commas and only indent by two spaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return result.changedState; | ||
| } | ||
|
|
||
| Future<int> getSemanticsId(SerializableFinder finder, { Duration timeout = _kShortTimeout}) async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doc comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in #19047
|
|
||
|
|
||
| /// A Flutter driver command that retrieves a semantics id using a specified | ||
| /// finder. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if the thing identified by finder doesn't own a semantics node?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in #19047
| /// A Flutter driver command that retrieves a semantics id using a specified | ||
| /// finder. | ||
| /// | ||
| /// Semantics must be enabled before using this command. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link to how to enable semantics?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in #19047
| await tap(backButton); | ||
| } | ||
|
|
||
| /// Attempts to find the [SemanticsData] of first result from [finder]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
document what happens if the object identified by finder doesn't own its own semantics node?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd even make it 4 PRs: |
|
It seems a bit... overly complicated that we are sending the semantic data from the framework to the Java code, then back to the framework, then to the driver. Ideally we'd want to check the actual data from Android, as if we were Talkback. Is there some way we can do that instead? Reading the data using the Android accessibility APIs directly? |
|
I don't believe we can use TalkBack directly - in theory we could write our own accessibility service, but there is the added complexity of trying to synchronize three separate processes instead of just two. Ultimately TalkBack just sees the (maybe this is technically an end to end test?) |
|
How do regular android apps test their accessibility? |
|
A combination of manual testing and a few automated lints like missing labels |
This test still needs more config, and requires talkback to be enabled to function. This can be done through adb:
adb shell settings put secure enabled_accessibility_services com.google.android.marvin.talkback/com.google.android.marvin.talkback.TalkBackServicecan be disabled (apparently) with same command replaced with null for the value
Also adds additional finders/matcher for semantics