From 6c6d5a5270c8b65147faf253c04fe34276dc6ccd Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Tue, 4 Jan 2022 12:11:08 -0800 Subject: [PATCH 1/6] Impl and test (#30488) --- .../lib/src/engine/keyboard_binding.dart | 41 ++++++++++++++----- lib/web_ui/test/keyboard_converter_test.dart | 21 +++++++--- 2 files changed, 46 insertions(+), 16 deletions(-) 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(); From 5ca3fa2b80ec657dfad811fc75f458c2f9f042fb Mon Sep 17 00:00:00 2001 From: Chris Bracken Date: Wed, 5 Jan 2022 15:50:03 -0800 Subject: [PATCH 2/6] Fix missing shcore.dll error on Windows 7 (#30699) --- third_party/accessibility/ax/BUILD.gn | 1 - third_party/accessibility/base/win/display.cc | 70 ++++++++++++++++++- 2 files changed, 67 insertions(+), 4 deletions(-) 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; } From 1cb8baf43656a2f832437221202820b16f92afb6 Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Mon, 10 Jan 2022 11:25:06 -0800 Subject: [PATCH 3/6] Remove glitch when displaying platform views (#30724) --- .../embedding/android/FlutterSurfaceView.java | 12 ++++- .../embedding/android/FlutterTextureView.java | 5 +- .../embedding/engine/FlutterEngine.java | 2 +- .../engine/renderer/FlutterRenderer.java | 25 +++++++--- .../engine/renderer/RenderSurface.java | 2 +- .../engine/renderer/FlutterRendererTest.java | 47 +++++++++++++++---- .../platform/PlatformViewsControllerTest.java | 7 ++- 7 files changed, 75 insertions(+), 25 deletions(-) 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(); } From 633cc96b6168d4926944599329830242effbc7a0 Mon Sep 17 00:00:00 2001 From: Chris Bracken Date: Wed, 12 Jan 2022 10:00:58 -0800 Subject: [PATCH 4/6] Win32: Fix Korean text input (#30805) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes an issue with Korean IMEs wherein a text input state update may be sent to the framework that misleads the framework into assuming that IME composing has ended. When inputting Korean text, characters are built up keystroke by keystroke until the point that either: * the user presses space/enter to terminate composing and commit the character, or; * the user presses a key such that the character currently being composed cannot be modified further, and the IME determines that the user has begun composing the next character. The following is an example sequence of events for the latter case: 1. User presses ㅂ. GCS_COMPSTR event received with ㅂ. Embedder sends state update to framework. 2. User presses ㅏ. GCS_COMPSTR event received with 바. Embedder sends state update to framework. 3. User presses ㄴ. GCS_COMPSTR event received with 반. Embedder sends state update to framework. 4. User presses ㅏ. At this point, the current character being composed (반) cannot be modified in a meaningful way, and the IME determines that the user is typing 바 followed by 나. GCS_RESULTSTR event received with 바, immediately followed by GCS_COMPSTR event with 나. In step 4, we previously sent two events to the framework, one immediately after the other: * GCS_RESULTSTR triggers the text input model to commit the current composing region to the string under edit. This causes the composing region to collapse to an empty range. * GCS_COMPSTR triggers the text input model to insert the new composing character and set the composing region to that character. Conceptually, this is an atomic operation. The fourth keystroke causes the 반 character to be broken into two (바 and ㄴ) and the latter to be modified to 나. From the user's point of view, as well as from the IME's point of view, the user has NOT stopped composing, and the composing region has simply moved on to the next character. Flutter has no concept of whether the user is composing or not other that whether a non-empty composing region exists. As such, sending a state update after the GCS_RESULTSTR event misleads the framework into believing that composing has ended. This triggers a serious bug: Text fields with input formatters applied do not perform input formatting updates while composing is active; instead they wait until composing has ended to apply any formatting. The previous behaviour would thus trigger input formatters to be applied each time the user input caused a new character to be input. This has the add-on negative effect that once formatting has been applied, it sends an update back to the embedder so that the native OS text input state can be updated. However, since the GCS_RESULTSTR event is _immediately_ followed by a GCS_COMPSTR, the state has changed in the meantime, and the embedder is left processing an update (the intermediate state sent after the GCS_RESULTSTR) which is now out of date (i.e. missing the new state from the GCS_COMPSTR event). Since GCS_RESULTR events are always immediately followed by a subsequent GCS_COMPSTR (in the case where composing continues) or a WM_IME_ENDCOMPOSITION (in the case where composing is finished), and because the event handlers for both of those send updated state to the framework, this change eliminates sending the (intermediate) state in response to GCS_COMPSTR events. Issue: https://github.com/flutter/flutter/issues/96209 (full fix) Issue: https://github.com/flutter/flutter/issues/88645 (partial fix) --- shell/platform/windows/text_input_plugin.cc | 24 +++++++- .../windows/text_input_plugin_unittest.cc | 56 +++++++++++++++++++ 2 files changed, 79 insertions(+), 1 deletion(-) 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 From 8d2e5228e4bfccfdb1082421d5b82ddc42ff07e4 Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Thu, 13 Jan 2022 18:40:02 -0800 Subject: [PATCH 5/6] Ensure that PlatformViewIOS does not call into Shell semantics APIs during destruction (#30835) --- .../Source/accessibility_bridge_test.mm | 47 +++++++++++++++++++ shell/platform/darwin/ios/platform_view_ios.h | 22 +++++---- .../platform/darwin/ios/platform_view_ios.mm | 38 +++++++-------- 3 files changed, 76 insertions(+), 31 deletions(-) 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..4be60971438b2 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_; }; @@ -136,8 +136,12 @@ class PlatformViewIOS final : public PlatformView { std::unique_ptr ios_surface_; std::shared_ptr ios_context_; const std::shared_ptr& platform_views_controller_; +<<<<<<< HEAD PlatformMessageRouter platform_message_router_; AccessibilityBridgePtr accessibility_bridge_; +======= + AccessibilityBridgeManager accessibility_bridge_; +>>>>>>> fb3ee7f2b5 (Ensure that PlatformViewIOS does not call into Shell semantics APIs during destruction (#30835)) 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; From 0d54fdef2a5327cf1aaea8b484f715c941624ada Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Thu, 13 Jan 2022 18:40:02 -0800 Subject: [PATCH 6/6] Ensure that PlatformViewIOS does not call into Shell semantics APIs during destruction (#30835) --- shell/platform/darwin/ios/platform_view_ios.h | 4 ---- 1 file changed, 4 deletions(-) diff --git a/shell/platform/darwin/ios/platform_view_ios.h b/shell/platform/darwin/ios/platform_view_ios.h index 4be60971438b2..aef27035a5940 100644 --- a/shell/platform/darwin/ios/platform_view_ios.h +++ b/shell/platform/darwin/ios/platform_view_ios.h @@ -136,12 +136,8 @@ class PlatformViewIOS final : public PlatformView { std::unique_ptr ios_surface_; std::shared_ptr ios_context_; const std::shared_ptr& platform_views_controller_; -<<<<<<< HEAD PlatformMessageRouter platform_message_router_; - AccessibilityBridgePtr accessibility_bridge_; -======= AccessibilityBridgeManager accessibility_bridge_; ->>>>>>> fb3ee7f2b5 (Ensure that PlatformViewIOS does not call into Shell semantics APIs during destruction (#30835)) fml::scoped_nsprotocol text_input_plugin_; fml::closure firstFrameCallback_; ScopedObserver dealloc_view_controller_observer_;