diff --git a/lib/web_ui/lib/src/engine/keyboard_binding.dart b/lib/web_ui/lib/src/engine/keyboard_binding.dart index 480f790d53c1c..63e94eb8190eb 100644 --- a/lib/web_ui/lib/src/engine/keyboard_binding.dart +++ b/lib/web_ui/lib/src/engine/keyboard_binding.dart @@ -371,9 +371,7 @@ class KeyboardConverter { // followed by an immediate cancel event. (_shouldSynthesizeCapsLockUp() && event.code! == _kPhysicalCapsLock); - final int? lastLogicalRecord = _pressingRecords[physicalKey]; - - ui.KeyEventType type; + final ui.KeyEventType type; if (_shouldSynthesizeCapsLockUp() && event.code! == _kPhysicalCapsLock) { // Case 1: Handle CapsLock on macOS @@ -399,28 +397,45 @@ class KeyboardConverter { } else if (isPhysicalDown) { // Case 2: Handle key down of normal keys - type = ui.KeyEventType.down; - if (lastLogicalRecord != null) { + if (_pressingRecords[physicalKey] != null) { // This physical key is being pressed according to the record. if (event.repeat ?? false) { // A normal repeated key. type = ui.KeyEventType.repeat; } else { // A non-repeated key has been pressed that has the exact physical key as - // a currently pressed one, usually indicating multiple keyboards are - // pressing keys with the same physical key, or the up event was lost - // during a loss of focus. The down event is ignored. - event.preventDefault(); - return; + // a currently pressed one. This can mean one of the following cases: + // + // * Multiple keyboards are pressing keys with the same physical key. + // * The up event was lost during a loss of focus. + // * The previous down event was a system shortcut and its release + // was skipped (see `_startGuardingKey`,) such as holding Ctrl and + // pressing V then V, within the "guard window". + // + // The three cases can't be distinguished, and in the 3rd case, the + // latter event must be dispatched as down events for the framework to + // correctly recognize and choose to not to handle. Therefore, an up + // event is synthesized before it. + _dispatchKeyData!(ui.KeyData( + timeStamp: timeStamp, + type: ui.KeyEventType.up, + physical: physicalKey, + logical: logicalKey, + character: null, + synthesized: true, + )); + _pressingRecords.remove(physicalKey); + type = ui.KeyEventType.down; } } else { // This physical key is not being pressed according to the record. It's a // normal down event, whether the system event is a repeat or not. + type = ui.KeyEventType.down; } } else { // isPhysicalDown is false and not CapsLock // Case 2: Handle key up of normal keys - if (lastLogicalRecord == null) { + if (_pressingRecords[physicalKey] == null) { // The physical key has been released before. It indicates multiple // keyboards pressed keys with the same physical key. Ignore the up event. event.preventDefault(); @@ -430,6 +445,10 @@ class KeyboardConverter { type = ui.KeyEventType.up; } + // The _pressingRecords[physicalKey] might have been changed during the last + // `if` clause. + final int? lastLogicalRecord = _pressingRecords[physicalKey]; + final int? nextLogicalRecord; switch (type) { case ui.KeyEventType.down: diff --git a/lib/web_ui/test/keyboard_converter_test.dart b/lib/web_ui/test/keyboard_converter_test.dart index 6183097f65a02..0e9aae79ff3bc 100644 --- a/lib/web_ui/test/keyboard_converter_test.dart +++ b/lib/web_ui/test/keyboard_converter_test.dart @@ -378,7 +378,7 @@ void testMain() { converter.handleEvent(keyUpEvent('ShiftLeft', 'Shift', 0, kLocationLeft)); }); - test('Duplicate down is ignored', () { + test('Duplicate down is preceded with synthesized up', () { final List keyDataList = []; final KeyboardConverter converter = KeyboardConverter((ui.KeyData key) { keyDataList.add(key); @@ -392,15 +392,26 @@ void testMain() { ); expect(preventedDefault, isTrue); preventedDefault = false; - // A KeyUp of ShiftLeft is missed due to loss of focus. + // A KeyUp of ShiftLeft is missed. keyDataList.clear(); converter.handleEvent(keyDownEvent('ShiftLeft', 'Shift', kShift, kLocationLeft) ..onPreventDefault = onPreventDefault ); - expect(keyDataList, hasLength(1)); - expect(keyDataList[0].physical, 0); - expect(keyDataList[0].logical, 0); + expect(keyDataList, hasLength(2)); + expectKeyData(keyDataList.first, + type: ui.KeyEventType.up, + physical: kPhysicalShiftLeft, + logical: kLogicalShiftLeft, + character: null, + synthesized: true, + ); + expectKeyData(keyDataList.last, + type: ui.KeyEventType.down, + physical: kPhysicalShiftLeft, + logical: kLogicalShiftLeft, + character: null, + ); expect(preventedDefault, isTrue); keyDataList.clear(); diff --git a/shell/platform/android/io/flutter/embedding/android/FlutterSurfaceView.java b/shell/platform/android/io/flutter/embedding/android/FlutterSurfaceView.java index 1d6bccc77a7ea..5944aa0d635ee 100644 --- a/shell/platform/android/io/flutter/embedding/android/FlutterSurfaceView.java +++ b/shell/platform/android/io/flutter/embedding/android/FlutterSurfaceView.java @@ -37,6 +37,7 @@ public class FlutterSurfaceView extends SurfaceView implements RenderSurface { private final boolean renderTransparently; private boolean isSurfaceAvailableForRendering = false; + private boolean isPaused = false; private boolean isAttachedToFlutterRenderer = false; @Nullable private FlutterRenderer flutterRenderer; @@ -200,6 +201,7 @@ public void attachToRenderer(@NonNull FlutterRenderer flutterRenderer) { "Surface is available for rendering. Connecting FlutterRenderer to Android surface."); connectSurfaceToRenderer(); } + isPaused = false; } /** @@ -241,6 +243,7 @@ public void pause() { // Don't remove the `flutterUiDisplayListener` as `onFlutterUiDisplayed()` will make // the `FlutterSurfaceView` visible. flutterRenderer = null; + isPaused = true; isAttachedToFlutterRenderer = false; } else { Log.w(TAG, "pause() invoked when no FlutterRenderer was attached."); @@ -253,8 +256,13 @@ private void connectSurfaceToRenderer() { throw new IllegalStateException( "connectSurfaceToRenderer() should only be called when flutterRenderer and getHolder() are non-null."); } - - flutterRenderer.startRenderingToSurface(getHolder().getSurface()); + // When connecting the surface to the renderer, it's possible that the surface is currently + // paused. For instance, when a platform view is displayed, the current FlutterSurfaceView + // is paused, and rendering continues in a FlutterImageView buffer while the platform view + // is displayed. + // + // startRenderingToSurface stops rendering to an active surface if it isn't paused. + flutterRenderer.startRenderingToSurface(getHolder().getSurface(), isPaused); } // FlutterRenderer must be non-null. diff --git a/shell/platform/android/io/flutter/embedding/android/FlutterTextureView.java b/shell/platform/android/io/flutter/embedding/android/FlutterTextureView.java index a151977520784..5e2d8e03dfb67 100644 --- a/shell/platform/android/io/flutter/embedding/android/FlutterTextureView.java +++ b/shell/platform/android/io/flutter/embedding/android/FlutterTextureView.java @@ -36,6 +36,7 @@ public class FlutterTextureView extends TextureView implements RenderSurface { private boolean isSurfaceAvailableForRendering = false; private boolean isAttachedToFlutterRenderer = false; + private boolean isPaused = false; @Nullable private FlutterRenderer flutterRenderer; @Nullable private Surface renderSurface; @@ -187,6 +188,7 @@ public void detachFromRenderer() { public void pause() { if (flutterRenderer != null) { flutterRenderer = null; + isPaused = true; isAttachedToFlutterRenderer = false; } else { Log.w(TAG, "pause() invoked when no FlutterRenderer was attached."); @@ -217,7 +219,8 @@ private void connectSurfaceToRenderer() { } renderSurface = new Surface(getSurfaceTexture()); - flutterRenderer.startRenderingToSurface(renderSurface); + flutterRenderer.startRenderingToSurface(renderSurface, isPaused); + isPaused = false; } // FlutterRenderer must be non-null. diff --git a/shell/platform/android/io/flutter/embedding/engine/FlutterEngine.java b/shell/platform/android/io/flutter/embedding/engine/FlutterEngine.java index ff712d4edcc7e..144cf52086b61 100644 --- a/shell/platform/android/io/flutter/embedding/engine/FlutterEngine.java +++ b/shell/platform/android/io/flutter/embedding/engine/FlutterEngine.java @@ -134,7 +134,7 @@ public void onEngineWillDestroy() { * *

A new {@code FlutterEngine} will not display any UI until a {@link RenderSurface} is * registered. See {@link #getRenderer()} and {@link - * FlutterRenderer#startRenderingToSurface(Surface)}. + * FlutterRenderer#startRenderingToSurface(Surface, boolean)}. * *

A new {@code FlutterEngine} automatically attaches all plugins. See {@link #getPlugins()}. * diff --git a/shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java b/shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java index d68b17bdf7c44..d58e1b791870e 100644 --- a/shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java +++ b/shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java @@ -221,11 +221,24 @@ public void run() { * Notifies Flutter that the given {@code surface} was created and is available for Flutter * rendering. * + *

If called more than once, the current native resources are released. This can be undesired + * if the Engine expects to reuse this surface later. For example, this is true when platform + * views are displayed in a frame, and then removed in the next frame. + * + *

To avoid releasing the current surface resources, set {@code keepCurrentSurface} to true. + * *

See {@link android.view.SurfaceHolder.Callback} and {@link * android.view.TextureView.SurfaceTextureListener} + * + * @param surface The render surface. + * @param keepCurrentSurface True if the current active surface should not be released. */ - public void startRenderingToSurface(@NonNull Surface surface) { - if (this.surface != null) { + public void startRenderingToSurface(@NonNull Surface surface, boolean keepCurrentSurface) { + // Don't stop rendering the surface if it's currently paused. + // Stop rendering to the surface releases the associated native resources, which + // causes a glitch when showing platform views. + // For more, https://github.com/flutter/flutter/issues/95343 + if (this.surface != null && !keepCurrentSurface) { stopRenderingToSurface(); } @@ -248,8 +261,8 @@ public void swapSurface(@NonNull Surface surface) { /** * Notifies Flutter that a {@code surface} previously registered with {@link - * #startRenderingToSurface(Surface)} has changed size to the given {@code width} and {@code - * height}. + * #startRenderingToSurface(Surface, boolean)} has changed size to the given {@code width} and + * {@code height}. * *

See {@link android.view.SurfaceHolder.Callback} and {@link * android.view.TextureView.SurfaceTextureListener} @@ -260,8 +273,8 @@ public void surfaceChanged(int width, int height) { /** * Notifies Flutter that a {@code surface} previously registered with {@link - * #startRenderingToSurface(Surface)} has been destroyed and needs to be released and cleaned up - * on the Flutter side. + * #startRenderingToSurface(Surface, boolean)} has been destroyed and needs to be released and + * cleaned up on the Flutter side. * *

See {@link android.view.SurfaceHolder.Callback} and {@link * android.view.TextureView.SurfaceTextureListener} diff --git a/shell/platform/android/io/flutter/embedding/engine/renderer/RenderSurface.java b/shell/platform/android/io/flutter/embedding/engine/renderer/RenderSurface.java index 48131a5dc545f..58af9a49fd418 100644 --- a/shell/platform/android/io/flutter/embedding/engine/renderer/RenderSurface.java +++ b/shell/platform/android/io/flutter/embedding/engine/renderer/RenderSurface.java @@ -37,7 +37,7 @@ public interface RenderSurface { * FlutterRenderer} at the appropriate times: * *

    - *
  1. {@link FlutterRenderer#startRenderingToSurface(Surface)} + *
  2. {@link FlutterRenderer#startRenderingToSurface(Surface, boolean)} *
  3. {@link FlutterRenderer#surfaceChanged(int, int)}} *
  4. {@link FlutterRenderer#stopRenderingToSurface()} *
diff --git a/shell/platform/android/test/io/flutter/embedding/engine/renderer/FlutterRendererTest.java b/shell/platform/android/test/io/flutter/embedding/engine/renderer/FlutterRendererTest.java index dfe4b407d93a9..2b9f007d1ed2d 100644 --- a/shell/platform/android/test/io/flutter/embedding/engine/renderer/FlutterRendererTest.java +++ b/shell/platform/android/test/io/flutter/embedding/engine/renderer/FlutterRendererTest.java @@ -6,6 +6,7 @@ import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -45,7 +46,7 @@ public void itForwardsSurfaceCreationNotificationToFlutterJNI() { FlutterRenderer flutterRenderer = new FlutterRenderer(fakeFlutterJNI); // Execute the behavior under test. - flutterRenderer.startRenderingToSurface(fakeSurface); + flutterRenderer.startRenderingToSurface(fakeSurface, /*keepCurrentSurface=*/ false); // Verify the behavior under test. verify(fakeFlutterJNI, times(1)).onSurfaceCreated(eq(fakeSurface)); @@ -57,7 +58,7 @@ public void itForwardsSurfaceChangeNotificationToFlutterJNI() { Surface fakeSurface = mock(Surface.class); FlutterRenderer flutterRenderer = new FlutterRenderer(fakeFlutterJNI); - flutterRenderer.startRenderingToSurface(fakeSurface); + flutterRenderer.startRenderingToSurface(fakeSurface, /*keepCurrentSurface=*/ false); // Execute the behavior under test. flutterRenderer.surfaceChanged(100, 50); @@ -72,7 +73,7 @@ public void itForwardsSurfaceDestructionNotificationToFlutterJNI() { Surface fakeSurface = mock(Surface.class); FlutterRenderer flutterRenderer = new FlutterRenderer(fakeFlutterJNI); - flutterRenderer.startRenderingToSurface(fakeSurface); + flutterRenderer.startRenderingToSurface(fakeSurface, /*keepCurrentSurface=*/ false); // Execute the behavior under test. flutterRenderer.stopRenderingToSurface(); @@ -87,10 +88,10 @@ public void itStopsRenderingToOneSurfaceBeforeRenderingToANewSurface() { Surface fakeSurface2 = mock(Surface.class); FlutterRenderer flutterRenderer = new FlutterRenderer(fakeFlutterJNI); - flutterRenderer.startRenderingToSurface(fakeSurface); + flutterRenderer.startRenderingToSurface(fakeSurface, /*keepCurrentSurface=*/ false); // Execute behavior under test. - flutterRenderer.startRenderingToSurface(fakeSurface2); + flutterRenderer.startRenderingToSurface(fakeSurface2, /*keepCurrentSurface=*/ false); // Verify behavior under test. verify(fakeFlutterJNI, times(1)).onSurfaceDestroyed(); // notification of 1st surface's removal. @@ -101,7 +102,7 @@ public void itStopsRenderingToSurfaceWhenRequested() { // Setup the test. FlutterRenderer flutterRenderer = new FlutterRenderer(fakeFlutterJNI); - flutterRenderer.startRenderingToSurface(fakeSurface); + flutterRenderer.startRenderingToSurface(fakeSurface, /*keepCurrentSurface=*/ false); // Execute the behavior under test. flutterRenderer.stopRenderingToSurface(); @@ -110,6 +111,32 @@ public void itStopsRenderingToSurfaceWhenRequested() { verify(fakeFlutterJNI, times(1)).onSurfaceDestroyed(); } + @Test + public void iStopsRenderingToSurfaceWhenSurfaceAlreadySet() { + // Setup the test. + FlutterRenderer flutterRenderer = new FlutterRenderer(fakeFlutterJNI); + + flutterRenderer.startRenderingToSurface(fakeSurface, /*keepCurrentSurface=*/ false); + + flutterRenderer.startRenderingToSurface(fakeSurface, /*keepCurrentSurface=*/ false); + + // Verify behavior under test. + verify(fakeFlutterJNI, times(1)).onSurfaceDestroyed(); + } + + @Test + public void itNeverStopsRenderingToSurfaceWhenRequested() { + // Setup the test. + FlutterRenderer flutterRenderer = new FlutterRenderer(fakeFlutterJNI); + + flutterRenderer.startRenderingToSurface(fakeSurface, /*keepCurrentSurface=*/ false); + + flutterRenderer.startRenderingToSurface(fakeSurface, /*keepCurrentSurface=*/ true); + + // Verify behavior under test. + verify(fakeFlutterJNI, never()).onSurfaceDestroyed(); + } + @Test public void itStopsSurfaceTextureCallbackWhenDetached() { // Setup the test. @@ -120,7 +147,7 @@ public void itStopsSurfaceTextureCallbackWhenDetached() { FlutterRenderer.SurfaceTextureRegistryEntry entry = (FlutterRenderer.SurfaceTextureRegistryEntry) flutterRenderer.createSurfaceTexture(); - flutterRenderer.startRenderingToSurface(fakeSurface); + flutterRenderer.startRenderingToSurface(fakeSurface, /*keepCurrentSurface=*/ false); // Execute the behavior under test. flutterRenderer.stopRenderingToSurface(); @@ -143,7 +170,7 @@ public void itRegistersExistingSurfaceTexture() { (FlutterRenderer.SurfaceTextureRegistryEntry) flutterRenderer.registerSurfaceTexture(surfaceTexture); - flutterRenderer.startRenderingToSurface(fakeSurface); + flutterRenderer.startRenderingToSurface(fakeSurface, /*keepCurrentSurface=*/ false); // Verify behavior under test. assertEquals(surfaceTexture, entry.surfaceTexture()); @@ -164,7 +191,7 @@ public void itUnregistersTextureWhenSurfaceTextureFinalized() { (FlutterRenderer.SurfaceTextureRegistryEntry) flutterRenderer.createSurfaceTexture(); long id = entry.id(); - flutterRenderer.startRenderingToSurface(fakeSurface); + flutterRenderer.startRenderingToSurface(fakeSurface, /*keepCurrentSurface=*/ false); // Execute the behavior under test. runFinalization(entry); @@ -190,7 +217,7 @@ public void itStopsUnregisteringTextureWhenDetached() { (FlutterRenderer.SurfaceTextureRegistryEntry) flutterRenderer.createSurfaceTexture(); long id = entry.id(); - flutterRenderer.startRenderingToSurface(fakeSurface); + flutterRenderer.startRenderingToSurface(fakeSurface, /*keepCurrentSurface=*/ false); flutterRenderer.stopRenderingToSurface(); diff --git a/shell/platform/android/test/io/flutter/plugin/platform/PlatformViewsControllerTest.java b/shell/platform/android/test/io/flutter/plugin/platform/PlatformViewsControllerTest.java index 346807172073d..4f6da7c826927 100644 --- a/shell/platform/android/test/io/flutter/plugin/platform/PlatformViewsControllerTest.java +++ b/shell/platform/android/test/io/flutter/plugin/platform/PlatformViewsControllerTest.java @@ -1,9 +1,11 @@ package io.flutter.plugin.platform; +import static android.os.Looper.getMainLooper; import static io.flutter.embedding.engine.systemchannels.PlatformViewsChannel.PlatformViewTouch; import static org.junit.Assert.*; import static org.mockito.ArgumentMatchers.*; import static org.mockito.Mockito.*; +import static org.robolectric.Shadows.shadowOf; import android.content.Context; import android.content.res.AssetManager; @@ -486,10 +488,7 @@ public void onEndFrame__destroysOverlaySurfaceAfterFrameOnFlutterSurfaceView() { platformViewsController.onBeginFrame(); platformViewsController.onEndFrame(); - verify(overlayImageView, never()).detachFromRenderer(); - - // Simulate first frame from the framework. - jni.onFirstFrame(); + shadowOf(getMainLooper()).idle(); verify(overlayImageView, times(1)).detachFromRenderer(); } diff --git a/shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm b/shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm index 1e281e250e251..b8e6ca5fdb654 100644 --- a/shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm +++ b/shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm @@ -1664,4 +1664,51 @@ - (void)testFlutterSemanticsScrollViewManagedObjectLifecycleCorrectly { // flutterSemanticsScrollView) will cause an EXC_BAD_ACCESS. XCTAssertFalse([flutterSemanticsScrollView isAccessibilityElement]); } + +- (void)testPlatformViewDestructorDoesNotCallSemanticsAPIs { + class TestDelegate : public flutter::MockDelegate { + public: + void OnPlatformViewSetSemanticsEnabled(bool enabled) override { set_semantics_enabled_calls++; } + int set_semantics_enabled_calls = 0; + }; + + TestDelegate test_delegate; + auto thread = std::make_unique("AccessibilityBridgeTest"); + auto thread_task_runner = thread->GetTaskRunner(); + flutter::TaskRunners runners(/*label=*/self.name.UTF8String, + /*platform=*/thread_task_runner, + /*raster=*/thread_task_runner, + /*ui=*/thread_task_runner, + /*io=*/thread_task_runner); + + fml::AutoResetWaitableEvent latch; + thread_task_runner->PostTask([&] { + auto platform_view = std::make_unique( + /*delegate=*/test_delegate, + /*rendering_api=*/flutter::IOSRenderingAPI::kSoftware, + /*platform_views_controller=*/nil, + /*task_runners=*/runners); + + id mockFlutterViewController = OCMClassMock([FlutterViewController class]); + auto flutterPlatformViewsController = + std::make_shared(); + OCMStub([mockFlutterViewController platformViewsController]) + .andReturn(flutterPlatformViewsController.get()); + auto weakFactory = + std::make_unique>(mockFlutterViewController); + platform_view->SetOwnerViewController(weakFactory->GetWeakPtr()); + + platform_view->SetSemanticsEnabled(true); + XCTAssertNotEqual(test_delegate.set_semantics_enabled_calls, 0); + + // Deleting PlatformViewIOS should not call OnPlatformViewSetSemanticsEnabled + test_delegate.set_semantics_enabled_calls = 0; + platform_view.reset(); + XCTAssertEqual(test_delegate.set_semantics_enabled_calls, 0); + + latch.Signal(); + }); + latch.Wait(); +} + @end diff --git a/shell/platform/darwin/ios/platform_view_ios.h b/shell/platform/darwin/ios/platform_view_ios.h index 3af69b7b2bf8a..aef27035a5940 100644 --- a/shell/platform/darwin/ios/platform_view_ios.h +++ b/shell/platform/darwin/ios/platform_view_ios.h @@ -111,20 +111,20 @@ class PlatformViewIOS final : public PlatformView { id observer_; }; - /// Smart pointer that guarantees we communicate clearing Accessibility + /// Wrapper that guarantees we communicate clearing Accessibility /// information to Dart. - class AccessibilityBridgePtr { + class AccessibilityBridgeManager { public: - explicit AccessibilityBridgePtr(const std::function& set_semantics_enabled); - AccessibilityBridgePtr(const std::function& set_semantics_enabled, - AccessibilityBridge* bridge); - ~AccessibilityBridgePtr(); + explicit AccessibilityBridgeManager(const std::function& set_semantics_enabled); + AccessibilityBridgeManager(const std::function& set_semantics_enabled, + AccessibilityBridge* bridge); explicit operator bool() const noexcept { return static_cast(accessibility_bridge_); } - AccessibilityBridge* operator->() const noexcept { return accessibility_bridge_.get(); } - void reset(AccessibilityBridge* bridge = nullptr); + AccessibilityBridge* get() const noexcept { return accessibility_bridge_.get(); } + void Set(std::unique_ptr bridge); + void Clear(); private: - FML_DISALLOW_COPY_AND_ASSIGN(AccessibilityBridgePtr); + FML_DISALLOW_COPY_AND_ASSIGN(AccessibilityBridgeManager); std::unique_ptr accessibility_bridge_; std::function set_semantics_enabled_; }; @@ -137,7 +137,7 @@ class PlatformViewIOS final : public PlatformView { std::shared_ptr ios_context_; const std::shared_ptr& platform_views_controller_; PlatformMessageRouter platform_message_router_; - AccessibilityBridgePtr accessibility_bridge_; + AccessibilityBridgeManager accessibility_bridge_; fml::scoped_nsprotocol text_input_plugin_; fml::closure firstFrameCallback_; ScopedObserver dealloc_view_controller_observer_; diff --git a/shell/platform/darwin/ios/platform_view_ios.mm b/shell/platform/darwin/ios/platform_view_ios.mm index 081bbe0db130a..ad47dac00279d 100644 --- a/shell/platform/darwin/ios/platform_view_ios.mm +++ b/shell/platform/darwin/ios/platform_view_ios.mm @@ -16,11 +16,11 @@ namespace flutter { -PlatformViewIOS::AccessibilityBridgePtr::AccessibilityBridgePtr( +PlatformViewIOS::AccessibilityBridgeManager::AccessibilityBridgeManager( const std::function& set_semantics_enabled) - : AccessibilityBridgePtr(set_semantics_enabled, nullptr) {} + : AccessibilityBridgeManager(set_semantics_enabled, nullptr) {} -PlatformViewIOS::AccessibilityBridgePtr::AccessibilityBridgePtr( +PlatformViewIOS::AccessibilityBridgeManager::AccessibilityBridgeManager( const std::function& set_semantics_enabled, AccessibilityBridge* bridge) : accessibility_bridge_(bridge), set_semantics_enabled_(set_semantics_enabled) { @@ -29,20 +29,14 @@ } } -PlatformViewIOS::AccessibilityBridgePtr::~AccessibilityBridgePtr() { - if (accessibility_bridge_) { - set_semantics_enabled_(false); - } +void PlatformViewIOS::AccessibilityBridgeManager::Set(std::unique_ptr bridge) { + accessibility_bridge_ = std::move(bridge); + set_semantics_enabled_(true); } -void PlatformViewIOS::AccessibilityBridgePtr::reset(AccessibilityBridge* bridge) { - if (accessibility_bridge_) { - set_semantics_enabled_(false); - } - accessibility_bridge_.reset(bridge); - if (accessibility_bridge_) { - set_semantics_enabled_(true); - } +void PlatformViewIOS::AccessibilityBridgeManager::Clear() { + set_semantics_enabled_(false); + accessibility_bridge_.reset(); } PlatformViewIOS::PlatformViewIOS( @@ -86,7 +80,7 @@ if (ios_surface_ || !owner_controller) { NotifyDestroyed(); ios_surface_.reset(); - accessibility_bridge_.reset(); + accessibility_bridge_.Clear(); } owner_controller_ = owner_controller; @@ -98,7 +92,7 @@ queue:[NSOperationQueue mainQueue] usingBlock:^(NSNotification* note) { // Implicit copy of 'this' is fine. - accessibility_bridge_.reset(); + accessibility_bridge_.Clear(); owner_controller_.reset(); }] retain]); @@ -122,7 +116,7 @@ FML_DCHECK(ios_surface_ != nullptr); if (accessibility_bridge_) { - accessibility_bridge_.reset(new AccessibilityBridge( + accessibility_bridge_.Set(std::make_unique( owner_controller_.get(), this, [owner_controller_.get() platformViewsController])); } } @@ -169,10 +163,10 @@ return; } if (enabled && !accessibility_bridge_) { - accessibility_bridge_.reset(new AccessibilityBridge( + accessibility_bridge_.Set(std::make_unique( owner_controller_.get(), this, [owner_controller_.get() platformViewsController])); } else if (!enabled && accessibility_bridge_) { - accessibility_bridge_.reset(); + accessibility_bridge_.Clear(); } else { PlatformView::SetSemanticsEnabled(enabled); } @@ -188,7 +182,7 @@ flutter::CustomAccessibilityActionUpdates actions) { FML_DCHECK(owner_controller_); if (accessibility_bridge_) { - accessibility_bridge_->UpdateSemantics(std::move(update), std::move(actions)); + accessibility_bridge_.get()->UpdateSemantics(std::move(update), std::move(actions)); [[NSNotificationCenter defaultCenter] postNotificationName:FlutterSemanticsUpdateNotification object:owner_controller_.get()]; } @@ -201,7 +195,7 @@ void PlatformViewIOS::OnPreEngineRestart() const { if (accessibility_bridge_) { - accessibility_bridge_->clearState(); + accessibility_bridge_.get()->clearState(); } if (!owner_controller_) { return; diff --git a/shell/platform/windows/text_input_plugin.cc b/shell/platform/windows/text_input_plugin.cc index 93b538ccb7d98..8c0776d9959d2 100644 --- a/shell/platform/windows/text_input_plugin.cc +++ b/shell/platform/windows/text_input_plugin.cc @@ -112,7 +112,29 @@ void TextInputPlugin::ComposeCommitHook() { return; } active_model_->CommitComposing(); - SendStateUpdate(*active_model_); + + // We do not trigger SendStateUpdate here. + // + // Until a WM_IME_ENDCOMPOSING event, the user is still composing from the OS + // point of view. Commit events are always immediately followed by another + // composing event or an end composing event. However, in the brief window + // between the commit event and the following event, the composing region is + // collapsed. Notifying the framework of this intermediate state will trigger + // any framework code designed to execute at the end of composing, such as + // input formatters, which may try to update the text and send a message back + // to the engine with changes. + // + // This is a particular problem with Korean IMEs, which build up one + // character at a time in their composing region until a keypress that makes + // no sense for the in-progress character. At that point, the result + // character is committed and a compose event is immedidately received with + // the new composing region. + // + // In the case where this event is immediately followed by a composing event, + // the state will be sent in ComposeChangeHook. + // + // In the case where this event is immediately followed by an end composing + // event, the state will be sent in ComposeEndHook. } void TextInputPlugin::ComposeEndHook() { diff --git a/shell/platform/windows/text_input_plugin_unittest.cc b/shell/platform/windows/text_input_plugin_unittest.cc index 2da9c77aae3d5..30af07d96e42d 100644 --- a/shell/platform/windows/text_input_plugin_unittest.cc +++ b/shell/platform/windows/text_input_plugin_unittest.cc @@ -81,5 +81,61 @@ TEST(TextInputPluginTest, ClearClientResetsComposing) { EXPECT_TRUE(delegate.ime_was_reset()); } +// Verify that the embedder sends state update messages to the framework during +// IME composing. +TEST(TextInputPluginTest, VerifyComposingSendStateUpdate) { + bool sent_message = false; + TestBinaryMessenger messenger( + [&sent_message](const std::string& channel, const uint8_t* message, + size_t message_size, + BinaryReply reply) { sent_message = true; }); + BinaryReply reply_handler = [](const uint8_t* reply, size_t reply_size) {}; + + EmptyTextInputPluginDelegate delegate; + TextInputPlugin handler(&messenger, &delegate); + + auto& codec = JsonMethodCodec::GetInstance(); + + // Call TextInput.setClient to initialize the TextInputModel. + auto arguments = std::make_unique(rapidjson::kArrayType); + auto& allocator = arguments->GetAllocator(); + arguments->PushBack(42, allocator); + rapidjson::Value config(rapidjson::kObjectType); + config.AddMember("inputAction", "done", allocator); + config.AddMember("inputType", "text", allocator); + arguments->PushBack(config, allocator); + auto message = + codec.EncodeMethodCall({"TextInput.setClient", std::move(arguments)}); + messenger.SimulateEngineMessage("flutter/textinput", message->data(), + message->size(), reply_handler); + + // ComposeBeginHook should send state update. + sent_message = false; + handler.ComposeBeginHook(); + EXPECT_TRUE(sent_message); + + // ComposeChangeHook should send state update. + sent_message = false; + handler.ComposeChangeHook(u"4", 1); + EXPECT_TRUE(sent_message); + + // ComposeCommitHook should NOT send state update. + // + // Commit messages are always immediately followed by a change message or an + // end message, both of which will send an update. Sending intermediate state + // with a collapsed composing region will trigger the framework to assume + // composing has ended, which is not the case until a WM_IME_ENDCOMPOSING + // event is received in the main event loop, which will trigger a call to + // ComposeEndHook. + sent_message = false; + handler.ComposeCommitHook(); + EXPECT_FALSE(sent_message); + + // ComposeEndHook should send state update. + sent_message = false; + handler.ComposeEndHook(); + EXPECT_TRUE(sent_message); +} + } // namespace testing } // namespace flutter diff --git a/third_party/accessibility/ax/BUILD.gn b/third_party/accessibility/ax/BUILD.gn index ef38c51dee33e..3c234b72344a7 100644 --- a/third_party/accessibility/ax/BUILD.gn +++ b/third_party/accessibility/ax/BUILD.gn @@ -99,7 +99,6 @@ source_set("ax") { ] libs = [ "oleacc.lib", - "shcore.lib", "uiautomationcore.lib", ] } diff --git a/third_party/accessibility/base/win/display.cc b/third_party/accessibility/base/win/display.cc index 64024e125e642..85c927dbec8c5 100644 --- a/third_party/accessibility/base/win/display.cc +++ b/third_party/accessibility/base/win/display.cc @@ -7,11 +7,75 @@ namespace base { namespace win { +namespace { + +template +bool AssignProcAddress(HMODULE comBaseModule, const char* name, T*& outProc) { + outProc = reinterpret_cast(GetProcAddress(comBaseModule, name)); + return *outProc != nullptr; +} + +// Helper class for supporting display scale factor lookup across Windows +// versions, with fallbacks where these lookups are unavailable. +class ScaleHelperWin32 { + public: + ScaleHelperWin32(); + ~ScaleHelperWin32(); + + /// Returns the scale factor for the specified monitor. Sets |scale| to + /// SCALE_100_PERCENT if the API is not available. + HRESULT GetScaleFactorForMonitor(HMONITOR hmonitor, + DEVICE_SCALE_FACTOR* scale) const; + + private: + using GetScaleFactorForMonitor_ = + HRESULT __stdcall(HMONITOR hmonitor, DEVICE_SCALE_FACTOR* scale); + + GetScaleFactorForMonitor_* get_scale_factor_for_monitor_ = nullptr; + + HMODULE shlib_module_ = nullptr; + bool scale_factor_for_monitor_supported_ = false; +}; + +ScaleHelperWin32::ScaleHelperWin32() { + if ((shlib_module_ = LoadLibraryA("Shcore.dll")) != nullptr) { + scale_factor_for_monitor_supported_ = + AssignProcAddress(shlib_module_, "GetScaleFactorForMonitor", + get_scale_factor_for_monitor_); + } +} + +ScaleHelperWin32::~ScaleHelperWin32() { + if (shlib_module_ != nullptr) { + FreeLibrary(shlib_module_); + } +} + +HRESULT ScaleHelperWin32::GetScaleFactorForMonitor( + HMONITOR hmonitor, + DEVICE_SCALE_FACTOR* scale) const { + if (hmonitor == nullptr || scale == nullptr) { + return E_INVALIDARG; + } + if (!scale_factor_for_monitor_supported_) { + *scale = SCALE_100_PERCENT; + return S_OK; + } + return get_scale_factor_for_monitor_(hmonitor, scale); +} + +ScaleHelperWin32* GetHelper() { + static ScaleHelperWin32* helper = new ScaleHelperWin32(); + return helper; +} + +} // namespace + float GetScaleFactorForHWND(HWND hwnd) { HMONITOR monitor = MonitorFromWindow(hwnd, MONITOR_DEFAULTTONEAREST); - DEVICE_SCALE_FACTOR scale_factor = DEVICE_SCALE_FACTOR_INVALID; - if (SUCCEEDED(GetScaleFactorForMonitor(monitor, &scale_factor))) { - return ScaleFactorToFloat(scale_factor); + DEVICE_SCALE_FACTOR scale = DEVICE_SCALE_FACTOR_INVALID; + if (SUCCEEDED(GetHelper()->GetScaleFactorForMonitor(monitor, &scale))) { + return ScaleFactorToFloat(scale); } return 1.0f; }