[platform_view]integration test for platform view not tappable issue #120844
Conversation
flutter-dashboard
bot
added
the
c: contributor-productivity
hellohuanlin
force-pushed
the
platform_view_z_order_xcuitest
branch
from
5978b7d to
0a7df2c
Compare
There was a problem hiding this comment.
let me update to be more clear.
There was a problem hiding this comment.
So we discovered that it is not a bug rather a correct behavior of the framework. So we should probably explain that behavior here.
There was a problem hiding this comment.
So it needs to be hittable for tap on the element to work?
There was a problem hiding this comment.
yeah otherwise it will just crash.
There was a problem hiding this comment.
Can this test be done with scenario app using void onPointerDataPacket(PointerDataPacket packet) ?
The idea is to schedule a new frame after receiving a touch gesture, then change the composition order of the platform views.
See: https://github.com/flutter/engine/blob/main/testing/scenario_app/lib/src/platform_view.dart#L992
You will also need to inject some indication in a11y parameters of the platform view so that the order change can be recognized by XCTest.
There was a problem hiding this comment.
You can add a semantics node when "showBackground is true", then in the XCTest, you can search for that node instead of delaying.
There was a problem hiding this comment.
The issue I had is that for some reason it fails to query the second platform view which is controlled by showBackground (Refer to https://github.com/flutter/flutter/pull/120844/files#diff-3e48a5714c1ea61112bebbd881876bea61cf86f065890eda9b019360d62cee97R85-R88)
Are you saying I should add a "semantics node" for the second platform view to work?
There was a problem hiding this comment.
Offline discussion, it is not possible as dialog seems to remove other semantics nodes from the tree. I couldn't think of another way better than using the delay in the test.
|
@cyanglaz my understanding is that scenario test is not a full integration test - it does not use flutter framework. So passing that scenario test does not give us enough confidence that this issue is fixed. |
Ah, are we also testing framework behavior? I couldn't find anything related to the framework to be tested for the fix. |
@cyanglaz We need the framework to verify that the platform view (UiKitView widget) is indeed tappable. |
|
@cyanglaz I did look into the scenario test yesterday. My understanding is (i'm new to this so I could be terribly wrong): For framework integration test (this PR), the framework directs engine to re-order subviews: On the other hand, for scenario test, a "FakeUI" layer mimics the framework behavior by sending fake draw commands for engine to re-order subviews: So my conclusion was: (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) (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. |
cyanglaz
left a comment
cyanglaz
left a comment
There was a problem hiding this comment.
LGTM % comments regarding why a delay is necessary in the test .
hellohuanlin
force-pushed
the
platform_view_z_order_xcuitest
branch
3 times, most recently
from
eb85f79 to
4a92cec
Compare
For some reason I kept getting Android failure. Let me rebase again. |
hellohuanlin
force-pushed
the
platform_view_z_order_xcuitest
branch
from
4a92cec to
ec474f9
Compare
|
plugin_lint_mac timed out on an arm machine, I think you hit #119750 |
hellohuanlin
added
the
autosubmit
|
It looks like this change is consistently failing the Can someone in this thread please take a look? I've added details and links to the logs here: #121381 |
4 participants
Note: This will need to wait for engine roll for flutter/engine#39527 to pass.
Add integration test for platform view becomes not tappable issue. This happens when we have 2 platform views stacking on each other. For example, one of the platform views is put in an alert dialog prompt, which covers a background platform view underneath, as demonstrated in this PR.
List which issues are fixed by this PR. You must list at least one issue.
#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
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.