[platform_view] fix overlapping platform view not touchable #39527

hellohuanlin · 2023-02-09T23:30:06Z

Note: Added integration test here, but needs engine roll of this PR in order to pass that test.

It turns out that the rendering Z order was correct, but the touch behavior is incorrect. So instead of setting the zPosition, we actually change subview order in the view hierarchy, which works for both z order and touch behavior.

Added a unit test for this particular issue. The existing scenario/screenshot tests should already be sufficient.

List which issues are fixed by this PR. You must list at least one issue.

flutter/flutter#118366

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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.
[x ] 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.

flutter-dashboard · 2023-02-09T23:30:09Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

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

loic-sharma · 2023-02-10T19:48:28Z

@hellohuanlin Would you mind putting slightly more information in the pull request title even if it is a work in progress? This makes it easier to manage our inboxes 😄

hellohuanlin · 2023-02-10T20:12:28Z

@loic-sharma oops. sorry i was not expecting this to trigger any email notifications yet. it just came from an offline discussion. Let me make it a draft.

flutter-dashboard · 2023-02-10T20:16:25Z

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

hellohuanlin · 2023-02-16T00:41:47Z

testing/ios/IosUnitTests/Tests/FlutterEngineConfig.xcconfig

@@ -1,2 +1,3 @@
 FLUTTER_ENGINE[arch=x86_64]=ios_debug_sim_unopt
 FLUTTER_ENGINE[arch=arm64]=ios_debug_sim_unopt_arm64
+FLUTTER_ENGINE=ios_debug_sim_unopt


@jmagman Not sure if you want to keep this or fix it in a separate PR?

Bleh may as well keep it, my attempt to do the right thing based on architecture was totally thwarted by Xcode passing in ARCH=unknown at certain build steps.

jmagman · 2023-02-16T02:08:13Z

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

-    // composition_order_.
-    platform_view_root.layer.zPosition = zIndex++;
-
+    // Simply having a higher zPosition is not enough since it does not affect touch behavior.


I don't think this comment about zindex makes sense without also looking at the previous code, since now the current code has no reference to a zindex.
I think you can just remove this line, the point is the next sentence: it will be reordered if it's already added.

jmagman · 2023-02-16T02:10:52Z

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

-      layer->overlay_view_wrapper.get().layer.zPosition = zIndex++;
-      FML_DCHECK(layer->overlay_view_wrapper.get().layer.zPosition >
-                 platform_view_root.layer.zPosition);
+      [flutter_view addSubview:layer->overlay_view_wrapper];


Can you manually test that this change didn't regress flutter/flutter#86787 (see #29930)

I can confirm that it didn't regress flutter/flutter#86787.

Though I found a different regression (see pic below). The navigation bar becomes transparent, so the platform view is displayed underneath. This seems to be similar to flutter/flutter#119485.

It is NOT caused by this PR because I get the same behaviors with or without this change; but it is a regression, because the released build is working fine. I will comment in that issue about this finding.

jmagman · 2023-02-16T02:11:55Z

testing/ios/IosUnitTests/Tests/FlutterEngineConfig.xcconfig

@@ -1,2 +1,3 @@
 FLUTTER_ENGINE[arch=x86_64]=ios_debug_sim_unopt
 FLUTTER_ENGINE[arch=arm64]=ios_debug_sim_unopt_arm64
+FLUTTER_ENGINE=ios_debug_sim_unopt


Bleh may as well keep it, my attempt to do the right thing based on architecture was totally thwarted by Xcode passing in ARCH=unknown at certain build steps.

cyanglaz

~~LGTM~~

Edit: I think we might be able to move the test here in the engine: flutter/flutter#120844 (review)

Thoughts?

hellohuanlin · 2023-02-16T20:39:17Z

@cyanglaz Bring the discussion back in this PR since the other PR is stamped. Let me know what you think about flutter/flutter#120844 (comment)

cyanglaz · 2023-02-16T20:46:54Z

The framework integration test does not add additional verification of the engine behavior. Since we know the issue was due to re-ordering the platform views and the bug is in the engine. The scenario test is sufficient to test against this bug.
The framework integration test is still valuable as I commented

(1) In an ideal world, these 2 tests verify the same engine behavior; however, in practice I'd argue that scenario tests will not give us as much confidence about engine behavior, since we could made mistakes mimicking the FakeUI behavior. (e.g. engine may pass the test, but under a wrong FakeUI command)

The FakeUI command only needs to create the necessary composition to reproduce the bug, we don't need to match the framework exactly.

(2) Another drawback with scenario test is that framework is not part of the integration. it doesn't give any confidence that the UiKitView is indeed tappable, which is the very purpose of this bugfix.

The PlatformView can listen to the tap event, right?

hellohuanlin · 2023-02-16T21:24:50Z

The PlatformView can listen to the tap event, right?

@cyanglaz Not sure if i follow this. Are you talking about dart side? I think I am able to create 2 frames and reorder platform views using addPlatformView (like this), but may need some help with the tapping part 😆

cyanglaz · 2023-02-16T23:21:38Z

Yes, here is one example: https://github.com/flutter/engine/blob/main/testing/scenario_app/lib/src/platform_view.dart#L992-L1017.

When you send acceptGesture, the platform view will receive the gesture.

hellohuanlin · 2023-02-17T17:52:11Z

@cyanglaz just looked into onPointerDataPacket and it makes sense. Will add the scenario test here.

jmagman · 2023-02-21T19:14:27Z

testing/scenario_app/lib/src/platform_view.dart

+  @override
+  void onDrawFrame() {
+    _frameCount += 1;
+    // TODO: (hellohuanlin) Need further investigation - the first 2 frames are dropped for some reason.


info - scenario_app/lib/src/platform_view.dart:1093:5 - To-do comment doesn't follow the Flutter style. Try following the Flutter style for to-do comments. - flutter_style_todos

cyanglaz

LGTM! Thanks for looking into the scenario test.

…flutter/engine#39527)

…121286) * 0fd15eece Started using bgr10_xr for opaque surfaces. (flutter/engine#39727) * f0e5c08f7 [platform_view] fix overlapping platform view not touchable (flutter/engine#39527) * 03b5998b4 Roll Fuchsia Mac SDK from 25mb67G0L_sZ_OQCC... to dj-2XkYHvZUsqlZTc... (flutter/engine#39782) * 2e5b1bf04 Updated the wide-gamut constant and added a unit test for it. (flutter/engine#39776) * 1ce93ca0a Roll Dart SDK from 0db7573c6b70 to c9a4fd7fd7fa (1 revision) (flutter/engine#39783) * 8368fb5b6 Roll Dart SDK from c9a4fd7fd7fa to 5003b952affb (1 revision) (flutter/engine#39787) * ee5ea205b Roll Dart SDK from 5003b952affb to 6f5df63655ad (1 revision) (flutter/engine#39790) * d9be356a8 Roll Fuchsia Mac SDK from dj-2XkYHvZUsqlZTc... to cGa3bk2BmrHL_waB8... (flutter/engine#39791) * 50af61302 [web] Add `dart:js_interop` to BUILD.gn. (flutter/engine#39792) * eb5e5624d [common] Use FML macro to prevent copy/assignment (flutter/engine#39786) * 6acfddc78 Add more log when receiving semantics node not part of update (flutter/engine#39777) * 336e7ce63 Roll Skia from 22b594afb4d3 to 78bad646fe78 (24 revisions) (flutter/engine#39793) * e65391f8b [Impeller] Add root dockspace to playgrounds (flutter/engine#39789) * 3848f6429 [Impeller] ensure glyph type contributes to FontGlyphPair hash/eq (flutter/engine#39794) * cb1ef6f08 Roll Skia from 78bad646fe78 to 335cabcf8b99 (2 revisions) (flutter/engine#39795) * ac6129db2 made opaque wide gamut images take up 32 bits per pixel (flutter/engine#39691) * c74a4bc67 save/restore around clipping the (potentially cached) frame canvas (flutter/engine#39797) * ed3f15d94 [Embedder API] Add CODEOWNERS (flutter/engine#39784) * 1a3bddcc1 Fix a race in ShellTest.PushBackdropFilterToVisitedPlatformViews (flutter/engine#39798) * 137caa761 Roll Dart SDK from 6f5df63655ad to 602941be8007 (2 revisions) (flutter/engine#39800)

flutter-dashboard bot added the platform-ios label Feb 9, 2023

flutter-dashboard bot added the needs tests label Feb 9, 2023

hellohuanlin commented Feb 10, 2023

View reviewed changes

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

hellohuanlin marked this pull request as draft February 10, 2023 20:12

hellohuanlin force-pushed the platform_view_z_index branch from 0877fde to 4a6b7ac Compare February 15, 2023 19:04

hellohuanlin changed the title ~~wip~~ [platform_view] fix overlapping platform view not touchable Feb 15, 2023

hellohuanlin force-pushed the platform_view_z_index branch from 4a6b7ac to e328874 Compare February 15, 2023 19:21

hellohuanlin mentioned this pull request Feb 16, 2023

[platform_view]integration test for platform view not tappable issue flutter/flutter#120844

Merged

8 tasks

hellohuanlin force-pushed the platform_view_z_index branch 3 times, most recently from 9862a1d to 3207d2a Compare February 16, 2023 00:26

hellohuanlin marked this pull request as ready for review February 16, 2023 00:41

hellohuanlin commented Feb 16, 2023

View reviewed changes

hellohuanlin requested review from cyanglaz and jmagman February 16, 2023 00:41

hellohuanlin removed the needs tests label Feb 16, 2023

jmagman reviewed Feb 16, 2023

View reviewed changes

cyanglaz approved these changes Feb 16, 2023

View reviewed changes

cyanglaz self-requested a review February 16, 2023 19:34

jmagman reviewed Feb 21, 2023

View reviewed changes

cyanglaz approved these changes Feb 21, 2023

View reviewed changes

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 22, 2023

f0e5c08f7 [platform_view] fix overlapping platform view not touchable (…

69380a1

…flutter/engine#39527)

engine-flutter-autoroll mentioned this pull request Feb 22, 2023

Roll Flutter Engine from 3aa112c75ccb to 50af61302d2e (9 revisions) flutter/flutter#121254

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 22, 2023

f0e5c08f7 [platform_view] fix overlapping platform view not touchable (…

c1ffcef

…flutter/engine#39527)

engine-flutter-autoroll mentioned this pull request Feb 22, 2023

Roll Flutter Engine from 3aa112c75ccb to eb5e5624deda (10 revisions) flutter/flutter#121255

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 22, 2023

f0e5c08f7 [platform_view] fix overlapping platform view not touchable (…

f3ea38b

…flutter/engine#39527)

engine-flutter-autoroll mentioned this pull request Feb 22, 2023

Roll Flutter Engine from 3aa112c75ccb to 6acfddc7837e (11 revisions) flutter/flutter#121260

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 22, 2023

f0e5c08f7 [platform_view] fix overlapping platform view not touchable (…

93ddfc1

…flutter/engine#39527)

engine-flutter-autoroll mentioned this pull request Feb 22, 2023

Roll Flutter Engine from 3aa112c75ccb to 336e7ce633cd (12 revisions) flutter/flutter#121263

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 22, 2023

f0e5c08f7 [platform_view] fix overlapping platform view not touchable (…

306b4d4

…flutter/engine#39527)

engine-flutter-autoroll mentioned this pull request Feb 22, 2023

Roll Flutter Engine from 3aa112c75ccb to e65391f8b5a2 (13 revisions) flutter/flutter#121266

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 22, 2023

f0e5c08f7 [platform_view] fix overlapping platform view not touchable (…

a17b88d

…flutter/engine#39527)

engine-flutter-autoroll mentioned this pull request Feb 22, 2023

Roll Flutter Engine from 3aa112c75ccb to 3848f64296ff (14 revisions) flutter/flutter#121267

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 22, 2023

f0e5c08f7 [platform_view] fix overlapping platform view not touchable (…

80707b3

…flutter/engine#39527)

engine-flutter-autoroll mentioned this pull request Feb 22, 2023

Roll Flutter Engine from 3aa112c75ccb to cb1ef6f08a28 (15 revisions) flutter/flutter#121274

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 22, 2023

f0e5c08f7 [platform_view] fix overlapping platform view not touchable (…

28e198c

…flutter/engine#39527)

engine-flutter-autoroll mentioned this pull request Feb 22, 2023

Roll Flutter Engine from 3aa112c75ccb to ac6129db2248 (16 revisions) flutter/flutter#121279

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 22, 2023

f0e5c08f7 [platform_view] fix overlapping platform view not touchable (…

a754a54

…flutter/engine#39527)

engine-flutter-autoroll mentioned this pull request Feb 22, 2023

Roll Flutter Engine from 3aa112c75ccb to c74a4bc674a3 (17 revisions) flutter/flutter#121280

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 22, 2023

f0e5c08f7 [platform_view] fix overlapping platform view not touchable (…

dad5d57

…flutter/engine#39527)

engine-flutter-autoroll mentioned this pull request Feb 22, 2023

Roll Flutter Engine from 3aa112c75ccb to ed3f15d94340 (18 revisions) flutter/flutter#121283

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 22, 2023

f0e5c08f7 [platform_view] fix overlapping platform view not touchable (…

a4e31e7

…flutter/engine#39527)

engine-flutter-autoroll mentioned this pull request Feb 22, 2023

Roll Flutter Engine from 3aa112c75ccb to 1a3bddcc1f63 (19 revisions) flutter/flutter#121284

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 22, 2023

f0e5c08f7 [platform_view] fix overlapping platform view not touchable (…

f0e5da6

…flutter/engine#39527)

engine-flutter-autoroll mentioned this pull request Feb 22, 2023

Roll Flutter Engine from 3aa112c75ccb to 137caa761540 (20 revisions) flutter/flutter#121286

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 22, 2023

f0e5c08f7 [platform_view] fix overlapping platform view not touchable (…

55178ab

…flutter/engine#39527)

jmagman linked an issue Mar 1, 2023 that may be closed by this pull request

A uikitview is below B uikitview, but A uikitview is refreshed, so that B uikitview cannot respond to the click event flutter/flutter#118366

Closed

jason-simmons mentioned this pull request Mar 2, 2023

Regression in platform_views_scroll_perf_non_intersecting_impeller_ios__timeline_summary average CPU/GPU usage flutter/flutter#121833

Closed

This was referenced Mar 6, 2023

[platform_view]fix regression for addSubview when re-ordering #40091

Merged

Regression in Impeller benchmarks flutter/flutter#122473

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[platform_view] fix overlapping platform view not touchable #39527

[platform_view] fix overlapping platform view not touchable #39527

hellohuanlin commented Feb 9, 2023 •

edited

flutter-dashboard bot commented Feb 9, 2023

loic-sharma commented Feb 10, 2023

hellohuanlin commented Feb 10, 2023

flutter-dashboard bot commented Feb 10, 2023

hellohuanlin Feb 16, 2023

jmagman Feb 16, 2023

jmagman Feb 16, 2023

jmagman Feb 16, 2023

hellohuanlin Feb 21, 2023

jmagman Feb 16, 2023

cyanglaz left a comment •

edited

hellohuanlin commented Feb 16, 2023 •

edited

cyanglaz commented Feb 16, 2023

hellohuanlin commented Feb 16, 2023 •

edited

cyanglaz commented Feb 16, 2023

hellohuanlin commented Feb 17, 2023

jmagman Feb 21, 2023

cyanglaz left a comment

[platform_view] fix overlapping platform view not touchable #39527

[platform_view] fix overlapping platform view not touchable #39527

Conversation

hellohuanlin commented Feb 9, 2023 • edited

Pre-launch Checklist

flutter-dashboard bot commented Feb 9, 2023

loic-sharma commented Feb 10, 2023

hellohuanlin commented Feb 10, 2023

flutter-dashboard bot commented Feb 10, 2023

hellohuanlin Feb 16, 2023

Choose a reason for hiding this comment

jmagman Feb 16, 2023

Choose a reason for hiding this comment

jmagman Feb 16, 2023

Choose a reason for hiding this comment

jmagman Feb 16, 2023

Choose a reason for hiding this comment

hellohuanlin Feb 21, 2023

Choose a reason for hiding this comment

jmagman Feb 16, 2023

Choose a reason for hiding this comment

cyanglaz left a comment • edited

Choose a reason for hiding this comment

hellohuanlin commented Feb 16, 2023 • edited

cyanglaz commented Feb 16, 2023

hellohuanlin commented Feb 16, 2023 • edited

cyanglaz commented Feb 16, 2023

hellohuanlin commented Feb 17, 2023

jmagman Feb 21, 2023

Choose a reason for hiding this comment

cyanglaz left a comment

Choose a reason for hiding this comment

hellohuanlin commented Feb 9, 2023 •

edited

cyanglaz left a comment •

edited

hellohuanlin commented Feb 16, 2023 •

edited

hellohuanlin commented Feb 16, 2023 •

edited