Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

hybrid_android_views_integration_test depends on NOP rendering calls for correctness #130738

Closed
flar opened this issue Jul 17, 2023 · 2 comments · Fixed by #130751
Closed

hybrid_android_views_integration_test depends on NOP rendering calls for correctness #130738

flar opened this issue Jul 17, 2023 · 2 comments · Fixed by #130751

Comments

@flar
Copy link
Contributor

flar commented Jul 17, 2023

This test is blocking an engine PR that ignores rendering calls that don't render anything. The test expects a second FlutterImageView which only contains non-rendering operations. If we stop recording those operations then the second FlutterImageView will not be generated and this is treated as a bug by the test.

One way to demonstrate the failure without the changes to the engine is to remove the offending calls from the platform and try to run the test - it will fail in the absence of these non-rendering calls. (Note that the indicated diffs are not necessarily an optimal suggested patch for the Framework, but they just serve to eliminate the cases of non-rendering calls which are encountered during this one test and demonstrate its change in expectations.)

There are 3 possible solutions here:

  • Modify the test so that one or more of the indicated call sites below actually render something - thereby causing the operations to not be elided in the engine and the secondary FlutterImageView to be needed as per the current test expectations. (Does the test want the second View to be present in order to test its conditions properly?)
  • Modify the test to have new expectations and patch the Framework to eliminate these non-rendering calls even in the absence of the engine's PR to elide these calls at the native level
  • Modify the test to have new expectations and do a manual roll of the engine that elides the rendering operations along with the patch to the test so that it will expect the new results.
Framework patch to eliminate non-rendering calls
diff --git a/packages/flutter/lib/src/painting/box_border.dart b/packages/flutter/lib/src/painting/box_border.dart
index dfc9a606a5..954aa84671 100644
--- a/packages/flutter/lib/src/painting/box_border.dart
+++ b/packages/flutter/lib/src/painting/box_border.dart
@@ -188,6 +188,9 @@ abstract class BoxBorder extends ShapeBorder {
     // is always painted. There is no borderRadius parameter for
     // ShapeDecoration or Border, only for BoxDecoration, which doesn't call
     // this method.
+    if (paint.shader == null && paint.blendMode == BlendMode.srcOver && paint.color.alpha == 0) {
+      return;
+    }
     canvas.drawRect(rect, paint);
   }
 
diff --git a/packages/flutter/lib/src/rendering/proxy_box.dart b/packages/flutter/lib/src/rendering/proxy_box.dart
index a089fbe145..003385bbc7 100644
--- a/packages/flutter/lib/src/rendering/proxy_box.dart
+++ b/packages/flutter/lib/src/rendering/proxy_box.dart
@@ -2134,14 +2134,6 @@ class RenderPhysicalShape extends _RenderPhysicalModelBase<Path> {
 
     final Canvas canvas = context.canvas;
     if (elevation != 0.0 && paintShadows) {
-      // The drawShadow call doesn't add the region of the shadow to the
-      // picture's bounds, so we draw a hardcoded amount of extra space to
-      // account for the maximum potential area of the shadow.
-      // TODO(jsimmons): remove this when Skia does it for us.
-      canvas.drawRect(
-        offsetBounds.inflate(20.0),
-        _transparentPaint,
-      );
       canvas.drawShadow(
         offsetPath,
         shadowColor,
@@ -2151,10 +2143,12 @@ class RenderPhysicalShape extends _RenderPhysicalModelBase<Path> {
     }
     final bool usesSaveLayer = clipBehavior == Clip.antiAliasWithSaveLayer;
     if (!usesSaveLayer) {
-      canvas.drawPath(
-        offsetPath,
-        Paint()..color = color
-      );
+      if (color.alpha != 0) {
+        canvas.drawPath(
+            offsetPath,
+            Paint()..color = color
+        );
+      }
     }
     layer = context.pushClipPath(
       needsCompositing,
@flar
Copy link
Contributor Author

flar commented Jul 17, 2023

This issue is the root cause behind this revert of the engine's NOP culling mechanism: flutter/engine#43721

johnmccutchan pushed a commit that referenced this issue Jul 18, 2023
Fixes: #130738

A widget was added to explicitly and intentionally overlap the
PlatformView so that the rendering tree of Views would always have an
underlay and an overlay to match the test expectations.
LouiseHsu pushed a commit to LouiseHsu/flutter that referenced this issue Jul 31, 2023
…er#130751)

Fixes: flutter#130738

A widget was added to explicitly and intentionally overlap the
PlatformView so that the rendering tree of Views would always have an
underlay and an overlay to match the test expectations.
@github-actions
Copy link

github-actions bot commented Aug 1, 2023

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant