Skip to content

Fix dashed/dotted border drawing when border-radius is 0 - #28359

Closed
IjzerenHein wants to merge 1 commit into
react:mainfrom
IjzerenHein:android-border-dashed-dotted-fix
Closed

Fix dashed/dotted border drawing when border-radius is 0#28359
IjzerenHein wants to merge 1 commit into
react:mainfrom
IjzerenHein:android-border-dashed-dotted-fix

Conversation

@IjzerenHein

Copy link
Copy Markdown
Contributor

Summary

This PR fixes the border-style that is not respected when drawing a border with 0 border-radius on Android. This would cause the faster drawRectangularBackgroundWithBorders path to be used, but that uses rectangular drawing and doesn't support dashed/dotted stroke patterns.
This PR changes the behavior to use the generic drawRoundedBackgroundWithBorders code-path which does support dashed/dotted border-styles.

Changelog

[Android] [Fixed] - Fix dashed/dotted border-drawing when border-radius is 0

Test Plan

Faulty situation:

Screenshot_1584721992

After the fix:

Screenshot_1584721410

@facebook-github-botfacebook-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 Mar 20, 2020
@analysis-bot

analysis-bot commented Mar 20, 2020

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
ios-universal10,903,7920

Base commit: 25ed045

@facebook-github-botfacebook-github-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@shergin is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@sherginshergin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Internal CI tests failed because of:

java.lang.NullPointerException: Attempt to read from field 'boolean android.graphics.Path.isSimplePath' on a null object reference
at android.graphics.BaseCanvas.drawPath(BaseCanvas.java:295)
at android.graphics.Canvas.drawPath(Canvas.java:1652)
at com.facebook.react.views.view.ReactViewBackgroundDrawable.drawRoundedBackgroundWithBorders(ReactViewBackgroundDrawable.java:338)
at com.facebook.react.views.view.ReactViewBackgroundDrawable.draw(ReactViewBackgroundDrawable.java:134)
at android.view.View.drawBackground(View.java:19399)
at android.view.View.draw(View.java:19183)
at android.view.View.draw(View.java:19062)
at android.view.ViewGroup.drawChild(ViewGroup.java:4236)
at android.view.ViewGroup.dispatchDraw(ViewGroup.java:4022)
at com.facebook.react.ReactRootView.dispatchDraw(ReactRootView.java:229)
at android.view.View.draw(View.java:19060)
at android.view.ViewGroup.drawChild(ViewGroup.java:4236)
at android.view.ViewGroup.dispatchDraw(ViewGroup.java:4022)
at com.facebook.react.testing.ScreenshotingFrameLayout.dispatchDraw(ScreenshotingFrameLayout.java:44)
at android.view.View.updateDisplayListIfDirty(View.java:18133)
at android.view.View.draw(View.java:18920)
at android.view.ViewGroup.drawChild(ViewGroup.java:4236)
at android.view.ViewGroup.dispatchDraw(ViewGroup.java:4022)
at android.view.View.updateDisplayListIfDirty(View.java:18133)
at android.view.View.draw(View.java:18920)
at android.view.ViewGroup.drawChild(ViewGroup.java:4236)
at android.view.ViewGroup.dispatchDraw(ViewGroup.java:4022)
at android.view.View.updateDisplayListIfDirty(View.java:18133)
at android.view.View.draw(View.java:18920)
at android.view.ViewGroup.drawChild(ViewGroup.java:4236)
at android.view.ViewGroup.dispatchDraw(ViewGroup.java:4022)
at android.view.View.updateDisplayListIfDirty(View.java:18133)
at android.view.View.draw(View.java:18920)
at android.view.ViewGroup.drawChild(ViewGroup.java:4236)
at android.view.ViewGroup.dispatchDraw(ViewGroup.java:4022)
at android.view.View.draw(View.java:19195)
at com.android.internal.policy.DecorView.draw(DecorView.java:788)
at android.view.View.updateDisplayListIfDirty(View.java:18142)
at android.view.ThreadedRenderer.updateViewTreeDisplayList(ThreadedRenderer.java:669)
at android.view.ThreadedRenderer.updateRootDisplayList(ThreadedRenderer.java:675)
at android.view.ThreadedRenderer.draw(ThreadedRenderer.java:783)
at android.view.ViewRootImpl.draw(ViewRootImpl.java:2992)
at android.view.ViewRootImpl.performDraw(ViewRootImpl.java:2806)
at android.view.ViewRootImpl.performTraversals(ViewRootImpl.java:2359)
at android.view.ViewRootImpl.doTraversal(ViewRootImpl.java:1392)
at android.view.ViewRootImpl$TraversalRunnable.run(ViewRootImpl.java:6752)
at android.view.Choreographer$CallbackRecord.run(Choreographer.java:911)
at android.view.Choreographer.doCallbacks(Choreographer.java:723)
at android.view.Choreographer.doFrame(Choreographer.java:658)
at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:897)
at android.os.Handler.handleCallback(Handler.java:790)
at android.os.Handler.dispatchMessage(Handler.java:99)
at android.os.Looper.loop(Looper.java:164)
at android.app.ActivityThread.main(ActivityThread.java:6494)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:438)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:807)

@IjzerenHein Could you please figure it out?

@IjzerenHein

IjzerenHein commented Mar 23, 2020

Copy link
Copy Markdown
ContributorAuthor

@shergin Which test specifically is failing? How can I run that test?

When I look at the CI Android tests, they seem to fail at:

+ buck fetch ReactAndroid/src/test/java/com/facebook/react/modules
Not using buckd because watchman isn't installed.
Picked up _JAVA_OPTIONS: -XX:+UnlockExperimentalVMOptions -XX:+UseCGroupMemoryLimitForHeap
PARSING BUCK FILES: FINISHED IN 1.4s
Buck wasn't able to parse /root/react-native/ReactAndroid/src/main/java/com/facebook/fbreact/specs/BUCK:
KeyError: 'load label @fbsource//tools/build_defs:platform_defs.bzl references an unknown cell named fbsource known cells: {}'
Call stack:
File "/root/react-native/.buckd/resources/2019.10.17.01/buck_server/buck_parser/profiler.py", line 507, in wrapped
return func(*args, **kwargs)
File "/root/react-native/ReactAndroid/src/main/java/com/facebook/fbreact/specs/BUCK", line 1
load("@fbsource//tools/build_defs:platform_defs.bzl", "ANDROID")
This error happened while trying to get dependency '//ReactAndroid/src/main/java/com/facebook/fbreact/specs:FBReactNativeSpec' of target '//ReactAndroid/src/main/java/com/facebook/react/devsupport:devsupport'
Exited with code exit status 1

And when running test-android-all locally I also get this error:

Step 14/24 : RUN buck fetch ReactAndroid/src/test/java/com/facebook/react/modules
---> Running in 4edb170a2f99
Not using buckd because watchman isn't installed.
Picked up JAVA_TOOL_OPTIONS: -Dfile.encoding=UTF8
PARSING BUCK FILES: FINISHED IN 2.7s
Buck wasn't able to parse /app/ReactAndroid/src/main/java/com/facebook/fbreact/specs/BUCK:
KeyError: 'load label @fbsource//tools/build_defs:platform_defs.bzl references an unknown cell named fbsource known cells: {}'
Call stack:
File "/app/.buckd/resources/2019.10.17.01/buck_server/buck_parser/profiler.py", line 507, in wrapped
return func(*args, **kwargs)
File "/app/ReactAndroid/src/main/java/com/facebook/fbreact/specs/BUCK", line 1
load("@fbsource//tools/build_defs:platform_defs.bzl", "ANDROID")
This error happened while trying to get dependency '//ReactAndroid/src/main/java/com/facebook/fbreact/specs:FBReactNativeSpec' of target '//ReactAndroid/src/main/java/com/facebook/react/devsupport:devsupport'
The command '/bin/sh -c buck fetch ReactAndroid/src/test/java/com/facebook/react/modules' returned a non-zero code: 1

