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

[iOS] Opacity prop of Text applies double times. #24229

Closed
PaxLyj opened this issue Apr 1, 2019 · 16 comments
Closed

[iOS] Opacity prop of Text applies double times. #24229

PaxLyj opened this issue Apr 1, 2019 · 16 comments
Labels
Bug Platform: iOS iOS applications. Resolution: Locked This issue was locked by the bot.

Comments

@PaxLyj
Copy link

PaxLyj commented Apr 1, 2019

🐛 Bug Report

On iOS, Text component with style prop opacity applies twice.
It seems the base class of RCTTextView is RCTView, and it causes the style prop opacity applies double times to RCTTevtView.

Roughly, delete this line in RCTBaseTextViewManager.m fix this issue.

To Reproduce

//Please check the example code below.
const styles = StyleSheet.create({
  rgba: {
    fontSize: 20,
    marginBottom: 10,
    color: 'rgba(0, 0, 0, 0.25)'
  },
  opacity_50: {
    fontSize: 20,
    marginBottom: 10,
    color: '#000',
    opacity: 0.5
  },
  opacity_25: {
    fontSize: 20,
    marginBottom: 10,
    color: '#000',
    opacity: 0.25
  },
  hexa: {
    fontSize: 20,
    color: '#00000040' //rrggbbaa
  }
}

Expected Behavior

It should be rendered as same as Android (right image above).

Code Example

https://snack.expo.io/Hy8RhMkYN

In iOS environment, compare the rendered colors of Text component with style { opacity: 0.5 } and { color: 'rgba(0, 0, 0, 0.25)' } - those are seems same.

Environment

React Native Environment Info:
    System:
      OS: macOS 10.14
      CPU: (4) x64 Intel(R) Core(TM) i5-7500 CPU @ 3.40GHz
      Memory: 225.85 MB / 16.00 GB
      Shell: 3.2.57 - /bin/bash
    Binaries:
      Node: 11.5.0 - ~/.nvm/versions/node/v11.5.0/bin/node
      Yarn: 1.10.1 - /usr/local/bin/yarn
      npm: 6.4.1 - ~/.nvm/versions/node/v11.5.0/bin/npm
      Watchman: 4.9.0 - /usr/local/bin/watchman
    SDKs:
      iOS SDK:
        Platforms: iOS 12.1, macOS 10.14, tvOS 12.1, watchOS 5.1
      Android SDK:
        API Levels: 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28
        Build Tools: 23.0.1, 23.0.2, 23.0.3, 25.0.2, 25.0.3, 26.0.0, 26.0.1, 26.0.2, 26.0.3, 27.0.3, 28.0.2, 28.0.3
        System Images: android-19 | Google APIs Intel x86 Atom, android-22 | Google APIs Intel x86 Atom, android-25 | Google Play Intel x86 Atom, android-26 | Google Play Intel x86 Atom
    IDEs:
      Android Studio: 3.2 AI-181.5540.7.32.5056338
      Xcode: 10.1/10B61 - /usr/bin/xcodebuild
    npmPackages:
      react: 16.8.3 => 16.8.3 
      react-native: 0.59.2 => 0.59.2 
    npmGlobalPackages:
      react-native-cli: 2.0.1
      react-native-create-library: 3.1.2
@react-native-bot react-native-bot added the Platform: iOS iOS applications. label Apr 1, 2019
@shergin
Copy link
Contributor

shergin commented Apr 1, 2019

cc @ericlewis

@lukewalczak
Copy link
Contributor

I think the problem exists in text attributes file where opacity value is multiplied by textAttributes->_opacity. Removing that multiplication should fix the problem.

@michalchudziak
Copy link
Contributor

@lukewalczak Perhaps you're right. Would you mind sending a PR with your proposal?

@ericlewis
Copy link
Contributor

Another angle to look at this would be how we solved semi-transparent backgrounds here: #23872

It’s likely we can apply a similar solution for opacities.

@lukewalczak
Copy link
Contributor

Hmm, but what can be the reason to keep that multiplication in opacity value?

@ericlewis
Copy link
Contributor

@lukewalczak indeed, the multiplication does seem like a mistake that probably needs to be fixed. If it addresses the issue at hand then cool. I can open a PR if you like, it’s a relatively simple thing to test.

@ericlewis
Copy link
Contributor

Seems like the * was meant to be a comment like the rest, just bad copy paste or something.

@lukewalczak
Copy link
Contributor

I've also tested your approach and setting the initial value for self.textAttributes.opacity = 1.0 is fixing the issue.

@PaxLyj
Copy link
Author

PaxLyj commented Apr 2, 2019

@lukewalczak Nice, both solutions solve the problem. Thanks for work.
It seems simpler and clearer to remove * textAttributes->_opacity from the line that you mentioned.

@shergin
Copy link
Contributor

shergin commented Apr 4, 2019

Let's discuss that in a PR!

@lukewalczak
Copy link
Contributor

Unfortunately, the solution above is fixing the problem only for text without backgroundColor. I've spent some time debugging the issue, but cannot find a solution which will fix it completely.

@shergin
Copy link
Contributor

shergin commented Apr 12, 2019

I reread everything one more time, and now I actually think that iOS behavior is correct, and Android one is not.

@PaxLyj From the code example in the description is not clear which kind of nesting of components we have, can you improve the example?

@ericlewis, @lukewalczak What makes you think that we should NOT multiply opacity? That's how opacity works in layers, right? What should render this example: <div opacity=0.5><div opacity=1>hello world</div></div>?

@shergin
Copy link
Contributor

shergin commented Apr 12, 2019

cc @rigdern

@shergin
Copy link
Contributor

shergin commented Apr 12, 2019

Oh, I see now, the issue is not about nested text components at all!

So, the solution should be pretty simple, we should just add
self.textAttributes.opacity = NAN; similarly to what we did in #23872.

Who wants to contribute? 😉

@lukewalczak
Copy link
Contributor

lukewalczak commented Apr 13, 2019

Hey, I've checked your PR and it's fixing the problem with a text but unfortunately, once you add a backgroundColor it seems to be broken again.

Screenshot 2019-04-14 at 00 54 10

@PaxLyj
Copy link
Author

PaxLyj commented Apr 15, 2019

@lukewalczak Well, you may thought that text with OPACITY BG is not rendered as translucent, right?
The conbined style of that text component is -

  color: '#000000',
  backgroundColor: '#ff0000',
  opacity: 0.5,

and I think it's reasonable that applying opacity to Textcomponent once, make both color of text and background as 50% of translucency. (Which is as shown in the image you attached.)

On the other hand, the text components of both RGBA BG and HEXA BG has 50% of translucency, so the colors of texts are mixed with each color of background.

grabbou pushed a commit that referenced this issue May 6, 2019
Summary:
This PR fixes #24229.
Seems currently `opacity` props for Text is being applied twice (one for text color and one for the whole view). This PR disables applying the prop to the text.

[CATEGORY] [TYPE] - Fixed double applying opacity prop for Text
Pull Request resolved: #24435

Differential Revision: D14932795

Pulled By: cpojer

fbshipit-source-id: f9280fc75f788424cb5f1e42d2e79efdb354d645
@facebook facebook locked as resolved and limited conversation to collaborators Apr 15, 2020
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Apr 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Platform: iOS iOS applications. Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants