Distinguish transparent colors from undefined props - #58093
Conversation
|
@Abbondanzo has imported this pull request. If you are a Meta employee, you can view this in D117292692. |
It's fine, thank you. |
|
Outcome: pass @brentvatne — here is the verification. The pull request is by ngocdevv. It fixes #58085, filed by t0maboro. The cause is confirmed. On Android People hitting this can work around it today. Use a transparent color with non-zero RGB, for example A maintainer must decide three things. First, this change is wider than its title. It also makes Environment, and what I could not runLinux sandbox, Debian 12, g++ 12.2.0. No simulator and no emulator ran. There are no screenshots, because nothing decisive in this run happened on a device. What is measured and what is read. The measured unit is No Android build. This sandbox has no Android SDK or NDK, so I did not confirm that the whole Android C++ tree still compiles. That is the largest residual risk, because the change replaces a scalar type with a struct across every props struct on Android. I considered a GitHub Actions No host test job covers this code. The new test does not run in this repository. Not covered. I did not run the reporter's What the reporter asked for, and whether this answers itI read #58085 in the sandbox. The reporter states the symptom this way: with Props 2.0 enabled, Their own diagnosis names The reporter also links a third-party case, I did not run their repro, so I did not observe the Procedure and resultI cloned the repository in the sandbox and checked out two revisions: base One harness compiles against the real headers of each revision. It includes The base arm needs one extra flag to compile at all: This is the whole harness: // Standalone harness: exercises the REAL Color.h / android HostPlatformColor.h
// from the react-native tree under test. Mirrors the SharedColor comparison
// that Props 2.0 getDiffProps() performs: if (color != oldProps->color) {...}
#include <react/renderer/graphics/Color.h>
#include <cstdio>
#include <unordered_set>
using namespace facebook::react;
static int failures = 0;
static void check(bool cond, const char* what) {
std::printf("%-62s %s\n", what, cond ? "PASS" : "FAIL");
if (!cond) failures++;
}
// Same conversion fromRawValueShared.h performs for a JS integer color.
static SharedColor fromArgb(int64_t argb) {
ColorComponents c{};
float ratio = 255.f;
c.alpha = ((argb >> 24) & 0xFF) / ratio;
c.red = ((argb >> 16) & 0xFF) / ratio;
c.green = ((argb >> 8) & 0xFF) / ratio;
c.blue = (argb & 0xFF) / ratio;
return colorFromComponents(c);
}
int main() {
SharedColor absent; // prop not supplied
SharedColor transparent = fromArgb(0); // JS: 'transparent' / 'rgba(0,0,0,0)'
SharedColor opaqueRed = fromArgb(0xFFFF0000);
check(absent != transparent,
"absent prop != explicit transparent (mount diff emits it)");
check(!(absent == transparent), "operator== agrees");
check(static_cast<bool>(transparent),
"explicit transparent is truthy (SharedColor::operator bool)");
check(!static_cast<bool>(absent), "absent prop is falsy");
check(transparent == clearColor(), "clearColor() == explicit transparent");
check(absent != clearColor(), "clearColor() != absent prop");
// Values that must NOT change.
check(static_cast<int32_t>(*transparent) == 0, "transparent serializes to 0");
check(static_cast<int32_t>(*absent) == 0, "absent serializes to 0");
check(static_cast<int32_t>(*opaqueRed) == static_cast<int32_t>(0xFFFF0000),
"opaque red round-trips to 0xFFFF0000");
check(opaqueRed != transparent, "opaque red != transparent");
check(opaqueRed == fromArgb(0xFFFF0000), "opaque red == same opaque red");
check(alphaFromColor(opaqueRed) == 255 && redFromColor(opaqueRed) == 255 &&
greenFromColor(opaqueRed) == 0 && blueFromColor(opaqueRed) == 0,
"channel accessors unchanged for opaque red");
check(!isColorMeaningful(transparent), "isColorMeaningful(transparent)==false");
check(isColorMeaningful(opaqueRed), "isColorMeaningful(opaqueRed)==true");
check(*blackColor() == *fromArgb(0xFF000000), "blackColor() unchanged");
check(*whiteColor() == *fromArgb(0xFFFFFFFF), "whiteColor() unchanged");
// BoxShadow / gradient / drop-shadow path: coerceColor("transparent") ends in
// fromCSSColor(CSSColor{0,0,0,0}) -> hostPlatformColorFromRGBA(0,0,0,0).
// Those parsers do `if (!color) { return {}; }` and drop the WHOLE prop.
SharedColor cssTransparent = hostPlatformColorFromRGBA(0, 0, 0, 0);
check(static_cast<bool>(cssTransparent),
"fromCSSColor(transparent) truthy: boxShadow/gradient keep it");
SharedColor cssRed = hostPlatformColorFromRGBA(255, 0, 0, 255);
check(static_cast<bool>(cssRed), "fromCSSColor(red) truthy (unchanged)");
// std::hash must still work and must separate the two states.
std::unordered_set<SharedColor> set;
set.insert(absent);
set.insert(transparent);
check(set.size() == 2, "std::hash<SharedColor>: absent and transparent differ");
std::printf("\nsizeof(Color)=%zu sizeof(SharedColor)=%zu\n", sizeof(Color),
sizeof(SharedColor));
std::printf("%s\n", failures == 0 ? "ALL PASS" : "SOME FAILED");
return failures == 0 ? 0 : 1;
}Raw output, base Raw output, head I also ran the repository's own generator test for the changed codegen template, after So the committed snapshot matches the changed generator. Cause
Props 2.0 emits a prop only when the values differ. See
iOS does not have this defect. Behavior that changes beyond the descriptionThe description says the change affects Props 2.0 reconciliation. That path is behind The change also flips
Cases 1, 2 and 3 reach I did not measure any of the five on a device. I read the code and measured the predicate. A maintainer should decide whether these belong in this pull request, and whether they want test coverage for them. Review notes
Size. The Implicit conversions. The new Android Duplicates. The supplied related list is empty. I did not run my own search for duplicate issues or competing pull requests. |
|
Just a heads up, landing this today! Sorry for the radio silence--wanted to hold off until the breaking change window opened yesterday to get this in |
Summary:
Fixes #58085.
Android represents both an undefined color and explicit transparent black as ARGB
0.SharedColorpreviously compared only that raw value, so Props 2.0 considered an explicitly supplied transparent color equal to an absent prop and omitted it from the mount diff.This change tracks color presence separately from the platform value and includes it in equality, boolean conversion, and hashing. Platform parsers and the few call sites that intentionally produce an undefined color now preserve that state explicitly.
Changelog:
[ANDROID] [FIXED] - Preserve explicitly transparent colors during Props 2.0 reconciliation.
Test Plan:
ColorTest.testTransparentColorIsDistinctFromUndefined.Color.cpp; it failed before this change and passes afterward.ReactAndroidCMake Debug forarm64-v8asuccessfully.git diff --check.