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

[0.72 New Architecture] added missing text measure cache text attributes #37946

Conversation

yungblud
Copy link

@yungblud yungblud commented Jun 17, 2023

Summary:

Related issue

Changelog:

Added missing text measure cache text attributes

Pick one each for the category and type tags:

[IOS] [FIXED] - added missing text measure cache text attributes

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

Test Plan:

  • init 0.72 with npx react-native@latest init RN0720RC1 --version 0.72.0-rc.6
  • cd RN0720RC1/ios
  • RCT_NEW_ARCH_ENABLED=1 bundle exec pod install --verbose
  • test with iOS simulator with example code
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>
)

@facebook-github-bot
Copy link
Contributor

Hi @yungblud!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@yungblud yungblud changed the title fix: added missing text measure cache text attributes RN 0.72: added missing text measure cache text attributes Jun 17, 2023
@yungblud yungblud changed the title RN 0.72: added missing text measure cache text attributes [0.72 New Architecture] added missing text measure cache text attributes Jun 17, 2023
@yungblud
Copy link
Author

yungblud commented Jun 17, 2023

I'm wondering
Do we need to add all text attributes in RCTTextAttributes.h to TextMeasureCache.h?

@analysis-bot
Copy link

analysis-bot commented Jun 17, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,755,709 -217
android hermes armeabi-v7a 8,067,975 -700
android hermes x86 9,248,595 -935
android hermes x86_64 9,097,380 -301
android jsc arm64-v8a 9,316,900 -230
android jsc armeabi-v7a 8,506,490 -722
android jsc x86 9,380,669 -943
android jsc x86_64 9,633,537 -317

Base commit: af57ce1
Branch: main

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 17, 2023
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@kelset
Copy link
Contributor

kelset commented Jun 19, 2023

@yungblud any specific reasons you are targetting 0.72-stable for this fix? I'd rather it point to main and we cherry-pick to 0.72, so that we ensure that the main codebase and future versions also have this fix.

@yungblud
Copy link
Author

@kelset Oh, right. that is the right way. I will change base branch to main.

@yungblud yungblud changed the base branch from 0.72-stable to main June 19, 2023 10:08
@yungblud yungblud changed the base branch from main to 0.72-stable June 19, 2023 10:09
@yungblud
Copy link
Author

Hmm too many file changes from main.
I will temporarily set the base branch to 0.72-stable.
Will fix it soon.

@yungblud yungblud changed the base branch from 0.72-stable to main June 19, 2023 10:20
@yungblud yungblud force-pushed the fix/0.72-text-measure-cache-attributes branch from 44225ca to b82bb20 Compare June 19, 2023 10:20
@yungblud
Copy link
Author

@kelset
successfully changed base branch to main.

@@ -89,6 +89,9 @@ inline bool areTextAttributesEquivalentLayoutWise(
// attributes that affect only a decorative aspect of displayed text (like
// colors).
return std::tie(
lhs.foregroundColor,
lhs.backgroundColor,
lhs.opacity,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does opacity, backgroundColor and foregroundColor change size/position of text? I'm not how it affects layout of text.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I know TextMeasureCache is for size, position of Text component.
So I'm not sure this is the right way to fix style caching issue.
But it works.

@sammy-SC
Copy link
Contributor

Thank you very much for the PR. I eventually went with slightly different solution: #38070

I still need to test it in production.

@sammy-SC sammy-SC closed this Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Pick Request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants