A11y callback #8005

dnfield · 2019-03-01T21:18:19Z

This is intended to allow tests to force semantics to turn on, and then register a callback when at least one semantic tree has been built by the accessibility bridge. This will enable tests to be less flaky. A simple implementation using this is going to be added to ios_add2app in the framework (I'll add the PR here when I open that).

/cc @AlbertWang0116, @matthew-carroll

dnfield · 2019-03-01T21:19:26Z

flutter/flutter#28746

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

jamesderlin · 2019-03-01T21:44:08Z

shell/platform/darwin/ios/platform_view_ios.mm

  }
+  return true;


This could be:

return semantics_available_callback_ && accessibility_bridge_;

However, unless I'm misunderstanding, this doesn't seem to follow the documented contract (which is to return whether or not the callback was successfully registered)?

I think my documentation and implementation got out of sync. I'm going to update the docs, because I don't think I have a good way to force the tree to rebuild.

And if I try to track whether it's been built there are thread safety issues at play.

jamesderlin · 2019-03-01T21:50:19Z

shell/platform/darwin/ios/framework/Headers/FlutterEngine.h

+ * Registers a callback that will be called once when semantics are available.
+ *
+ * If the semantics tree has already been build at least once, this callback
+ * will fire immediately. If semantics are disabled, this callback will never


Where does the code fire the callback immediately if the semantics tree has already been built?

jamesderlin · 2019-03-01T22:11:34Z

shell/platform/darwin/ios/framework/Headers/FlutterEngine.h

+ *
+ * If semantics are disabled, this callback will be stored but will not fire
+ * until they are enabled and the tree has been updated. Consider calling
+ * `-ensureSemanticsEnabled` first.


One potential problem is that if a caller does call -ensureSemanticsEnabled first, what if the semantics tree is built before the callback is registered?

Calling -ensureSemanticsEnabled after registering the callback seems safer, but there would still be a problem if semantics are already enabled.

Agreed - however, in my testing once semantics are enabled this fires pretty regularly.

Do you know what triggers that? Is it because the UI hasn't settled down, or we're rebuilding the semantics tree periodically even when it's not necessary?

I'm concerned about this too but not entirely sure how to resolve it. If I ask the Accessibility Bridge if it has semantics, it could tell me no, then we get interrupted by the UI thread with an update, and I go on to register the callback which thinks there was no data when by now there is. I really dont' want to put a mutex in there for this because this is performance sensitive stuff that happens a lot. We could try to do a spin lock around it though.

If I try to force the framework to do a semantics update (as long as semantics are enabled), it's also possible that will someday get optimized out by a well intentioned person.

I believe it's a combination of both.

It seems weird to me if the semantics tree is periodically updating when nothing has changed.

I like the approach of forcing an update. If you made, say, a forceSemanticsUpdate function and documented what it's used for, then I wouldn't be too worried about somebody unwittingly removing it.

Would caching the signal help with this? @GaryQian just addressed an independent but similar kind of problem. The very first lifecycle message from Android to Flutter might be missed due to timing. In that case, Gary used the Window object to cache the state to avoid losing it. I'm not suggesting the Window for this case, necessarily, but maybe a cache within the C++ would eliminate race conditions here?

matthew-carroll · 2019-03-02T00:07:45Z

shell/common/platform_view.cc

@@ -85,6 +85,13 @@ void PlatformView::UpdateSemantics(
    blink::SemanticsNodeUpdates update,
    blink::CustomAccessibilityActionUpdates actions) {}

+bool PlatformView::RegisterSemanticsAvailableCallback(fml::closure closure) {
+  FML_LOG(WARNING) << "The default implementation of "


Just a thought, but it might not be clear to the developer what alternative there is to the "default implementation". I know for me, despite lots of tracing through the C++ code, I'm still surprised by many delegates and I still don't have a meaningful appreciation for why certain things are implemented downstream. It might be helpful to point the developer in the direction of where they might find the desired implementation.

In this case, it'd be up to the embedder to implement it. If you're consuming this in an add2app scenario, I want to make sure it's clear if your embedding treats it as a no-op. Maybe this message would be better as "The embedder for your platform does not implemented this feature, and ..."?

That's probably a step in the right direction. Anything that helps suggest what a developer might need to do, or where to look, or what to submit in a bug report, I think is preferable to a mere statement of fact that may or may not hold significance to the reader.

matthew-carroll · 2019-03-02T00:08:41Z

shell/platform/darwin/ios/framework/Headers/FlutterEngine.h

+ * This method must only be called after launching the engine via
+ * `-runWithEntrypoint:` or `-runWithEntryPoint:libraryURI`.
+ */
+- (void)ensureSemanticsEnabled;


Nomenclature question: Is there a reason that this is more idiomatic than enableSemantics?

I'm trying to make it clear that this is not something you could ever try to set to false. It's a one way, turn on only method. It would be dangerous to let people arbitrarily turn off semantics.

Right, I didn't mean enableSemantics(bool) but just enableSemantics() which would also imply one-way.

matthew-carroll · 2019-03-02T00:10:45Z

shell/platform/darwin/ios/framework/Headers/FlutterEngine.h

+ *
+ * If semantics are disabled, this callback will be stored but will not fire
+ * until they are enabled and the tree has been updated. Consider calling
+ * `-ensureSemanticsEnabled` first.


Would caching the signal help with this? @GaryQian just addressed an independent but similar kind of problem. The very first lifecycle message from Android to Flutter might be missed due to timing. In that case, Gary used the Window object to cache the state to avoid losing it. I'm not suggesting the Window for this case, necessarily, but maybe a cache within the C++ would eliminate race conditions here?

dnfield · 2019-03-02T00:21:02Z

@matthew-carroll the problem with caching the signal is that we could get interrupted between when we last cached it and the cache would be invalid when we actually try to use it. One way to solve that would be with a lock, which I don't want to use here - this code is on a critical path for rendering.

Another option would be to use a compare and swap algorithm, which should be better but still might slow us down more than we need to.

Another option is to create a path where we can tell the framework we want it to do a semantics update even if it doesn't think it's dirty.

matthew-carroll · 2019-03-02T00:22:36Z

Wouldn't the caching risk that you mention merely cause (at most) 1 extra invocation of the callback?

I would think that risk is safer than the risk of getting called zero times when you needed 1, right?

dnfield · 2019-03-02T00:26:05Z

The risk is that it would never get called. It's possible that we optimize the framework to the point where it never rebuilds a non-dirty semantics tree, and that we have an app where the semantics tree stays clean for a long time after we register the callback.

AlbertWang0116 · 2019-03-02T04:16:44Z

shell/platform/darwin/ios/framework/Headers/FlutterEngine.h

+ * until they are enabled and the tree has been updated. Consider calling
+ * `-ensureSemanticsEnabled` first.
+ *
+ * Multiple calls to this method will replace the callback.


I would propose (weakly) holding all of the callbacks instead of overriding, and make this API in the form of register/unregisterSomeThing.

As both a downstream user of the callback and a framework provider, EarlGrey will register this callback to ensure Flutter is synchronized, but end user may also be interested in registering the callback and causing unexpected result at either side. Though there is hacky way to get around this, it's always best to act as a 'nice' user.

I think register/deregister methods can be put either here or downstream FlutterViewController.

That makes sense. I was hoping I could avoid the complexity involved there but you're right.

Should it still be a fire once and automatically unregister?

Maybe we could use NSNotificationCenter for this.

Vote for NSNotificationCenter. One-shot subscription could be done by user-side block so I think it should be fine.

AlbertWang0116 · 2019-03-02T04:38:13Z

shell/platform/darwin/ios/framework/Headers/FlutterEngine.h

+ * accessibility services an end user has requested.
+ *
+ * This method must only be called after launching the engine via
+ * `-runWithEntrypoint:` or `-runWithEntryPoint:libraryURI`.


Let me know if I'm wrong: In the perspective FlutterViewController user, it's hard to find a right time to enable semantics before the first semantics tree being built.

I think it will be easier if FlutterViewController can take the preference and invoke this method when _engineNeedsLaunch is used :).

Semantics might already be enabled based on device settings or if it's a simulator. It should be safe to call this method even if semantics are enabled - if they're not, no semantics tree will be built.

We could make it an option in the initializer, but I'm not sure how that would help for testing.

Or maybe a property/one-way-method of FlutterViewController? User can check if they need to force enabling semantics through several ways, such as plist or envs. If it's the case, they can flip the bit after calling initilizer, but before load the VC.

The ensureSemanticsEnabled is safe to call at any time. Basically, the idea is to just call it right after you register for the callback. You could in theory call it in a loop after that as long as you let the main runloop make progress.

dnfield · 2019-03-04T22:20:00Z

I've refactored this to use NSNotificationCenter.

ensureSemanticsEnabled, as written today, will force a semantics frame update. I think for now we can just leave this as is. I'd rather not go mucking in the framework for something special for this - and we'll be able to detect if someone decides to try to make enabling semantics over and over again not force a re-creation of the tree.

dnfield · 2019-03-06T18:35:17Z

@jamesderlin @AlbertWang0116 any thoughts on the latest rev of this?

jamesderlin · 2019-03-06T21:33:08Z

