Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[ios platform view] fix overlay zPosition #29930

Merged
fluttergithubbot merged 8 commits into from
Dec 8, 2021
Merged

[ios platform view] fix overlay zPosition #29930

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
daa40c7
draft
Nov 23, 2021
1a93d9f
add test
Nov 24, 2021
aa9c694
cleanup
Nov 24, 2021
8dbe59f
more cleanup
Nov 24, 2021
0b4efab
starting index to 0
Nov 29, 2021
202f55d
review
Nov 30, 2021
d3e2358
cleanup
Nov 30, 2021
104e21d
cleanup
Nov 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -575,15 +575,19 @@

if (platform_view_root.superview != flutter_view) {
[flutter_view addSubview:platform_view_root];
} else {
platform_view_root.layer.zPosition = zIndex++;
}
// Make sure the platform_view_root is higher than the last platform_view_root in
// composition_order_.
platform_view_root.layer.zPosition = zIndex++;

for (const std::shared_ptr<FlutterPlatformViewLayer>& layer : layers) {
if ([layer->overlay_view_wrapper superview] != flutter_view) {
if ([layer->overlay_view_wrapper.get() superview] != flutter_view) {
[flutter_view addSubview:layer->overlay_view_wrapper];
} else {
layer->overlay_view_wrapper.get().layer.zPosition = zIndex++;
}
// Make sure all the overlays are higher than the platform view.
layer->overlay_view_wrapper.get().layer.zPosition = zIndex++;
FML_DCHECK(layer->overlay_view_wrapper.get().layer.zPosition >
platform_view_root.layer.zPosition);
}
active_composition_order_.push_back(platform_view_id);
}
Expand Down
1 change: 1 addition & 0 deletions testing/scenario_app/ios/Scenarios/Scenarios/AppDelegate.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ - (BOOL)application:(UIApplication*)application
@"--bogus-font-text" : @"bogus_font_text",
@"--spawn-engine-works" : @"spawn_engine_works",
@"--pointer-events" : @"pointer_events",
@"--platform-view-scrolling-under-widget" : @"platform_view_scrolling_under_widget"
};
__block NSString* flutterViewControllerTestName = nil;
[launchArgsMap
Expand Down
37 changes: 37 additions & 0 deletions testing/scenario_app/ios/Scenarios/ScenariosUITests/PlatformViewUITests.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,3 +257,40 @@ - (void)testPlatformViewWithContinuousTexture {
}

@end

@interface PlatformViewScrollingUnderWidget : XCTestCase

@end

@implementation PlatformViewScrollingUnderWidget

- (void)setUp {
[super setUp];
self.continueAfterFailure = NO;
}

- (void)testPlatformViewScrollingUnderWidget {
XCUIApplication* app = [[XCUIApplication alloc] init];
app.launchArguments =
@[ @"--platform-view-scrolling-under-widget", @"--with-continuous-texture" ];
[app launch];

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

@blasten blasten Dec 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@cyanglaz cyanglaz Dec 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@jmagman jmagman Dec 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@blasten blasten Dec 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@cyanglaz cyanglaz Dec 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@blasten blasten Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@cyanglaz cyanglaz Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

@blasten blasten Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

XCTFail(@"It took longer than %@ second to find the platform view."
@"There might be issues with the platform view's construction,"
@"or with how the scenario is built.",
@(kSecondsToWaitForPlatformView));
}

// Wait and let the scenario app scroll a bit.
XCTWaiterResult waitResult = [XCTWaiter
waitForExpectations:@[ [[XCTestExpectation alloc] initWithDescription:@"Wait for 5 seconds"] ]
timeout:5];
// If the waiter is not interrupted, we know the app is in a valid state after timeout, thus the
// test passes.
XCTAssert(waitResult != XCTWaiterResultInterrupted);
}

@end
Loading