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

Round SkRects in ClipRectContainsPlatformViewBoundingRect #50387

Closed
wants to merge 2 commits into from
Closed

Round SkRects in ClipRectContainsPlatformViewBoundingRect #50387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a2cc737
Round SkRects in ClipRectContainsPlatformViewBoundingRect
jmagman Feb 2, 2024
dc07ab6
roundOut
jmagman Feb 16, 2024
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
5 changes: 4 additions & 1 deletion 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 @@ -45,7 +45,10 @@ static bool ClipRectContainsPlatformViewBoundingRect(const SkRect& clip_rect,
const SkRect& platformview_boundingrect,
const SkMatrix& transform_matrix) {
SkRect transformed_rect = transform_matrix.mapRect(clip_rect);
return transformed_rect.contains(platformview_boundingrect);

// Tolerate floating point errors when comparing flow clipping view bounds
// and quartz platform view bounds. Round platform view to integers.
return transformed_rect.roundOut().contains(platformview_boundingrect);
}

// Determines if the `clipRRect` from a clipRRect mutator contains the
Expand Down
17 changes: 14 additions & 3 deletions shell/platform/darwin/ios/framework/Source/FlutterPlatformViewsTest.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1507,6 +1507,17 @@ - (void)testChildClippingViewShouldBeTheBoundingRectOfPlatformView {
SkMatrix finalMatrix;
finalMatrix.setConcat(screenScaleMatrix, rotateMatrix);

// The childclippingview's frame is set based on flow, but the platform view's frame is set based
// on quartz. Although they should be the same, we should tolerate small floating point
// errors.
// Push clip rects the same size as the platform view but faking a floating point error.
SkRect clipRectSmaller =
SkRect::MakeXYWH(0, 0, 300 - kFloatCompareEpsilon, 300 - kFloatCompareEpsilon);
stack.PushClipRect(clipRectSmaller);
SkRect clipRectLarger =
SkRect::MakeXYWH(0, 0, 300 + kFloatCompareEpsilon, 300 + kFloatCompareEpsilon);
stack.PushClipRect(clipRectLarger);

auto embeddedViewParams =
std::make_unique<flutter::EmbeddedViewParams>(finalMatrix, SkSize::Make(300, 300), stack);

Expand All @@ -1516,9 +1527,7 @@ - (void)testChildClippingViewShouldBeTheBoundingRectOfPlatformView {
toView:mockFlutterView];
XCTAssertTrue([gMockPlatformView.superview.superview isKindOfClass:ChildClippingView.class]);
ChildClippingView* childClippingView = (ChildClippingView*)gMockPlatformView.superview.superview;
// The childclippingview's frame is set based on flow, but the platform view's frame is set based
// on quartz. Although they should be the same, but we should tolerate small floating point
// errors.
// See above comment about tolerating small floating point errors.
XCTAssertLessThan(fabs(platformViewRectInFlutterView.origin.x - childClippingView.frame.origin.x),
kFloatCompareEpsilon);
XCTAssertLessThan(fabs(platformViewRectInFlutterView.origin.y - childClippingView.frame.origin.y),
Expand All @@ -1529,6 +1538,8 @@ - (void)testChildClippingViewShouldBeTheBoundingRectOfPlatformView {
XCTAssertLessThan(
fabs(platformViewRectInFlutterView.size.height - childClippingView.frame.size.height),
kFloatCompareEpsilon);

XCTAssertNil(childClippingView.maskView);
Copy link
Member Author

Choose a reason for hiding this comment

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

This check fails on master but passes on this PR.

}

- (void)testClipsDoNotInterceptWithPlatformViewShouldNotAddMaskView {
Expand Down
4 changes: 0 additions & 4 deletions testing/ios/IosUnitTests/IosUnitTests.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@
0AC232F724BA71D300A85907 /* FlutterEngineTest.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = FlutterEngineTest.mm; sourceTree = "<group>"; };
0AC2330324BA71D300A85907 /* accessibility_bridge_test.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = accessibility_bridge_test.mm; sourceTree = "<group>"; };
0AC2330B24BA71D300A85907 /* FlutterTextInputPluginTest.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = FlutterTextInputPluginTest.mm; sourceTree = "<group>"; };
0AC2330F24BA71D300A85907 /* FlutterBinaryMessengerRelayTest.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = FlutterBinaryMessengerRelayTest.mm; sourceTree = "<group>"; };
Copy link
Member Author

Choose a reason for hiding this comment

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

FlutterBinaryMessengerRelayTest is no longer in the iOS directory and is tested in common #44395

0AC2331024BA71D300A85907 /* connection_collection_test.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = connection_collection_test.mm; sourceTree = "<group>"; };
0AC2331224BA71D300A85907 /* FlutterEnginePlatformViewTest.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = FlutterEnginePlatformViewTest.mm; sourceTree = "<group>"; };
0AC2331924BA71D300A85907 /* FlutterPluginAppLifeCycleDelegateTest.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = FlutterPluginAppLifeCycleDelegateTest.mm; sourceTree = "<group>"; };
Expand All @@ -72,7 +71,6 @@
3DD7D38C27D2B81000DA365C /* FlutterUndoManagerPluginTest.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = FlutterUndoManagerPluginTest.mm; sourceTree = "<group>"; };
689EC1E2281B30D3008FEB58 /* FlutterSpellCheckPluginTest.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = FlutterSpellCheckPluginTest.mm; sourceTree = "<group>"; };
68B6091227F62F990036AC78 /* VsyncWaiterIosTest.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = VsyncWaiterIosTest.mm; sourceTree = "<group>"; };
73F12C22288F92FF00AFC3A6 /* FlutterViewControllerTest_mrc.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = FlutterViewControllerTest_mrc.mm; sourceTree = "<group>"; };
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed in #48162

D2D361A52B234EAC0018964E /* FlutterMetalLayerTest.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = FlutterMetalLayerTest.mm; sourceTree = "<group>"; };
F7521D7226BB671E005F15C5 /* libios_test_flutter.dylib */ = {isa = PBXFileReference; lastKnownFileType = "compiled.mach-o.dylib"; name = libios_test_flutter.dylib; path = "../../../../out/$(FLUTTER_ENGINE)/libios_test_flutter.dylib"; sourceTree = "<group>"; };
F7521D7526BB673E005F15C5 /* libocmock_shared.dylib */ = {isa = PBXFileReference; lastKnownFileType = "compiled.mach-o.dylib"; name = libocmock_shared.dylib; path = "../../../../out/$(FLUTTER_ENGINE)/libocmock_shared.dylib"; sourceTree = "<group>"; };
Expand Down Expand Up @@ -109,12 +107,10 @@
0AC232F724BA71D300A85907 /* FlutterEngineTest.mm */,
0AC2330324BA71D300A85907 /* accessibility_bridge_test.mm */,
0AC2330B24BA71D300A85907 /* FlutterTextInputPluginTest.mm */,
0AC2330F24BA71D300A85907 /* FlutterBinaryMessengerRelayTest.mm */,
0AC2331024BA71D300A85907 /* connection_collection_test.mm */,
0AC2331224BA71D300A85907 /* FlutterEnginePlatformViewTest.mm */,
0AC2331924BA71D300A85907 /* FlutterPluginAppLifeCycleDelegateTest.mm */,
0AC2332124BA71D300A85907 /* FlutterViewControllerTest.mm */,
73F12C22288F92FF00AFC3A6 /* FlutterViewControllerTest_mrc.mm */,
D2D361A52B234EAC0018964E /* FlutterMetalLayerTest.mm */,
);
name = Source;
Expand Down