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.

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

[Android] Fix overflow hidden with transform rotate on a View with borderRadius #28881

Closed
wants to merge 6 commits into from

Conversation

fabOnReact
Copy link
Contributor

@fabOnReact fabOnReact commented May 12, 2020

Summary

This issue fixes #18266 fixes #22215 fixes #19637 fixes #29312

Related #29265 #18208

The bug affects Android Devices running Sdk from 24 (Nougat) to 28 (Pie). The method clipPath does not correctly crop Views with overflow:hidden and LAYER_TYPE_NONE if the parent View has rounded border and transform rotate. The View is cropped without considering the transform rotate effect. Additional information are available on the discussion #18266. Ramanpreet Nara (RSNara) contributed to solve this issue implementing rounded borders functionality in 4994d6a.

Changelog

[Android] [Fixed] - Fix overflow hidden with transform rotate on a View with borderRadius

Test Plan

CLICK TO OPEN TESTS RESULTS

The solution uses setLayerType to change the layer to LAYER_TYPE_HARDWARE. setLayerType specifies the type of layer backing this view.

The change is applied only to Views with overflow hidden running on sdk 24-28.

The below screenshots are from RNTester transformation examples.

The screenshot on the left displays how the clipPath method crops incorrectly using a Path shape without the transform: { rotate: "30deg" } effect. The View is cropped as if no rotation was applied. The screenshot on the right displays the result after implementing the patch.

BEFORE AFTER

The below tests performed with RNTester Animated examples do not identify any issue when running animations. Gif image quality was reduced to respect github upload limits. Thanks a lot. 😃 Fabrizio Bertoglio

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 12, 2020
@react-native-bot react-native-bot added Bug Platform: Android Android applications. labels May 12, 2020
@fabOnReact fabOnReact changed the title Fix overflow hidden with transform rotate on a View with borderRadius [Android] Fix overflow hidden with transform rotate on a View with borderRadius Jun 3, 2020
@jinshiyi11
Copy link

jinshiyi11 commented Jul 9, 2020

We found a crash on some android 9 and android 10 devices(xiaomi ,vivo).

#28881 Fixes this issue

The crash is fixed when we use setLayerType(View.LAYER_TYPE_HARDWARE, null) in com.facebook.react.views.view.ReactViewGroup#dispatchOverflowDraw as @fabriziobertoglio1987 suggested in #28881
the crash likes below:
image

@fabOnReact

This comment has been minimized.

@analysis-bot
Copy link

analysis-bot commented Aug 20, 2020

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,903,844 +362
android hermes armeabi-v7a 8,401,016 +368
android hermes x86 9,392,892 +373
android hermes x86_64 9,336,989 +375
android jsc arm64-v8a 10,637,742 +185
android jsc armeabi-v7a 10,118,009 +194
android jsc x86 10,688,289 +192
android jsc x86_64 11,272,653 +195

Base commit: 84a81da

@analysis-bot
Copy link

analysis-bot commented Aug 20, 2020

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 84a81da

@brhndursun
Copy link

brhndursun commented Nov 4, 2020

Is this a fix for this issue? Because on #18208, issue still happens on my device. I took those screenshots today.

Version:

react: 16.13.1 => 16.13.1
react-native: 0.63.2 => 0.63.2

Expected:

expected

Result:

Screenshot_20201104-150823

@fabOnReact

This comment has been minimized.

Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

@fabOnReact fabOnReact marked this pull request as ready for review March 3, 2021 15:30
@fabOnReact
Copy link
Contributor Author

fabOnReact commented Aug 4, 2021

#18266 (comment)

You could try fixing this by using prop rendertohardwaretextureandroid with value of true

https://reactnative.dev/docs/view#rendertohardwaretextureandroid

Whether this View should render itself (and all of its children) into a single hardware texture on the GPU.

On Android, this is useful for animations and interactions that only modify opacity, rotation, translation, and/or scale: in those cases, the view doesn't have to be redrawn and display lists don't need to be re-executed. The texture can be re-used and re-composited with different parameters. The downside is that this can use up limited video memory, so this prop should be set back to false at the end of the interaction/animation.

My pr would just enable hardware accelleration on that view after applying the border radius, but you could solve this issue by enabling hardware accelleration on the view in Javascript for Android API N till P

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N
&& Build.VERSION.SDK_INT <= Build.VERSION_CODES.P) {
setLayerType(View.LAYER_TYPE_HARDWARE, null);
}

the prop allows you to call setLayerType(HARDWARE) on each React View.

public void setRenderToHardwareTexture(@NonNull T view, boolean useHWTexture) {
view.setLayerType(useHWTexture ? View.LAYER_TYPE_HARDWARE : View.LAYER_TYPE_NONE, null);
}

@fabOnReact fabOnReact marked this pull request as draft August 4, 2021 06:55
@fabOnReact fabOnReact marked this pull request as ready for review August 10, 2021 23:42
@SjaakSchilperoort
Copy link

Thanks a lot for this fix! It solves an important issue for us. We are running app builds with this fix since almost a year now. This requires us to build patched versions of React Native ourselves, which is quite a hassle. Is there any chance this PR will get merged?

@Stevemoretz
Copy link

Stevemoretz commented Feb 4, 2022

Thanks a lot for this fix! It solves an important issue for us. We are running app builds with this fix since almost a year now. This requires us to build patched versions of React Native ourselves, which is quite a hassle. Is there any chance this PR will get merged?

Hello, I have an unrelated question, doesn't your app target under N api? , the last comment says it works from N onwards.

@SjaakSchilperoort
Copy link

Thanks a lot for this fix! It solves an important issue for us. We are running app builds with this fix since almost a year now. This requires us to build patched versions of React Native ourselves, which is quite a hassle. Is there any chance this PR will get merged?

Hello, I have an unrelated question, doesn't your app target under N api? , the last comment says it works from N onwards.

Our app supports Android 21+. This fix is for Android 24-28, which is 5% of our Android users.

@Stevemoretz
Copy link

Thanks a lot for this fix! It solves an important issue for us. We are running app builds with this fix since almost a year now. This requires us to build patched versions of React Native ourselves, which is quite a hassle. Is there any chance this PR will get merged?

Hello, I have an unrelated question, doesn't your app target under N api? , the last comment says it works from N onwards.

Our app supports Android 21+. This fix is for Android 24-28, which is 5% of our Android users.

Thank you so much for responding, what I'm exactly trying to know is what do you do for 21 to 23 then? Conditionally remove the border I guess?

@SjaakSchilperoort
Copy link

Thank you so much for responding, what I'm exactly trying to know is what do you do for 21 to 23 then? Conditionally remove the border I guess?

As far as I know, this bug doesn't occur on these versions

@fabOnReact fabOnReact closed this Jan 10, 2023
@shubhamguptadream11
Copy link
Collaborator

@fabOnReact Why this PR has not been merged? This issue still seems to be open but yet not resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Platform: Android Android applications.
Projects
None yet
10 participants