facebook / react-native

A framework for building native applications using React
https://reactnative.dev
MIT License
119.25k stars 24.34k forks source link

Flatten margin styles are not properly overriding when applied from specific to general properties #46052

Open alexruzenhack opened 3 months ago

alexruzenhack commented 3 months ago

Description

By flattening the styles { marginTop: 15 } and { marginVertical: 0 }, it results in a component with margin: 0, however a "ghost" hidden component retains the marginTop: 15. A complete description can be found here https://github.com/HathorNetwork/hathor-wallet-mobile/issues/532.

Steps to reproduce

Use the flatten style syntax with a flag in a component, like: style={[{ marginTop: 15 }, flag && { marginVertical: 0 }]}

React Native Version

0.75.1 (Verified it happens on latest version)

Affected Platforms

Runtime - Android, Runtime - iOS

Output of npx react-native info

System:
  OS: macOS 14.5
  CPU:
  Memory:
  Shell:
    version: "5.9"
    path:
Binaries:
  Node:
    version: 20.16.0
    path:
  Yarn:
    version: 3.7.0
    path:
  npm:
    version: 10.8.1
    path:
  Watchman: Not Found
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"
    Build Tools:
      - 30.0.2
      - 30.0.3
      - 31.0.0
      - 33.0.0
      - 33.0.2
      - 34.0.0
    System Images:
      - android-30 | Google APIs ARM 64 v8a
      - android-31 | Google APIs ARM 64 v8a
      - android-32 | Google APIs ARM 64 v8a
      - android-33 | Google APIs ARM 64 v8a
      - android-33 | Google Play ARM 64 v8a
    Android NDK: Not Found
IDEs:
  Android Studio: 2022.1 AI-221.6008.13.2211.9619390
  Xcode:
    version: 15.4/15F31d
    path:
Languages:
  Java:
    version: 11.0.19
    path:
  Ruby:
    version: 2.6.10
    path:
npmPackages:
  "@react-native-community/cli": Not Found
  react:
    installed: 18.2.0
    wanted: 18.2.0
  react-native:
    installed: 0.72.5
    wanted: 0.72.5
  react-native-macos: Not Found
npmGlobalPackages:
  "*react-native*": Not Found
Android:
  hermesEnabled: Not found
  newArchEnabled: Not found
iOS:
  hermesEnabled: Not found
  newArchEnabled: Not found

Stacktrace or Logs

Not needed.

Reproducer

https://snack.expo.dev/@alexruzenhack/margin-rules-application https://github.com/dream-sports-labs/reproducer-react-native

Screenshots and Videos

The images are in this issue: https://github.com/HathorNetwork/hathor-wallet-mobile/issues/532

react-native-bot commented 3 months ago
:warning: Unsupported Version of React Native
:information_source: It looks like your issue or the example you provided uses an unsupported version of React Native.

Due to the number of issues we receive, we're currently only accepting new issues against one of the supported versions. Please upgrade to latest and verify if the issue persists (alternatively, create a new project and repro the issue in it). If you cannot upgrade, please open your issue on StackOverflow to get further community support.
react-native-bot commented 3 months ago
:warning: Unsupported Version of React Native
:information_source: It looks like your issue or the example you provided uses an unsupported version of React Native.

Due to the number of issues we receive, we're currently only accepting new issues against one of the supported versions. Please upgrade to latest and verify if the issue persists (alternatively, create a new project and repro the issue in it). If you cannot upgrade, please open your issue on StackOverflow to get further community support.
shubhamguptadream11 commented 3 months ago

@alexruzenhack Here the problem is with marginVertical. It is not able to override marginTop and marginBottom. Even marginHorizontal wasn't able to override marginLeft and marginRight. I am looking into this.

shubhamguptadream11 commented 3 months ago

@alexruzenhack For a simple workaround, you can replace marginVertical:0 with {marginTop:0, marginBottom:0} for now.

shubhamguptadream11 commented 3 months ago

Seems to be very old issue: https://github.com/facebook/react-native/issues/14587. Not fixed yet

shubhamguptadream11 commented 3 months ago

@cortinico Since similiar issue is raised earlier as well. Are we tracking it anywhere or have any plan for this?

I debugged this and found that Yoga applies individual margins (marginLeft, marginRight) before aggregate margins (marginHorizontal, marginVertical). Aggregate margins only set the edges that are not already specified by individual margins. Also this is not only limited to margins even paddingHorizontal and paddingVertical behaves similarly.

cortinico commented 3 months ago

@cortinico Since similiar issue is raised earlier as well. Are we tracking it anywhere or have any plan for this?

Sadly everything that is not New Architecture related is having lower priority at the moment. If the issue have a valid reproducer we apply the "Issue: Author Provided Repro" which won't let them stale.

The problem with this one is that is on 0.72 and we would need the same repro but for 0.75 as 0.72 is currently unsupported

shubhamguptadream11 commented 3 months ago

@cortinico I tested this on main branch with rn-tester as well. Here is the reproducer for this.

One possible solution which I think of currently is implementing flattening of styles in react native world itself. And then for aggregateStyles like paddingHorizontal paddingVertical marginHorizontal and marginVertical replace it with individual style at the very end. Let me know your thoughts on this. Thanks

meetdhanani17 commented 2 months ago

One possible solution I think of currently is implementing flattening styles in the react native world itself. And then for aggregateStyles like paddingHorizontal paddingVertical marginHorizontal and marginVertical replace it with individual style at the very end. Let me know your thoughts on this. Thanks

I think it prioritises marginTop over marginVertical, @shubhamguptadream11 To apply your solution we have to apply rules like if marginTop is set and the user wants to override it by marginVertical or marginVertical declare and override by marginTop at this moment we have to apply rules for prioritise marginTop over marginVertical which is already there