From 674e74a963f604858232f0517734f9da3d709bb8 Mon Sep 17 00:00:00 2001
From: Denis Koroskin <dkoroskin@fb.com>
Date: Thu, 28 Jan 2016 18:02:40 -0800
Subject: [PATCH] Only clip node against its own boundaries IFF clipToBounds()
 is set to true (i.e. overflow is visible)

Summary:
Previously, every node would be clipped by its own boundaries. This made sense, because
a) RCTView cannot draw outside of its bounds, so clipping is harmless
b) all other node types were not allowed to control overflow behavior, and for them overflow was implicitly set to hidden, which means it should clip.

This diff changes the logic so that overflow can be set on any node (image, text, view etc).

The change has an important implication that make the diff a little trickier that it could be: AbstractDrawCommand clipping logic needs to be relaxed. Previously, clip bounds were *always* intersected with node bounds, which means that clip rect was always within node rect. This is no longer true, and thus shouldClip() logic needs to be modified.

Because of the above, RCTText no longer needs to artificially clip bounds against layout width/height.

Note: RCTImage is still not working with overflow: visible correctly (it still always clips). This is fixed in a follow up diff.

Reviewed By: ahmedre

Differential Revision: D2867849
---
 .../react/flat/AbstractDrawCommand.java       |  2 +-
 .../java/com/facebook/react/flat/RCTText.java | 12 ++-------
 .../com/facebook/react/flat/StateBuilder.java | 25 ++++++++-----------
 3 files changed, 13 insertions(+), 26 deletions(-)

diff --git a/ReactAndroid/src/main/java/com/facebook/react/flat/AbstractDrawCommand.java b/ReactAndroid/src/main/java/com/facebook/react/flat/AbstractDrawCommand.java
index 24961d92b..aca4723cf 100644
--- a/ReactAndroid/src/main/java/com/facebook/react/flat/AbstractDrawCommand.java
+++ b/ReactAndroid/src/main/java/com/facebook/react/flat/AbstractDrawCommand.java
@@ -140,7 +140,7 @@ import android.graphics.Canvas;
   protected abstract void onDraw(Canvas canvas);
 
   protected boolean shouldClip() {
-    return mLeft != mClipLeft || mTop != mClipTop || mRight != mClipRight || mBottom != mClipBottom;
+    return mLeft < mClipLeft || mTop < mClipTop || mRight > mClipRight || mBottom > mClipBottom;
   }
 
   protected void onBoundsChanged() {
diff --git a/ReactAndroid/src/main/java/com/facebook/react/flat/RCTText.java b/ReactAndroid/src/main/java/com/facebook/react/flat/RCTText.java
index 8d1f643a6..4b1eb6026 100644
--- a/ReactAndroid/src/main/java/com/facebook/react/flat/RCTText.java
+++ b/ReactAndroid/src/main/java/com/facebook/react/flat/RCTText.java
@@ -186,16 +186,8 @@ import com.facebook.react.uimanager.annotations.ReactProp;
     top += padding.get(Spacing.TOP);
 
     // these are actual right/bottom coordinates where this DrawCommand will draw.
-    float layoutRight = left + mDrawCommand.getLayoutWidth();
-    float layoutBottom = top + mDrawCommand.getLayoutHeight();
-
-    // We need to adjust right/bottom because Layout size is in many cases different than the
-    // current node's size. We could use layoutRight/layoutBottom instead of taking the minumum of
-    // (right,layoutRight) and that would work correctly, but it will also make AbstractDrawCommand
-    // clip when the clipping is not really necessary (because it doesn't like draw bounds smaller
-    // than clip bounds).
-    right = Math.max(right, layoutRight);
-    bottom = Math.max(bottom, layoutBottom);
+    right = left + mDrawCommand.getLayoutWidth();
+    bottom = top + mDrawCommand.getLayoutHeight();
 
     mDrawCommand = (DrawTextLayout) mDrawCommand.updateBoundsAndFreeze(
         left,
diff --git a/ReactAndroid/src/main/java/com/facebook/react/flat/StateBuilder.java b/ReactAndroid/src/main/java/com/facebook/react/flat/StateBuilder.java
index 15e7b3d49..f0622089d 100644
--- a/ReactAndroid/src/main/java/com/facebook/react/flat/StateBuilder.java
+++ b/ReactAndroid/src/main/java/com/facebook/react/flat/StateBuilder.java
@@ -347,10 +347,10 @@ import com.facebook.react.uimanager.events.EventDispatcher;
       float top,
       float right,
       float bottom,
-      float parentClipLeft,
-      float parentClipTop,
-      float parentClipRight,
-      float parentClipBottom,
+      float clipLeft,
+      float clipTop,
+      float clipRight,
+      float clipBottom,
       boolean isAndroidView,
       boolean needsCustomLayoutForChildren) {
     if (node.hasNewLayout()) {
@@ -368,20 +368,15 @@ import com.facebook.react.uimanager.events.EventDispatcher;
               Math.round(bottom - top)));
     }
 
-    float clipLeft = Math.max(left, parentClipLeft);
-    float clipTop = Math.max(top, parentClipTop);
-    float clipRight = Math.min(right, parentClipRight);
-    float clipBottom = Math.min(bottom, parentClipBottom);
+    if (node.clipToBounds()) {
+      clipLeft = Math.max(left, clipLeft);
+      clipTop = Math.max(top, clipTop);
+      clipRight = Math.min(right, clipRight);
+      clipBottom = Math.min(bottom, clipBottom);
+    }
 
     node.collectState(this, left, top, right, bottom, clipLeft, clipTop, clipRight, clipBottom);
 
-    if (!node.clipToBounds()) {
-      clipLeft = parentClipLeft;
-      clipTop = parentClipTop;
-      clipRight = parentClipRight;
-      clipBottom = parentClipBottom;
-    }
-
     for (int i = 0, childCount = node.getChildCount(); i != childCount; ++i) {
       FlatShadowNode child = (FlatShadowNode) node.getChildAt(i);
       if (child.isVirtual()) {