Fix incorrect view flattening when usign a specific not fully transparent color - #51379
Closed
rubennorte wants to merge 1 commit into
Closed
Fix incorrect view flattening when usign a specific not fully transparent color#51379rubennorte wants to merge 1 commit into
rubennorte wants to merge 1 commit into
Conversation
facebook-github-bot
added
CLA Signed
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D74869311 |
rubennorte
force-pushed
the
export-D74869311
branch
from
May 19, 2025 09:41
d743607 to
47b7417
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D74869311 |
rubennorte
force-pushed
the
export-D74869311
branch
2 times, most recently
from
May 20, 2025 10:46
5897dbd to
ae24f0b
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D74869311 |
rubennorte
force-pushed
the
export-D74869311
branch
from
May 20, 2025 10:50
ae24f0b to
5da4997
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D74869311 |
rubennorte
force-pushed
the
export-D74869311
branch
from
May 20, 2025 10:59
5da4997 to
389ae86
Compare
…rent color (#51379) Summary: Changelog: [General][Fixed] Fix incorrect flattening / non-rendering of views with backgroundColor set to `rgba(255, 255, 255, 127/256)` Fixes #51378. ## Context When testing some unrelated things with Fantom is realized that the color for some text that I wasn't explicitly defining was being set to `rgba(255, 255, 255, 127)`, like here: https://github.com/facebook/react-native/blob/249a24ac756275eadbe3b4df1ff9c974af1671d2/packages/react-native-fantom/src/__tests__/Fantom-itest.js#L540-L542 When digging a bit more about why, I realized that was actually the value for `UndefinedColor`. When looking a bit deeper, I saw that the value for that constant was being set like this: ``` using Color = int32_t; namespace HostPlatformColor { static const facebook::react::Color UndefinedColor = std::numeric_limits<facebook::react::Color>::max(); } ``` I'm not sure what the logic could've been here: - Defining it as a value out of bounds for all valid colors? In this case, it's a 32 bit value so all the range of values are actually valid RGBA colors. - Defining it as a fully opaque white? Seems dangerous for a default because you wouldn't be able to distinguish a explicitly set white color from a non-set color, relevant if you're seeing a white background color in a view on top of another view with any other background color. The result of this existing logic was actually setting `UndefinedColor` to `rgba(255, 255, 255, 127)` because the alpha channel is defined in the first bits of the value, and `Color` being a signed int with 32 bits, the largest value is `01111....1`, so extracting the first 8 bits, you get 127. ## Changes This changes the value set for the `UndefinedColor` constant (which is used, among other things, to determine if a view sets a background color, or otherwise could potentially be flattened). The new value, instead of white with a 127/256 opacity, is black with 0% opacity (or simply the number 0 in `int32_t`). Reviewed By: javache Differential Revision: D74869311
rubennorte
force-pushed
the
export-D74869311
branch
from
May 20, 2025 15:55
389ae86 to
66e587e
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D74869311 |
Collaborator
|
This pull request was successfully merged by @rubennorte in b1e8729 When will my fix make it into a release? | How to file a pick request? |
|
@rubennorte |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
Changelog: [General][Fixed] Fix incorrect flattening / non-rendering of views with backgroundColor set to
rgba(255, 255, 255, 127/256)Fixes #51378.
Context
When testing some unrelated things with Fantom is realized that the color for some text that I wasn't explicitly defining was being set to
rgba(255, 255, 255, 127), like here:https://github.com/facebook/react-native/blob/249a24ac756275eadbe3b4df1ff9c974af1671d2/packages/react-native-fantom/src/__tests__/Fantom-itest.js#L540-L542
When digging a bit more about why, I realized that was actually the value for
UndefinedColor. When looking a bit deeper, I saw that the value for that constant was being set like this:I'm not sure what the logic could've been here:
The result of this existing logic was actually setting
UndefinedColortorgba(255, 255, 255, 127)because the alpha channel is defined in the first bits of the value, andColorbeing a signed int with 32 bits, the largest value is01111....1, so extracting the first 8 bits, you get 127.Changes
This changes the value set for the
UndefinedColorconstant (which is used, among other things, to determine if a view sets a background color, or otherwise could potentially be flattened).The new value, instead of white with a 127/256 opacity, is back with 0% opacity (or simply the number 0 in
int32_t).Differential Revision: D74869311