Cover percentage border radii on Android ScrollViews - #58745
Closed
Abbondanzo wants to merge 2 commits into
Closed
Abbondanzo wants to merge 2 commits into
Abbondanzo wants to merge 2 commits into
Conversation
…extInput, and ScrollView (react#57869) Summary: Follow-up to react#57795, which fixed the percentage-`borderRadius` crash for `<Image>` (react#53977). The same latent bug existed in every remaining Android view manager whose `borderRadius` `ReactPropGroup` setter was still typed as `Float`: - `ReactTextViewManager` (`<Text>`) - `PreparedLayoutTextViewManager` (internal, prepared-layout `<Text>`) - `ReactTextInputManager` (`<TextInput>`) - `ReactScrollViewManager` (`<ScrollView>`) - `ReactHorizontalScrollViewManager` (`<ScrollView horizontal>`) - `ReactNestedScrollViewManager` (internal, nested-scroll variant) Percentage border radii arrive from JS as strings (`'50%'`), so the reflection-based property updater throws: ``` com.facebook.react.bridge.JSApplicationIllegalArgumentException: Error while updating property 'borderRadius' of a view managed by: RCTText Caused by: java.lang.ClassCastException: java.lang.String cannot be cast to java.lang.Double ``` Each setter now accepts a `Dynamic` parsed with `LengthPercentage.setFromDynamic`, completing the migration `ReactViewManager` received in 0.75 and `ReactImageManager` in react#57795. The `Float` overloads on the four public managers are kept as deprecated pass-throughs for source/binary compatibility (public API dump updated); the two internal managers are migrated outright. Rendering needs no changes since all six managers already delegate to `BackgroundStyleApplicator`, which resolves percentages. ## Changelog: [ANDROID] [FIXED] - Fix crash when setting a percentage borderRadius on Text, TextInput, and ScrollView Pull Request resolved: react#57869 Test Plan: **Unit tests** — new Robolectric regression tests mirroring the one merged in react#57795: - `ReactTextViewPropertyTest.testBorderRadius` (new file) - `ReactTextInputPropertyTest.testBorderRadius` Both verified red before the fix (failing with `JSApplicationIllegalArgumentException: Error while updating property 'borderRadius' of a view managed by: RCTText`) and green after (`:packages:react-native:ReactAndroid:testDebugUnitTest`, 25/25 passing). **Manual QA** — rn-tester on an Android emulator (API 35), rendering the snippet below with `borderRadius: '50%'` on `<Text>` and `'20%'` on `<TextInput>`, `<ScrollView>`, and `<ScrollView horizontal>`: ```jsx <Text style={{borderWidth: 2, borderColor: 'green', borderRadius: '50%', padding: 20}}>Text 50%</Text> <TextInput style={{borderWidth: 2, borderColor: 'blue', borderRadius: '20%', padding: 12}} defaultValue="TextInput 20%" /> <ScrollView style={{borderWidth: 2, borderColor: 'purple', borderRadius: '20%', height: 80}}>...</ScrollView> <ScrollView horizontal style={{borderWidth: 2, borderColor: 'purple', borderRadius: '20%', height: 80}}>...</ScrollView> ``` On main this screen crashes with the exception above; with this change all four render rounded corners. ### Screenshots | Before (main) — surface fails to mount, exception above in logcat | After (this PR) — all four components render their percentage radii | | --- | --- | | <img src="https://github.com/sbaiahmed1/react-native/releases/download/pr-57869-assets/before-blank-surface-v2.png" width="320" /> | <img src="https://github.com/sbaiahmed1/react-native/releases/download/pr-57869-assets/after-percentage-radii.png" width="320" /> | Differential Revision: D122262619 Pulled By: Abbondanzo
Summary: Add property-updater regression coverage for vertical and horizontal Android ScrollViews. Exercise every supported radius index, point values, percentages, negative values, null resets, and the deprecated Float overload. Changelog: [Internal] Differential Revision: D122278945
meta-cla
Bot
added
the
CLA Signed
|
@Abbondanzo has exported this pull request. If you are a Meta employee, you can view the originating Diff in D122278945. |
|
This pull request has been merged in 3bc61ec. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
Add property-updater regression coverage for vertical and horizontal Android ScrollViews. Exercise every supported radius index, point values, percentages, negative values, null resets, and the deprecated Float overload.
Changelog: [Internal]
Differential Revision: D122278945