Skip to content

Commit 8da8f41

Browse files
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: #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** <table> <tr><th>Before</th><th>After</th></tr> <tr> <td> https://github.com/user-attachments/assets/483306f2-f308-401e-8f4c-809337ec2292 </td> <td> https://github.com/user-attachments/assets/cf978447-071e-4537-8d3f-31e663f42bc0 </td> </tr> </table> **Remove `tintColor` (Playground screen below)** <table> <tr><th>Before</th><th>After</th></tr> <tr> <td> https://github.com/user-attachments/assets/349be881-8de6-4731-b01b-362fabd0278f </td> <td> https://github.com/user-attachments/assets/04a7e35a-a64a-4317-8d91-cc30a7534b0d </td> </tr> </table> <details><summary><code>RNTesterPlayground.js</code>: paste into <code>packages/rn-tester/js/examples/Playground/RNTesterPlayground.js</code> and open Playground.</summary> ```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 ( <View> <Button testID="toggle" title={on ? 'Remove tintColor' : 'Set tintColor'} onPress={() => setOn(v => !v)} /> <Text testID="state">tintColor: {on ? 'purple' : 'unset'}</Text> <View style={styles.row}> <Image source={require('../../assets/bunny.png')} style={[styles.image, on && {tintColor: 'purple'}]} /> </View> </View> ); } function Playground() { return ( <View style={styles.container}> <Tint /> </View> ); } const styles = StyleSheet.create({ container: {padding: 10}, image: {backgroundColor: '#e0e0e0', height: 160, width: 160}, row: { alignItems: 'center', flexDirection: 'row', flexWrap: 'wrap', justifyContent: 'center', marginTop: 24, }, }); export default { title: 'Playground', name: 'playground', description: 'Test out new features and ideas.', render: (): React.Node => <Playground />, } as RNTesterModuleExample; ``` </details> Differential Revision: D125438476 Pulled By: bartlomiejbloniarz
1 parent e04aaf6 commit 8da8f41

6 files changed

Lines changed: 293 additions & 7 deletions

File tree

‎packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/image/ReactImageView.kt‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -369,8 +369,12 @@ public class ReactImageView(
369369
// no worth marking as dirty if it already rendered.
370370
}
371371

372+
/** Sets the request headers. A change rebuilds the image request on the next update. */
372373
public fun setHeaders(headers: ReadableMap?) {
373-
this.headers = headers
374+
if (this.headers != headers) {
375+
this.headers = headers
376+
isDirty = true
377+
}
374378
}
375379

376380
// Disable rasterizing to offscreen layer in order to preserve background effects like box-shadow

‎packages/react-native/ReactAndroid/src/test/java/com/facebook/react/views/image/ReactImagePropertyTest.kt‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,17 @@ import android.graphics.Color
1515
import android.util.DisplayMetrics
1616
import com.facebook.common.logging.FLog
1717
import com.facebook.drawee.backends.pipeline.Fresco
18+
import com.facebook.drawee.backends.pipeline.PipelineDraweeControllerBuilder
1819
import com.facebook.drawee.drawable.ScalingUtils
20+
import com.facebook.imagepipeline.request.ImageRequest
1921
import com.facebook.react.bridge.Arguments
2022
import com.facebook.react.bridge.BridgeReactContext
2123
import com.facebook.react.bridge.CatalystInstance
2224
import com.facebook.react.bridge.JavaOnlyMap
2325
import com.facebook.react.bridge.ReactTestHelper.createMockCatalystInstance
2426
import com.facebook.react.common.ReactConstants
2527
import com.facebook.react.internal.featureflags.ReactNativeFeatureFlagsForTests
28+
import com.facebook.react.modules.fresco.ReactNetworkImageRequest
2629
import com.facebook.react.uimanager.BackgroundStyleApplicator
2730
import com.facebook.react.uimanager.DisplayMetricsHolder
2831
import com.facebook.react.uimanager.LengthPercentage
@@ -40,12 +43,15 @@ import org.junit.Before
4043
import org.junit.Test
4144
import org.junit.runner.RunWith
4245
import org.mockito.MockedStatic
46+
import org.mockito.Mockito.RETURNS_SELF
4347
import org.mockito.Mockito.anyString
4448
import org.mockito.Mockito.mockStatic
4549
import org.mockito.kotlin.any
50+
import org.mockito.kotlin.argumentCaptor
4651
import org.mockito.kotlin.mock
4752
import org.mockito.kotlin.never
4853
import org.mockito.kotlin.reset
54+
import org.mockito.kotlin.times
4955
import org.mockito.kotlin.verify
5056
import org.robolectric.RobolectricTestRunner
5157
import org.robolectric.RuntimeEnvironment
@@ -281,4 +287,27 @@ class ReactImagePropertyTest {
281287
viewManager.setHeaders(mockView, headers)
282288
verify(mockView).setHeaders(headers)
283289
}
290+
291+
@Test
292+
fun testHeadersChangeRebuildsImageRequest() {
293+
val controllerBuilder = mock<PipelineDraweeControllerBuilder>(defaultAnswer = RETURNS_SELF)
294+
val viewManager = ReactImageManager(controllerBuilder)
295+
val view = viewManager.createViewInstance(themeContext)
296+
val sources = Arguments.createArray()
297+
sources.pushMap(JavaOnlyMap.of("uri", "https://example.com/image.png"))
298+
viewManager.setSource(view, sources)
299+
viewManager.setHeaders(view, JavaOnlyMap.of("Authorization", "token-1"))
300+
view.maybeUpdateView()
301+
302+
viewManager.setHeaders(view, JavaOnlyMap.of("Authorization", "token-2"))
303+
view.maybeUpdateView()
304+
viewManager.setHeaders(view, JavaOnlyMap.of("Authorization", "token-2"))
305+
view.maybeUpdateView()
306+
307+
val requests = argumentCaptor<ImageRequest>()
308+
verify(controllerBuilder, times(2)).setImageRequest(requests.capture())
309+
assertThat(requests.firstValue.sourceUri).isEqualTo(requests.secondValue.sourceUri)
310+
assertThat((requests.secondValue as ReactNetworkImageRequest).headers)
311+
.isEqualTo(JavaOnlyMap.of("Authorization", "token-2"))
312+
}
284313
}

‎packages/react-native/ReactCommon/react/renderer/components/image/ImageProps.cpp‎

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,9 @@
1212
#include <react/renderer/debug/debugStringConvertibleUtils.h>
1313
#include <react/renderer/graphics/Color.h>
1414

15+
#include <algorithm>
16+
#include <tuple>
17+
1518
namespace facebook::react {
1619

1720
ImageProps::ImageProps(
@@ -145,6 +148,30 @@ static folly::dynamic convertEdgeInsets(const EdgeInsets& edgeInsets) {
145148
return edgeInsetsResult;
146149
}
147150

151+
// ImageSource::operator== compares only the request identity: type and URI.
152+
static bool hasSamePayload(const ImageSource& lhs, const ImageSource& rhs) {
153+
return std::tie(
154+
lhs.type,
155+
lhs.uri,
156+
lhs.bundle,
157+
lhs.scale,
158+
lhs.size,
159+
lhs.body,
160+
lhs.method,
161+
lhs.cache,
162+
lhs.headers) ==
163+
std::tie(
164+
rhs.type,
165+
rhs.uri,
166+
rhs.bundle,
167+
rhs.scale,
168+
rhs.size,
169+
rhs.body,
170+
rhs.method,
171+
rhs.cache,
172+
rhs.headers);
173+
}
174+
148175
ComponentName ImageProps::getDiffPropsImplementationTarget() const {
149176
return "Image";
150177
}
@@ -158,11 +185,14 @@ folly::dynamic ImageProps::getDiffProps(const Props* prevProps) const {
158185

159186
folly::dynamic result = ViewProps::getDiffProps(oldProps);
160187

161-
if (sources != oldProps->sources) {
188+
if (!std::ranges::equal(sources, oldProps->sources, hasSamePayload)) {
162189
auto sourcesArray = folly::dynamic::array();
163190
for (const auto& source : sources) {
164191
sourcesArray.push_back(toDynamic(source));
165192
}
193+
if (!sources.empty()) {
194+
result["headers"] = sourcesArray[0]["headers"];
195+
}
166196
result["source"] = sourcesArray;
167197
}
168198

@@ -221,7 +251,8 @@ folly::dynamic ImageProps::getDiffProps(const Props* prevProps) const {
221251
SharedColor prevTintColorValue =
222252
oldProps->tintColor.value_or(SharedColor{});
223253
if (tintColorValue != prevTintColorValue) {
224-
result["tintColor"] = *tintColorValue;
254+
result["tintColor"] = tintColorValue ? folly::dynamic(*tintColorValue)
255+
: folly::dynamic(nullptr);
225256
}
226257
}
227258

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
/*
2+
* Copyright (c) Meta Platforms, Inc. and affiliates.
3+
*
4+
* This source code is licensed under the MIT license found in the
5+
* LICENSE file in the root directory of this source tree.
6+
*/
7+
8+
#include <gtest/gtest.h>
9+
10+
#include <react/renderer/components/image/ImageProps.h>
11+
#include <react/test_utils/PropsPayloads.h>
12+
13+
#ifdef RN_SERIALIZABLE_STATE
14+
15+
namespace facebook::react {
16+
17+
namespace {
18+
19+
using folly::dynamic;
20+
21+
// Props 2.0 serializes every field of a source, JS only the ones it sets.
22+
void expectPayloads(const std::vector<PropsCommit>& commits) {
23+
expectPayloadsMatchRawProps<ImageProps>(commits, {"source"});
24+
}
25+
26+
} // namespace
27+
28+
// ReactImageView.setSource reads width and height at the top level of each
29+
// source when there are several, and ReactImageManager reads the request
30+
// headers from the top-level headers prop, which JS derives from the first
31+
// source.
32+
TEST(ImagePropsDiffTest, multipleSourcesCarrySizesAndHeaders) {
33+
dynamic headers = dynamic::object("Authorization", "token");
34+
expectPayloads({{dynamic::object(
35+
"source",
36+
dynamic::array(
37+
dynamic::object("uri", "https://a")("width", 10)("height", 20)(
38+
"headers", headers),
39+
dynamic::object("uri", "https://b")("width", 30)("height", 40)))(
40+
"headers", headers)}});
41+
}
42+
43+
// ReactImageManager ignores a null headers prop, so only an empty map clears
44+
// the headers of the previous source.
45+
TEST(ImagePropsDiffTest, clearsHeadersWhenTheNewSourceHasNone) {
46+
dynamic headers = dynamic::object("Authorization", "old");
47+
expectPayloads({
48+
{dynamic::object(
49+
"source",
50+
dynamic::array(
51+
dynamic::object("uri", "https://a")("headers", headers)))(
52+
"headers", headers)},
53+
{dynamic::object(
54+
"source", dynamic::array(dynamic::object("uri", "https://b")))(
55+
"headers", nullptr),
56+
dynamic::object("headers", dynamic::object())},
57+
});
58+
}
59+
60+
// ReactImageManager clears the tint only for null; 0 is a transparent SRC_IN
61+
// tint that hides the image.
62+
TEST(ImagePropsDiffTest, removingTintColorEmitsNull) {
63+
expectPayloads({
64+
{dynamic::object("tintColor", static_cast<int32_t>(0xFFFF0000))},
65+
{dynamic::object("tintColor", nullptr)},
66+
});
67+
}
68+
69+
// ReactImageView reads the cache policy and the sizes of each source, and
70+
// ReactImageManager the headers; a source with the same URI can change them.
71+
TEST(ImagePropsDiffTest, emitsASameUriSourceWhenItsOtherFieldsChange) {
72+
dynamic headers = dynamic::object("Authorization", "old");
73+
dynamic source = dynamic::object("uri", "https://a")("width", 10)(
74+
"height", 20)("scale", 1)("cache", "default")("headers", headers);
75+
std::vector<PropsCommit> commits{
76+
{dynamic::object("source", dynamic::array(source))("headers", headers)}};
77+
dynamic changes = dynamic::object("cache", "reload")("width", 30)("scale", 2);
78+
for (const auto& [field, value] : changes.items()) {
79+
source[field] = value;
80+
// Props 2.0 sends the first source's headers whenever the sources change.
81+
commits.push_back(
82+
{dynamic::object("source", dynamic::array(source)),
83+
dynamic::object("headers", headers)});
84+
}
85+
source["headers"] = dynamic::object("Authorization", "new");
86+
commits.push_back({dynamic::object("source", dynamic::array(source))(
87+
"headers", source["headers"])});
88+
expectPayloads(commits);
89+
}
90+
91+
} // namespace facebook::react
92+
93+
#endif

‎packages/react-native/ReactCommon/react/renderer/imagemanager/primitives.h‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -62,10 +62,8 @@ class ImageSource {
6262
imageSourceResult["bundle"] = bundle;
6363
imageSourceResult["scale"] = scale;
6464

65-
folly::dynamic sizeResult = folly::dynamic::object();
66-
sizeResult["width"] = size.width;
67-
sizeResult["height"] = size.height;
68-
imageSourceResult["size"] = sizeResult;
65+
imageSourceResult["width"] = size.width;
66+
imageSourceResult["height"] = size.height;
6967

7068
imageSourceResult["body"] = body;
7169
imageSourceResult["method"] = method;
Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
/*
2+
* Copyright (c) Meta Platforms, Inc. and affiliates.
3+
*
4+
* This source code is licensed under the MIT license found in the
5+
* LICENSE file in the root directory of this source tree.
6+
*/
7+
8+
#pragma once
9+
10+
#include <gtest/gtest.h>
11+
12+
#include <react/renderer/core/DynamicPropsUtilities.h>
13+
#include <react/renderer/core/PropsParserContext.h>
14+
#include <react/renderer/core/RawProps.h>
15+
#include <react/renderer/core/RawPropsParser.h>
16+
#include <react/utils/ContextContainer.h>
17+
18+
#include <algorithm>
19+
#include <cmath>
20+
#include <memory>
21+
#include <set>
22+
#include <string>
23+
#include <vector>
24+
25+
#ifdef RN_SERIALIZABLE_STATE
26+
27+
namespace facebook::react {
28+
29+
/*
30+
* One commit: the props JS sends (a removed prop is null), and the Props 2.0
31+
* values that intentionally differ from what Props 1.5 sends.
32+
*/
33+
struct PropsCommit {
34+
folly::dynamic props;
35+
folly::dynamic intendedDifferences = folly::dynamic::object();
36+
};
37+
38+
namespace detail {
39+
40+
inline bool payloadValuesMatch(const folly::dynamic &actual, const folly::dynamic &expected)
41+
{
42+
if (actual.isNumber() && expected.isNumber()) {
43+
// Props 2.0 stores floats, so 0.998 comes back as 0.99800002574...
44+
auto tolerance = 1e-6 * std::max(1.0, std::abs(expected.asDouble()));
45+
return std::abs(actual.asDouble() - expected.asDouble()) <= tolerance;
46+
}
47+
if (actual.isArray() && expected.isArray()) {
48+
return actual.size() == expected.size() &&
49+
std::equal(actual.begin(), actual.end(), expected.begin(), payloadValuesMatch);
50+
}
51+
if (actual.isObject() && expected.isObject()) {
52+
return actual.size() == expected.size() && std::ranges::all_of(expected.items(), [&](const auto &item) {
53+
return actual.count(item.first) != 0 && payloadValuesMatch(actual[item.first], item.second);
54+
});
55+
}
56+
return actual == expected;
57+
}
58+
59+
// Each element carries the fields of the expected element, and may add more.
60+
inline bool elementsContainFields(const folly::dynamic &actual, const folly::dynamic &expected)
61+
{
62+
if (!actual.isArray() || !expected.isArray() || actual.size() != expected.size()) {
63+
return payloadValuesMatch(actual, expected);
64+
}
65+
for (size_t i = 0; i < expected.size(); i++) {
66+
for (const auto &[field, value] : expected[i].items()) {
67+
if (actual[i].count(field) == 0 || !payloadValuesMatch(actual[i][field], value)) {
68+
return false;
69+
}
70+
}
71+
}
72+
return true;
73+
}
74+
75+
} // namespace detail
76+
77+
/*
78+
* Applies the commits in order and checks that each Props 2.0 payload
79+
* (`getDiffProps`) has the keys, nulls and values of the Props 1.5 payload
80+
* (accumulated raw props), apart from the commit's intended differences. The
81+
* elements of the arrays under `keysWithAddedFields` may carry fields that JS
82+
* does not send.
83+
*/
84+
template <typename PropsT>
85+
void expectPayloadsMatchRawProps(
86+
const std::vector<PropsCommit> &commits,
87+
const std::set<std::string> &keysWithAddedFields = {})
88+
{
89+
ContextContainer contextContainer{};
90+
PropsParserContext parserContext{-1, contextContainer};
91+
92+
auto previous = std::make_shared<const PropsT>();
93+
folly::dynamic previousRawProps = folly::dynamic::object();
94+
for (size_t i = 0; i < commits.size(); i++) {
95+
const auto &commit = commits[i];
96+
auto rawProps = RawProps(commit.props);
97+
auto parser = RawPropsParser();
98+
parser.template prepare<PropsT>();
99+
rawProps.parse(parser);
100+
auto props = std::make_shared<const PropsT>(parserContext, *previous, rawProps);
101+
auto accumulatedRawProps = mergeDynamicProps(previousRawProps, commit.props, NullValueStrategy::Override);
102+
103+
auto actual = props->getDiffProps(i == 0 ? nullptr : previous.get());
104+
auto expected = i == 0 ? accumulatedRawProps : diffDynamicProps(previousRawProps, accumulatedRawProps);
105+
for (const auto &[key, value] : commit.intendedDifferences.items()) {
106+
expected[key] = value;
107+
}
108+
109+
std::set<std::string> keys;
110+
for (const auto &payload : {actual, expected}) {
111+
for (const auto &key : payload.keys()) {
112+
keys.insert(key.asString());
113+
}
114+
}
115+
for (const auto &key : keys) {
116+
bool matches = actual.count(key) != 0 && expected.count(key) != 0 &&
117+
(keysWithAddedFields.contains(key) ? detail::elementsContainFields(actual[key], expected[key])
118+
: detail::payloadValuesMatch(actual[key], expected[key]));
119+
EXPECT_TRUE(matches) << "commit " << i << " " << commit.props << ", key '" << key
120+
<< "'\n Props 2.0: " << actual.getDefault(key, "(absent)")
121+
<< "\n expected: " << expected.getDefault(key, "(absent)");
122+
}
123+
124+
previous = props;
125+
previousRawProps = accumulatedRawProps;
126+
}
127+
}
128+
129+
} // namespace facebook::react
130+
131+
#endif

0 commit comments

Comments
 (0)