Fixed android bounding box

[floriancargoet]

Summary

This PR fixes #19637.

Summary of the issue: on Android, transformed touchables have press issues because the touchable's measurements are incorrect in the release phase. UIManager.measure() returns an incorrect size and position as explained here

This is easily seen with the inspector :

Screenshot of a { scale: 2 } transform
The real view (scaled) is in pink, the incorrect touchable area is in blue.

Screenshot of a { rotateZ: "-45deg" } transform
The real view (rotated) is in pink, the incorrect touchable area is in blue.

Changelog

[Android] [Fixed] - Fix UIManager.measure()

Test Plan

Android now produces the same results as iOS as seen on these screenshots

Android without fix	Android with fix	iOS

[osdnk]

Thanks for this contribution! Are you aware that on this blue area on iOS does not mean that this area is touchable? Please consider using touchables button in an inspector while attaching screenshot :)
This is how we virtualise touchable areas on iOS:

So clickable area is limited to bounds on component. Can you confirm that your code works like this?

[floriancargoet]

Yes, I know the blue rectangle represents the bounds, but it's the bounds that are used to detect the LEAVE_PRESS_RECT event.

1. You press in the touchable area, pressIn is triggered.
2. You move a bit. Since the bounds are incorrect, a LEAVE_PRESS_RECT is triggered.
3. You release your touch. Since you are considered out of the "press rect", pressOut in NOT triggered.

You can see that the bounds are used for step 2 :

• _remeasureMetricsOnActivation() is called
• UIManager.measure() is called, the results are store in positionOnActivate and dimensionsOnActivate which are used to compute isTouchWithinActive and to trigger LEAVE_PRESS_RECT.

Does this clarify things?

Sorry if this wasn't included in the PR description but when I investigated this issue a year ago the Touchable.js code was very different and I haven't a good understanding of this new code.

[mdvacca]

Thanks for working on this PR @floriancargoet! I added some comments on the review.

@yungsters has more context in Touchable.js, @yungsters can you take a look into this PR?

[mdvacca]

can you reduce visibility to private?

[floriancargoet]

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

[mdvacca]

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

[floriancargoet]

I can't see a Math.round() in View.mapRectFromViewToScreenCoords but I've updated the PR.

[mdvacca]

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

[floriancargoet]

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);
}

[makovkastar]

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).

[facebook-github-bot]

@makovkastar has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[makovkastar]

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.

[floriancargoet]

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);

[makovkastar]

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.

[makovkastar]

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

[floriancargoet]

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

[makovkastar]

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.

[makovkastar]

@floriancargoet could you please address my comment related to the missing Javadoc and we should be good to merge this PR. @mdvacca what do you think?

[facebook-github-bot]

@makovkastar has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[react-native-bot]

This pull request was successfully merged by @floriancargoet in 28d5018.

^{When will my fix make it into a release? | Upcoming Releases}