-
Notifications
You must be signed in to change notification settings - Fork 6k
[a11y] Support TalkBack reading by word, character, and paragraph #17626
Conversation
@@ -756,13 +761,7 @@ public AccessibilityNodeInfo createAccessibilityNodeInfo(int virtualViewId) { | |||
} else if (hasToggledState) { | |||
result.setChecked(semanticsNode.hasFlag(Flag.IS_TOGGLED)); | |||
result.setClassName("android.widget.Switch"); | |||
result.setContentDescription(semanticsNode.getValueLabelHint()); | |||
} else if (!semanticsNode.hasFlag(Flag.SCOPES_ROUTE)) { |
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.
Does this change now make SemanticsNodes that are scoping a route and have a label focusable?
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'll update the condition so we don't apply it to those nodes, thanks for jogging my memory a bit on the context
/cc @xster Do you want to review this one? |
Thanks for the fix @jonahwilliams. We should add a test for this. The good news is that writing Java tests is a lot easier now than before. See https://github.com/flutter/engine/blob/master/shell/platform/android/test/README.md#L24. The tests and dependencies all build in ninja and you can use robolectric, mockito and all that good stuff to construction inject etc. Give it a try. Happy to work with you on it if needed. |
@xster I updated run_test to use a configured Java so I can point it at Android Studio's JRE. However now I'm getting:
But this is 1.8.0_212-release-1586-b4-5784211 , so it should work according to the readme? |
Are you on a mac? If you just hard-call a specific java binary but don't set your JAVA_HOME, it's gonna do really weird things. What does your JAVA_HOME and /usr/libexec/java_home -V return? i.e. I think it's going to be a bit more involved to changing the java instance than just calling a specific binary. If documentation is lacking, adding more docs to let people set JAVA_HOME is probably the least scary option. |
@@ -29,6 +29,7 @@ | |||
import test.io.flutter.embedding.engine.FlutterShellArgsTest; | |||
import test.io.flutter.embedding.engine.PluginComponentTest; | |||
import test.io.flutter.embedding.engine.dart.DartExecutorTest; | |||
import io.flutter.view.AccessibilityBridgeTest; |
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 assume the formatter will complain about this
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.
perhaps ... I will format another day :)
testing/run_tests.py
Outdated
EXPECTED_VERSION = '1.8' | ||
# `java -version` is output to stderr. https://bugs.java.com/bugdatabase/view_bug.do?bug_id=4380614 | ||
version_output = subprocess.check_output(['java', '-version'], stderr=subprocess.STDOUT) | ||
version_output = subprocess.check_output([java_path, '-version'], stderr=subprocess.STDOUT) |
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.
if you really want this, you'll probably need an env map too
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.
for JAVA_HOME
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.
Ahh yeah, with JAVA_HOME set everything is working. Thanks!
shell/platform/android/io/flutter/view/AccessibilityBridge.java
Outdated
Show resolved
Hide resolved
shell/platform/android/test/io/flutter/view/AccessibilityBridgeTest.java
Show resolved
Hide resolved
I rolled back the run_test.py changes since that wasn't going to be sufficient. Test files are now formatted correctly. I added the necessary test code to simulate semantic updates, though I only added tests for the conditions I changed |
Friendly ping @xster |
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.
doh, I had this tab opened twice and didn't reply. Sorry for the wait. Awesome tests!
} | ||
|
||
/// The encoding for semantics is described in platform_view_android.cc | ||
class TestSemanticsUpdate { |
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.
if we want this mapping to be maintained manually, can we add notes to each of accessibilitybridge.updatesemantics, platformviewandroid.updatesemantics and here to remind maintainers?
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.
Something like : "This logic is also used in the test fixture at path/to/test.java"?
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.
Sure. Something like "if you change stuff here, don't forget to update the test file as well"
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 added a comment to platform_view_android.cc, since that defines the encoding format. Elsewhere it is only consumed
LGTM! |
The logic in talkback does not handle reading from the text portion of an accessibility node when checking if that node could support reading at different granularities. See: https://github.com/google/talkback/blob/9b1d5132b074174d02886faccf731e814d69363c/talkback/src/main/java/GranularityTraversal.java#L207
Fix this by always setting the content description if something is not a text field.
flutter/flutter#13406
Fixes flutter/flutter#13406