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

Header height not getting calculated in pageY value for components in new architecture #43500

Closed
ManiTWIndia opened this issue Mar 15, 2024 · 9 comments
Labels
Issue: Author Provided Repro This issue can be reproduced in Snack or an attached project. Needs: Attention Issues where the author has responded to feedback. Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules)

Comments

@ManiTWIndia
Copy link

Description

When using default header from the stack navigation, facing inconsistency in pageY value of component rendered in old and new architecture. In the example screenshots, I am passing screenOptions={{headerStyle: {height: 150}}} to Stack navigator in React native new architecture and the pageY is being returned without taking the header height into consideration.

The issue happens in iOS new architecture only. In old architecture, the header height is taken into consideration when calculating pY value

Steps to reproduce

https://github.com/ManiTWIndia/stack-navigation-header-issue

React Native Version

0.73.6

Affected Platforms

Runtime - iOS

Areas

Fabric - The New Renderer

Output of npx react-native info

npx react-native info
info Fetching system and libraries information...
System:
  OS: macOS 14.4
  CPU: (8) arm64 Apple M1 Pro
  Memory: 1.53 GB / 16.00 GB
  Shell:
    version: "5.9"
    path: /bin/zsh
Binaries:
  Node:
    version: 16.15.1
    path: ~/.nvm/versions/node/v16.15.1/bin/node
  Yarn:
    version: 1.22.19
    path: ~/.nvm/versions/node/v16.15.1/bin/yarn
  npm:
    version: 8.11.0
    path: ~/.nvm/versions/node/v16.15.1/bin/npm
  Watchman:
    version: 2024.01.22.00
    path: /opt/homebrew/bin/watchman
Managers:
  CocoaPods:
    version: 1.14.3
    path: /Users/manikanda.s/.rvm/rubies/ruby-2.7.6/bin/pod
SDKs:
  iOS SDK:
    Platforms:
      - DriverKit 23.4
      - iOS 17.4
      - macOS 14.4
      - tvOS 17.4
      - visionOS 1.1
      - watchOS 10.4
  Android SDK: Not Found
IDEs:
  Android Studio: 2023.1 AI-231.9392.1.2311.11076708
  Xcode:
    version: 15.3/15E204a
    path: /usr/bin/xcodebuild
Languages:
  Java:
    version: 17.0.10
    path: /opt/homebrew/opt/openjdk@17/bin/javac
  Ruby:
    version: 2.7.6
    path: /Users/manikanda.s/.rvm/rubies/ruby-2.7.6/bin/ruby
npmPackages:
  "@react-native-community/cli": Not Found
  react:
    installed: 18.2.0
    wanted: 18.2.0
  react-native:
    installed: 0.73.6
    wanted: 0.73.6
  react-native-macos: Not Found
npmGlobalPackages:
  "*react-native*": Not Found
Android:
  hermesEnabled: true
  newArchEnabled: false
iOS:
  hermesEnabled: true
  newArchEnabled: true

Stacktrace or Logs

NA

Reproducer

https://github.com/ManiTWIndia/stack-navigation-header-issue

Screenshots and Videos

iOS New architecture
iOS new architecture

iOS Old architecture
iOS old architecture

@ManiTWIndia ManiTWIndia added Needs: Triage 🔍 Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules) labels Mar 15, 2024
@cortinico cortinico added Issue: Author Provided Repro This issue can be reproduced in Snack or an attached project. and removed Needs: Triage 🔍 labels Mar 15, 2024
@cortinico
Copy link
Contributor

@ManiTWIndia Thanks for reporting this.
I believe the issue here is that you're using react-native-screens under the hood which is affected by this bug:
#36710

Can you try again on 0.74 + do a patch-package of react-native-screens

software-mansion/react-native-screens#2028

and report back if the issue is fixed?

@ManiTWIndia
Copy link
Author

Hi @cortinico I am facing some issues in using patch-package of react-native-screens. There are lot of commits after the last release version. Can we get a beta release of react-native-screens to test it out in local?
Thanks

@github-actions github-actions bot added Needs: Attention Issues where the author has responded to feedback. and removed Needs: Author Feedback labels Mar 25, 2024
@tboba
Copy link

tboba commented Mar 25, 2024

Hi @ManiTWIndia, we will be releasing a new version of react-native-screens today (or tomorrow), so I believe beta won't be needed for now! 😄
The release will include fixes with incorrect target of touchables, so this issue should be resolved after updating screens to 3.30.0 version (or higher ones).

@ManiTWIndia
Copy link
Author

ManiTWIndia commented Mar 26, 2024

@tboba @cortinico With and without header, its still showing the same pY value. I have updated and using the latest packages in the project. I have pushed the changes. Can you please check in the repo to reverify the issue?
Thanks

WithoutHeader
WithHeader

FYI, I am running the project with RN 0.73.6
When running with RN 0.74, facing below issue when running ios

CompileC /Users/manikanda.s/Library/Developer/Xcode/DerivedData/popoverIssueNew-cmchexkcvzazmngmguhjojjurspp/Build/Intermediates.noindex/Pods.build/Debug-iphonesimulator/react-native-safe-area-context.build/Objects-normal/arm64/RNCSafeAreaViewShadowNode.o /Users/manikanda.s/Desktop/TWMarch/popoverIssueNew/node_modules/react-native-safe-area-context/common/cpp/react/renderer/components/safeareacontext/RNCSafeAreaViewShadowNode.cpp normal arm64 c++ com.apple.compilers.llvm.clang.1_0.compiler (in target 'react-native-safe-area-context' from project 'Pods')

@tboba
Copy link

tboba commented Mar 26, 2024

@ManiTWIndia Yeah, I can reproduce that on your example. However, after changing the navigator from stack to native-stack, the button has correct target!
image

I'm curious why this bug still occurs on JS stack, maybe because it still uses ~3.20.0 as the dev dependency of stack?
cc @satya164 Do you know if we somehow need to bump screens version to make this issue fixed? I know that screens are defined as a peer dependency in stack, but maybe defining it also as the dev dependency impacts that "contract"?

@satya164
Copy link
Contributor

Do you know if we somehow need to bump screens version to make this issue fixed? I know that screens are defined as a peer dependency in stack, but maybe defining it also as the dev dependency impacts that "contract"?

Nope, only the version installed and linked into the project can impact it.

@cortinico
Copy link
Contributor

@ManiTWIndia are you still able to reproduce on 0.74 with the latest version of react-native-screens? As I believe this is practically fixed

@WoLewicki
Copy link
Contributor

It didn't work on normal stack probably due to my mistake with ifs. It should be fixed with this PR: software-mansion/react-native-screens#2107. Could you check it?

@cortinico
Copy link
Contributor

Closing as potentially fixed by:

As @ManiTWIndia we haven't heard from you, we assume this is fixed. Please get back to us if this is not fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue: Author Provided Repro This issue can be reproduced in Snack or an attached project. Needs: Attention Issues where the author has responded to feedback. Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules)
Projects
None yet
Development

No branches or pull requests

5 participants