From 7d52672a31997ddf9f8d8f3dac9b236bbdd558e7 Mon Sep 17 00:00:00 2001
From: Adam Barth <abarth@chromium.org>
Date: Wed, 12 Aug 2015 16:37:56 -0700
Subject: [PATCH] RenderBox.hitTest should do the bounds check

Previously we were doing the bounds testing for hit tests in the parent, but
that doesn't work if the child paints at a location other than 0.0, 0.0. Now we
do the bounds check in the child. This also simplifies Scaffold's hit testing.

Fixes #558
---
 sky/packages/sky/lib/rendering/box.dart    | 39 ++++++++++------------
 sky/packages/sky/lib/widgets/scaffold.dart | 28 ++--------------
 2 files changed, 20 insertions(+), 47 deletions(-)

diff --git a/sky/packages/sky/lib/rendering/box.dart b/sky/packages/sky/lib/rendering/box.dart
index b9f729b3db9..c1068927461 100644
--- a/sky/packages/sky/lib/rendering/box.dart
+++ b/sky/packages/sky/lib/rendering/box.dart
@@ -414,9 +414,13 @@ abstract class RenderBox extends RenderObject {
   }
 
   bool hitTest(HitTestResult result, { Point position }) {
-    hitTestChildren(result, position: position);
-    result.add(new BoxHitTestEntry(this, position));
-    return true;
+    if (position.x >= 0.0 && position.x < _size.width &&
+        position.y >= 0.0 && position.y < _size.height) {
+      hitTestChildren(result, position: position);
+      result.add(new BoxHitTestEntry(this, position));
+      return true;
+    }
+    return false;
   }
   void hitTestChildren(HitTestResult result, { Point position }) { }
 
@@ -1008,11 +1012,8 @@ abstract class RenderShiftedBox extends RenderBox with RenderObjectWithChildMixi
   void hitTestChildren(HitTestResult result, { Point position }) {
     if (child != null) {
       assert(child.parentData is BoxParentData);
-      Rect childBounds = child.parentData.position & child.size;
-      if (childBounds.contains(position)) {
-        child.hitTest(result, position: new Point(position.x - child.parentData.position.x,
-                                                      position.y - child.parentData.position.y));
-      }
+      child.hitTest(result, position: new Point(position.x - child.parentData.position.x,
+                                                position.y - child.parentData.position.y));
     }
   }
 
@@ -1319,10 +1320,8 @@ class RenderViewport extends RenderBox with RenderObjectWithChildMixin<RenderBox
   void hitTestChildren(HitTestResult result, { Point position }) {
     if (child != null) {
       assert(child.parentData is BoxParentData);
-      Rect childBounds = child.parentData.position & child.size;
-      Point transformedPosition = position + _scrollOffsetRoundedToIntegerDevicePixels;
-      if (childBounds.contains(transformedPosition))
-        child.hitTest(result, position: transformedPosition);
+      Point transformed = position + _scrollOffsetRoundedToIntegerDevicePixels;
+      child.hitTest(result, position: transformed);
     }
   }
 }
@@ -1578,15 +1577,15 @@ class RenderTransform extends RenderProxyBox {
     markNeedsPaint();
   }
 
-  void hitTestChildren(HitTestResult result, { Point position }) {
+  bool hitTest(HitTestResult result, { Point position }) {
     Matrix4 inverse = new Matrix4.zero();
-    /* double det = */ inverse.copyInverse(_transform);
     // TODO(abarth): Check the determinant for degeneracy.
+    inverse.copyInverse(_transform);
 
     Vector3 position3 = new Vector3(position.x, position.y, 0.0);
     Vector3 transformed3 = inverse.transform3(position3);
     Point transformed = new Point(transformed3.x, transformed3.y);
-    super.hitTestChildren(result, position: transformed);
+    return super.hitTest(result, position: transformed);
   }
 
   void paint(PaintingCanvas canvas, Offset offset) {
@@ -1810,12 +1809,10 @@ abstract class RenderBoxContainerDefaultsMixin<ChildType extends RenderBox, Pare
     ChildType child = lastChild;
     while (child != null) {
       assert(child.parentData is ParentDataType);
-      Rect childBounds = child.parentData.position & child.size;
-      if (childBounds.contains(position)) {
-        if (child.hitTest(result, position: new Point(position.x - child.parentData.position.x,
-                                                          position.y - child.parentData.position.y)))
-          break;
-      }
+      Point transformed = new Point(position.x - child.parentData.position.x,
+                                    position.y - child.parentData.position.y);
+      if (child.hitTest(result, position: transformed))
+        break;
       child = child.parentData.previousSibling;
     }
   }
diff --git a/sky/packages/sky/lib/widgets/scaffold.dart b/sky/packages/sky/lib/widgets/scaffold.dart
index 835e5891f9d..79ab95dfc19 100644
--- a/sky/packages/sky/lib/widgets/scaffold.dart
+++ b/sky/packages/sky/lib/widgets/scaffold.dart
@@ -9,8 +9,6 @@ import 'package:sky/rendering/object.dart';
 import 'package:sky/theme/view_configuration.dart';
 import 'package:sky/widgets/framework.dart';
 
-import 'package:vector_math/vector_math.dart';
-
 // Slots are painted in this order and hit tested in reverse of this order
 enum ScaffoldSlots {
   body,
@@ -153,35 +151,13 @@ class RenderScaffold extends RenderBox {
     }
   }
 
-  static Point _transformPoint(Matrix4 transform, Point point) {
-    Vector3 position3 = new Vector3(point.x, point.y, 0.0);
-    Vector3 transformed3 = transform.transform3(position3);
-    return new Point(transformed3.x, transformed3.y);
-  }
-
-  Point parentToLocal(RenderBox box, Point point) {
-    assert(attached);
-    Matrix4 transform = new Matrix4.identity();
-    box.applyPaintTransform(transform);
-    /* double det = */ transform.invert();
-    // TODO(abarth): Check the determinant for degeneracy.
-    return _transformPoint(transform, point);
-  }
-
   void hitTestChildren(HitTestResult result, { Point position }) {
     for (ScaffoldSlots slot in ScaffoldSlots.values.reversed) {
       RenderBox box = _slots[slot];
       if (box != null) {
         assert(box.parentData is BoxParentData);
-        // TODO(abarth): Need to solve this problem in general.
-        // Apply the box's transform to check if it contains position.
-        // But when we pass the position to box.hitTest, we only want to apply
-        // the top-level transform (box will apply its own transforms).
-        Point local = parentToLocal(box, position);
-        if ((Point.origin & box.size).contains(local)) {
-          if (box.hitTest(result, position: (position - box.parentData.position).toPoint()))
-            return;
-        }
+        if (box.hitTest(result, position: (position - box.parentData.position).toPoint()))
+          return;
       }
     }
   }