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.

Sign up for GitHub

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

Fixed android bounding box #25836

Closed
wants to merge 3 commits into from
Closed

Fixed android bounding box #25836

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c9cae37
Fixed android bounding box
floriancargoet Jun 17, 2019
2a6dee3
Applied changes requested by @mdvacca
floriancargoet Jul 28, 2019
1f441d0
added javadoc
floriancargoet Aug 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 53 additions & 6 deletions ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
package com.facebook.react.uimanager;

import android.content.res.Resources;
import android.graphics.Matrix;
import android.graphics.RectF;
import android.util.SparseArray;
import android.util.SparseBooleanArray;
import android.util.SparseIntArray;
Expand Down Expand Up @@ -74,6 +76,7 @@ public class NativeViewHierarchyManager {
private final LayoutAnimationController mLayoutAnimator = new LayoutAnimationController();
private final SparseArray<SparseIntArray> mTagsToPendingIndicesToDelete = new SparseArray<>();
private final int[] mDroppedViewArray = new int[100];
private final RectF mBoundingBox = new RectF();

private boolean mLayoutAnimationEnabled;
private PopupMenu mPopupMenu;
Expand Down Expand Up @@ -649,16 +652,60 @@ public synchronized void measure(int tag, int[] outputBuffer) {
if (rootView == null) {
throw new NoSuchNativeViewException("Native view " + tag + " is no longer on screen");
}
rootView.getLocationInWindow(outputBuffer);
computeBoundingBox(rootView, outputBuffer);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we could still use rootView.getLocationInWindow(outputBuffer), but I assume you used computeBoundingBox for consistency? We don't need the width and the height (only x and y) of the root view and it's not supposed to be transformed, so getLocationInWindow should still be sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes consistency and cancellation of the missing offsets in my version of mapRectFromViewToScreenCoords. Have you read my annotations on the original code in response to mdvacca? By using computeBoundingBox twice, offsets cancel each other and I don't need to port this part of the function (hard to do since everything is private).

    // I think the 2 following offsets would be the same for the measured view and 
    // the React root view. Since we subtract the root view position from the 
    // measured view position in measure(), these offsets are cancelled.
    // In short: (a + offsets) - (b + offsets) = a - b
    // 
    // class ViewRootImpl is hidden, I could not find a way to rewrite this code.
    // Since we don't need it for our purpose, I removed it.
    if (parent instanceof ViewRootImpl) {
        ViewRootImpl viewRootImpl = (ViewRootImpl) parent;
        rect.offset(0, -viewRootImpl.mCurScrollY);
    }
    rect.offset(mAttachInfo.mWindowLeft, mAttachInfo.mWindowTop);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the explanation. I think I got the idea now: if we call rootView.getLocationInWindow(outputBuffer) instead of computeBoundingBox(rootView, outputBuffer), getLocationInWindow internally will calculate the offset for the rootView:

if (parent instanceof ViewRootImpl) {
    ViewRootImpl viewRootImpl = (ViewRootImpl) parent;
    rect.offset(0, -viewRootImpl.mCurScrollY);
}

And if mCurScrollY != 0 for our root view, the bounding box rect of the child view will be missing this offset since we don't have access to the private API of ViewRootImpl to calculate it accordingly.

int rootX = outputBuffer[0];
int rootY = outputBuffer[1];
computeBoundingBox(v, outputBuffer);
outputBuffer[0] -= rootX;
outputBuffer[1] -= rootY;
}

v.getLocationInWindow(outputBuffer);
/**
* Fills the outputBuffer with the view's bounding box [x, y, width, height]
*
* @param v
* @param outputBuffer
*/
private void computeBoundingBox(View v, int[] outputBuffer) {
mBoundingBox.set(0, 0, v.getWidth(), v.getHeight());
mapRectFromViewToWindowCoords(v, mBoundingBox);

outputBuffer[0] = outputBuffer[0] - rootX;
outputBuffer[1] = outputBuffer[1] - rootY;
outputBuffer[2] = v.getWidth();
outputBuffer[3] = v.getHeight();
outputBuffer[0] = Math.round(mBoundingBox.left);
outputBuffer[1] = Math.round(mBoundingBox.top);
outputBuffer[2] = Math.round(mBoundingBox.right - mBoundingBox.left);
outputBuffer[3] = Math.round(mBoundingBox.bottom - mBoundingBox.top);
}

/**
* Map a rectangle from view-relative coordinates to root-view-relative coordinates.
* This is a simplified version of the hidden Android method View.mapRectFromViewToScreenCoords()
* @see {@link View#mapRectFromViewToScreenCoords}
*
* @param v The view the coordinates are relative to
* @param rect The rectangle to be mapped
*/
private void mapRectFromViewToWindowCoords(View v, RectF rect) {
Matrix m = v.getMatrix();
if (!m.isIdentity()) {
m.mapRect(rect);
}

rect.offset(v.getLeft(), v.getTop());

ViewParent parent = v.getParent();
while (parent instanceof View) {
View parentView = (View) parent;

rect.offset(-parentView.getScrollX(), -parentView.getScrollY());
m = parentView.getMatrix();
if (!m.isIdentity()) {
m.mapRect(rect);
}

rect.offset(parentView.getLeft(), parentView.getTop());

parent = parentView.getParent();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you describe why you have simplified the method View.mapRectFromViewToScreenCoords()? and why each of those simplifications will not create a regression?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've commented the original Android code, explaining each change.
Most of the changes are about replacing private access with a public method.
The most important comment is the last one.

public void mapRectFromViewToScreenCoords(RectF rect, boolean clipToParent) {
    // hasIdentityMatrix is not public, instead I used getMatrix().isIdentity()
    if (!hasIdentityMatrix()) {
        getMatrix().mapRect(rect);
    }
    // mLeft & mTop are not public, instead I used getLeft() & getTop()
    rect.offset(mLeft, mTop);

    ViewParent parent = mParent;
    while (parent instanceof View) {
        View parentView = (View) parent;
        
        // mScrollX & mScrollY are not public, instead I used getScrollX() & getScrollY()
        rect.offset(-parentView.mScrollX, -parentView.mScrollY);
        
        // We don't need clipping to measure, so I removed this block entirely
        if (clipToParent) {
            rect.left = Math.max(rect.left, 0);
            rect.top = Math.max(rect.top, 0);
            rect.right = Math.min(rect.right, parentView.getWidth());
            rect.bottom = Math.min(rect.bottom, parentView.getHeight());
        }

        // hasIdentityMatrix is not public, instead I used getMatrix().isIdentity()
        if (!parentView.hasIdentityMatrix()) {
            parentView.getMatrix().mapRect(rect);
        }

        // mLeft & mTop are not public, instead I used getLeft() & getTop()
        rect.offset(parentView.mLeft, parentView.mTop);

        // mParent is not public, instead I used getParent()
        parent = parentView.mParent;
    }
    // I think the 2 following offsets would be the same for the measured view and 
    // the React root view. Since we subtract the root view position from the 
    // measured view position in measure(), these offsets are cancelled.
    // In short: (a + offsets) - (b + offsets) = a - b
    // 
    // class ViewRootImpl is hidden, I could not find a way to rewrite this code.
    // Since we don't need it for our purpose, I removed it.
    if (parent instanceof ViewRootImpl) {
        ViewRootImpl viewRootImpl = (ViewRootImpl) parent;
        rect.offset(0, -viewRootImpl.mCurScrollY);
    }
    rect.offset(mAttachInfo.mWindowLeft, mAttachInfo.mWindowTop);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We definitely don't need rect.offset(mAttachInfo.mWindowLeft, mAttachInfo.mWindowTop); here since it actually offsets the coordinates relative to the screen coordinates, which isn't what we want (we want the coordinates relative to the window coordinates).

}

/**
Expand Down