From 690791e88a43e7152c3f4cda29ddf304372f5efb Mon Sep 17 00:00:00 2001 From: yaakovschectman <109111084+yaakovschectman@users.noreply.github.com> Date: Thu, 20 Apr 2023 16:28:00 -0400 Subject: [PATCH] Move ownership of `AccessibilityBridgeWindows` to `FlutterWindowsView` (flutter/engine#41308) Move ownership of the pointer to `AccessibilityBridgeWindows` from `FlutterWindowsEngine` to `FlutterWindowsView` in preparation for multi-window. https://github.com/flutter/flutter/issues/124995 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [ ] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat --- .../windows/accessibility_bridge_windows.cc | 9 ++--- .../windows/accessibility_bridge_windows.h | 4 +- .../accessibility_bridge_windows_unittests.cc | 39 +++++++++---------- .../windows/flutter_windows_engine.cc | 34 ++++++++-------- .../platform/windows/flutter_windows_engine.h | 15 +------ .../platform/windows/flutter_windows_view.cc | 17 ++++++++ .../platform/windows/flutter_windows_view.h | 15 +++++++ 7 files changed, 72 insertions(+), 61 deletions(-) diff --git a/engine/src/flutter/shell/platform/windows/accessibility_bridge_windows.cc b/engine/src/flutter/shell/platform/windows/accessibility_bridge_windows.cc index 159ba224599..5d90d03c33a 100644 --- a/engine/src/flutter/shell/platform/windows/accessibility_bridge_windows.cc +++ b/engine/src/flutter/shell/platform/windows/accessibility_bridge_windows.cc @@ -10,11 +10,8 @@ namespace flutter { -AccessibilityBridgeWindows::AccessibilityBridgeWindows( - FlutterWindowsEngine* engine, - FlutterWindowsView* view) - : engine_(engine), view_(view) { - FML_DCHECK(engine_); +AccessibilityBridgeWindows::AccessibilityBridgeWindows(FlutterWindowsView* view) + : view_(view) { FML_DCHECK(view_); } @@ -168,7 +165,7 @@ void AccessibilityBridgeWindows::DispatchAccessibilityAction( AccessibilityNodeId target, FlutterSemanticsAction action, fml::MallocMapping data) { - engine_->DispatchSemanticsAction(target, action, std::move(data)); + view_->GetEngine()->DispatchSemanticsAction(target, action, std::move(data)); } std::shared_ptr diff --git a/engine/src/flutter/shell/platform/windows/accessibility_bridge_windows.h b/engine/src/flutter/shell/platform/windows/accessibility_bridge_windows.h index 2655f1e3ef6..18660bcf605 100644 --- a/engine/src/flutter/shell/platform/windows/accessibility_bridge_windows.h +++ b/engine/src/flutter/shell/platform/windows/accessibility_bridge_windows.h @@ -27,8 +27,7 @@ class FlutterPlatformNodeDelegateWindows; class AccessibilityBridgeWindows : public AccessibilityBridge, public ui::AXFragmentRootDelegateWin { public: - AccessibilityBridgeWindows(FlutterWindowsEngine* engine, - FlutterWindowsView* view); + AccessibilityBridgeWindows(FlutterWindowsView* view); virtual ~AccessibilityBridgeWindows() = default; // |AccessibilityBridge| @@ -70,7 +69,6 @@ class AccessibilityBridgeWindows : public AccessibilityBridge, CreateFlutterPlatformNodeDelegate() override; private: - FlutterWindowsEngine* engine_; FlutterWindowsView* view_; FML_DISALLOW_COPY_AND_ASSIGN(AccessibilityBridgeWindows); diff --git a/engine/src/flutter/shell/platform/windows/accessibility_bridge_windows_unittests.cc b/engine/src/flutter/shell/platform/windows/accessibility_bridge_windows_unittests.cc index 849ed81be38..92cfcc7ff89 100644 --- a/engine/src/flutter/shell/platform/windows/accessibility_bridge_windows_unittests.cc +++ b/engine/src/flutter/shell/platform/windows/accessibility_bridge_windows_unittests.cc @@ -40,7 +40,7 @@ class AccessibilityBridgeWindowsSpy : public AccessibilityBridgeWindows { explicit AccessibilityBridgeWindowsSpy(FlutterWindowsEngine* engine, FlutterWindowsView* view) - : AccessibilityBridgeWindows(engine, view) {} + : AccessibilityBridgeWindows(view) {} void DispatchWinAccessibilityEvent( std::shared_ptr node_delegate, @@ -71,34 +71,33 @@ class AccessibilityBridgeWindowsSpy : public AccessibilityBridgeWindows { FML_DISALLOW_COPY_AND_ASSIGN(AccessibilityBridgeWindowsSpy); }; -// A FlutterWindowsEngine whose accessibility bridge is a +// A FlutterWindowsView whose accessibility bridge is an // AccessibilityBridgeWindowsSpy. -class FlutterWindowsEngineSpy : public FlutterWindowsEngine { +class FlutterWindowsViewSpy : public FlutterWindowsView { public: - explicit FlutterWindowsEngineSpy(const FlutterProjectBundle& project) - : FlutterWindowsEngine(project) {} + explicit FlutterWindowsViewSpy(std::unique_ptr handler) + : FlutterWindowsView(std::move(handler)) {} protected: - virtual std::shared_ptr CreateAccessibilityBridge( - FlutterWindowsEngine* engine, - FlutterWindowsView* view) override { - return std::make_shared(engine, view); + virtual std::shared_ptr + CreateAccessibilityBridge() override { + return std::make_shared(GetEngine(), this); } private: - FML_DISALLOW_COPY_AND_ASSIGN(FlutterWindowsEngineSpy); + FML_DISALLOW_COPY_AND_ASSIGN(FlutterWindowsViewSpy); }; // Returns an engine instance configured with dummy project path values, and // overridden methods for sending platform messages, so that the engine can // respond as if the framework were connected. -std::unique_ptr GetTestEngine() { +std::unique_ptr GetTestEngine() { FlutterDesktopEngineProperties properties = {}; properties.assets_path = L"C:\\foo\\flutter_assets"; properties.icu_data_path = L"C:\\foo\\icudtl.dat"; properties.aot_library_path = L"C:\\foo\\aot.so"; FlutterProjectBundle project(properties); - auto engine = std::make_unique(project); + auto engine = std::make_unique(project); EngineModifier modifier(engine.get()); modifier.embedder_api().UpdateSemanticsEnabled = @@ -167,10 +166,8 @@ ui::AXNode* AXNodeFromID(std::shared_ptr bridge, std::shared_ptr GetAccessibilityBridgeSpy( FlutterWindowsEngine* engine) { - FlutterWindowsEngineSpy* engine_spy = - static_cast(engine); return std::static_pointer_cast( - engine_spy->accessibility_bridge().lock()); + engine->accessibility_bridge().lock()); } void ExpectWinEventFromAXEvent(int32_t node_id, @@ -178,7 +175,7 @@ void ExpectWinEventFromAXEvent(int32_t node_id, ax::mojom::Event expected_event) { auto window_binding_handler = std::make_unique<::testing::NiceMock>(); - FlutterWindowsView view(std::move(window_binding_handler)); + FlutterWindowsViewSpy view(std::move(window_binding_handler)); view.SetEngine(GetTestEngine()); view.OnUpdateSemanticsEnabled(true); @@ -197,7 +194,7 @@ void ExpectWinEventFromAXEvent(int32_t node_id, TEST(AccessibilityBridgeWindows, GetParent) { auto window_binding_handler = std::make_unique<::testing::NiceMock>(); - FlutterWindowsView view(std::move(window_binding_handler)); + FlutterWindowsViewSpy view(std::move(window_binding_handler)); view.SetEngine(GetTestEngine()); view.OnUpdateSemanticsEnabled(true); @@ -213,7 +210,7 @@ TEST(AccessibilityBridgeWindows, GetParent) { TEST(AccessibilityBridgeWindows, GetParentOnRootRetunsNullptr) { auto window_binding_handler = std::make_unique<::testing::NiceMock>(); - FlutterWindowsView view(std::move(window_binding_handler)); + FlutterWindowsViewSpy view(std::move(window_binding_handler)); view.SetEngine(GetTestEngine()); view.OnUpdateSemanticsEnabled(true); @@ -227,7 +224,7 @@ TEST(AccessibilityBridgeWindows, GetParentOnRootRetunsNullptr) { TEST(AccessibilityBridgeWindows, DispatchAccessibilityAction) { auto window_binding_handler = std::make_unique<::testing::NiceMock>(); - FlutterWindowsView view(std::move(window_binding_handler)); + FlutterWindowsViewSpy view(std::move(window_binding_handler)); view.SetEngine(GetTestEngine()); view.OnUpdateSemanticsEnabled(true); @@ -245,7 +242,7 @@ TEST(AccessibilityBridgeWindows, DispatchAccessibilityAction) { return kSuccess; })); - AccessibilityBridgeWindows delegate(view.GetEngine(), &view); + AccessibilityBridgeWindows delegate(&view); delegate.DispatchAccessibilityAction(1, kFlutterSemanticsActionCopy, {}); EXPECT_EQ(actual_action, kFlutterSemanticsActionCopy); } @@ -263,7 +260,7 @@ TEST(AccessibilityBridgeWindows, OnAccessibilityEventChildrenChanged) { TEST(AccessibilityBridgeWindows, OnAccessibilityEventFocusChanged) { auto window_binding_handler = std::make_unique<::testing::NiceMock>(); - FlutterWindowsView view(std::move(window_binding_handler)); + FlutterWindowsViewSpy view(std::move(window_binding_handler)); view.SetEngine(GetTestEngine()); view.OnUpdateSemanticsEnabled(true); diff --git a/engine/src/flutter/shell/platform/windows/flutter_windows_engine.cc b/engine/src/flutter/shell/platform/windows/flutter_windows_engine.cc index 8a71913d4ab..95d17aa93c3 100644 --- a/engine/src/flutter/shell/platform/windows/flutter_windows_engine.cc +++ b/engine/src/flutter/shell/platform/windows/flutter_windows_engine.cc @@ -342,19 +342,22 @@ bool FlutterWindowsEngine::Run(std::string_view entrypoint) { args.update_semantics_callback2 = [](const FlutterSemanticsUpdate2* update, void* user_data) { auto host = static_cast(user_data); + auto accessibility_bridge = host->accessibility_bridge().lock(); + if (!accessibility_bridge) { + return; + } for (size_t i = 0; i < update->node_count; i++) { const FlutterSemanticsNode2* node = update->nodes[i]; - host->accessibility_bridge_->AddFlutterSemanticsNodeUpdate(*node); + accessibility_bridge->AddFlutterSemanticsNodeUpdate(*node); } for (size_t i = 0; i < update->custom_action_count; i++) { const FlutterSemanticsCustomAction2* action = update->custom_actions[i]; - host->accessibility_bridge_->AddFlutterSemanticsCustomActionUpdate( - *action); + accessibility_bridge->AddFlutterSemanticsCustomActionUpdate(*action); } - host->accessibility_bridge_->CommitUpdates(); + accessibility_bridge->CommitUpdates(); }; args.root_isolate_create_callback = [](void* user_data) { auto host = static_cast(user_data); @@ -667,21 +670,10 @@ void FlutterWindowsEngine::UpdateSemanticsEnabled(bool enabled) { if (engine_ && semantics_enabled_ != enabled) { semantics_enabled_ = enabled; embedder_api_.UpdateSemanticsEnabled(engine_, enabled); - - if (!semantics_enabled_ && accessibility_bridge_) { - accessibility_bridge_.reset(); - } else if (semantics_enabled_ && !accessibility_bridge_) { - accessibility_bridge_ = CreateAccessibilityBridge(this, view()); - } + view_->UpdateSemanticsEnabled(enabled); } } -std::shared_ptr -FlutterWindowsEngine::CreateAccessibilityBridge(FlutterWindowsEngine* engine, - FlutterWindowsView* view) { - return std::make_shared(engine, view); -} - void FlutterWindowsEngine::OnPreEngineRestart() { // Reset the keyboard's state on hot restart. if (view_) { @@ -690,11 +682,12 @@ void FlutterWindowsEngine::OnPreEngineRestart() { } gfx::NativeViewAccessible FlutterWindowsEngine::GetNativeViewAccessible() { - if (!accessibility_bridge_) { + auto bridge = accessibility_bridge().lock(); + if (!bridge) { return nullptr; } - return accessibility_bridge_->GetChildOfAXFragmentRoot(); + return bridge->GetChildOfAXFragmentRoot(); } std::string FlutterWindowsEngine::GetExecutableName() const { @@ -776,4 +769,9 @@ void FlutterWindowsEngine::OnQuit(std::optional hwnd, lifecycle_manager_->Quit(hwnd, wparam, lparam, exit_code); } +std::weak_ptr +FlutterWindowsEngine::accessibility_bridge() { + return view_->accessibility_bridge(); +} + } // namespace flutter diff --git a/engine/src/flutter/shell/platform/windows/flutter_windows_engine.h b/engine/src/flutter/shell/platform/windows/flutter_windows_engine.h index 39ee739ed5a..3b73f0a8659 100644 --- a/engine/src/flutter/shell/platform/windows/flutter_windows_engine.h +++ b/engine/src/flutter/shell/platform/windows/flutter_windows_engine.h @@ -145,9 +145,8 @@ class FlutterWindowsEngine { // rendering using software instead of OpenGL. AngleSurfaceManager* surface_manager() { return surface_manager_.get(); } - std::weak_ptr accessibility_bridge() { - return accessibility_bridge_; - } + // Return the AccessibilityBridgeWindows for this engine's view. + std::weak_ptr accessibility_bridge(); WindowProcDelegateManager* window_proc_delegate_manager() { return window_proc_delegate_manager_.get(); @@ -284,14 +283,6 @@ class FlutterWindowsEngine { virtual std::unique_ptr CreateTextInputPlugin( BinaryMessenger* messenger); - // Creates an accessibility bridge with the provided parameters. - // - // By default this method calls AccessibilityBridge's constructor. Exposing - // this method allows unit tests to override in order to capture information. - virtual std::shared_ptr CreateAccessibilityBridge( - FlutterWindowsEngine* engine, - FlutterWindowsView* view); - // Invoked by the engine right before the engine is restarted. // // This should reset necessary states to as if the engine has just been @@ -394,8 +385,6 @@ class FlutterWindowsEngine { bool high_contrast_enabled_ = false; - std::shared_ptr accessibility_bridge_; - // The manager for WindowProc delegate registration and callbacks. std::unique_ptr window_proc_delegate_manager_; diff --git a/engine/src/flutter/shell/platform/windows/flutter_windows_view.cc b/engine/src/flutter/shell/platform/windows/flutter_windows_view.cc index 2c3cca6e6aa..77288a39911 100644 --- a/engine/src/flutter/shell/platform/windows/flutter_windows_view.cc +++ b/engine/src/flutter/shell/platform/windows/flutter_windows_view.cc @@ -644,4 +644,21 @@ ui::AXPlatformNodeWin* FlutterWindowsView::AlertNode() const { return binding_handler_->GetAlert(); } +std::shared_ptr +FlutterWindowsView::CreateAccessibilityBridge() { + return std::make_shared(this); +} + +void FlutterWindowsView::UpdateSemanticsEnabled(bool enabled) { + if (semantics_enabled_ != enabled) { + semantics_enabled_ = enabled; + + if (!semantics_enabled_ && accessibility_bridge_) { + accessibility_bridge_.reset(); + } else if (semantics_enabled_ && !accessibility_bridge_) { + accessibility_bridge_ = CreateAccessibilityBridge(); + } + } +} + } // namespace flutter diff --git a/engine/src/flutter/shell/platform/windows/flutter_windows_view.h b/engine/src/flutter/shell/platform/windows/flutter_windows_view.h index 259d4cc2b89..9160add1663 100644 --- a/engine/src/flutter/shell/platform/windows/flutter_windows_view.h +++ b/engine/src/flutter/shell/platform/windows/flutter_windows_view.h @@ -16,6 +16,7 @@ #include "flutter/shell/platform/common/client_wrapper/include/flutter/plugin_registrar.h" #include "flutter/shell/platform/common/geometry.h" #include "flutter/shell/platform/embedder/embedder.h" +#include "flutter/shell/platform/windows/accessibility_bridge_windows.h" #include "flutter/shell/platform/windows/angle_surface_manager.h" #include "flutter/shell/platform/windows/flutter_windows_engine.h" #include "flutter/shell/platform/windows/public/flutter_windows.h" @@ -200,10 +201,21 @@ class FlutterWindowsView : public WindowBindingHandlerDelegate, // |WindowBindingHandlerDelegate| virtual ui::AXFragmentRootDelegateWin* GetAxFragmentRootDelegate() override; + // Called to re/set the accessibility bridge pointer. + virtual void UpdateSemanticsEnabled(bool enabled); + + std::weak_ptr accessibility_bridge() { + return accessibility_bridge_; + } + protected: virtual void NotifyWinEventWrapper(ui::AXPlatformNodeWin* node, ax::mojom::Event event); + // Create an AccessibilityBridgeWindows using this view. + virtual std::shared_ptr + CreateAccessibilityBridge(); + private: // Struct holding the state of an individual pointer. The engine doesn't keep // track of which buttons have been pressed, so it's the embedding's @@ -374,6 +386,9 @@ class FlutterWindowsView : public WindowBindingHandlerDelegate, // True when flutter's semantics tree is enabled. bool semantics_enabled_ = false; + // The accessibility bridge associated with this view. + std::shared_ptr accessibility_bridge_; + FML_DISALLOW_COPY_AND_ASSIGN(FlutterWindowsView); };