[ios platform view] fix overlay zPosition #29930

cyanglaz · 2021-11-24T22:06:19Z

Explicitly set every platform view and overlay view's zPosition so we are sure the overlays are on top of the platform views.

Fixes: flutter/flutter#86787

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 and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
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.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

blasten · 2021-11-29T20:02:26Z

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

+                // Clear the `active_composition_order_`, which will be populated down below.
+                active_composition_order_.clear();


mistaken edits?

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

blasten · 2021-11-29T20:05:44Z

testing/scenario_app/lib/src/platform_view.dart

@@ -765,6 +765,84 @@ class TwoPlatformViewsWithOtherBackDropFilter extends Scenario with _BasePlatfor
  }
 }

+/// Builds a scenario where many platform views are scrolling and pass under a widget


nit: there's no concept of widget in these tests

blasten · 2021-11-29T20:06:31Z

testing/scenario_app/lib/src/platform_view.dart

+  PlatformViewScrollingUnderWidget(PlatformDispatcher dispatcher, {required this.minId, required this.maxId})
+      : assert(dispatcher != null),
+        super(dispatcher) {
+    for (int i = minId; i < maxId+1; i ++) {


Suggested change

for (int i = minId; i < maxId+1; i ++) {

for (int i = minId; i <= maxId; i++) {

blasten · 2021-11-29T20:07:20Z

testing/scenario_app/lib/src/platform_view.dart

+  }
+
+  /// The platform view identifier to use for the first platform view.
+  final int minId;


why not starting with 0?

blasten · 2021-11-29T20:12:18Z

testing/scenario_app/lib/src/platform_view.dart

+  }
+
+  @override
+  void onDrawFrame() {


Have you found an minimal sequence of events that yields into the issue? If possible, it would be preferred.

The only time I can repro this issue is scroll very fast.

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

testing/scenario_app/ios/Scenarios/ScenariosUITests/PlatformViewUITests.m

jmagman · 2021-11-29T18:26:53Z

testing/scenario_app/ios/Scenarios/ScenariosUITests/PlatformViewUITests.m

+  }
+
+  // Wait and let the scenario app scroll a bit.
+  sleep(5);


Let's not sleep the main thread, even in XCUITests where it's running in a different process than the app. It may cause strange timeout behavior.

Instead, leverage the XCTest wait: https://stackoverflow.com/a/50248104

jmagman · 2021-11-29T18:27:43Z

testing/scenario_app/lib/src/platform_view.dart

+  }
+
+  /// The platform view identifier to use for the first platform view.
+  final int minId;


Can you name these better instead of putting the important info in the comment? firstPlatformViewId?

jmagman · 2021-11-29T18:28:02Z

testing/scenario_app/lib/src/platform_view.dart

+
+  void _buildOneFrame(double offset) {
+    const double cellWidth = 1000;
+    print(offset);


Remove print.

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

testing/scenario_app/ios/Scenarios/ScenariosUITests/PlatformViewUITests.m

cyanglaz · 2021-11-30T19:10:29Z

testing/scenario_app/lib/src/platform_view.dart

@@ -765,6 +765,84 @@ class TwoPlatformViewsWithOtherBackDropFilter extends Scenario with _BasePlatfor
  }
 }

+/// Builds a scenario where many platform views are scrolling and pass under a widget


cyanglaz · 2021-11-30T19:14:15Z

testing/scenario_app/lib/src/platform_view.dart

+  PlatformViewScrollingUnderWidget(PlatformDispatcher dispatcher, {required this.minId, required this.maxId})
+      : assert(dispatcher != null),
+        super(dispatcher) {
+    for (int i = minId; i < maxId+1; i ++) {


cyanglaz · 2021-11-30T19:14:20Z

testing/scenario_app/lib/src/platform_view.dart

+  }
+
+  /// The platform view identifier to use for the first platform view.
+  final int minId;


cyanglaz · 2021-11-30T19:14:54Z

testing/scenario_app/lib/src/platform_view.dart

+  }
+
+  /// The platform view identifier to use for the first platform view.
+  final int minId;


cyanglaz · 2021-11-30T19:16:03Z

testing/scenario_app/lib/src/platform_view.dart

