Fix #16464: Pass hit test when a big child is inside RenderFittedBox

[blasten]

I took a look at issue #16464 and found that RenderProxyBoxMixin already overrides hitTestChildren. However, when RenderBox.hitTest is called, the _size.contains(position) seems to be false. Thus, causing the entire test to return false.

[Hixie]

The superclass already calls hitTestChildren. The fix here is for this method to override hitTestChildren rather than overriding hitTest, that way the adjusted position isn't compared to the size, it's only adjusted just before we actually pass it to the child.

[blasten]

Thanks for reviewing the code! I got the following test failures:

(I probably missed something)

1. packages/flutter/test/material/text_field_test.dart: - Can drag handles to change selection
2. packages/flutter/test/widgets/composited_transform_test.dart: - Composited transforms - hit testing
3. packages/flutter/test/widgets/composited_transform_test.dart: - Composited transforms - hit testing

--- a/packages/flutter/lib/src/rendering/proxy_box.dart
+++ b/packages/flutter/lib/src/rendering/proxy_box.dart
@@ -2036,7 +2036,7 @@ class RenderTransform extends RenderProxyBox {
   }

   @override
-  bool hitTest(HitTestResult result, { Offset position }) {
+  bool hitTestChildren(HitTestResult result, { Offset position }) {
     if (transformHitTests) {
       Matrix4 inverse;
       try {
@@ -2048,7 +2048,7 @@ class RenderTransform extends RenderProxyBox {
       }
       position = MatrixUtils.transformPoint(inverse, position);
     }
-    return hitTestChildren(result, position: position) ||super.hitTest(result, position: position);
+    return child?.hitTest(result, position: position) ?? false;
   }

   @override
@@ -2219,7 +2219,7 @@ class RenderFittedBox extends RenderProxyBox {
   }

   @override
-  bool hitTest(HitTestResult result, { Offset position }) {
+  bool hitTestChildren(HitTestResult result, { Offset position }) {
     if (size.isEmpty)
       return false;
     _updatePaintData();
@@ -2232,7 +2232,7 @@ class RenderFittedBox extends RenderProxyBox {
       return false;
     }
     position = MatrixUtils.transformPoint(inverse, position);
-    return hitTestChildren(result, position: position) || super.hitTest(result, position: position);
+    return child?.hitTest(result, position: position) ?? false;
   }

   @override
@@ -2298,7 +2298,7 @@ class RenderFractionalTranslation extends RenderProxyBox {
   bool transformHitTests;

   @override
-  bool hitTest(HitTestResult result, { Offset position }) {
+  bool hitTestChildren(HitTestResult result, { Offset position }) {
     assert(!debugNeedsLayout);
     if (transformHitTests) {
       position = new Offset(
@@ -2306,7 +2306,7 @@ class RenderFractionalTranslation extends RenderProxyBox {
         position.dy - translation.dy * size.height,
       );
     }
-    return hitTestChildren(result, position: position) || super.hitTest(result, position: position);
+    return child?.hitTest(result, position: position) ?? false;
   }

   @override
@@ -4029,7 +4029,7 @@ class RenderFollowerLayer extends RenderProxyBox {
   }

   @override
-  bool hitTest(HitTestResult result, { Offset position }) {
+  bool hitTestChildren(HitTestResult result, { Offset position }) {
     Matrix4 inverse;
     try {
       inverse = new Matrix4.inverted(getCurrentTransform());
@@ -4039,7 +4039,7 @@ class RenderFollowerLayer extends RenderProxyBox {
       return false;
     }
     position = MatrixUtils.transformPoint(inverse, position);
-    return hitTestChildren(result, position: position) || super.hitTest(result, position: position);
+    return child?.hitTest(result, position: position) ?? false;
   }

   @override

[Hixie]

We should just be able to call the superclass hitTestChildren, no need to call child.hitTest. The idea is just to adjust the position for the purposes of the child.

I'm not sure about the test failures, we'll have to examine them and see if they are errors in the tests (quite possible; I'm surprised that we've never run into this bug before), or if they indicate that I'm wrong about what the fix should be.

[blasten]

Thanks for the explanation. I can confirm that calling super.hitTestChildren does cause the same tests to fail. I'm new to Flutter, but I'd try to gather more information :)

[Hixie]

Sorry for the delay.

Can you update your PR to do the fix as described above? I'll have a look at the test failures.

[blasten]

No worries. Done. I can add the remaining unit tests once #16553 is merged.

[Hixie]

Cool. The constructors are landed.

[Hixie]

we'll also need to test the fix for the other 3 classes.
(The constructors in #16553 might help with the Transform tests.)

[Hixie]

Yeah those are some weird test failures. I'll have to examine them in more detail next week.

[Hixie]

These test failures unfortunately are real bugs in the framework. I'll work on fixes for them.

[Hixie]

Turns out the problems are really subtle, and are caused by how it's really unintuitive to think about how RenderTransform does hit testing. Technically, with the fix applied here, what happens is that the hit testing has to hit the location of the Transform widget, which then transforms the point and then requires that you hit the child. That's fine in theory but in practice the child will often be offset and so the two boxes won't overlap.

To make this API more intuitive, we should IMHO just have the RenderTransform and RenderFollowerLayer objects skip trying to check if you are hit testing on them, and just have them defer entirely to the child (after transforming the point).

I think the solution is therefore to add the following to this patch for RenderTransform:

  @override
  bool hitTest(HitTestResult result, { Offset position }) {
    // RenderTransform objects don't check if they are
    // themselves hit, because it's confusing to think about
    // how the untransformed size and the child's transformed
    // position interact.
    return hitTestChildren(result, position: position);
  }

...and the following for RenderFollowerLayer:

  @override
  bool hitTest(HitTestResult result, { Offset position }) {
    // RenderFollowerLayer objects don't check if they are
    // themselves hit, because it's confusing to think about
    // how the untransformed size and the child's transformed
    // position interact.
    return hitTestChildren(result, position: position);
  }

(This is in addition to what you have here already.)

The RenderFittedBox class doesn't need this, because it's much more intuitive to think about how it works.

[Hixie]

@blasten Will you be able to continue working on this? It's totally fine if not; we can take it over (though it might take longer). I'm just asking because I'm trying to clean out our review queue.

[blasten]

@Hixie Yup. I will work on this one this week :)

[Hixie]

This is looking great.

[Hixie]

@blasten Do you intend to add any more tests?

[blasten]

@Hixie This test seems legit to me, but it's failing if we override RenderFractionalTranslation.hitTestChildren instead of RenderFractionalTranslation.hitTest. Does this seem suspicious?

[Hixie]

We probably want the same hack as for RenderTransform, where we skip the usual hitTest logic.

What's going on is that the key1 box is entirely outside (offset by 100% of the height and width) of the FractionalTranslation widget. So when we do the hit testing for the FractionalTranslation, we find that the hit is not at all inside that widget, and we therefore fail the hit test, without even checking with the child.

[blasten]

I see. That should be fixed now. I also added two more tests for entirely/partially inside the bounding box.

For RenderFollowerLayer, it appears to be tested here:

flutter/packages/flutter/test/widgets/composited_transform_test.dart

Line 140 in c46347b

testWidgets('Composited transforms - hit testing', (WidgetTester tester) async {

[Hixie]

Cool.

[Hixie]

Thanks! Please feel free to squash and merge when ready! :-)