@IjzerenHein
IjzerenHeinforce-pushed the android-border-dashed-dotted-fix branch from f34f6ff to 3585eefCompareApril 16, 2020 08:21
@analysis-bot

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a6,759,03365
androidhermesarmeabi-v7a6,406,07060
androidhermesx867,140,43850
androidhermesx86_647,031,77166
androidjscarm64-v8a8,927,82760
androidjscarmeabi-v7a8,567,29267
androidjscx868,752,38877
androidjscx86_649,329,33374

Base commit: 25ed045

@IjzerenHein

Copy link
Copy Markdown
ContributorAuthor

@shergin I've rebased on master, all tests seem to pass. I was unable to reproduce/find the issue that you mentioned. Which test did you run?

@fabOnReact

Copy link
Copy Markdown
Contributor

#29099

@facebook-github-botfacebook-github-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@charlesbdudleycharlesbdudley self-assigned this Aug 12, 2021
@facebook-github-bot

Copy link
Copy Markdown
Contributor

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

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@charlesbdudley merged this pull request in 3e5998e.

@facebook-github-botfacebook-github-bot added the Merged This PR has been merged. label Aug 16, 2021
@charlesbdudley

charlesbdudley commented Aug 17, 2021

Copy link
Copy Markdown
Contributor

Hi @IjzerenHein, unfortunately this merge was reverted as it caused crashes on physical devices. Interestingly we weren't able to reproduce it in a simulator. Do you have access to a physical device to try to reproduce this error?

java.lang.NullPointerException:com.facebook.react.views.view.ReactViewBackgroundDrawable.drawRoundedBackgroundWithBorders
stack_trace:	java.lang.IllegalStateException: java.lang.RuntimeException: java.lang.NullPointerException: Attempt to read from field 'boolean android.graphics.Path.isSimplePath' on a null object reference
at com.facebook.react.bridge.ReactContext.handleException(ReactContext.java:425)
at com.facebook.react.views.modal.ReactModalHostView$DialogRootViewGroup.handleException(ReactModalHostView.java:508)
at com.facebook.react.views.view.ReactViewGroup.dispatchDraw(ReactViewGroup.java:749)
at android.view.View.updateDisplayListIfDirty(View.java:22762)
at android.view.View.draw(View.java:23631)
at android.view.ViewGroup.drawChild(ViewGroup.java:5336)
at android.view.ViewGroup.dispatchDraw(ViewGroup.java:5093)
at com.facebook.react.views.view.ReactViewGroup.dispatchDraw(ReactViewGroup.java:743)

@charlesbdudleycharlesbdudley added Needs: Author Feedback and removed Merged This PR has been merged. labels Aug 17, 2021
@IjzerenHein

Copy link
Copy Markdown
ContributorAuthor

@charlesbdudley Unfortunately I don't have any bandwidth available to look at this issue :/

@github-actionsgithub-actionsBot added Needs: Attention Issues where the author has responded to feedback. and removed Needs: Author Feedback labels Sep 2, 2021
@charlesbdudley

Copy link
Copy Markdown
Contributor

@charlesbdudley Unfortunately I don't have any bandwidth available to look at this issue :/

No problem. I'm going to close this for now to focus on active PRs but feel free to mention me if your bandwidth changes and I'll open it back up!

lukmccall added a commit to expo/expo that referenced this pull request Nov 8, 2022
# Why
Removes custom `BorderDrawable` class. That class was a modified version of ReactBackgroundDrawable that fixes various issues. Most of them were already fixed upstream. However, one of the fixes was reverted - react/react-native#28359, because that change was leading to crashes. # How
- Removed custom `BorderDrawable` class.
- Used `ReactViewBackgroundDrawable` instead. # Test Plan
- bare-expo with NCL ✅
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BugCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.Needs: AttentionIssues where the author has responded to feedback.Platform: AndroidAndroid applications.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@IjzerenHein@analysis-bot@fabOnReact@facebook-github-bot@charlesbdudley@shergin@sota000@react-native-bot