shell/platform/darwin/ios/platform_view_ios.mm

  if (accessibility_bridge_) {
    accessibility_bridge_->UpdateSemantics(std::move(update), std::move(actions));
+    [[NSNotificationCenter defaultCenter] postNotificationName:@"FlutterSemanticsUpdate"


Things interested in this notification will need to know the notification name, right? The name probably should be exposed in a public header.

Good point. I'm not quite sure where to expose that though. Perhaps on ensureSemanticsEnabled in FlutterViewController?

er - FlutterEngine.h

Documenting it is good, but I think adding something like const NSNotificationName FlutterSemanticsUpdateNotification would be better.

jamesderlin · 2019-03-06T22:07:56Z

shell/platform/darwin/ios/platform_view_ios.mm

  if (accessibility_bridge_) {
    accessibility_bridge_->UpdateSemantics(std::move(update), std::move(actions));
+    [[NSNotificationCenter defaultCenter] postNotificationName:@"FlutterSemanticsUpdate"


Should use FlutterSemanticsUpdateNotification. =)

Done. I had to switch what header it was in to make sure I get one that the right people have access to. It makes more sense in FutterViewController.h anyway since that's the object that'll get passed through. We may have to rethink if we want semantics to be enabled for an engine that doesn't have a ViewController though

AlbertWang0116 · 2019-03-07T00:22:43Z

LGTM!

…ine (19 commits) (#28688) git log --oneline --no-merges f4951df..a48cd16 a48cd16 Update a11y word forward/back enum names (flutter/engine#8073) b5f59ed Delay the vsync callback till the frame start time specified by embedder. (flutter/engine#8072) 7426305 Mark const extern (flutter/engine#8077) d3f6d7a only partial rule revert (flutter/engine#8078) d71bfe5 Only build a full Dart SDK when building for the host system (flutter/engine#8071) de90dbf Refactor web configuration/ Add dartdevc (flutter/engine#7978) ff46dd3 Roll src/third_party/skia 4c1ea43a79b5..88b8d1124b72 (8 commits) (flutter/engine#8070) 80c6dd2 Roll src/third_party/skia 692122e3ef23..4c1ea43a79b5 (3 commits) (flutter/engine#8069) 68ed654 Roll src/third_party/skia 3c957d575c58..692122e3ef23 (6 commits) (flutter/engine#8067) ca0bac4 Revert "add signal to pointer kinds" (flutter/engine#8066) 3fb627f add signal to pointer kinds (flutter/engine#8065) 5a06afa Roll src/third_party/skia 801a9c16d81e..3c957d575c58 (19 commits) (flutter/engine#8063) a93d99d A11y callback (flutter/engine#8005) 3661d5e Re-land "Buffer lifecycle in WindowData" (flutter/engine#8032) 471a2c8 Send scroll events from the macOS shell (flutter/engine#8056) 2fe9c9b Roll src/third_party/skia 72542816cadb..801a9c16d81e (46 commits) (flutter/engine#8060) 3335764 Skip skp files in license check (flutter/engine#8050) 7f16789 Remove redundant thread checker in FML. (flutter/engine#8053) 840c523 Correct URL for Cirrus CI build status badge (flutter/engine#8054) 57c120a remove extra source files (flutter/engine#8052) 4773375 Used named conditionals for platform specific dependencies and suppress Android and Windows hooks on Mac. (flutter/engine#8051) 70a18b5 Add clang static analysis support to gn wrapper (flutter/engine#8047) b30f989 Improve elevation bounds for physical shape layers (flutter/engine#8044) e37bd27 Fix weak pointer use violations in shell and platform view. (flutter/engine#8046) dd80fc9 Add engine support for scrollwheel events (flutter/engine#7494)

dnfield added 2 commits March 1, 2019 13:00

implement callback for a11y readiness

acb9dd9

Merge remote-tracking branch 'upstream/master' into a11y_callback

6f0125f

dnfield requested review from chinmaygarde, cbracken, kangwang1988 and jamesderlin March 1, 2019 21:18

googlebot added the cla: yes label Mar 1, 2019

dnfield mentioned this pull request Mar 1, 2019

Remove timeout from add2app test for iOS flutter/flutter#28746

Merged

10 tasks

jamesderlin reviewed Mar 1, 2019

View reviewed changes

dnfield added 2 commits March 1, 2019 14:05

fix unintended change, update docs, nit

c51c40b

save before commiting

d8e6ba5

jamesderlin reviewed Mar 1, 2019

View reviewed changes

matthew-carroll reviewed Mar 2, 2019

View reviewed changes

AlbertWang0116 reviewed Mar 2, 2019

View reviewed changes

Use NSNotificationCenter

f814fcc

jamesderlin reviewed Mar 6, 2019

View reviewed changes

dnfield added 2 commits March 6, 2019 13:43

Add doc with notification name

865043e

add constant

1d2b8c1

jamesderlin reviewed Mar 6, 2019

View reviewed changes

use FlutterSemanticsUpdateNotification from header, move to VC header

f5e8b39

dnfield merged commit a93d99d into flutter:master Mar 7, 2019

dnfield deleted the a11y_callback branch March 7, 2019 00:53

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 7, 2019

a93d99d A11y callback (flutter/engine#8005)

247f33b

engine-flutter-autoroll mentioned this pull request Mar 7, 2019

Roll engine f4951df193a7..80c6dd2864bc (18 commits) flutter/flutter#29001

Closed

GaryQian mentioned this pull request Mar 7, 2019

Initialize the lifecycle state with initial state in window. Roll engine (19 commits) flutter/flutter#28688

Merged

dnfield mentioned this pull request Mar 7, 2019

Mark const extern #8077

Merged

A11y callback #8005

A11y callback #8005

Uh oh!

Conversation

dnfield commented Mar 1, 2019

Uh oh!

dnfield commented Mar 1, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield commented Mar 2, 2019

Uh oh!

matthew-carroll commented Mar 2, 2019

Uh oh!

dnfield commented Mar 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield commented Mar 4, 2019

Uh oh!

dnfield commented Mar 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment