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 1 commit
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
46 changes: 40 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,47 @@ 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);
void computeBoundingBox(View v, int[] outputBuffer) {
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 reduce visibility to private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I guess this comment is valid for the two added methods, not just this one.

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] = (int) mBoundingBox.left;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use Math.round() instead of (int)?
View.mapRectFromViewToScreenCoords uses Math.round() and using (int) will change behavior.

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 can't see a Math.round() in View.mapRectFromViewToScreenCoords but I've updated the PR.

outputBuffer[1] = (int) mBoundingBox.top;
outputBuffer[2] = (int) (mBoundingBox.right - mBoundingBox.left);
outputBuffer[3] = (int) (mBoundingBox.bottom - mBoundingBox.top);
}

// simplified version of the hidden Android method View.mapRectFromViewToScreenCoords()
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you replace this comment with the proper Javadoc (perhaps borrow it from View.mapRectFromViewToScreenCoords) and add the @see annotation referring to that method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@makovkastar Done.
However the @link in the @see annotation cannot resolve the hidden method. Is that an issue?
image

Copy link
Contributor

Choose a reason for hiding this comment

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

The Javadoc is actually misleading since the method doesn't map coordinates relative to the root view, but rather to the window. Nevermind, I just realised that this method is private and the name is actually quite self-descriptive, so I will remove it and merge the PR now.

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