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] fontWeight number value error if not in an array #45285

Open
Freddy03h opened this issue Jul 4, 2024 · 14 comments
Open

[Android] fontWeight number value error if not in an array #45285

Freddy03h opened this issue Jul 4, 2024 · 14 comments
Labels
Good first issue Interested in collaborating? Take a stab at fixing one of these issues. Help Wanted :octocat: Issues ideal for external contributors. Needs: Version Info Platform: Android Android applications. Resolution: Fixed A PR that fixes this issue has been merged.

Comments

@Freddy03h
Copy link
Contributor

Freddy03h commented Jul 4, 2024

Description

fontWeight accept number values since 0.71

But there is an issue on Android on a specific case:

It's only happen on the old Architecture (no issue with new arch) and with a style created with Stylesheet.create and given to style prop directly, not in an array.


style from Stylesheet.create directly passed to style prop

// Error

const styles = StyleSheet.create({
  highlight: {
    fontWeight: 700,
  },
});

<Text style={styles.highlight}>App.tsx</Text>

style from Stylesheet.create given to style prop in an array

// No error

const styles = StyleSheet.create({
  highlight: {
    fontWeight: 700,
  },
});

<Text style={[styles.highlight]}>App.tsx</Text>

inline style

// No error

<Text style={{ fontWeight: 700 }}>App.tsx</Text>

style not created with Stylesheet.create

// No error

const styles = {
  highlight: {
    fontWeight: 700,
  },
};

<Text style={styles.highlight}>App.tsx</Text>

Steps to reproduce

  1. Create a new RN project with npx react-native init
  2. Run the Android project.
  3. At the bottom of App.tsx, in the highlight style, change fontWeight from '700' to 700.

React Native Version

0.74.3

Affected Platforms

Runtime - Android

Output of npx react-native info

System:
  OS: macOS 14.5
  CPU: (10) x64 Apple M1 Pro
  Memory: 24.67 MB / 16.00 GB
  Shell:
    version: "5.9"
    path: /bin/zsh
Binaries:
  Node:
    version: 18.16.0
    path: ~/.nvm/versions/node/v18.16.0/bin/node
  Yarn:
    version: 3.6.4
    path: /usr/local/bin/yarn
  npm:
    version: 9.5.1
    path: ~/.nvm/versions/node/v18.16.0/bin/npm
  Watchman:
    version: 2023.08.14.00
    path: /usr/local/bin/watchman
Managers:
  CocoaPods:
    version: 1.11.2
    path: /Users/freddy/.rbenv/shims/pod
SDKs:
  iOS SDK:
    Platforms:
      - DriverKit 23.5
      - iOS 17.5
      - macOS 14.5
      - tvOS 17.5
      - visionOS 1.2
      - watchOS 10.5
  Android SDK:
    API Levels:
      - "27"
      - "28"
      - "29"
      - "30"
      - "31"
      - "33"
      - "34"
    Build Tools:
      - 27.0.3
      - 28.0.3
      - 29.0.3
      - 30.0.2
      - 30.0.3
      - 31.0.0
      - 32.0.0
      - 33.0.0
      - 34.0.0
    System Images:
      - android-27 | Google APIs Intel x86 Atom
      - android-29 | Google APIs Intel x86 Atom
      - android-30 | Google APIs ARM 64 v8a
      - android-31 | Google APIs ARM 64 v8a
      - android-33 | Google APIs ARM 64 v8a
    Android NDK: Not Found
IDEs:
  Android Studio: 2021.1 AI-211.7628.21.2111.8309675
  Xcode:
    version: 15.4/15F31d
    path: /usr/bin/xcodebuild
Languages:
  Java:
    version: 17.0.11
    path: /usr/bin/javac
  Ruby:
    version: 2.7.4
    path: /Users/freddy/.rbenv/shims/ruby
npmPackages:
  "@react-native-community/cli": Not Found
  react:
    installed: 18.2.0
    wanted: 18.2.0
  react-native:
    installed: 0.74.3
    wanted: 0.74.3
  react-native-macos: Not Found
npmGlobalPackages:
  "*react-native*": Not Found
Android:
  hermesEnabled: true
  newArchEnabled: false
iOS:
  hermesEnabled: Not found
  newArchEnabled: false

Stacktrace or Logs

Error while updating property 'fontWeight in shadow node of type: RCTVirtualText
java.lang.Double cannot be cast to java.lang.String

updateShadowNodeProp
ViewManagersPropertyCache.java:125

setProperty
ViewManagerPropertyUpdater.java: 161

updateProps
ViewManagerPropertyUpdater.java: 65

updateProperties
ReactShadowNodeImp1. java: 321

updateView
UI Implementation.java: 279

updateView
UIManagerModule. java:433

invoke
Method. java

invoke
JavaMethodwrapper. java: 372

invoke
JavaModuleWrapper. java: 146

run
NativeRunnable. java

handleCallback
Handler.java:958

dispatchMessage
Handler.java:99

dispatchMessage
MessageQueueThreadHandler. java: 27

Reproducer

https://github.com/Freddy03h/react-native-font-weight-number-android-issue

Screenshots and Videos

345393532-b85d6f27-98b4-4bd9-b9ff-e0640fa6e938

Copy link

github-actions bot commented Jul 4, 2024

⚠️ Add or Reformat Version Info
ℹ️ We could not find or parse the version number of React Native in your issue report. Please use the template, and report your version including major, minor, and patch numbers - e.g. 0.70.2

@cortinico
Copy link
Contributor

It's only happen on the old Architecture (no issue with new arch) and with a style created with Stylesheet.create and given to style prop directly, not in an array.

Just a heads up that we're currently hyperfocused on New Architecture, so we won't be able to fix this bug in the immediate future (a community PR is more than welcome though).

Is there a reason why you're not moving to the New Architecture @Freddy03h ?

@Freddy03h
Copy link
Contributor Author

Freddy03h commented Jul 4, 2024

I have multiple projects for my clients with different dependencies and react-native versions (from 0.71), so it's not always easy to migrate to new architecture, depending on specific native dep on the project.

But we found this bug while creating types for fontWeight in rescript-react-native rescript-react-native/rescript-react-native#806

And since the type number are also added in the Typescript definition, it can happen to Typescript users too:

Also, because it's a weird case, it's not always easy to know why this error happened.

I would like to help but I have no native android skills, also I don't know why this error exist only with Stylesheet.create because now it's supposed to be an identity function, right?

@github-actions github-actions bot added Needs: Attention Issues where the author has responded to feedback. and removed Needs: Author Feedback labels Jul 4, 2024
@cortinico
Copy link
Contributor

So the issue is:

java.lang.Double cannot be cast to java.lang.String

I believe we do some props pre-processing if you pass an array vs if you pass a single prop. In this case it being Double cause the framework to fail.

I don't have time to debug this, but for folks interested in helping out, I would put a breakpoint here:

public void updateShadowNodeProp(ReactShadowNode nodeToUpdate, Object value) {
try {
Object[] args;
if (mIndex == null) {
args = SHADOW_ARGS.get();
args[0] = getValueOrDefault(value, nodeToUpdate.getThemedContext());
} else {
args = SHADOW_GROUP_ARGS.get();
args[0] = mIndex;
args[1] = getValueOrDefault(value, nodeToUpdate.getThemedContext());
}
mSetter.invoke(nodeToUpdate, args);
Arrays.fill(args, null);
} catch (Throwable t) {
FLog.e(ViewManager.class, "Error while updating prop " + mPropName, t);
throw new JSApplicationIllegalArgumentException(
"Error while updating property '"
+ mPropName
+ "' in shadow node of type: "
+ nodeToUpdate.getViewClass(),
t);
}
}

and investigate how the array/single-value codepath diverge.
The fix is probably trivial

@cortinico cortinico added Good first issue Interested in collaborating? Take a stab at fixing one of these issues. Help Wanted :octocat: Issues ideal for external contributors. and removed Needs: Attention Issues where the author has responded to feedback. labels Jul 4, 2024
@shubhamguptadream11
Copy link
Contributor

I am working on fixing this.

@Freddy03h
Copy link
Contributor Author

I believe we do some props pre-processing if you pass an array vs if you pass a single prop.

It's a little more vicious than this because direct inline style works well

@shubhamguptadream11
Copy link
Contributor

shubhamguptadream11 commented Jul 5, 2024

In StyleSheet.js we are doing Object.freeze() for Dev Mode only. By removing Object.freeze there this issue was resolved. But that code is just for Dev Mode.
@Freddy03h Can you confirm if this issue is coming on release builds as well?

For Dev Mode only we are making style object immutable. So Ideally this issue should not come in production.

I checked this on release build as well. It's working fine for me. The issue is only on DEV Mode

@shubhamguptadream11
Copy link
Contributor

shubhamguptadream11 commented Jul 5, 2024

@cortinico Here are my findings
Reason: This issue is coming because of we are sending fontWeight as number instead of string from JS to Android.

When we are creating styles using StyleSheet.create() we are calling Object.freeze() on that making styles completely immutable.
So when in node_modules/react-native/Libraries/Text/Text.js we are trying to mutate fontWeight with its string conversion it's not get updated. The problem is not just with this props even textAlignVertical props will also not get updated.

Why this is not happening in other cases?

  • In case of inling styles no freeze method is called.
  • In case of same style passed as an array. While flattening the object we had created an new object in node_modules/react-native/Libraries/StyleSheet/flattenStyle.js so there wouldn't be any problem.

Solution

  • Since we are doing Object.freeze() in dev mode only we can remove this from StyleSheet.js file, because now we actually want to reassign few props like fontweight.

Let me know what you think. I can raise the PR for same

@Freddy03h
Copy link
Contributor Author

@shubhamguptadream11 Effectively I can't reproduce the error on release mode, so it's dev mode only.

And your change https://github.com/facebook/react-native/pull/45299/files fix the issue, thank you!

@shubhamguptadream11
Copy link
Contributor

@Freddy03h Thanks for checking this.
This is not my PR: https://github.com/facebook/react-native/pull/45299/files (As per this PR we are actually doing one extra operation for each style object in release mode as well which will be redundant since issue is not on release mode. And if let's say we put a DEV mode check there then it doesn't make sense to do freeze in StyleSheet file).

So I am waiting for inputs from @cortinico.

@meetdhanani17
Copy link

meetdhanani17 commented Jul 5, 2024

And

freeze make sense while development we preventing user to modify style and I have added extra layer that also removed on release. that time it check if object is freeze than only its return new object

Share your suggestions into https://github.com/facebook/react-native/pull/45299/files PR @cortinico

@javache
Copy link
Member

javache commented Jul 9, 2024

Addressing in #45340

@meetdhanani17
Copy link

meetdhanani17 commented Jul 9, 2024

Addressing in #45340

FlowFixMe it is also there in your PR why you need it. i think its not proper solution of it. @cortinico i think now you have to raised a pr (Please read my 1st PR comments first and provide us feedback, why it's closed without any proper valid reason)

@meetdhanani17
Copy link

@javache you have to do changes in TextInput.js related to styles changes

@cortinico cortinico added Resolution: Fixed A PR that fixes this issue has been merged. and removed Resolution: PR Submitted A pull request with a fix has been provided. labels Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good first issue Interested in collaborating? Take a stab at fixing one of these issues. Help Wanted :octocat: Issues ideal for external contributors. Needs: Version Info Platform: Android Android applications. Resolution: Fixed A PR that fixes this issue has been merged.
Projects
None yet
5 participants