facebook / react-native

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

Setting `left: auto` and `right` to a certain value does not change the position of the `View` #45817

Open pklatka opened 1 month ago

pklatka commented 1 month ago

Description

Setting left: auto and right to a certain value has no effect on a new architecture. On the old architecture, a container with position: absolute/relative would change its position according to the web rules.

Steps to reproduce

  1. Clone the reproducer
  2. Install project dependencies with npm
  3. Run the app with npm run ios or npm run android
  4. Click the "change right value" button and observe that view is not changing its position

React Native Version

0.74.3

Affected Platforms

Runtime - Android, Runtime - iOS

Areas

Fabric - The New Renderer

Output of npx react-native info

System:
  OS: macOS 14.5
  CPU: (11) arm64 Apple M3 Pro
  Memory: 363.97 MB / 18.00 GB
  Shell:
    version: "5.9"
    path: /bin/zsh
Binaries:
  Node:
    version: 20.11.1
    path: /usr/local/bin/node
  Yarn:
    version: 1.22.21
    path: /opt/homebrew/bin/yarn
  npm:
    version: 10.2.4
    path: /usr/local/bin/npm
  Watchman:
    version: 2024.07.15.00
    path: /opt/homebrew/bin/watchman
Managers:
  CocoaPods:
    version: 1.15.2
    path: /opt/homebrew/bin/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: Not Found
IDEs:
  Android Studio: 2024.1 AI-241.15989.150.2411.11948838
  Xcode:
    version: 15.4/15F31d
    path: /usr/bin/xcodebuild
Languages:
  Java:
    version: 18.0.2
    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.3
    wanted: 0.74.3
  react-native-macos: Not Found
npmGlobalPackages:
  "*react-native*": Not Found
Android:
  hermesEnabled: true
  newArchEnabled: true
iOS:
  hermesEnabled: true
  newArchEnabled: true

Stacktrace or Logs

This issue doesn't produce any logs or crashes.

Reproducer

https://github.com/pklatka/css-left-repro

Screenshots and Videos

New architecture Old architecture
s77rt commented 1 month ago

For the relatively positioned element this seems to be the culprit

https://github.com/facebook/yoga/blob/5009f5c1accb3dde14e1e18930df70c18c70dc75/yoga/node/Node.cpp#L219-L234

// If both left and right are defined, then use left. Otherwise return +left or
// -right depending on which is defined. Ignore statically positioned nodes as
// insets do not apply to them.
float Node::relativePosition(
    FlexDirection axis,
    Direction direction,
    float axisSize) const {
  if (style_.positionType() == PositionType::Static) {
    return 0;
  }
  if (style_.isInlineStartPositionDefined(axis, direction)) {
    return style_.computeInlineStartPosition(axis, direction, axisSize);
  }

  return -1 * style_.computeInlineEndPosition(axis, direction, axisSize);
}

The right style is ignored if left is specified.

j-piasecki commented 1 month ago

To add to the above, Node::relativePosition will be called for both views - the one positioned relatively and the one positioned absolutely.

The difference between the old and the new architecture comes from auto being treaded differently by them. On the old architecture when auto is passed to left or right, it will be set to YGUndefined https://github.com/facebook/react-native/blob/a4cd5994a758b5db8b29d59db332322d626ace80/packages/react-native/React/Views/RCTShadowView.m#L63-L68

https://github.com/facebook/react-native/blob/a4cd5994a758b5db8b29d59db332322d626ace80/packages/react-native/React/Views/RCTShadowView.m#L530-L534

On the new architecture auto is actually converted to an empty StyleLength with the unit of auto https://github.com/facebook/react-native/blob/a4cd5994a758b5db8b29d59db332322d626ace80/packages/react-native/ReactCommon/react/renderer/components/view/conversions.h#L422 This causes the above function to change behavior - since the left position is no longer undefined, it enters the if body and returns 0 there.

Obviously, changing auto to undefined in the above file fixes the issue, though I don't think this is a proper solution for that as it looks like a bug (or a missing feature?) in Yoga. I'd love to hear your opinion on that.

cortinico commented 1 month ago

Obviously, changing auto to undefined in the above file fixes the issue, though I don't think this is a proper solution for that as it looks like a bug (or a missing feature?) in Yoga. I'd love to hear your opinion on that.

CC @NickGerleman here

NickGerleman commented 4 weeks ago

Yes, the right fix for this is in Yoga. Really, Yoga's concept of auto vs undefined is pretty silly/at odds with how CSS values processing works (where an unset value becomes auto by default). In the meantime, teaching the checks in Yoga absolute layout to handle auto instead of just undefined would be the right way to fix this, and probably relatively straightforward (Yoga also has a lot of unit tests, easily runnable from OSS!).

cortinico commented 4 weeks ago

teaching the checks in Yoga absolute layout to handle auto instead of just undefined would be the right way to fix this, and probably relatively straightforward (Yoga also has a lot of unit tests, easily runnable from OSS!).

@j-piasecki @coado is this something you could take on?

j-piasecki commented 4 weeks ago

@cortinico Yeah, I think we can take this one