Plumb a reference of PlatformViewsController and AccessibilityBridge to each other #8208
Conversation
| ); | ||
| platformViewsController.attachAccessibilityBridge(accessibilityBridge); |
There was a problem hiding this comment.
Do we need to detach this ever?
There was a problem hiding this comment.
I tried to keep the same lifespan as the AccessibilityBridge reference, should accessibilityBridge be unset in the new FlutterView like in the current one?
There was a problem hiding this comment.
@matthew-carroll May have more context to answer that question.
|
@amirh can you avoid the new FlutterView and just make this change to the old FlutterView for now? Also, I don't think we should have cyclical dependencies like this between |
|
@matthew-carroll sure I'll remove it from the in progress FlutterView, I'm leaving a TODO for you to do the plumbing there. I agree this cyclic dependency is smelly, but I'm not sure there's much we can do other than merge the whole thing into a single object: as we are proxying a 2-way communication channel between the Android a11y framework and the view. |
|
I'm confused as to what the relationship is. I don't see any usages of either artifact by the other, just references held. I think we need to see what the actual behavior is if we're going to evaluate the necessity of this relationship. I could imagine a few alternatives, but they depend entirely on the specific interactions. |
|
You can check the (hacky) prototype to get an idea of the interactions, that change is going to be hard to review at once so I'm leading incremental pieces, with this being the first step. |
|
I think it's pretty important that we review the actual behaviors before giving an LGTM to the fundamental relationship. Can you describe why it will be difficult to review at one time? If you'd like, I can come by and we can chat in person if that would make it easier. But I'm really not comfortable with this dependency in the absence of the supporting purpose for its existence. |
|
@goderbauer PTAL |
| * Facilitates interaction between the accessibility bridge and embedded platform views. | ||
| */ | ||
| public interface PlatformViewsAccessibilityDelegate { | ||
| } |
There was a problem hiding this comment.
Should there be a TODO in here to fill this with ... stuff (I am wondering what that would be?)?
|
Amir and I spoke about the relationship between LGTM |
…yBridge to each other (flutter/engine#8208)
…yBridge to each other (flutter/engine#8208)
…yBridge to each other (flutter/engine#8208)
flutter/engine@96ad0c8...a1dcb2e git log 96ad0c8..a1dcb2e --no-merges --oneline a1dcb2e [ios] Set contentsScale before we commit CATransaction (flutter/engine#8218) fa1931f Send macOS keyboard data to the engine (flutter/engine#8219) 45f69ac Plumb a reference of PlatformViewsController and AccessibilityBridge to each other (flutter/engine#8208) 7cbbdb4 libtxt: more accurate tracking of run positioning and width for justified text (flutter/engine#8214) 1728103 Add a build dependencies script for Linux desktop (flutter/engine#8160) d764b69 Add docs for helpful commands to fix format (flutter/engine#8171) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.
…yBridge to each other (flutter/engine#8208)
…yBridge to each other (flutter/engine#8208)
flutter/engine@96ad0c8...6a8a45f git log 96ad0c8..6a8a45f --no-merges --oneline 6a8a45f Have the AccessibilityBridge attach/detach itself to the (flutter/engine#8229) 45b19e4 Roll src/third_party/skia ba91f65f2070..7e2c0614a2fd (13 commits) (flutter/engine#8228) 1dbd204 Moved io.flutter.embedding.engine.android package to io.flutter.embedding.android (flutter/engine#8221) bb35436 Roll src/third_party/dart 70e3e67dd7..5e9df35a57 (45 commits) 3b19a4d Removed dart_plugin_tag from DEPS d9b6629 Roll src/third_party/skia 4273a150f84d..ba91f65f2070 (6 commits) (flutter/engine#8225) e88573a Roll src/third_party/skia d7d93001ead2..4273a150f84d (1 commits) (flutter/engine#8224) be9067c Roll src/third_party/skia 37a9294d2eb9..d7d93001ead2 (2 commits) (flutter/engine#8223) ee4abba Roll src/third_party/skia 2894d13a0d9b..37a9294d2eb9 (1 commits) (flutter/engine#8222) 3496156 Roll src/third_party/skia dd0544078d05..2894d13a0d9b (33 commits) (flutter/engine#8220) a1dcb2e [ios] Set contentsScale before we commit CATransaction (flutter/engine#8218) fa1931f Send macOS keyboard data to the engine (flutter/engine#8219) 45f69ac Plumb a reference of PlatformViewsController and AccessibilityBridge to each other (flutter/engine#8208) 7cbbdb4 libtxt: more accurate tracking of run positioning and width for justified text (flutter/engine#8214) 1728103 Add a build dependencies script for Linux desktop (flutter/engine#8160) d764b69 Add docs for helpful commands to fix format (flutter/engine#8171) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.
…to each other (flutter#8208) This is in preparation for implementing platform views a11y on Android. And e2e working prototype is available here: https://github.com/amirh/engine/tree/a11y_hacks flutter/flutter#19418
…yBridge to each other (flutter#8208)" This reverts commit f0e02d3.
…yBridge to each other (flutter#8208)" This reverts commit f0e02d3.
This is in preparation for implementing platform views a11y on Android.
And e2e working prototype is available here: https://github.com/amirh/engine/tree/a11y_hacks
flutter/flutter#19418