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

fix: foreground ripple crash on api < 23 #37901

Closed
wants to merge 1 commit into from

Conversation

vonovak
Copy link
Contributor

@vonovak vonovak commented Jun 14, 2023

Summary:

The Pressable component supports the foreground option for ripple. However, using it on old android api levels (e.g. android 5) crashes with

Error while updating property
nativeForegroundAndroid' of a view managed by:
RCTView
null
No virtual method setForeground(Landroid/ graphics/drawable/Drawable;)V in class Lcom/ facebook/react/views/view/ReactViewGroup; or its super classes

TouchableNativeFeedback.js has a check to make sure this does not happen as seen here

I also want to point out that this PR can be closed #30466 as it's already implemented

Changelog:

Test Plan:

tested locally on another project because I wasn't able to get RN tester running

@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 Jun 14, 2023
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,757,695 +25
android hermes armeabi-v7a 8,070,268 +32
android hermes x86 9,250,290 +26
android hermes x86_64 9,099,434 +29
android jsc arm64-v8a 9,318,885 +14
android jsc armeabi-v7a 8,508,785 +13
android jsc x86 9,382,367 +4
android jsc x86_64 9,635,612 +6

Base commit: b0485be
Branch: main

@NickGerleman
Copy link
Contributor

I think this works, but we could also fix on the native side so that any usage of the prop no-ops without adding a check on the JS side.

@vonovak
Copy link
Contributor Author

vonovak commented Jun 14, 2023

I think this works, but we could also fix on the native side so that any usage of the prop no-ops without adding a check on the JS side.

I can do that, but it probably shouldn't be a noop - if foreground is not supported, it should fall back to using background ripple?

@NickGerleman
Copy link
Contributor

Oh, yes, I missed that the JS was changing to a specific fallback behavior. JS side solution makes sense to me then.

@facebook-github-bot
Copy link
Contributor

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

@github-actions
Copy link

This pull request was successfully merged by @vonovak in ca65d97.

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

@github-actions github-actions bot added the Merged This PR has been merged. label Jun 15, 2023
adamaveray added a commit to adamaveray/react-native that referenced this pull request Jun 16, 2023
* main: (135 commits)
  translation auto-update for i18n/twilight.config.json on master
  Interop: Introduce Bridge proxy
  Remove okhttp internal util usage (facebook#37843)
  Update debian to fix CI while updating Node (facebook#37841)
  fix: foreground ripple crash on api < 23 (facebook#37901)
  Re-add the top level LICENSE file (facebook#37916)
  Deploy 0.209.0 to xplat (facebook#37921)
  Re-enable direct debugging with JSC on iOS 16.4+ (facebook#37914)
  add emitObjectProp in parser primitives (facebook#37904)
  Make React-utils its own pod (facebook#37659)
  feat: allow custom assignment of rootView to rootViewController (facebook#37873)
  Switch xplat prettier config to hermes plugin (facebook#37915)
  Set iOS AppState to inactive when app is launching (facebook#37690)
  Use `fileExists` in replace_hermes script (facebook#37911)
  (docs): fix license url (facebook#37909)
  Revert D46719890: Re-enable direct debugging with JSC on iOS 16.4+
  Re-enable direct debugging with JSC on iOS 16.4+ (facebook#37874)
  Fix component type references in xplat (facebook#37903)
  Remove usage of passthroughAnimatedPropExplicitValues in ScrollViewStickyHeader (facebook#37867)
  test runtime lifecycle callback (facebook#37897)
  ...
kacperkapusciak pushed a commit to react-navigation/react-navigation that referenced this pull request Aug 1, 2023
Please provide enough information so that others can review your pull
request.

**Motivation**

closes #9794

also observed that the ripple radius can be very slightly increased to
be like in the native android toolbar

why the `foreground` isn't simply `true`? see
facebook/react-native#37901

**Test plan**

tested locally
kacperkapusciak pushed a commit to react-navigation/react-navigation that referenced this pull request Aug 1, 2023
Please provide enough information so that others can review your pull
request.

**Motivation**

closes #9794

also observed that the ripple radius can be very slightly increased to
be like in the native android toolbar

why the `foreground` isn't simply `true`? see
facebook/react-native#37901

**Test plan**

tested locally
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants