Handle changing clip delegates (#5444)

Turns out that previously we weren't updating the clip if the layout
didn't change, even if the delegate was different.

packages/flutter/lib/src/rendering/proxy_box.dart

@@ -881,13 +881,11 @@ abstract class _RenderCustomClip<T> extends RenderProxyBox {
       return;
     CustomClipper<T> oldClipper = _clipper;
     _clipper = newClipper;
-    if (newClipper == null) {
+    assert(newClipper != null || oldClipper != null);
-      assert(oldClipper != null);
+    if (newClipper == null || oldClipper == null ||
-      markNeedsPaint();
-      markNeedsSemanticsUpdate(onlyChanges: true);
-    } else if (oldClipper == null ||
         oldClipper.runtimeType != oldClipper.runtimeType ||
         newClipper.shouldRepaint(oldClipper)) {
+      _clip = null;
       markNeedsPaint();
       markNeedsSemanticsUpdate(onlyChanges: true);
     }
@@ -898,12 +896,20 @@ abstract class _RenderCustomClip<T> extends RenderProxyBox {
 
   @override
   void performLayout() {
+    final Size oldSize = hasSize ? size : null;
     super.performLayout();
-    _clip = _clipper?.getClip(size) ?? _defaultClip;
+    if (oldSize != size)
+      _clip = null;
+  }
+
+  void _updateClip() {
+    _clip ??= _clipper?.getClip(size) ?? _defaultClip;
   }
 
   @override
-  Rect describeApproximatePaintClip(RenderObject child) => _clipper?.getApproximateClipRect(size) ?? Point.origin & size;
+  Rect describeApproximatePaintClip(RenderObject child) {
+    return _clipper?.getApproximateClipRect(size) ?? Point.origin & size;
+  }
 }
 
 /// Clips its child using a rectangle.
@@ -927,8 +933,8 @@ class RenderClipRect extends _RenderCustomClip<Rect> {
   @override
   bool hitTest(HitTestResult result, { Point position }) {
     if (_clipper != null) {
-      Rect clipRect = _clip;
+      assert(_clip != null);
-      if (!clipRect.contains(position))
+      if (!_clip.contains(position))
         return false;
     }
     return super.hitTest(result, position: position);
@@ -936,8 +942,10 @@ class RenderClipRect extends _RenderCustomClip<Rect> {
 
   @override
   void paint(PaintingContext context, Offset offset) {
-    if (child != null)
+    if (child != null) {
+      _updateClip();
       context.pushClipRect(needsCompositing, offset, _clip, super.paint);
+    }
   }
 }
 
@@ -974,6 +982,7 @@ class RenderClipRRect extends RenderProxyBox {
 
   // TODO(ianh): either convert this to the CustomClipper world, or
   // TODO(ianh): implement describeApproximatePaintClip for this class
+  // TODO(ianh): implement hit testing for this class
 
   @override
   void paint(PaintingContext context, Offset offset) {
@@ -1016,11 +1025,11 @@ class RenderClipOval extends _RenderCustomClip<Rect> {
 
   @override
   bool hitTest(HitTestResult result, { Point position }) {
-    Rect clipBounds = _clip;
+    assert(_clip != null);
-    Point center = clipBounds.center;
+    Point center = _clip.center;
     // convert the position to an offset from the center of the unit circle
-    Offset offset = new Offset((position.x - center.x) / clipBounds.width,
+    Offset offset = new Offset((position.x - center.x) / _clip.width,
-                               (position.y - center.y) / clipBounds.height);
+                               (position.y - center.y) / _clip.height);
     // check if the point is outside the unit circle
     if (offset.distanceSquared > 0.25) // x^2 + y^2 > r^2
       return false;
@@ -1030,8 +1039,8 @@ class RenderClipOval extends _RenderCustomClip<Rect> {
   @override
   void paint(PaintingContext context, Offset offset) {
     if (child != null) {
-      Rect clipBounds = _clip;
+      _updateClip();
-      context.pushClipPath(needsCompositing, offset, clipBounds, _getClipPath(clipBounds), super.paint);
+      context.pushClipPath(needsCompositing, offset, _clip, _getClipPath(_clip), super.paint);
     }
   }
 }
@@ -1064,15 +1073,20 @@ class RenderClipPath extends _RenderCustomClip<Path> {
 
   @override
   bool hitTest(HitTestResult result, { Point position }) {
-    if (_clip == null || !_clip.contains(position))
+    if (_clipper != null) {
-      return false;
+      assert(_clip != null);
+      if (!_clip.contains(position))
+        return false;
+    }
     return super.hitTest(result, position: position);
   }
 
   @override
   void paint(PaintingContext context, Offset offset) {
-    if (child != null)
+    if (child != null) {
+      _updateClip();
       context.pushClipPath(needsCompositing, offset, Point.origin & size, _clip, super.paint);
+    }
   }
 }
 

packages/flutter/test/widget/clip_test.dart

@@ -15,7 +15,25 @@ class PathClipper extends CustomClipper<Path> {
       ..addRect(new Rect.fromLTWH(50.0, 50.0, 100.0, 100.0));
   }
   @override
-  bool shouldRepaint(PathClipper oldWidget) => false;
+  bool shouldRepaint(PathClipper oldClipper) => false;
+}
+
+class ValueClipper<T> extends CustomClipper<T> {
+  ValueClipper(this.message, this.value);
+
+  final String message;
+  final T value;
+
+  @override
+  T getClip(Size size) {
+    log.add(message);
+    return value;
+  }
+
+  @override
+  bool shouldRepaint(ValueClipper<T> oldClipper) {
+    return oldClipper.message != message || oldClipper.value != value;
+  }
 }
 
 void main() {
@@ -26,7 +44,6 @@ void main() {
         child: new GestureDetector(
           behavior: HitTestBehavior.opaque,
           onTap: () { log.add('tap'); },
-          child: new Container()
         )
       )
     );
@@ -47,7 +64,6 @@ void main() {
         child: new GestureDetector(
           behavior: HitTestBehavior.opaque,
           onTap: () { log.add('tap'); },
-          child: new Container()
         )
       )
     );
@@ -61,4 +77,127 @@ void main() {
     expect(log, equals(<String>['tap']));
     log.clear();
   });
+
+  testWidgets('ClipRect', (WidgetTester tester) async {
+    await tester.pumpWidget(
+      new Align(
+        alignment: FractionalOffset.topLeft,
+        child: new SizedBox(
+          width: 100.0,
+          height: 100.0,
+          child: new ClipRect(
+            clipper: new ValueClipper<Rect>('a', new Rect.fromLTWH(5.0, 5.0, 10.0, 10.0)),
+            child: new GestureDetector(
+              behavior: HitTestBehavior.opaque,
+              onTap: () { log.add('tap'); },
+            )
+          )
+        )
+      )
+    );
+    expect(log, equals(<String>['a']));
+
+    await tester.tapAt(new Point(10.0, 10.0));
+    expect(log, equals(<String>['a', 'tap']));
+
+    await tester.tapAt(new Point(100.0, 100.0));
+    expect(log, equals(<String>['a', 'tap']));
+
+    await tester.pumpWidget(
+      new Align(
+        alignment: FractionalOffset.topLeft,
+        child: new SizedBox(
+          width: 100.0,
+          height: 100.0,
+          child: new ClipRect(
+            clipper: new ValueClipper<Rect>('a', new Rect.fromLTWH(5.0, 5.0, 10.0, 10.0)),
+            child: new GestureDetector(
+              behavior: HitTestBehavior.opaque,
+              onTap: () { log.add('tap'); },
+            )
+          )
+        )
+      )
+    );
+    expect(log, equals(<String>['a', 'tap']));
+
+    await tester.pumpWidget(
+      new Align(
+        alignment: FractionalOffset.topLeft,
+        child: new SizedBox(
+          width: 200.0,
+          height: 200.0,
+          child: new ClipRect(
+            clipper: new ValueClipper<Rect>('a', new Rect.fromLTWH(5.0, 5.0, 10.0, 10.0)),
+            child: new GestureDetector(
+              behavior: HitTestBehavior.opaque,
+              onTap: () { log.add('tap'); },
+            )
+          )
+        )
+      )
+    );
+    expect(log, equals(<String>['a', 'tap', 'a']));
+
+    await tester.pumpWidget(
+      new Align(
+        alignment: FractionalOffset.topLeft,
+        child: new SizedBox(
+          width: 200.0,
+          height: 200.0,
+          child: new ClipRect(
+            clipper: new ValueClipper<Rect>('a', new Rect.fromLTWH(5.0, 5.0, 10.0, 10.0)),
+            child: new GestureDetector(
+              behavior: HitTestBehavior.opaque,
+              onTap: () { log.add('tap'); },
+            )
+          )
+        )
+      )
+    );
+    expect(log, equals(<String>['a', 'tap', 'a']));
+
+    await tester.pumpWidget(
+      new Align(
+        alignment: FractionalOffset.topLeft,
+        child: new SizedBox(
+          width: 200.0,
+          height: 200.0,
+          child: new ClipRect(
+            clipper: new ValueClipper<Rect>('b', new Rect.fromLTWH(5.0, 5.0, 10.0, 10.0)),
+            child: new GestureDetector(
+              behavior: HitTestBehavior.opaque,
+              onTap: () { log.add('tap'); },
+            )
+          )
+        )
+      )
+    );
+    expect(log, equals(<String>['a', 'tap', 'a', 'b']));
+
+    await tester.pumpWidget(
+      new Align(
+        alignment: FractionalOffset.topLeft,
+        child: new SizedBox(
+          width: 200.0,
+          height: 200.0,
+          child: new ClipRect(
+            clipper: new ValueClipper<Rect>('c', new Rect.fromLTWH(25.0, 25.0, 10.0, 10.0)),
+            child: new GestureDetector(
+              behavior: HitTestBehavior.opaque,
+              onTap: () { log.add('tap'); },
+            )
+          )
+        )
+      )
+    );
+    expect(log, equals(<String>['a', 'tap', 'a', 'b', 'c']));
+
+    await tester.tapAt(new Point(30.0, 30.0));
+    expect(log, equals(<String>['a', 'tap', 'a', 'b', 'c', 'tap']));
+
+    await tester.tapAt(new Point(100.0, 100.0));
+    expect(log, equals(<String>['a', 'tap', 'a', 'b', 'c', 'tap']));
+  });
+
 }