Add warning message when PlatformView's origin is not 0 #35501

cyanglaz · 2022-08-18T17:47:23Z

Add a DCheck and warning message when PlatformView's origin is not 0.
This PR also updates the PlatformView in scenario app to have a none-zero origin.

First step of: flutter/flutter#109700

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.

draft 2 fix tests

cyanglaz · 2022-08-18T17:48:13Z

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

  XCTAssertTrue([platform_view waitForExistenceWithTimeout:1.0]);
-  XCTAssertEqual(platform_view.frame.origin.x, 25);
-  XCTAssertEqual(platform_view.frame.origin.y, 25);
+  XCTAssertEqual(platform_view.frame.origin.x, 0);


Since the frame of the platform view changed, there are some changes in the frame of the platform views and overlays in this test.

cyanglaz · 2022-08-18T17:49:07Z

testing/scenario_app/lib/src/platform_view.dart

    finishBuilder(
      builder,
-      overlayOffset: const Offset(150, 250),
+      overlayOffset: const Offset(150, 240),


Because the platform view's frame changed, the old offset won't create an overlay. To make the test valid again, i had to update the offset so that the widget is partially cover the platformview and an overlay is created.

I think this is an iOS only test so no android tests are effected by this change.

dnfield · 2022-08-18T19:25:35Z

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

+  if (!CGPointEqualToPoint([touchInterceptor embeddedView].frame.origin, CGPointZero)) {
+    FML_LOG(WARNING)
+        << "Embedded PlatformView's origin is not CGPointZero."
+           "A none-zero origin might cause undefined behavior."


non-zero (and below).

Do we want them to file a new issue or to add a comment to the existing issue?

How does an application developer fix their code if they see this warning?

We should at least log the view_id, which hopefully is enough for them to figure out which view it is - if it's possible to tell them something about the type that would be good too (e.g. if an application has multiple platform views).

Do we want them to file a new issue or to add a comment to the existing issue?

This is a good point, ill instead ask them to comment on the issue.

How does an application developer fix their code if they see this warning?

There is probably nothing they can do. It would be the same behavior they have been had. They will just have to ignore this error message. (It can be annoying seeing this every frame tho, I can add some code to only print this once. I'm not sure how to test this tho)

We should at least log the view_id, which hopefully is enough for them to figure out which view it is - if it's possible to tell them something about the type that would be good too (e.g. if an application has multiple platform views).

Good point, will do.

How about we ifdef this out of release and profile builds, and just keep a set of view_ids - if the set contains the ID don't print the message, otherwise insert it and print.

We should also make sure that this log level gets printed from the command line in an opt build without specifying --verbose-system-logs.

dnfield · 2022-08-18T19:26:44Z

testing/scenario_app/ios/Scenarios/Scenarios/TextPlatformView.m

              binaryMessenger:(NSObject<FlutterBinaryMessenger>*)messenger {
  if ([super init]) {
-    _textView = [[UITextView alloc] initWithFrame:CGRectMake(50.0, 50.0, 250.0, 100.0)];
+    _textView = [[UITextView alloc] initWithFrame:CGRectMake(0.0, 0.0, 250.0, 100.0)];


I know you asked offline, but I'm pretty sure I did this as a way to arbitrarily position the text within the view.

This seems like a valid use case to me and I'm not really sure why we need to restrict it. I think that if we pursue this,we should make sure the warning lands for at least a full stable cycle and we collect feedback on that before changing it.

For example, in testing/scenario_app/ios/Scenarios/ScenariosUITests/golden_platform_view_clippath_iPhone 8_13.0_simulator.png the text is no longer as visible.

The PlatformView developer should not determine where the PlatformView is displayed, the frame of the PlatformView should be determined by the user of the PlatformView. So the widget tree should be the source of truth where the PlatformView widget is displayed.

PlatformView developer can determine where the subviews of PlatformView is displayed, that is allowed. In this case, they will create the PlatformView with 0 origin and create a subview inside the PlatformView to position it arbitrarily. However, this can create an issue where in theory -- not sure what is a valid use case -- PlatformView developer can create a PlatformView that has a subview displaying way outside of the PlatformView widget's frame. It messed up the unobstructed platform view logic, which we used to determine the frame of overlays.
And I'm not sure how we can prevent it unless we loop through all the subviews of the PlatformView.

So I do think it is valid to restrict this, at least to give a warning. Because this change is harmless, we can wait for some time to gather feedbacks and decided if we should revert this or add the hard restriction.

For example, in testing/scenario_app/ios/Scenarios/ScenariosUITests/golden_platform_view_clippath_iPhone 8_13.0_simulator.png the text is no longer as visible.

~One thing we can do is to have a plain UIView as the PlatformView and having the textView to be a child of the UIView, where the origin of textView is (50, 50), so the text are still visible in those screenshots.

Makes sense.

We may want to print this kind of explanation in the warning.

....alternatively, should we just automatically wrap non-zero origin in a UI view with a zero origin?

....alternatively, should we just automatically wrap non-zero origin in a UI view with a zero origin?

Yeah we can do that when we know there is no valid case of having a non-zero origin. However, it doesn't solve the problem when there is a subview of the PlatformView displaying outside of the frame. Maybe clipToBounds is a better solution.

We may want to print this kind of explanation in the warning.

This is explained in flutter/flutter#109700, i think having a link and ask people to comment on the issue is probably sufficient.

....alternatively, should we just automatically wrap non-zero origin in a UI view with a zero origin?

Yeah we can do this, this can change the behavior of the platform view so we will wait for feedbacks before implement it.

cyanglaz · 2022-08-18T20:10:38Z

If we don't receive any valid use case of displaying PlatformView outside of the frame, a potential fix is to have clipToBounds to be true.

jmagman · 2022-08-18T21:33:32Z

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

+        << "Embedded PlatformView's origin is not CGPointZero."
+           "A none-zero origin might cause undefined behavior."
+           "If you are the author of the PlatformView and you have a valid case of using a "
+           "none-zero origin,"


Dan already said this, but the typo is also here.

Suggested change

"none-zero origin,"

"non-zero origin,"

cyanglaz · 2022-08-19T18:27:50Z

Error message looks like:

A Embedded PlatformView's origin is not CGPointZero.
  View id: 0
  View info: 
 <UIView: 0x7f8e6b422240; frame = (50 50; 0 0); clipsToBounds = YES; autoresize = W+H; gestureRecognizers = <NSArray: 0x600000525170>; layer = <CALayer: 0x600000be8320>> 
A non-zero origin might cause undefined behavior.
See https://github.com/flutter/flutter/issues/109700 for more details.
If you are the author of the PlatformView, please update the implementation of the PlatformView to have a (0, 0) origin.
If you have a valid case of using a non-zero origin, please leave a comment at https://github.com/flutter/flutter/issues/109700 with details.

cyanglaz

@dnfield @jmagman Updated as per reviews. PTAL

dnfield · 2022-08-25T20:17:51Z

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

-  UIView* touchInterceptor = touch_interceptors_[view_id].get();
+  FlutterTouchInterceptingView* touchInterceptor = touch_interceptors_[view_id].get();
+#if FLUTTER_RUNTIME_MODE == FLUTTER_RUNTIME_MODE_DEBUG
+  FML_CHECK(CGPointEqualToPoint([touchInterceptor embeddedView].frame.origin, CGPointZero));


Please make this a dcheck.

This will cause the application to crash in debug mode for end developers.

(as opposed to a crash/test failure for unopt builds by engine developers)

dnfield

Once the comment about the FML_CHECK is addressed this LGTM.

auto-submit · 2022-08-27T05:05:20Z

The status or check suite Linux Android AOT Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

auto-submit · 2022-08-29T15:35:56Z

The status or check suite Linux Android AOT Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

…utter/engine#35501)

In #35501, handling was added to log a debug message to the console in the case where a platform view with a non-zero origin was identified. Unfortunately: * In unopt builds, the first thing we do in that block is to call FML_DCHECK asserting that the origin is zero, so we never actually emit the log statement. * In opt builds, FML_DCHECK is a no-op, so users are unlikely to actually ever notice the crash. The proper fix is to eliminate this restriction, but in the meantime, this eliminates this block entirely and leaves the TODO. We've had only two comments on that bug in the 2.5 years since it was added. Issue: flutter/flutter#109700 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

…ne#56796) In flutter/engine#35501, handling was added to log a debug message to the console in the case where a platform view with a non-zero origin was identified. Unfortunately: * In unopt builds, the first thing we do in that block is to call FML_DCHECK asserting that the origin is zero, so we never actually emit the log statement. * In opt builds, FML_DCHECK is a no-op, so users are unlikely to actually ever notice the crash. The proper fix is to eliminate this restriction, but in the meantime, this eliminates this block entirely and leaves the TODO. We've had only two comments on that bug in the 2.5 years since it was added. Issue: flutter#109700 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

Chris Yang added 2 commits August 18, 2022 10:43

draft 1

d1486f9

draft 2 fix tests

cleanup

0f2ed5f

flutter-dashboard bot added the platform-ios label Aug 18, 2022

cyanglaz requested a review from dnfield August 18, 2022 17:47

cyanglaz marked this pull request as ready for review August 18, 2022 17:47

cyanglaz commented Aug 18, 2022

View reviewed changes

cyanglaz requested a review from jmagman August 18, 2022 17:52

update opacity golden

88b824f

dnfield reviewed Aug 18, 2022

View reviewed changes

jmagman reviewed Aug 18, 2022

View reviewed changes

Chris Yang added 2 commits August 18, 2022 16:05

draft

74d2e5c

draft

7373766

Chris Yang added 4 commits August 19, 2022 11:30

review

845ad98

reivew

bffbb25

try fix test

612bbcf

fix test

92f4895

cyanglaz commented Aug 22, 2022

View reviewed changes

dnfield reviewed Aug 25, 2022

View reviewed changes

dnfield approved these changes Aug 25, 2022

View reviewed changes

DCHECK

8d33a22

cyanglaz mentioned this pull request Aug 25, 2022

PlatformView Blur for Backdrop Filter #34596

Merged

8 tasks

jmagman approved these changes Aug 26, 2022

View reviewed changes

cyanglaz added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 26, 2022

retrigger ci

f1389e0

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 27, 2022

cyanglaz added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 29, 2022

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 29, 2022

Merge branch 'main' into platform_view_zero_origin

9e6319f

cyanglaz added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 29, 2022

auto-submit bot merged commit a46b721 into flutter:main Aug 29, 2022

cyanglaz deleted the platform_view_zero_origin branch August 29, 2022 20:24

engine-flutter-autoroll mentioned this pull request Aug 29, 2022

Roll Flutter Engine from 52e213ebbd97 to f7561a23fde2 (10 revisions) flutter/flutter#110544

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 29, 2022

a46b72126 Add warning message when PlatformView's origin is not 0 (fl…

e971bf0

…utter/engine#35501)

haroross pushed a commit to haroross/flutter_engine that referenced this pull request Aug 30, 2022

Add warning message when PlatformView's origin is not 0 (flutter#35501)

f752a61

cbracken mentioned this pull request Nov 25, 2024

iOS: Eliminate logging of non-zero origin platformviews #56796

Merged

8 tasks

Add warning message when PlatformView's origin is not 0 #35501

Add warning message when PlatformView's origin is not 0 #35501

Uh oh!

Conversation

cyanglaz commented Aug 18, 2022

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyanglaz Aug 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyanglaz Aug 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyanglaz commented Aug 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyanglaz commented Aug 19, 2022

Uh oh!

cyanglaz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Aug 27, 2022

Uh oh!

auto-submit bot commented Aug 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cyanglaz Aug 18, 2022 •

edited

Loading

cyanglaz Aug 18, 2022 •

edited

Loading

cyanglaz commented Aug 18, 2022 •

edited

Loading