+  }
+
+  @override
+  void onDrawFrame() {


The only time I can repro this issue is scroll very fast.

cyanglaz · 2021-11-30T19:16:13Z

testing/scenario_app/lib/src/platform_view.dart

+
+  void _buildOneFrame(double offset) {
+    const double cellWidth = 1000;
+    print(offset);


cyanglaz · 2021-11-30T20:06:09Z

testing/scenario_app/lib/src/platform_view.dart

@@ -32,7 +32,7 @@ class PlatformViewScenario extends Scenario with _BasePlatformViewScenarioMixin
  /// Creates the PlatformView scenario.
  ///
  /// The [dispatcher] parameter must not be null.
-  PlatformViewScenario(PlatformDispatcher dispatcher, String text, { required this.id })
+  PlatformViewScenario(PlatformDispatcher dispatcher, String text, {required this.id})


My formatter just did some auto formatting, which I think is good.

cyanglaz · 2021-11-30T20:06:43Z

Updated per comments.

jmagman

LGTM

cyanglaz · 2021-12-02T19:18:17Z

@blasten PTAL :)

blasten · 2021-12-03T01:36:20Z

testing/scenario_app/ios/Scenarios/ScenariosUITests/PlatformViewUITests.m

+
+  XCUIElement* platformView = app.textViews.firstMatch;
+  BOOL exists = [platformView waitForExistenceWithTimeout:kSecondsToWaitForPlatformView];
+  if (!exists) {


can it also check for the zPosition since that seems to be the bug?

AFAIK, XCUITest doesn't have access to the actual layers. I also did a light research and couldn't find anything online. :(

Right, these integration tests are black box, you don't have access to the app layers, just the accessibility hooks. A verification like that would need to happen in a XCTest unit test.

right. If the bug is that the zPosition was miscalculated, then the test would need to ensure the correct values.

I did something similar with the UIView in https://github.com/flutter/engine/blob/104e21d941fc8aed1073c6caaae71cf944837256/testing/scenario_app/ios/Scenarios/ScenariosUITests/UnobstructedPlatformViewTests.m

Yes, XCUITest can access the frame (I believe it is the accessiblyElement's frame, not UIView's frame, they are the same value by default but could actually be different depending on the implementation). I believe accessiblyElement doesn't have any knowledge on the zIndex.

The current test we have will crash if the zIndex is incorrect because of the DCHECK that I added (only on debug_unopt build, which is the one we use on CI)

I agree a better test would involve directly examine the zIndex value. This will require an XCTest. We then need to create a fake where the RTree thinks there's a picture layer on top of platform views. I don't think there's a way to do so in the current API set up?

One way to do it might be refactor the PlatformViewsController's implementation, introduce a mockable API, so we can mock and force to create overlays.

I thought an incorrect z index will make the tests fail in https://github.com/flutter/engine/blob/104e21d941fc8aed1073c6caaae71cf944837256/testing/scenario_app/ios/Scenarios/ScenariosUITests/UnobstructedPlatformViewTests.m

If you don't set zPosition, are there any test failures?

Which test would fail? I might be missing something but the tests in https://github.com/flutter/engine/blob/104e21d941fc8aed1073c6caaae71cf944837256/testing/scenario_app/ios/Scenarios/ScenariosUITests/UnobstructedPlatformViewTests.m are not checking zPosition or anything related to it right?

There’s a comment about z index in that file. I remember the index being determined from the position of the layer in the tree I believe

blasten

LGTM

* 9b74c21 Run iOS scenario apps on iPhone 11 and iOS 14 (flutter/engine#30104) * 4108898 Add error handler for os.rmtree calls (flutter/engine#30169) * 55c5e56 Don't run curl in verbose mode when --verbose set (flutter/engine#30170) * a1fe531 Win32: Implement DispatchAccessibilityAction (flutter/engine#30187) * a7a8024 Update buildroot to db6d037a778930ad9 (ToT). (flutter/engine#30171) * 7ccf290 Revert "Run iOS scenario apps on iPhone 11 and iOS 14 (#30104)" (flutter/engine#30193) * 1313e73 Add unconditional waits on fml::Semaphore. (flutter/engine#30165) * 094ff9e Roll Fuchsia Mac SDK from EAlr46NQ8... to zMg5gNi2E... (flutter/engine#30190) * a572ab5 Roll Fuchsia Linux SDK from WGMjaVH60... to s03VQc7lX... (flutter/engine#30191) * 07c6f41 Roll Skia from f333f5614a9b to 06f3d68627c2 (24 revisions) (flutter/engine#30192) * 8796f0f Update Swiftshader to dc0f131930 (flutter/engine#30188) * c1e235d Add length guard to fix Windows build with VS2019 16.11.7 headers (flutter/engine#30189) * 7831529 Roll Skia from 06f3d68627c2 to 1c4cf27965bd (2 revisions) (flutter/engine#30194) * 5672f9e Document rationale for Dart VM flag prefix match (flutter/engine#30195) * 1c10e68 Roll buildroot to 430b57c643883e6090b5af09faddd8048efee57c. (flutter/engine#30197) * e32d04a Roll Skia from 1c4cf27965bd to 543b8681c7f2 (1 revision) (flutter/engine#30198) * 6981c04 [ios platform view] fix overlay zPosition (flutter/engine#29930) * efaccf0 renew cirrus key (flutter/engine#30186) * 9b80c08 Roll Skia from 543b8681c7f2 to 00edeefab7f4 (1 revision) (flutter/engine#30200) * 164045d Roll Fuchsia Mac SDK from zMg5gNi2E... to QeaP059wu... (flutter/engine#30203) * 83b84d3 Roll Skia from 00edeefab7f4 to 21b8ccb7393c (3 revisions) (flutter/engine#30206)

Chris Yang added 3 commits November 24, 2021 10:38

draft

daa40c7

add test

1a93d9f

cleanup

aa9c694

flutter-dashboard bot added the platform-ios label Nov 24, 2021

cyanglaz requested a review from blasten November 24, 2021 22:06

google-cla bot added the cla: yes label Nov 24, 2021

Chris Yang added 2 commits November 24, 2021 14:27

more cleanup

8dbe59f

starting index to 0

0b4efab

cyanglaz requested a review from jmagman November 29, 2021 17:54

blasten reviewed Nov 29, 2021

View reviewed changes

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm Show resolved Hide resolved

blasten reviewed Nov 29, 2021

View reviewed changes

jmagman reviewed Nov 29, 2021

View reviewed changes

Chris Yang added 2 commits November 30, 2021 12:00

review

202f55d

cleanup

d3e2358

cyanglaz commented Nov 30, 2021

View reviewed changes

cyanglaz requested review from jmagman and blasten November 30, 2021 20:06

cleanup

104e21d

jmagman approved these changes Dec 2, 2021

View reviewed changes

blasten reviewed Dec 3, 2021

View reviewed changes

cyanglaz requested a review from blasten December 6, 2021 18:04

blasten approved these changes Dec 7, 2021

View reviewed changes

cyanglaz added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Dec 8, 2021

fluttergithubbot merged commit 6981c04 into flutter:main Dec 8, 2021

engine-flutter-autoroll mentioned this pull request Dec 8, 2021

Roll Engine from 6551934b9c37 to 83b84d3336ec (21 revisions) flutter/flutter#94862

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 8, 2021

6981c04 [ios platform view] fix overlay zPosition (flutter/engine#29930)

8b56e4f

engine-flutter-autoroll mentioned this pull request Dec 8, 2021

Roll Engine from 6551934b9c37 to 83b84d3336ec (21 revisions) flutter/flutter#94868

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 8, 2021

6981c04 [ios platform view] fix overlay zPosition (flutter/engine#29930)

a48ae5e

cyanglaz deleted the platform_view_flash branch December 8, 2021 18:44

yx-mike added a commit to yx-mike/engine that referenced this pull request Jan 12, 2022

[ios platform view] fix overlay zPosition (flutter#29930)

82a663f

yx-mike added a commit to yx-mike/engine that referenced this pull request Jan 18, 2022

[ios platform view] fix overlay zPosition (flutter#29930)

20a3fd6

cyanglaz mentioned this pull request Feb 16, 2022

Buggy scroll in WebView inside of the PageView flutter/flutter#94028

Open

cyanglaz mentioned this pull request May 4, 2022

iOS: PlatformView pixel glitches when going underneath a widget (iPhone 13 pro) flutter/flutter#103076

Closed

timtangcoding mentioned this pull request Jan 21, 2023

iOS platform view is flashing on top of AppBar, SliverAppBar and SliverPersistentHeader widgets flutter/flutter#118877

Closed

jmagman mentioned this pull request Feb 16, 2023

[platform_view] fix overlapping platform view not touchable #39527

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ios platform view] fix overlay zPosition #29930

[ios platform view] fix overlay zPosition #29930

cyanglaz commented Nov 24, 2021

blasten Nov 29, 2021

blasten Nov 29, 2021

cyanglaz Nov 30, 2021

blasten Nov 29, 2021 •

edited

Loading

cyanglaz Nov 30, 2021

blasten Nov 29, 2021

blasten Nov 29, 2021

cyanglaz Nov 30, 2021

jmagman Nov 29, 2021

jmagman Nov 29, 2021

cyanglaz Nov 30, 2021

cyanglaz Nov 30, 2021

jmagman Nov 29, 2021

cyanglaz Nov 30, 2021

cyanglaz Nov 30, 2021

cyanglaz Nov 30, 2021

cyanglaz Nov 30, 2021

cyanglaz Nov 30, 2021

cyanglaz Nov 30, 2021

cyanglaz Nov 30, 2021

cyanglaz Nov 30, 2021

cyanglaz commented Nov 30, 2021

jmagman left a comment

cyanglaz commented Dec 2, 2021

blasten Dec 3, 2021

cyanglaz Dec 3, 2021

jmagman Dec 3, 2021

blasten Dec 6, 2021

cyanglaz Dec 6, 2021

blasten Dec 7, 2021

cyanglaz Dec 7, 2021

blasten Dec 7, 2021

blasten left a comment

		// Clear the `active_composition_order_`, which will be populated down below.
		active_composition_order_.clear();

	for (int i = minId; i < maxId+1; i ++) {
	for (int i = minId; i <= maxId; i++) {

[ios platform view] fix overlay zPosition #29930

[ios platform view] fix overlay zPosition #29930

Conversation

cyanglaz commented Nov 24, 2021

Pre-launch Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blasten Nov 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyanglaz commented Nov 30, 2021

jmagman left a comment

Choose a reason for hiding this comment

cyanglaz commented Dec 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blasten left a comment

Choose a reason for hiding this comment

blasten Nov 29, 2021 •

edited

Loading