Rewire ios accessibility bridge receive semantics enabled signal #56691
Conversation
| /** | ||
| * Gets the accessibility bridge created in this platform view. | ||
| */ | ||
| AccessibilityBridge* GetAccessibilityBridge() { return accessibility_bridge_.get(); } |
There was a problem hiding this comment.
These are public for testing only, i am not sure how to best test the change without exposing these
There was a problem hiding this comment.
This is fine for now; refactoring this class is on my to do list for a rainy day, but please add a note pointing out this is only for testing so we can clean it up in the refactor.
|
|
||
| /// Wrapper that guarantees we communicate clearing Accessibility | ||
| /// information to Dart. | ||
| class AccessibilityBridgeManager { |
There was a problem hiding this comment.
The sole purpose for this class is to call setSemanticsEnabled when a11y bridge is created. This is not needed even before the change because the a11y bridge will only be created when setSemanticsEnabled is called before the change.
I think this class is obsolete
There was a problem hiding this comment.
Agreed, yeah this is ICantBelieveItsNotUniquePtr with one additional feature.
| if (generating) { | ||
| if (!accessibility_bridge_) { | ||
| accessibility_bridge_ = std::make_unique<AccessibilityBridge>(owner_controller_, this, | ||
| platform_views_controller_); |
There was a problem hiding this comment.
Can I assume platform_views_controller_ contains the same owner_controller_.platformViewsController. From the code I think yes, but not too sure.
chunhtai
force-pushed
the
issues/158399
branch
from
03a2e16 to
3ff32ab
Compare
| * detached sequentially to multiple `FlutterViewController`s and `FlutterView`s. | ||
| */ | ||
| class PlatformViewIOS final : public PlatformView { | ||
| class PlatformViewIOS : public PlatformView { |
There was a problem hiding this comment.
Why was it necessary to remove the final?
| "PlatformViewIOS has no ViewController."; | ||
| return; | ||
| } | ||
| if (enabled && !accessibility_bridge_) { |
There was a problem hiding this comment.
GitHub won't let me comment on the line above, but I think you're safe to delete the if (!owner_controller_) { ... } block now, since it's no longer used here.
| /** | ||
| * Handles accessibility message from accessibility channel. | ||
| */ | ||
| void handleAccessibilityMessage(__weak id message, FlutterReply reply); |
There was a problem hiding this comment.
This is a C++ class, so C++ naming applies:
| void handleAccessibilityMessage(__weak id message, FlutterReply reply); | |
| void HandleAccessibilityMessage(__weak id message, FlutterReply reply); |
Need to fix up the call sites though.
| /** | ||
| * Send accessibility message to accessibility channel. | ||
| */ | ||
| void sendAccessibilityMessage(__weak id message); |
There was a problem hiding this comment.
This is a C++ class, so C++ naming applies:
| void sendAccessibilityMessage(__weak id message); | |
| void SendAccessibilityMessage(__weak id message); |
Need to fix up the call sites.
| binaryMessenger:owner_controller.engine.binaryMessenger | ||
| codec:[FlutterStandardMessageCodec sharedInstance]]; | ||
| [accessibility_channel_ setMessageHandler:^(id message, FlutterReply reply) { | ||
| handleAccessibilityMessage(message, reply); |
There was a problem hiding this comment.
| handleAccessibilityMessage(message, reply); | |
| HandleAccessibilityMessage(message, reply); |
| } | ||
| } | ||
|
|
||
| void PlatformViewIOS::sendAccessibilityMessage(__weak id message) { |
There was a problem hiding this comment.
| void PlatformViewIOS::sendAccessibilityMessage(__weak id message) { | |
| void PlatformViewIOS::SendAccessibilityMessage(__weak id message) { |
| void AccessibilityBridge::AccessibilityObjectDidBecomeFocused(int32_t id) { | ||
| last_focused_semantics_object_id_ = id; | ||
| [accessibility_channel_ sendMessage:@{@"type" : @"didGainFocus", @"nodeId" : @(id)}]; | ||
| platform_view_->sendAccessibilityMessage(@{@"type" : @"didGainFocus", @"nodeId" : @(id)}); |
There was a problem hiding this comment.
| platform_view_->sendAccessibilityMessage(@{@"type" : @"didGainFocus", @"nodeId" : @(id)}); | |
| platform_view_->SendAccessibilityMessage(@{@"type" : @"didGainFocus", @"nodeId" : @(id)}); |
| if ([type isEqualToString:@"announce"]) { | ||
| NSString* message = annotatedEvent[@"data"][@"message"]; | ||
| ios_delegate_->PostAccessibilityNotification(UIAccessibilityAnnouncementNotification, message); | ||
| NSString* messageToAnnounce = message[@"data"][@"message"]; |
There was a problem hiding this comment.
This should be message_to_announce since we're in a C++ method. I'm not in love with it either.
|
|
||
| namespace flutter { | ||
|
|
||
| namespace testing { |
There was a problem hiding this comment.
Since everything in here is in flutter::testing, do:
namespace flutter::testing {
| void SetNeedsReportTimings(bool value) override {}; | ||
|
|
||
| std::unique_ptr<std::vector<std::string>> ComputePlatformResolvedLocale( | ||
| const std::vector<std::string>& supported_locale_data) override { |
There was a problem hiding this comment.
Nothing to do in your patch, but I'm going to take a look at the base class. Really curious why we're handing back a std::unique_ptr<std::vector<std::string>> here rather than just a std::vector<std::string>. I doubt these arrays are ever more than 5-10 entries even in extreme cases.
| return {}; | ||
| } | ||
|
|
||
| void SendChannelUpdate(std::string name, bool listening) override {} |
There was a problem hiding this comment.
Similarly, nothing to do here, but we should almost certainly make this a const std::string& or a std::string_view in the base class.
|
|
||
| MockRuntimeDelegate delegate_; | ||
| UIDartState::Context& context_; | ||
| RuntimeController runtime_controller_; |
There was a problem hiding this comment.
Should these members be private:? If so, move CanUpdateSemantics up, since that almost certainly should be public :)
| }; | ||
|
|
||
| TEST_F(RuntimeControllerTest, CanUpdateSemantics) { | ||
| auto message_latch = std::make_shared<fml::AutoResetWaitableEvent>(); |
There was a problem hiding this comment.
This should just be:
fml::AutoResetWaitableEvent message_latch;
|
|
||
| Settings settings = CreateSettingsForFixture(); | ||
| AddNativeCallback("SemanticsUpdate", | ||
| CREATE_NATIVE_ENTRY(nativeSemanticsUpdate)); |
There was a problem hiding this comment.
| CREATE_NATIVE_ENTRY(nativeSemanticsUpdate)); | |
| CREATE_NATIVE_ENTRY(native_semantics_update)); |
| UIDartState::Context context(task_runners); | ||
| RuntimeControllerTester tester(context); | ||
|
|
||
| auto nativeSemanticsUpdate = [&tester, |
There was a problem hiding this comment.
| auto nativeSemanticsUpdate = [&tester, | |
| auto native_semantics_update = [&tester, |
chunhtai
force-pushed
the
issues/158399
branch
from
3ff32ab to
5a51507
Compare
|
all comment addressed |
|
The presub failures look real. Are you also waiting on another review from @cbracken ? |
This comment was marked as outdated.
This comment was marked as outdated.
chunhtai
force-pushed
the
issues/158399
branch
from
5a51507 to
227703f
Compare
fixes #158399 engine ios flutter/engine#56691 ## Pre-launch Checklist - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [ ] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
chunhtai
force-pushed
the
issues/158399
branch
from
227703f to
33de166
Compare
chunhtai
force-pushed
the
issues/158399
branch
from
63bfeb9 to
14afe1a
Compare
…" (#160039) This reverts commit 1a2d6a3. This is a straight revert as we are pivot away from using message channel, instead we will be adding api to dart:ui. See flutter/engine#56691 ## Pre-launch Checklist - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [ ] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
| /// Informs the engine whether the framework is generating a semantics tree. | ||
| /// | ||
| /// After this has been set to true, platforms are expected to prepare for accepting | ||
| /// semantics update sent via [updateSemantics]. When this is set to false, platforms |
There was a problem hiding this comment.
So, it is possible that either the platform OR the framework turns on semantics. Who is in charge of keeping track of this? If the platform turned on semantics and then the framework turns off semantics by calling this method - do we just leave the platform hanging and not provide it with a semantics tree anymore?
There was a problem hiding this comment.
framework is keep track of this since it is the one who knows how many semantics handles are generated.
platform can send signal through SetSemanticsEnabled to framework, but it shouldnt initialize resource for accessibility until framework sends setSemanticsTreeEnabled back.
when framework turn off semantics(can happen when last handle is disposed), it will send setSemanticsTreeEnabled(false), platform will then know it can dispose its accessibility resource
There was a problem hiding this comment.
Can we document that contract here? And on where the signal from the platform is send to the framework?
There was a problem hiding this comment.
I find it tricky to write document in this class, this api is facing framework so i find it weird to document framework behavior in this doc, so I mainly document what this function does and when the consume should call this function. let me try to see what I can come up with
| /// semantics update sent via [updateSemantics]. When this is set to false, platforms | ||
| /// may dispose any resources associated with processing semantics as no further | ||
| /// semantics updates will be sent via [updateSemantics]. | ||
| void setSemanticsTreeEnabled(bool enabled) => _setSemanticsTreeEnabled(enabled); |
There was a problem hiding this comment.
nit: Should we just call this setSemanticsEnabled? I don't think we officially call this a tree anywhere else in the API.
There was a problem hiding this comment.
setSemanticsEnabled is used exclusively by platform to let framework know it "wants" semantics to turn on. SetSemanticsTreeEnabled is framework signal to tell platform that "yes i will start compiling semantics"
don't think we officially call this a tree anywhere else in the API.
Do you have other name suggestion? how about setSemanticsCompilingEnabled ?
goderbauer
left a comment
goderbauer
left a comment
There was a problem hiding this comment.
LGTM for the dart:ui stuff when the additional documentation is added.
Co-authored-by: Michael Goderbauer <[email protected]>
chunhtai
force-pushed
the
issues/158399
branch
from
14c405b to
f05255f
Compare
|
@chunhtai can you migrate this PR to the framework and close when you get a chance and we'll try to push the reviewers a bit so it doesn't get stuck again? |
|
yes I will do it today |
5 participants
fixes flutter/flutter#158399
previously the only correct way to enable semantics is that ios embedding receive signal from native OS, it call SetSemanticsEnabled to shell and then to dart to enable semantics tree generation.
If for some reason framework decide to enable semantics first, e.g. through
SemanticsBinding.instance.ensureSemantics(typically due to integration test or ci that wants to test semantics), the update will be dropped in shell. Even if it later on receive signal from native OS to turn on semantics, it can't construct the complete accessibility tree in embedding because the updatesemantics sends diff update and previous updates are gone forever. It will end up in a broken state.This pr changes so that the only source of truth will be in the framework side. When framework starts generating the the semantics tree, it will call SetSemanticsTreeEnabled through dart:ui, and the embedding needs to prepare itself to accept semantics update after receiving the message.
This however require some refactoring on iOS embedding because it will only create a11y bridge when receiving OS notification when assitive technologies turns on.
This requires three phase transition
I also need to do the Android part
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.