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

Intl.NumberFormat signDisplay option displays odd behaviour #1466

Open
Brawl345 opened this issue Aug 1, 2024 · 1 comment
Open

Intl.NumberFormat signDisplay option displays odd behaviour #1466

Brawl345 opened this issue Aug 1, 2024 · 1 comment

Comments

@Brawl345
Copy link

Brawl345 commented Aug 1, 2024

Description

The signDisplay option for Intl.NumberFormat is bugged and has an odd behaviour. E.g. the option exceptZero outputs "+8,537,71+" but it should output +8.537,71 €. Don't know if this only affects Android.

Steps to reproduce

  1. Create a new React Native application
  2. Insert this code somewhere:
          <Text>{
              new Intl.NumberFormat('de-DE', {
                  style: 'currency',
                  currency: 'EUR',
                  maximumFractionDigits: 2,
                  minimumFractionDigits: 2,
                  roundingMode: 'floor',
                  signDisplay: 'exceptZero',
              }).format(8537.71)
          }</Text>
  1. Start the app on Android

React Native Version

0.74.4

Affected Platforms

Runtime - Android

Output of npx react-native info

System:
  OS: macOS 14.5
  CPU: (10) arm64 Apple M1 Max
  Memory: 307.52 MB / 32.00 GB
  Shell:
    version: "5.9"
    path: /bin/zsh
Binaries:
  Node:
    version: 20.14.0
    path: /etc/profiles/per-user/user/bin/node
  Yarn:
    version: 3.6.4
    path: /etc/profiles/per-user/user/bin/yarn
  npm:
    version: 10.7.0
    path: /etc/profiles/per-user/user/bin/npm
  Watchman:
    version: 2024.03.11.00
    path: /etc/profiles/per-user/user/bin/watchman
Managers:
  CocoaPods: Not Found
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:
      - "30"
      - "31"
      - "32"
      - "33"
      - "33"
      - "34"
    Build Tools:
      - 30.0.1
      - 30.0.3
      - 31.0.0
      - 33.0.1
      - 34.0.0
    System Images:
      - android-22 | Google APIs ARM 64 v8a
      - android-31 | Google APIs ARM 64 v8a
      - android-33 | Wear OS 4 ARM 64 v8a
      - android-33 | Google APIs ARM 64 v8a
      - android-33 | Google Play ARM 64 v8a
      - android-34 | Google Play ARM 64 v8a
    Android NDK: Not Found
IDEs:
  Android Studio: 2024.1 AI-241.18034.62.2411.12071903
  Xcode:
    version: 15.4/15F31d
    path: /usr/bin/xcodebuild
Languages:
  Java:
    version: 17.0.12
    path: /usr/bin/javac
  Ruby:
    version: 2.6.10
    path: /usr/bin/ruby
npmPackages:
  "@react-native-community/cli": Not Found
  react:
    installed: 18.2.0
    wanted: 18.2.0
  react-native:
    installed: 0.74.4
    wanted: 0.74.4
  react-native-macos: Not Found
npmGlobalPackages:
  "*react-native*": Not Found
Android:
  hermesEnabled: true
  newArchEnabled: false
iOS:
  hermesEnabled: Not found
  newArchEnabled: false

Stacktrace or Logs

n/a

Reproducer

https://github.com/Brawl345/rn-signDisplay-bug-reproducer

Screenshots and Videos

Screenshot_1722504869

@cortinico cortinico transferred this issue from facebook/react-native Aug 1, 2024
@shubhamguptadream11
Copy link
Contributor

I was trying to fix this issue.
After debugging I found out that current logic which handles signDisplay is breaking in these cases:

  • The old logic replaces positivePrefix with just the plus sign if negativePrefix is not empty. This means any existing currency symbol or other content in positivePrefix would be lost.
    Example:
    Given positivePrefix = ¥ and negativePrefix = -, the old logic would replace positivePrefix with +, resulting in + instead of +¥.
    Also In case JP locale it is not respecting already appended currencySymbol.

  • It adds a Prefix and Suffix both in this case:

new Intl.NumberFormat('de-DE', {
                  style: 'currency',
                  currency: 'EUR',
                  signDisplay: 'exceptZero',
              }).format(8537.71)`

Result: +8,537,71+
Expected: +8.537,71 €

So in order to fix this we surely need to revisit the logic written here:

public PlatformNumberFormatterICU setSignDisplay(

Solution:

          if (decimalFormat.getPositivePrefix().isEmpty()) {
            decimalFormat.setPositivePrefix(new String(new char[]{symbols.getPlusSign()}));
          } else {
            // Preserve the existing prefix, which may include the currency symbol, and prepend the plus sign
            decimalFormat.setPositivePrefix(new String(new char[]{symbols.getPlusSign()}) + decimalFormat.getPositivePrefix());
          }

I tested this in above failing cases.
Let me know your thoughts on this, or any edge case which you thought will break this.

facebook-github-bot pushed a commit that referenced this issue Sep 17, 2024
Summary:
Fixes following issue:
- #1466
- #1138
- #789

**Why this change is made?**

There are few cases where signDisplay is not being handled correctly.
Examples:
```
1.
new Intl.NumberFormat('de-DE', {
 style: 'currency',
 currency: 'EUR',
 signDisplay: 'exceptZero',
}).format(8537.71)

Output: +8,537,71+
Expected: +8.537,71 €

2.
new Intl.NumberFormat('ja-JP', {
 style: 'currency',
 currency: 'JPY',
 signDisplay: 'exceptZero'
}).format(123456.789)

Output: +123,457
Expected: +¥123,457
```

**ChangeLog**

This PR updates the implementation and testing of the signDisplay functionality in the DecimalFormat class within the Hermes engine, specifically for Android API level 31 and above.

**Key Changes:**

Implementation:
- Integrated the **setSignAlwaysShown** method of DecimalFormat for API level 31 and above to control the display of the sign (+ or -) based on the signDisplay option. [For more detail about setSignAlwaysShown check [here](https://developer.android.com/reference/android/icu/text/DecimalFormat#setSignAlwaysShown(boolean))]
- For API levels below 31, maintained the existing logic for handling sign display, ensuring backward compatibility.

Pull Request resolved: #1483

Test Plan:
- Added a comprehensive set of test cases in **HermesIntlAndroidTest.java** to validate the behaviour of the signDisplay functionality, specifically for API level 31 and above.
- Test cases cover scenarios for signDisplay: `NEVER | ALWAYS | EXCEPTZERO`

Reviewed By: avp

Differential Revision: D62153166

Pulled By: neildhar

fbshipit-source-id: d45c55ae7ffbfbc38ec4b331339859b8c96486c7
facebook-github-bot pushed a commit that referenced this issue Sep 17, 2024
…1483)

Summary:
Original Author: 85783070+shubhamguptadream11@users.noreply.github.com
Original Git: a75e14a
Original Reviewed By: avp
Original Revision: D62153166

Fixes following issue:
- #1466
- #1138
- #789

**Why this change is made?**

There are few cases where signDisplay is not being handled correctly.
Examples:
```
1.
new Intl.NumberFormat('de-DE', {
 style: 'currency',
 currency: 'EUR',
 signDisplay: 'exceptZero',
}).format(8537.71)

Output: +8,537,71+
Expected: +8.537,71 €

2.
new Intl.NumberFormat('ja-JP', {
 style: 'currency',
 currency: 'JPY',
 signDisplay: 'exceptZero'
}).format(123456.789)

Output: +123,457
Expected: +¥123,457
```

**ChangeLog**

This PR updates the implementation and testing of the signDisplay functionality in the DecimalFormat class within the Hermes engine, specifically for Android API level 31 and above.

**Key Changes:**

Implementation:
- Integrated the **setSignAlwaysShown** method of DecimalFormat for API level 31 and above to control the display of the sign (+ or -) based on the signDisplay option. [For more detail about setSignAlwaysShown check [here](https://developer.android.com/reference/android/icu/text/DecimalFormat#setSignAlwaysShown(boolean))]
- For API levels below 31, maintained the existing logic for handling sign display, ensuring backward compatibility.

Pull Request resolved: #1483

Pulled By: neildhar

Reviewed By: neildhar

Differential Revision: D62883944

fbshipit-source-id: df41029b45400b9db038f32727293d5ccda27211
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants