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

RN 0.72 - Text component dynamic color style issue #37944

Closed
yungblud opened this issue Jun 17, 2023 · 16 comments
Closed

RN 0.72 - Text component dynamic color style issue #37944

yungblud opened this issue Jun 17, 2023 · 16 comments
Labels
Needs: Triage 🔍 Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules)

Comments

@yungblud
Copy link

Description

I've reported this issue on RN 0.72 Working group

But when I use 0.71.8 with new architecture, this issue was not happened.

With RN 0.72.0-rc.6 (even rc.5), I am having an issue with Text component color style issue.

React Native Version

0.72.0-rc.6

Output of npx react-native info

System:
OS: macOS 13.0
CPU: (10) arm64 Apple M1 Pro
Memory: 567.73 MB / 16.00 GB
Shell:
version: 5.8.1
path: /bin/zsh
Binaries:
Node:
version: 18.12.1
path: ~/.nvm/versions/node/v18.12.1/bin/node
Yarn:
version: 1.22.19
path: ~/.nvm/versions/node/v18.12.1/bin/yarn
npm:
version: 8.19.2
path: ~/.nvm/versions/node/v18.12.1/bin/npm
Watchman:
version: 2023.04.24.00
path: /opt/homebrew/bin/watchman
Managers:
CocoaPods:
version: 1.12.0
path: /Users/paul/.rvm/gems/ruby-2.7.5/bin/pod
SDKs:
iOS SDK:
Platforms:
- DriverKit 22.1
- iOS 16.1
- macOS 13.0
- tvOS 16.1
- watchOS 9.1
Android SDK:
API Levels:
- "23"
- "24"
- "28"
- "30"
- "31"
- "32"
- "33"
Build Tools:
- 29.0.2
- 30.0.2
- 30.0.3
- 31.0.0
- 32.0.0
- 32.1.0
- 33.0.0
- 34.0.0
System Images:
- android-23 | Google APIs ARM 64 v8a
- android-24 | Google APIs ARM 64 v8a
- android-29 | Google Play ARM 64 v8a
- android-31 | Google Play ARM 64 v8a
- android-32 | Google APIs ARM 64 v8a
- android-32 | Google Play ARM 64 v8a
- android-33 | Google Play ARM 64 v8a
- android-TiramisuPrivacySandbox | Google Play ARM 64 v8a
Android NDK: Not Found
IDEs:
Android Studio: 2022.2 AI-222.4459.24.2221.9971841
Xcode:
version: 14.1/14B47b
path: /usr/bin/xcodebuild
Languages:
Java:
version: 11.0.11
path: /usr/bin/javac
Ruby:
version: 2.7.5
path: /Users/paul/.rvm/rubies/ruby-2.7.5/bin/ruby
npmPackages:
"@react-native-community/cli": Not Found
react:
installed: 18.2.0
wanted: 18.2.0
react-native:
installed: 0.72.0-rc.6
wanted: 0.72.0-rc.6
react-native-macos: Not Found
npmGlobalPackages:
"react-native": Not Found
Android:
hermesEnabled: true
newArchEnabled: true
iOS:
hermesEnabled: true
newArchEnabled: true

Steps to reproduce

const [colorValue, setColorValue] = useState<string>('blue');

return (
          <View
            style={{ justifyContent: 'center', alignItems: 'center', flex: 1 }}>
            <Button
              title="Change Color"
              onPress={() => setColorValue('orange')}
            />
            <Text style={{ color: colorValue }}>Hello</Text>
          </View>
)

Text component's color style should be changed when I press Button.
But it doesn't.

But change with other style props like fontWeight. it's working.

Snack, code example, screenshot, or link to a repository

const [colorValue, setColorValue] = useState<string>('blue');

return (
          <View
            style={{ justifyContent: 'center', alignItems: 'center', flex: 1 }}>
            <Button
              title="Change Color"
              onPress={() => setColorValue('orange')}
            />
            <Text style={{ color: colorValue }}>Hello</Text>
          </View>
)
@yungblud yungblud added Needs: Triage 🔍 Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules) labels Jun 17, 2023
@yungblud
Copy link
Author

I think this issue is not triggering on rc.0

It's started from rc.1

@yungblud
Copy link
Author

05ba16a

bool EmptyReactNativeConfig::getBool(const std::string &param) const {
  if (param == "react_fabric:enabled_layout_animations_ios") {
    return true;
  }
  // if (param == "react_fabric:enable_nstextstorage_caching") {
  //   return true;
  // }
  return false;
}

I think text storage caching is an issue.
When I commented like above, the caching issue is not happenening.

@yungblud
Copy link
Author

I think the major issue is from TextMeasureCache.h

There are some missing caches about text attributes.

@yungblud
Copy link
Author

#37946

@cortinico
Copy link
Contributor

I think this issue is not triggering on rc.0

Can you confirm this?

I think the major issue is from TextMeasureCache.h
There are some missing caches about text attributes.

cc @NickGerleman

@yungblud
Copy link
Author

@cortinico
I've tested it with npx react-native@latest init RN0720RC0 --version 0.72.0-rc.0 and npx react-native@latest init RN0720RC1 --version 0.72.0-rc.1 with new architecture enabled.

and also I saw some commits about text caching was started from rc.1 from v0.72.0-rc.0...v0.72.0-rc.1

@NickGerleman
Copy link
Contributor

I think this issue is not triggering on rc.0

Can you confirm this?

I think the major issue is from TextMeasureCache.h
There are some missing caches about text attributes.

cc @NickGerleman

I believe this may be related to the text caching work done by @sammy-SC, who I added as a reviewer to the PR. I unfortunately do not have context here.

@sammy-SC
Copy link
Contributor

@yungblud thank you for the report and narrowing it down to NSTextStorage caching. I think it is coming from not invalidating cached NSTextStorage when node is cloned with new props. I will look into this.

@lunaleaps lunaleaps added the 0.72 label Jun 22, 2023
@kelset
Copy link
Contributor

kelset commented Jun 26, 2023

@sammy-SC any news on your end on this?

@sammy-SC
Copy link
Contributor

@kelset I have a fix but I'm still verifying it. I'm still debating myself whether it wouldn't be safer to disable it for now since it is behind a feature flag.

sammy-SC added a commit to sammy-SC/react-native that referenced this issue Jun 26, 2023
Summary:
changelog:
[iOS][fix]: Correctly invalidate NSTextStorage when non layout related props change

Fixes: facebook#37944

Problem:
NSTextStorage was not invalidated if non-layout props were changed. So for example 'color' dynamically changed, it wouldn't get invalidated and font of incorrect color would be rendered on screen.

Reviewed By: javache

Differential Revision: D47019250

fbshipit-source-id: 1116f76e86b356b0ee4c1035a6d6cd4541b0e830
sammy-SC added a commit to sammy-SC/react-native that referenced this issue Jun 26, 2023
Summary:
Pull Request resolved: facebook#38070

changelog:
[iOS][fix]: Correctly invalidate NSTextStorage when non layout related props change

Fixes: facebook#37944

Problem:
NSTextStorage was not invalidated if non-layout props were changed. So for example 'color' dynamically changed, it wouldn't get invalidated and font of incorrect color would be rendered on screen.

Reviewed By: javache

Differential Revision: D47019250

fbshipit-source-id: ba33d542dad2eaa0c3effcf827ad03f37fb11cc0
sammy-SC added a commit to sammy-SC/react-native that referenced this issue Jun 26, 2023
Summary:
Pull Request resolved: facebook#38070

changelog:
[iOS][fix]: Correctly invalidate NSTextStorage when non layout related props change

Fixes: facebook#37944

Problem:
NSTextStorage was not invalidated if non-layout props were changed. So for example 'color' dynamically changed, it wouldn't get invalidated and font of incorrect color would be rendered on screen.

Reviewed By: javache

Differential Revision: D47019250

fbshipit-source-id: 7926253c627788673b2f008d83b15b5374339fbc
@sammy-SC
Copy link
Contributor

@kelset I would opt for safer approach and disable the feature for next 0.72 patch release. What do you think?

@kelset
Copy link
Contributor

kelset commented Jun 27, 2023

I don't think I have enough understanding of the details to be able to give any actual insights 😅 if you think it's safer, let's go for that. It shouldn't cause any regressions right?

(and if that's the case, does it mean that we don't want to cp the commit you landed on main?)

@kelset
Copy link
Contributor

kelset commented Jun 28, 2023

@sammy-SC can you let me know what you end up deciding? 'cause if you want to go down the "disable feature" path I think it'd be better for you to setup a PR against the 0.72-stable branch with that, and we won't attempt to cherry-pick the fix you made here 247da6e (which, also, has merge conflicts so we'd skip for 0.72.1)

sammy-SC added a commit to sammy-SC/react-native that referenced this issue Jun 29, 2023
Summary:
changelog: [iOS] Disable NSTextStorage caching in OSS

A [bug was reported](facebook#37944) for NSTextStorage caching. Even thought I fixed the bug in D47019250, I want to disable the feature in OSS until the fix is verified in Facebook app.
My plan is to pick this commit for 0.72.1 and reenable NSTextStorage caching once the fix is validated.

Differential Revision: D47127912

fbshipit-source-id: 016de19b926b72195dbda1824d43e697fb98e85b
sammy-SC added a commit to sammy-SC/react-native that referenced this issue Jun 30, 2023
Summary:
Pull Request resolved: facebook#38129

changelog: [iOS] Disable NSTextStorage caching in OSS

A [bug was reported](facebook#37944) for NSTextStorage caching. Even thought I fixed the bug in D47019250, I want to disable the feature in OSS until the fix is verified in Facebook app.
My plan is to pick this commit for 0.72.1 and reenable NSTextStorage caching once the fix is validated.

Reviewed By: NickGerleman

Differential Revision: D47127912

fbshipit-source-id: 76d94e6e4c14a989765212faff57c78d91f176ee
facebook-github-bot pushed a commit that referenced this issue Jun 30, 2023
Summary:
Pull Request resolved: #38129

changelog: [iOS] Disable NSTextStorage caching in OSS

A [bug was reported](#37944) for NSTextStorage caching. Even thought I fixed the bug in D47019250, I want to disable the feature in OSS until the fix is verified in Facebook app.
My plan is to pick this commit for 0.72.1 and reenable NSTextStorage caching once the fix is validated.

Reviewed By: NickGerleman

Differential Revision: D47127912

fbshipit-source-id: 97694e383eb751e89b776c0599969f2c411bac6f
@billnbell
Copy link
Contributor

I am having this same problem. When I set color: '#FFFFFF' to color: '#000000' it does not work when I set a state in the return().

@billnbell
Copy link
Contributor

Happening on 0.72.1

@billnbell
Copy link
Contributor

File: react-native/ReactCommon/react/renderer/components/text/ParagraphLayoutManager.cpp for RN 0.72.1:

diff --git a/node_modules/react-native/ReactCommon/react/renderer/components/text/ParagraphLayoutManager.cpp b/node_modules/react-native/ReactCommon/react/renderer/components/text/ParagraphLayoutManager.cpp
index 1f4cbc4..1ad563b 100644
--- a/node_modules/react-native/ReactCommon/react/renderer/components/text/ParagraphLayoutManager.cpp
+++ b/node_modules/react-native/ReactCommon/react/renderer/components/text/ParagraphLayoutManager.cpp
@@ -29,15 +29,12 @@ TextMeasurement ParagraphLayoutManager::measure(
     return cachedTextMeasurement_;
   }
   if (CoreFeatures::cacheNSTextStorage) {
-    size_t newHash = folly::hash::hash_combine(
-        0,
-        textAttributedStringHashLayoutWise(attributedString),
-        paragraphAttributes);
-
-    if (!hostTextStorage_ || newHash != hash_) {
+    size_t newParagraphInputHash =
+      folly::hash::hash_combine(0, attributedString, paragraphAttributes);
+    if (!hostTextStorage_ || newParagraphInputHash != hash_) {
       hostTextStorage_ = textLayoutManager_->getHostTextStorage(
           attributedString, paragraphAttributes, layoutConstraints);
-      hash_ = newHash;
+      hash_ = newParagraphInputHash;
     }
   }

kelset pushed a commit that referenced this issue Jul 10, 2023
Summary:
Pull Request resolved: #38129

changelog: [iOS] Disable NSTextStorage caching in OSS

A [bug was reported](#37944) for NSTextStorage caching. Even thought I fixed the bug in D47019250, I want to disable the feature in OSS until the fix is verified in Facebook app.
My plan is to pick this commit for 0.72.1 and reenable NSTextStorage caching once the fix is validated.

Reviewed By: NickGerleman

Differential Revision: D47127912

fbshipit-source-id: 97694e383eb751e89b776c0599969f2c411bac6f
Kudo pushed a commit to expo/react-native that referenced this issue Jul 11, 2023
Summary:
Pull Request resolved: facebook#38129

changelog: [iOS] Disable NSTextStorage caching in OSS

A [bug was reported](facebook#37944) for NSTextStorage caching. Even thought I fixed the bug in D47019250, I want to disable the feature in OSS until the fix is verified in Facebook app.
My plan is to pick this commit for 0.72.1 and reenable NSTextStorage caching once the fix is validated.

Reviewed By: NickGerleman

Differential Revision: D47127912

fbshipit-source-id: 97694e383eb751e89b776c0599969f2c411bac6f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Triage 🔍 Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants