From 8da8f417215cabb28ccb04950b044ec5ae97b076 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20B=C5=82oniarz?= Date: Fri, 16 Oct 2026 02:43:53 -0700 Subject: [PATCH] Fix Image payloads that Props 2.0 sends wrong or drops (#59077) Summary: With Props 2.0 on Android (`enableExclusivePropsUpdateAndroid` and the related flags), `ReactImageManager` receives the payload from `ImageProps::getDiffProps`. Some Image props come out wrong: - Several sources or `srcSet`: crash with `NoSuchKeyException: width`, because each source's size is nested under `size`. - `headers`: never sent, and not cleared when the new source has none. - A source with the same URI but different headers, cache, size or scale: not sent. - A removed `tintColor`: sent as `0`, so the image disappears. This PR sends each of them the way the Java side reads them. Nothing changes with the flags off. ## Changelog: [ANDROID] [FIXED] - Fix Image sources, headers and a removed tintColor with Props 2.0 Pull Request resolved: https://github.com/react/react-native/pull/59077 Test Plan: - New `ImagePropsDiffTest`. It uses a small helper, `PropsPayloads.h`, that checks each Props 2.0 payload against what Props 1.5 sends for the same prop updates. Each test fails on main. Built with the NDK and run on an Android emulator (there is no open-source gtest target). - RNTester on an Android emulator with the Props 2.0 flags forced on. **Image example: `srcSet` and several sources**
BeforeAfter
https://github.com/user-attachments/assets/483306f2-f308-401e-8f4c-809337ec2292 https://github.com/user-attachments/assets/cf978447-071e-4537-8d3f-31e663f42bc0
**Remove `tintColor` (Playground screen below)**
BeforeAfter
https://github.com/user-attachments/assets/349be881-8de6-4731-b01b-362fabd0278f https://github.com/user-attachments/assets/04a7e35a-a64a-4317-8d91-cc30a7534b0d
RNTesterPlayground.js: paste into packages/rn-tester/js/examples/Playground/RNTesterPlayground.js and open Playground. ```js /** * Copyright (c) Meta Platforms, Inc. and affiliates. * * This source code is licensed under the MIT license found in the * LICENSE file in the root directory of this source tree. * * flow strict-local * format */ import type {RNTesterModuleExample} from '../../types/RNTesterTypes'; import * as React from 'react'; import {useState} from 'react'; import {Button, Image, StyleSheet, Text, View} from 'react-native'; function Tint() { const [on, setOn] = useState(true); return (
Differential Revision: D125438476 Pulled By: bartlomiejbloniarz --- .../react/views/image/ReactImageView.kt | 6 +- .../views/image/ReactImagePropertyTest.kt | 29 ++++ .../renderer/components/image/ImageProps.cpp | 35 ++++- .../image/tests/ImagePropsDiffTest.cpp | 93 +++++++++++++ .../react/renderer/imagemanager/primitives.h | 6 +- .../react/test_utils/PropsPayloads.h | 131 ++++++++++++++++++ 6 files changed, 293 insertions(+), 7 deletions(-) create mode 100644 packages/react-native/ReactCommon/react/renderer/components/image/tests/ImagePropsDiffTest.cpp create mode 100644 packages/react-native/ReactCommon/react/test_utils/PropsPayloads.h diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/image/ReactImageView.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/image/ReactImageView.kt index 3f413faaf7c0..b894ac8129f6 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/image/ReactImageView.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/image/ReactImageView.kt @@ -369,8 +369,12 @@ public class ReactImageView( // no worth marking as dirty if it already rendered. } + /** Sets the request headers. A change rebuilds the image request on the next update. */ public fun setHeaders(headers: ReadableMap?) { - this.headers = headers + if (this.headers != headers) { + this.headers = headers + isDirty = true + } } // Disable rasterizing to offscreen layer in order to preserve background effects like box-shadow diff --git a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/views/image/ReactImagePropertyTest.kt b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/views/image/ReactImagePropertyTest.kt index c3b78c3f3834..b69f015595e0 100644 --- a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/views/image/ReactImagePropertyTest.kt +++ b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/views/image/ReactImagePropertyTest.kt @@ -15,7 +15,9 @@ import android.graphics.Color import android.util.DisplayMetrics import com.facebook.common.logging.FLog import com.facebook.drawee.backends.pipeline.Fresco +import com.facebook.drawee.backends.pipeline.PipelineDraweeControllerBuilder import com.facebook.drawee.drawable.ScalingUtils +import com.facebook.imagepipeline.request.ImageRequest import com.facebook.react.bridge.Arguments import com.facebook.react.bridge.BridgeReactContext import com.facebook.react.bridge.CatalystInstance @@ -23,6 +25,7 @@ import com.facebook.react.bridge.JavaOnlyMap import com.facebook.react.bridge.ReactTestHelper.createMockCatalystInstance import com.facebook.react.common.ReactConstants import com.facebook.react.internal.featureflags.ReactNativeFeatureFlagsForTests +import com.facebook.react.modules.fresco.ReactNetworkImageRequest import com.facebook.react.uimanager.BackgroundStyleApplicator import com.facebook.react.uimanager.DisplayMetricsHolder import com.facebook.react.uimanager.LengthPercentage @@ -40,12 +43,15 @@ import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import org.mockito.MockedStatic +import org.mockito.Mockito.RETURNS_SELF import org.mockito.Mockito.anyString import org.mockito.Mockito.mockStatic import org.mockito.kotlin.any +import org.mockito.kotlin.argumentCaptor import org.mockito.kotlin.mock import org.mockito.kotlin.never import org.mockito.kotlin.reset +import org.mockito.kotlin.times import org.mockito.kotlin.verify import org.robolectric.RobolectricTestRunner import org.robolectric.RuntimeEnvironment @@ -281,4 +287,27 @@ class ReactImagePropertyTest { viewManager.setHeaders(mockView, headers) verify(mockView).setHeaders(headers) } + + @Test + fun testHeadersChangeRebuildsImageRequest() { + val controllerBuilder = mock(defaultAnswer = RETURNS_SELF) + val viewManager = ReactImageManager(controllerBuilder) + val view = viewManager.createViewInstance(themeContext) + val sources = Arguments.createArray() + sources.pushMap(JavaOnlyMap.of("uri", "https://example.com/image.png")) + viewManager.setSource(view, sources) + viewManager.setHeaders(view, JavaOnlyMap.of("Authorization", "token-1")) + view.maybeUpdateView() + + viewManager.setHeaders(view, JavaOnlyMap.of("Authorization", "token-2")) + view.maybeUpdateView() + viewManager.setHeaders(view, JavaOnlyMap.of("Authorization", "token-2")) + view.maybeUpdateView() + + val requests = argumentCaptor() + verify(controllerBuilder, times(2)).setImageRequest(requests.capture()) + assertThat(requests.firstValue.sourceUri).isEqualTo(requests.secondValue.sourceUri) + assertThat((requests.secondValue as ReactNetworkImageRequest).headers) + .isEqualTo(JavaOnlyMap.of("Authorization", "token-2")) + } } diff --git a/packages/react-native/ReactCommon/react/renderer/components/image/ImageProps.cpp b/packages/react-native/ReactCommon/react/renderer/components/image/ImageProps.cpp index 8cb669ebf127..2785fa720679 100644 --- a/packages/react-native/ReactCommon/react/renderer/components/image/ImageProps.cpp +++ b/packages/react-native/ReactCommon/react/renderer/components/image/ImageProps.cpp @@ -12,6 +12,9 @@ #include #include +#include +#include + namespace facebook::react { ImageProps::ImageProps( @@ -145,6 +148,30 @@ static folly::dynamic convertEdgeInsets(const EdgeInsets& edgeInsets) { return edgeInsetsResult; } +// ImageSource::operator== compares only the request identity: type and URI. +static bool hasSamePayload(const ImageSource& lhs, const ImageSource& rhs) { + return std::tie( + lhs.type, + lhs.uri, + lhs.bundle, + lhs.scale, + lhs.size, + lhs.body, + lhs.method, + lhs.cache, + lhs.headers) == + std::tie( + rhs.type, + rhs.uri, + rhs.bundle, + rhs.scale, + rhs.size, + rhs.body, + rhs.method, + rhs.cache, + rhs.headers); +} + ComponentName ImageProps::getDiffPropsImplementationTarget() const { return "Image"; } @@ -158,11 +185,14 @@ folly::dynamic ImageProps::getDiffProps(const Props* prevProps) const { folly::dynamic result = ViewProps::getDiffProps(oldProps); - if (sources != oldProps->sources) { + if (!std::ranges::equal(sources, oldProps->sources, hasSamePayload)) { auto sourcesArray = folly::dynamic::array(); for (const auto& source : sources) { sourcesArray.push_back(toDynamic(source)); } + if (!sources.empty()) { + result["headers"] = sourcesArray[0]["headers"]; + } result["source"] = sourcesArray; } @@ -221,7 +251,8 @@ folly::dynamic ImageProps::getDiffProps(const Props* prevProps) const { SharedColor prevTintColorValue = oldProps->tintColor.value_or(SharedColor{}); if (tintColorValue != prevTintColorValue) { - result["tintColor"] = *tintColorValue; + result["tintColor"] = tintColorValue ? folly::dynamic(*tintColorValue) + : folly::dynamic(nullptr); } } diff --git a/packages/react-native/ReactCommon/react/renderer/components/image/tests/ImagePropsDiffTest.cpp b/packages/react-native/ReactCommon/react/renderer/components/image/tests/ImagePropsDiffTest.cpp new file mode 100644 index 000000000000..22720793e0b3 --- /dev/null +++ b/packages/react-native/ReactCommon/react/renderer/components/image/tests/ImagePropsDiffTest.cpp @@ -0,0 +1,93 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +#include + +#include +#include + +#ifdef RN_SERIALIZABLE_STATE + +namespace facebook::react { + +namespace { + +using folly::dynamic; + +// Props 2.0 serializes every field of a source, JS only the ones it sets. +void expectPayloads(const std::vector& commits) { + expectPayloadsMatchRawProps(commits, {"source"}); +} + +} // namespace + +// ReactImageView.setSource reads width and height at the top level of each +// source when there are several, and ReactImageManager reads the request +// headers from the top-level headers prop, which JS derives from the first +// source. +TEST(ImagePropsDiffTest, multipleSourcesCarrySizesAndHeaders) { + dynamic headers = dynamic::object("Authorization", "token"); + expectPayloads({{dynamic::object( + "source", + dynamic::array( + dynamic::object("uri", "https://a")("width", 10)("height", 20)( + "headers", headers), + dynamic::object("uri", "https://b")("width", 30)("height", 40)))( + "headers", headers)}}); +} + +// ReactImageManager ignores a null headers prop, so only an empty map clears +// the headers of the previous source. +TEST(ImagePropsDiffTest, clearsHeadersWhenTheNewSourceHasNone) { + dynamic headers = dynamic::object("Authorization", "old"); + expectPayloads({ + {dynamic::object( + "source", + dynamic::array( + dynamic::object("uri", "https://a")("headers", headers)))( + "headers", headers)}, + {dynamic::object( + "source", dynamic::array(dynamic::object("uri", "https://b")))( + "headers", nullptr), + dynamic::object("headers", dynamic::object())}, + }); +} + +// ReactImageManager clears the tint only for null; 0 is a transparent SRC_IN +// tint that hides the image. +TEST(ImagePropsDiffTest, removingTintColorEmitsNull) { + expectPayloads({ + {dynamic::object("tintColor", static_cast(0xFFFF0000))}, + {dynamic::object("tintColor", nullptr)}, + }); +} + +// ReactImageView reads the cache policy and the sizes of each source, and +// ReactImageManager the headers; a source with the same URI can change them. +TEST(ImagePropsDiffTest, emitsASameUriSourceWhenItsOtherFieldsChange) { + dynamic headers = dynamic::object("Authorization", "old"); + dynamic source = dynamic::object("uri", "https://a")("width", 10)( + "height", 20)("scale", 1)("cache", "default")("headers", headers); + std::vector commits{ + {dynamic::object("source", dynamic::array(source))("headers", headers)}}; + dynamic changes = dynamic::object("cache", "reload")("width", 30)("scale", 2); + for (const auto& [field, value] : changes.items()) { + source[field] = value; + // Props 2.0 sends the first source's headers whenever the sources change. + commits.push_back( + {dynamic::object("source", dynamic::array(source)), + dynamic::object("headers", headers)}); + } + source["headers"] = dynamic::object("Authorization", "new"); + commits.push_back({dynamic::object("source", dynamic::array(source))( + "headers", source["headers"])}); + expectPayloads(commits); +} + +} // namespace facebook::react + +#endif diff --git a/packages/react-native/ReactCommon/react/renderer/imagemanager/primitives.h b/packages/react-native/ReactCommon/react/renderer/imagemanager/primitives.h index 86756373d6da..17593ca52f78 100644 --- a/packages/react-native/ReactCommon/react/renderer/imagemanager/primitives.h +++ b/packages/react-native/ReactCommon/react/renderer/imagemanager/primitives.h @@ -62,10 +62,8 @@ class ImageSource { imageSourceResult["bundle"] = bundle; imageSourceResult["scale"] = scale; - folly::dynamic sizeResult = folly::dynamic::object(); - sizeResult["width"] = size.width; - sizeResult["height"] = size.height; - imageSourceResult["size"] = sizeResult; + imageSourceResult["width"] = size.width; + imageSourceResult["height"] = size.height; imageSourceResult["body"] = body; imageSourceResult["method"] = method; diff --git a/packages/react-native/ReactCommon/react/test_utils/PropsPayloads.h b/packages/react-native/ReactCommon/react/test_utils/PropsPayloads.h new file mode 100644 index 000000000000..88da44225401 --- /dev/null +++ b/packages/react-native/ReactCommon/react/test_utils/PropsPayloads.h @@ -0,0 +1,131 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +#pragma once + +#include + +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include + +#ifdef RN_SERIALIZABLE_STATE + +namespace facebook::react { + +/* + * One commit: the props JS sends (a removed prop is null), and the Props 2.0 + * values that intentionally differ from what Props 1.5 sends. + */ +struct PropsCommit { + folly::dynamic props; + folly::dynamic intendedDifferences = folly::dynamic::object(); +}; + +namespace detail { + +inline bool payloadValuesMatch(const folly::dynamic &actual, const folly::dynamic &expected) +{ + if (actual.isNumber() && expected.isNumber()) { + // Props 2.0 stores floats, so 0.998 comes back as 0.99800002574... + auto tolerance = 1e-6 * std::max(1.0, std::abs(expected.asDouble())); + return std::abs(actual.asDouble() - expected.asDouble()) <= tolerance; + } + if (actual.isArray() && expected.isArray()) { + return actual.size() == expected.size() && + std::equal(actual.begin(), actual.end(), expected.begin(), payloadValuesMatch); + } + if (actual.isObject() && expected.isObject()) { + return actual.size() == expected.size() && std::ranges::all_of(expected.items(), [&](const auto &item) { + return actual.count(item.first) != 0 && payloadValuesMatch(actual[item.first], item.second); + }); + } + return actual == expected; +} + +// Each element carries the fields of the expected element, and may add more. +inline bool elementsContainFields(const folly::dynamic &actual, const folly::dynamic &expected) +{ + if (!actual.isArray() || !expected.isArray() || actual.size() != expected.size()) { + return payloadValuesMatch(actual, expected); + } + for (size_t i = 0; i < expected.size(); i++) { + for (const auto &[field, value] : expected[i].items()) { + if (actual[i].count(field) == 0 || !payloadValuesMatch(actual[i][field], value)) { + return false; + } + } + } + return true; +} + +} // namespace detail + +/* + * Applies the commits in order and checks that each Props 2.0 payload + * (`getDiffProps`) has the keys, nulls and values of the Props 1.5 payload + * (accumulated raw props), apart from the commit's intended differences. The + * elements of the arrays under `keysWithAddedFields` may carry fields that JS + * does not send. + */ +template +void expectPayloadsMatchRawProps( + const std::vector &commits, + const std::set &keysWithAddedFields = {}) +{ + ContextContainer contextContainer{}; + PropsParserContext parserContext{-1, contextContainer}; + + auto previous = std::make_shared(); + folly::dynamic previousRawProps = folly::dynamic::object(); + for (size_t i = 0; i < commits.size(); i++) { + const auto &commit = commits[i]; + auto rawProps = RawProps(commit.props); + auto parser = RawPropsParser(); + parser.template prepare(); + rawProps.parse(parser); + auto props = std::make_shared(parserContext, *previous, rawProps); + auto accumulatedRawProps = mergeDynamicProps(previousRawProps, commit.props, NullValueStrategy::Override); + + auto actual = props->getDiffProps(i == 0 ? nullptr : previous.get()); + auto expected = i == 0 ? accumulatedRawProps : diffDynamicProps(previousRawProps, accumulatedRawProps); + for (const auto &[key, value] : commit.intendedDifferences.items()) { + expected[key] = value; + } + + std::set keys; + for (const auto &payload : {actual, expected}) { + for (const auto &key : payload.keys()) { + keys.insert(key.asString()); + } + } + for (const auto &key : keys) { + bool matches = actual.count(key) != 0 && expected.count(key) != 0 && + (keysWithAddedFields.contains(key) ? detail::elementsContainFields(actual[key], expected[key]) + : detail::payloadValuesMatch(actual[key], expected[key])); + EXPECT_TRUE(matches) << "commit " << i << " " << commit.props << ", key '" << key + << "'\n Props 2.0: " << actual.getDefault(key, "(absent)") + << "\n expected: " << expected.getDefault(key, "(absent)"); + } + + previous = props; + previousRawProps = accumulatedRawProps; + } +} + +} // namespace facebook::react + +#endif