Skip to content

Commit 100f8fc

Browse files
cgoldsbymeta-codesync[bot]
authored andcommitted
fix(accessibility): Derive accessibilityTraits from role and state (#57515) (#57516)
Summary: Replaces the two independent writers on `accessibilityTraits` (one in C++ for role, one in ObjC for state) with a single `deriveAccessibilityTraits()` helper that rebuilds the full mask from `None` each commit. On recycled Fabric views, the ObjC writer's diff guard can skip when the stale `_props` match the new props, leaving `Selected` and `NotEnabled` traits stale. Fixes #57515 NOTE: this is a fix, not a breaking change (see below) 0.88 is a non-breaking release, and two behaviours change here, so this was worth settling explicitly rather than by omission. Both stop the code doing something it should never have done, so the window does not gate this diff. The first is `Selected` and `NotEnabled` surviving a view recycle, which is issue #57515 itself. The second is that `role` no longer honours values outside the `Role` type. The old code parsed the raw `role` string into traits without validating it, so `role="image"` set the Image trait. The public `Role` type in `Libraries/Components/View/ViewAccessibility.js` has 65 members, matching the 65 `toString(Role)` outputs one for one, and none of the 17 affected trait names appear in it. `role="image"` was always a Flow and TypeScript error. Nine of the 17 are valid `accessibilityRole` values and keep working through that prop and the new fallback branch; the remaining eight are internal trait names that appear in neither public type. The full breakdown is in the test plan. Residual risk worth naming: untyped JS that passes a runtime-computed string into `role` and happens to hit one of those nine would silently lose a trait. Typed code cannot reach this, and untyped code was relying on undocumented behaviour, but the determination rests on the declared types rather than on field data. ## Changelog: [IOS] [FIXED] - Derive `accessibilityTraits` from role and state in a single writer so `Selected` and `NotEnabled` traits are not lost on recycled Fabric views Pull Request resolved: #57516 Test Plan: Reproducer repo: https://github.com/cgoldsby/rn-accessibility-traits-bug ([[steps](https://github.com/cgoldsby/rn-accessibility-traits-bug#steps-to-reproduce-bug)](https://github.com/cgoldsby/rn-accessibility-traits-bug#steps-to-reproduce-bug)) ```sh git clone https://github.com/cgoldsby/rn-accessibility-traits-bug.git cd rn-accessibility-traits-bug yarn install cd ios && pod install && cd .. yarn ios ``` 1. Open Accessibility Inspector 2. Focus the target View 3. Tap "Toggle Role" to switch from `role="button"` to no role **Before fix:** `Selected` trait is missing **After fix:** ```sh git apply patches/react-native+0.86.0.patch cd ios && pod install && cd .. yarn ios ``` `Selected` trait persists through the role change. https://github.com/user-attachments/assets/79974b9d-59da-4066-9b31-e2feff91c407 ## Unit tests Adds `ReactCommon/react/renderer/components/view/tests/AccessibilityPropsTest.cpp`, picked up by the existing `:tests` glob. `AccessibilityProps` is a mixin rather than a `Props` descendant, so the tests parse through `ViewProps`, the concrete type that inherits it. ``` buck2 test fbsource//xplat/js/react-native-github/packages/react-native/ReactCommon/react/renderer/components/view:tests → Pass 87. Fail 0. ``` Three of the eight are true regression tests. Restoring the pre-fix `AccessibilityProps.cpp` and re-running fails exactly those three, with the other 84 still green: ``` ✗ AccessibilityPropsTest.folds_selected_state_into_role_traits ✗ AccessibilityPropsTest.folds_disabled_state_into_role_traits ✗ AccessibilityPropsTest.clearing_selected_state_drops_the_trait Tests finished: Pass 84. Fail 3. ``` The remaining five pin behaviour the refactor is *not* meant to change, and they pass against both the old and new code: traits derived from `role`, traits derived from `accessibilityRole`, `role` winning over `accessibilityRole`, no state traits when state is absent, and inheritance from `sourceProps` when the raw props are absent. ## NOTE: `role` no longer honours non-`Role` values (a fix, not a break) The old code parsed the raw `role` value straight into traits. The new code only consults `role` once it has parsed to a valid `Role` enum, and otherwise falls back to `accessibilityRole`. Comparing the two string tables in `accessibilityPropsConversions.h`: - `toString(Role)` can emit 65 strings; `fromString(-> AccessibilityTraits)` recognises 25. - Only 8 overlap, so only these ever produced a trait, before or after: `button`, `heading`, `img`, `link`, `none`, `progressbar`, `summary`, `switch`. - 17 trait names have no corresponding `Role`: `adjustable`, `allowsDirectInteraction`, `disabled`, `frequentUpdates`, `header`, `image`, `imagebutton`, `key`, `keyboardkey`, `pageTurn`, `plays`, `search`, `selected`, `startsMedia`, `tabbar`, `text`, `togglebutton`. Passing one of those through `role` used to set a trait and now yields `None`. This is not a breaking change, because none of those were ever valid `role` values. The public `Role` type in `Libraries/Components/View/ViewAccessibility.js` has exactly 65 members, matching the 65 `toString(Role)` outputs one for one, and **none** of the 17 appear in it. `role="image"` was always a Flow and TypeScript type error; the old C++ honoured it only because it parsed the raw string into traits without validating it against the `Role` enum. Of the 17, nine (`adjustable`, `header`, `image`, `imagebutton`, `keyboardkey`, `search`, `tabbar`, `text`, `togglebutton`) are valid `accessibilityRole` values and keep working through that prop and the fallback branch. The remaining eight are internal trait names that appear in neither public type. The new code is also better in the mixed case: with an invalid `role` plus a valid `accessibilityRole`, the old code let the bad `role` win, whereas the new code falls back correctly. ## Also restored The `// It is a (severe!) perf deoptimization to request props out-of-order` comment above the two `rawProps.at()` lookups. Its original rationale (needing `accessibilityRole` twice) is obsolete, but the general hazard is not, and the two lookups still have to stay adjacent and in order. Reviewed By: javache Differential Revision: D114880897 Pulled By: fabriziocucci fbshipit-source-id: bfefa0db0d0c38895ad43d781afa99e9b0283ee7
1 parent 5fb3ebc commit 100f8fc

3 files changed

Lines changed: 186 additions & 54 deletions

File tree

packages/react-native/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.mm

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -539,18 +539,6 @@ - (void)updateProps:(const Props::Shared &)props oldProps:(const Props::Shared &
539539
RCTUIAccessibilityTraitsFromAccessibilityTraits(newViewProps.accessibilityTraits);
540540
}
541541

542-
// `accessibilityState`
543-
if (oldViewProps.accessibilityState != newViewProps.accessibilityState) {
544-
self.accessibilityTraits &= ~(UIAccessibilityTraitNotEnabled | UIAccessibilityTraitSelected);
545-
const auto accessibilityState = newViewProps.accessibilityState.value_or(AccessibilityState{});
546-
if (accessibilityState.selected) {
547-
self.accessibilityTraits |= UIAccessibilityTraitSelected;
548-
}
549-
if (accessibilityState.disabled) {
550-
self.accessibilityTraits |= UIAccessibilityTraitNotEnabled;
551-
}
552-
}
553-
554542
// `accessibilityIgnoresInvertColors`
555543
if (oldViewProps.accessibilityIgnoresInvertColors != newViewProps.accessibilityIgnoresInvertColors) {
556544
self.accessibilityIgnoresInvertColors = newViewProps.accessibilityIgnoresInvertColors;

packages/react-native/ReactCommon/react/renderer/components/view/AccessibilityProps.cpp

Lines changed: 65 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,31 @@
1414

1515
namespace facebook::react {
1616

17+
// Derive accessibilityTraits from the resolved role and accessibilityState
18+
// members. This makes accessibilityTraits the single source of truth.
19+
// See: github.com/facebook/react-native/issues/57515
20+
static AccessibilityTraits deriveAccessibilityTraits(
21+
Role role,
22+
const std::string& accessibilityRole,
23+
const std::optional<AccessibilityState>& accessibilityState) {
24+
AccessibilityTraits traits = AccessibilityTraits::None;
25+
if (role != Role::None) {
26+
fromString(toString(role), traits);
27+
} else if (!accessibilityRole.empty()) {
28+
fromString(accessibilityRole, traits);
29+
}
30+
31+
const auto state = accessibilityState.value_or(AccessibilityState{});
32+
if (state.selected) {
33+
traits = traits | AccessibilityTraits::Selected;
34+
}
35+
if (state.disabled) {
36+
traits = traits | AccessibilityTraits::NotEnabled;
37+
}
38+
39+
return traits;
40+
}
41+
1742
AccessibilityProps::AccessibilityProps(
1843
const PropsParserContext& context,
1944
const AccessibilityProps& sourceProps,
@@ -146,19 +171,11 @@ AccessibilityProps::AccessibilityProps(
146171
ImportantForAccessibility::Auto)),
147172
testId(
148173
convertRawProp(context, rawProps, "testID", sourceProps.testId, "")) {
149-
// It is a (severe!) perf deoptimization to request props out-of-order.
150-
// Thus, since we need to request the same prop twice here
151-
// (accessibilityRole) we "must" do them subsequently here to prevent
152-
// a regression. It is reasonable to ask if the `at` function can be improved;
153-
// it probably can, but this is a fairly rare edge-case that (1) is easy-ish
154-
// to work around here, and (2) would require very careful work to address
155-
// this case and not regress the more common cases.
174+
// It is a (severe!) perf deoptimization to request props out-of-order, so
175+
// these two lookups stay adjacent and in this order.
156176
auto* accessibilityRoleValue = rawProps.at("accessibilityRole");
157177
auto* roleValue = rawProps.at("role");
158178

159-
auto* precedentRoleValue =
160-
roleValue != nullptr ? roleValue : accessibilityRoleValue;
161-
162179
if (accessibilityRoleValue == nullptr ||
163180
!accessibilityRoleValue->hasValue()) {
164181
accessibilityRole = sourceProps.accessibilityRole;
@@ -172,11 +189,8 @@ AccessibilityProps::AccessibilityProps(
172189
fromRawValue(context, *roleValue, role);
173190
}
174191

175-
if (precedentRoleValue == nullptr || !precedentRoleValue->hasValue()) {
176-
accessibilityTraits = sourceProps.accessibilityTraits;
177-
} else {
178-
fromRawValue(context, *precedentRoleValue, accessibilityTraits);
179-
}
192+
accessibilityTraits =
193+
deriveAccessibilityTraits(role, accessibilityRole, accessibilityState);
180194
}
181195

182196
void AccessibilityProps::setProp(
@@ -188,39 +202,48 @@ void AccessibilityProps::setProp(
188202

189203
switch (hash) {
190204
RAW_SET_PROP_SWITCH_CASE_BASIC(accessible);
191-
RAW_SET_PROP_SWITCH_CASE_BASIC(accessibilityState);
192-
RAW_SET_PROP_SWITCH_CASE_BASIC(accessibilityLabel);
193-
RAW_SET_PROP_SWITCH_CASE(
194-
accessibilityOrder, "experimental_accessibilityOrder");
195-
RAW_SET_PROP_SWITCH_CASE_BASIC(accessibilityLabelledBy);
196-
RAW_SET_PROP_SWITCH_CASE_BASIC(accessibilityLiveRegion);
197-
RAW_SET_PROP_SWITCH_CASE_BASIC(accessibilityHint);
198-
RAW_SET_PROP_SWITCH_CASE_BASIC(accessibilityLanguage);
199-
RAW_SET_PROP_SWITCH_CASE_BASIC(accessibilityShowsLargeContentViewer);
200-
RAW_SET_PROP_SWITCH_CASE_BASIC(accessibilityLargeContentTitle);
201-
RAW_SET_PROP_SWITCH_CASE_BASIC(accessibilityValue);
202-
RAW_SET_PROP_SWITCH_CASE_BASIC(accessibilityActions);
203-
RAW_SET_PROP_SWITCH_CASE_BASIC(accessibilityViewIsModal);
204-
RAW_SET_PROP_SWITCH_CASE_BASIC(accessibilityElementsHidden);
205-
RAW_SET_PROP_SWITCH_CASE_BASIC(accessibilityIgnoresInvertColors);
206-
RAW_SET_PROP_SWITCH_CASE_BASIC(accessibilityRespondsToUserInteraction);
207-
RAW_SET_PROP_SWITCH_CASE_BASIC(onAccessibilityTap);
208-
RAW_SET_PROP_SWITCH_CASE_BASIC(onAccessibilityMagicTap);
209-
RAW_SET_PROP_SWITCH_CASE_BASIC(onAccessibilityEscape);
210-
RAW_SET_PROP_SWITCH_CASE_BASIC(onAccessibilityAction);
211-
RAW_SET_PROP_SWITCH_CASE_BASIC(importantForAccessibility);
212-
RAW_SET_PROP_SWITCH_CASE_BASIC(role);
213-
RAW_SET_PROP_SWITCH_CASE(testId, "testID");
205+
case CONSTEXPR_RAW_PROPS_KEY_HASH("accessibilityState"): {
206+
fromRawValue(
207+
context, value, accessibilityState, defaults.accessibilityState);
208+
accessibilityTraits = deriveAccessibilityTraits(
209+
role, accessibilityRole, accessibilityState);
210+
return;
211+
}
212+
RAW_SET_PROP_SWITCH_CASE_BASIC(accessibilityLabel);
213+
RAW_SET_PROP_SWITCH_CASE(
214+
accessibilityOrder, "experimental_accessibilityOrder");
215+
RAW_SET_PROP_SWITCH_CASE_BASIC(accessibilityLabelledBy);
216+
RAW_SET_PROP_SWITCH_CASE_BASIC(accessibilityLiveRegion);
217+
RAW_SET_PROP_SWITCH_CASE_BASIC(accessibilityHint);
218+
RAW_SET_PROP_SWITCH_CASE_BASIC(accessibilityLanguage);
219+
RAW_SET_PROP_SWITCH_CASE_BASIC(accessibilityShowsLargeContentViewer);
220+
RAW_SET_PROP_SWITCH_CASE_BASIC(accessibilityLargeContentTitle);
221+
RAW_SET_PROP_SWITCH_CASE_BASIC(accessibilityValue);
222+
RAW_SET_PROP_SWITCH_CASE_BASIC(accessibilityActions);
223+
RAW_SET_PROP_SWITCH_CASE_BASIC(accessibilityViewIsModal);
224+
RAW_SET_PROP_SWITCH_CASE_BASIC(accessibilityElementsHidden);
225+
RAW_SET_PROP_SWITCH_CASE_BASIC(accessibilityIgnoresInvertColors);
226+
RAW_SET_PROP_SWITCH_CASE_BASIC(accessibilityRespondsToUserInteraction);
227+
RAW_SET_PROP_SWITCH_CASE_BASIC(onAccessibilityTap);
228+
RAW_SET_PROP_SWITCH_CASE_BASIC(onAccessibilityMagicTap);
229+
RAW_SET_PROP_SWITCH_CASE_BASIC(onAccessibilityEscape);
230+
RAW_SET_PROP_SWITCH_CASE_BASIC(onAccessibilityAction);
231+
RAW_SET_PROP_SWITCH_CASE_BASIC(importantForAccessibility);
232+
case CONSTEXPR_RAW_PROPS_KEY_HASH("role"): {
233+
fromRawValue(context, value, role, defaults.role);
234+
accessibilityTraits = deriveAccessibilityTraits(
235+
role, accessibilityRole, accessibilityState);
236+
return;
237+
}
238+
RAW_SET_PROP_SWITCH_CASE(testId, "testID");
214239
case CONSTEXPR_RAW_PROPS_KEY_HASH("accessibilityRole"): {
215-
AccessibilityTraits traits = AccessibilityTraits::None;
216240
std::string roleString;
217241
if (value.hasValue()) {
218-
fromRawValue(context, value, traits);
219242
fromRawValue(context, value, roleString);
220243
}
221-
222-
accessibilityTraits = traits;
223244
accessibilityRole = roleString;
245+
accessibilityTraits = deriveAccessibilityTraits(
246+
role, accessibilityRole, accessibilityState);
224247
return;
225248
}
226249
}
Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
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/view/AccessibilityPrimitives.h>
11+
#include <react/renderer/components/view/ViewProps.h>
12+
#include <react/renderer/core/RawProps.h>
13+
#include <react/renderer/core/RawPropsParser.h>
14+
15+
namespace facebook::react {
16+
17+
namespace {
18+
19+
// AccessibilityProps is a mixin rather than a Props descendant, so it is
20+
// parsed through ViewProps, the concrete type that inherits it.
21+
ViewProps parse(
22+
const folly::dynamic& rawPropsValue,
23+
const ViewProps& sourceProps = ViewProps()) {
24+
ContextContainer contextContainer{};
25+
PropsParserContext parserContext{-1, contextContainer};
26+
27+
auto raw = RawProps(rawPropsValue);
28+
auto parser = RawPropsParser();
29+
parser.prepare<ViewProps>();
30+
raw.parse(parser);
31+
32+
return {parserContext, sourceProps, raw};
33+
}
34+
35+
bool hasTrait(AccessibilityTraits traits, AccessibilityTraits expected) {
36+
return (traits & expected) == expected;
37+
}
38+
39+
} // namespace
40+
41+
TEST(AccessibilityPropsTest, derives_traits_from_role) {
42+
auto props = parse(folly::dynamic::object("role", "button"));
43+
44+
EXPECT_TRUE(hasTrait(props.accessibilityTraits, AccessibilityTraits::Button));
45+
}
46+
47+
TEST(AccessibilityPropsTest, derives_traits_from_accessibility_role) {
48+
auto props = parse(folly::dynamic::object("accessibilityRole", "image"));
49+
50+
EXPECT_TRUE(hasTrait(props.accessibilityTraits, AccessibilityTraits::Image));
51+
}
52+
53+
TEST(AccessibilityPropsTest, role_takes_precedence_over_accessibility_role) {
54+
auto props = parse(
55+
folly::dynamic::object("role", "button")("accessibilityRole", "image"));
56+
57+
EXPECT_TRUE(hasTrait(props.accessibilityTraits, AccessibilityTraits::Button));
58+
EXPECT_FALSE(hasTrait(props.accessibilityTraits, AccessibilityTraits::Image));
59+
}
60+
61+
// The selected/disabled bits used to be applied by RCTViewComponentView on top
62+
// of the role-derived bitmask, which meant they were lost whenever the traits
63+
// were rewritten. They are now folded in at the props layer.
64+
TEST(AccessibilityPropsTest, folds_selected_state_into_role_traits) {
65+
auto props = parse(
66+
folly::dynamic::object("role", "button")(
67+
"accessibilityState", folly::dynamic::object("selected", true)));
68+
69+
EXPECT_TRUE(hasTrait(props.accessibilityTraits, AccessibilityTraits::Button));
70+
EXPECT_TRUE(
71+
hasTrait(props.accessibilityTraits, AccessibilityTraits::Selected));
72+
}
73+
74+
TEST(AccessibilityPropsTest, folds_disabled_state_into_role_traits) {
75+
auto props = parse(
76+
folly::dynamic::object("role", "button")(
77+
"accessibilityState", folly::dynamic::object("disabled", true)));
78+
79+
EXPECT_TRUE(hasTrait(props.accessibilityTraits, AccessibilityTraits::Button));
80+
EXPECT_TRUE(
81+
hasTrait(props.accessibilityTraits, AccessibilityTraits::NotEnabled));
82+
}
83+
84+
TEST(AccessibilityPropsTest, omits_state_traits_when_state_is_absent) {
85+
auto props = parse(folly::dynamic::object("role", "button"));
86+
87+
EXPECT_FALSE(
88+
hasTrait(props.accessibilityTraits, AccessibilityTraits::Selected));
89+
EXPECT_FALSE(
90+
hasTrait(props.accessibilityTraits, AccessibilityTraits::NotEnabled));
91+
}
92+
93+
// A recycled view is re-parsed against the previous mount's props as source.
94+
// Clearing selected must drop the trait rather than leave it latched on.
95+
TEST(AccessibilityPropsTest, clearing_selected_state_drops_the_trait) {
96+
auto selected = parse(
97+
folly::dynamic::object("role", "button")(
98+
"accessibilityState", folly::dynamic::object("selected", true)));
99+
ASSERT_TRUE(
100+
hasTrait(selected.accessibilityTraits, AccessibilityTraits::Selected));
101+
102+
auto cleared = parse(
103+
folly::dynamic::object("role", "button")(
104+
"accessibilityState", folly::dynamic::object("selected", false)),
105+
selected);
106+
107+
EXPECT_TRUE(
108+
hasTrait(cleared.accessibilityTraits, AccessibilityTraits::Button));
109+
EXPECT_FALSE(
110+
hasTrait(cleared.accessibilityTraits, AccessibilityTraits::Selected));
111+
}
112+
113+
TEST(AccessibilityPropsTest, inherits_role_traits_when_raw_props_are_absent) {
114+
auto source = parse(folly::dynamic::object("role", "button"));
115+
116+
auto props = parse(folly::dynamic::object("nativeID", "abc"), source);
117+
118+
EXPECT_TRUE(hasTrait(props.accessibilityTraits, AccessibilityTraits::Button));
119+
}
120+
121+
} // namespace facebook::react

0 commit comments

Comments
 (0)