Skip to content

Conversation

@jonahwilliams
Copy link
Contributor

@jonahwilliams jonahwilliams commented Jul 2, 2018

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.TalkBackService
can be disabled (apparently) with same command replaced with null for the value

Also adds additional finders/matcher for semantics

@jonahwilliams
Copy link
Contributor Author

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

Copy link
Member

@goderbauer goderbauer left a 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(),
Copy link
Member

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);
Copy link
Member

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?

Copy link
Contributor Author

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.
Copy link
Member

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].
Copy link
Member

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].
Copy link
Member

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(
Copy link
Member

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.

Copy link
Contributor Author

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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doc comment?

Copy link
Contributor Author

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.
Copy link
Member

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?

Copy link
Contributor Author

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.
Copy link
Member

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?

Copy link
Contributor Author

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].
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yjbanov
Copy link
Contributor

yjbanov commented Jul 3, 2018

can we split this into at least 3 PRs for easier reviewing

I'd even make it 4 PRs: flutter_test, flutter_driver, new test app, driver/devicelab scripts.

@Hixie
Copy link
Contributor

Hixie commented Aug 7, 2018

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?

@jonahwilliams
Copy link
Contributor Author

jonahwilliams commented Aug 7, 2018

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 AccessibilityNodeInfos that I use here - but this at least gives us test coverage of the increasingly complex Android accessibility bridge.

(maybe this is technically an end to end test?)

@Hixie
Copy link
Contributor

Hixie commented Aug 7, 2018

How do regular android apps test their accessibility?

@jonahwilliams
Copy link
Contributor Author

https://developer.android.com/training/accessibility/testing

A combination of manual testing and a few automated lints like missing labels

@jonahwilliams
Copy link
Contributor Author

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 18, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 18, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants