Fix talkback in hybrid composition while using FlutterFragmentActivity #22429

blasten · 2020-11-10T22:49:22Z

To support TalkBack while using platform views, we detect whether the view is in a virtual display or in the view hierarchy. Currently, we assume that if the view is in a FlutterActivity context, then it's in a view hierarchy.

This breaks cases like FlutterFragmentActivity or hosting a FlutterView in a custom Activity (add-to-app).

Fixes flutter/flutter#68970

blasten · 2020-11-10T23:02:18Z

shell/platform/android/io/flutter/plugin/platform/SingleViewPresentation.java

@@ -52,7 +52,7 @@
 */
 @Keep
 @TargetApi(Build.VERSION_CODES.JELLY_BEAN_MR1)
-class SingleViewPresentation extends Presentation {
+public class SingleViewPresentation extends Presentation {


we should look into re-organizing the packages in the embedding. The current structure isn't well suited, and ends up requiring public classes when they aren't intended to be used outside of the embedding internal implementation. cc @xster

Sure, we can definitely re-organize things (which is easier if most things stay private to avoid breaking changes). It's low-priority on my mind so far since I don't have many concrete needs for refactoring. What's on your mind?

which is easier if most things stay private to avoid breaking changes

Right. Although, for the time being we should be able to use https://code.google.com/archive/p/doclava/wikis/JavadocTags.wiki#@hide.

shell/platform/android/io/flutter/view/AccessibilityBridge.java

amirh

nit: describing the nature of the change and the reason behind it in the PR description helps making the review easier and may help future code archeologists

shell/platform/android/io/flutter/plugin/platform/SingleViewPresentation.java

amirh · 2020-11-11T19:41:32Z

shell/platform/android/io/flutter/view/AccessibilityBridge.java

@@ -558,7 +558,7 @@ public AccessibilityNodeInfo createAccessibilityNodeInfo(int virtualViewId) {
    if (semanticsNode.platformViewId != -1) {
      View embeddedView =
          platformViewsAccessibilityDelegate.getPlatformViewById(semanticsNode.platformViewId);
-      boolean childUsesVirtualDisplay = !(embeddedView.getContext() instanceof FlutterActivity);
+      boolean childUsesVirtualDisplay = embeddedView.getContext() instanceof PresentationContext;


This isn't very robust, nothing stops platform views from providing their own contexts. Can we track the platform view type within the embedding code so that childUsesVirtualDisplay can't be changed by plugin code?

Under what situation someone just ignores the context passed to the factory when creating the view? PresentationContext has some logic like isCalledFromAlertDialog, does it mean that we support not using that functionality?

One doesn't have to ignore the context passed in, Android provides ContextWrapper which lets you change just the thing you need in the context. If that is done the instanceOf check will not check for what you want.

so given that we will deprecate this code in the near future, what about we use ContextWrapper#getBaseContext() to get the original context?

What's the deprecation timeline?
Is it hard to track a bit for whether this is a HC or VD platform view?

BTW isn't the robustness of this check important for both the VD and HC use cases?

On Android 10, b/171449547 is broken if the view is in a virtual display.

For HC, this check should always work since PresentationContext isn't a public API. Once VD is deprecated, this check will not exist as platform views using VD will use HC automatically. Deprecating VD doesn't break public APIs, we would just need to change the implementation of AndroidView in the framework.

Good point about PresentationContext not being public (though technically you just turned it public in this PR, is @hide for non-Android-SDK code enforces anything or does it just prune Javadocs?)
I see your point that this is likely not going to break folks anytime soon, however assuming a robust fix (keeping track of the kind of view) requires no more than a couple hours of work my preference would be to keep on the safe side.
My concern would be that we will end up staying with VDs for longer than we think right now and some non standard use-case (of a replaced context) will turn into a debugging nightmare.

The patch ended up being smaller. PTAL.

@hide would hide it from the public documentation. I wish it had the same meaning it has for the Android-SDK code :)

Shorter than this thread 😄

Thanks!

amirh

LGTM

…tActivity (flutter/engine#22429)

flutter#22429)

Fix talkback in hybrid composition while using FlutterFragmentActivity

af4f8f0

flutter-dashboard bot added the platform-android label Nov 10, 2020

google-cla bot added the cla: yes label Nov 10, 2020

googlebot added the cla: yes label Nov 10, 2020

blasten requested a review from amirh November 10, 2020 22:58

blasten commented Nov 10, 2020

View reviewed changes

xster reviewed Nov 11, 2020

View reviewed changes

shell/platform/android/io/flutter/view/AccessibilityBridge.java Outdated Show resolved Hide resolved

xster reviewed Nov 11, 2020

View reviewed changes

shell/platform/android/io/flutter/view/AccessibilityBridge.java Outdated Show resolved Hide resolved

Emmanuel Garcia added 3 commits November 10, 2020 21:17

Remove Log

42b3abe

Remove static import

d293ab0

Use @hide in javadoc

55dd940

amirh reviewed Nov 11, 2020

View reviewed changes

Emmanuel Garcia added 2 commits November 11, 2020 11:58

Add <pre>

9543e7e

Explicitly track platform views using virtual displays

9299ad8

blasten requested a review from amirh November 11, 2020 23:42

amirh approved these changes Nov 12, 2020

View reviewed changes

blasten merged commit 091b6cc into flutter:master Nov 12, 2020

blasten deleted the talkback_hybrid_composition branch November 12, 2020 18:30

engine-flutter-autoroll mentioned this pull request Nov 13, 2020

Roll Engine from c5c7e83b9e53 to d07368169e3b (35 revisions) flutter/flutter#70417

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 13, 2020

091b6cc Fix talkback in hybrid composition while using FlutterFragmen…

e71c425

…tActivity (flutter/engine#22429)

engine-flutter-autoroll mentioned this pull request Nov 13, 2020

Roll Engine from c5c7e83b9e53 to 1ff237fdca45 (38 revisions) flutter/flutter#70425

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 13, 2020

091b6cc Fix talkback in hybrid composition while using FlutterFragmen…

fd60b0e

…tActivity (flutter/engine#22429)

engine-flutter-autoroll mentioned this pull request Nov 13, 2020

Roll Engine from c5c7e83b9e53 to dddb532b5c7b (40 revisions) flutter/flutter#70429

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 13, 2020

091b6cc Fix talkback in hybrid composition while using FlutterFragmen…

97f97ef

…tActivity (flutter/engine#22429)

gaaclarke mentioned this pull request Nov 13, 2020

Dart version check tripping up on copyright notices with local engine flutter/flutter#70498

Closed

chaselatta pushed a commit to chaselatta/engine that referenced this pull request Nov 30, 2020

Fix talkback in hybrid composition while using FlutterFragmentActivity (

5c70469

flutter#22429)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix talkback in hybrid composition while using FlutterFragmentActivity #22429

Fix talkback in hybrid composition while using FlutterFragmentActivity #22429

blasten commented Nov 10, 2020 •

edited

blasten Nov 10, 2020

xster Nov 11, 2020

blasten Nov 11, 2020

amirh left a comment

amirh Nov 11, 2020

blasten Nov 11, 2020

amirh Nov 11, 2020

blasten Nov 11, 2020

amirh Nov 11, 2020

amirh Nov 11, 2020 •

edited

blasten Nov 11, 2020 •

edited

amirh Nov 11, 2020

blasten Nov 11, 2020

amirh Nov 12, 2020

amirh left a comment

Fix talkback in hybrid composition while using FlutterFragmentActivity #22429

Fix talkback in hybrid composition while using FlutterFragmentActivity #22429

Conversation

blasten commented Nov 10, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirh left a 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

amirh Nov 11, 2020 • edited

Choose a reason for hiding this comment

blasten Nov 11, 2020 • edited

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

amirh left a comment

Choose a reason for hiding this comment

blasten commented Nov 10, 2020 •

edited

amirh Nov 11, 2020 •

edited

blasten Nov 11, 2020 •

edited