Skip to content

[0.73] Fix new role prop after JS-Shim was removed by Meta - #2100

Merged
Alicia Drummond (FalseLobster) merged 10 commits into
microsoft:0.73-stablefrom
FalseLobster:allyrole73
Mar 22, 2024
Merged

[0.73] Fix new role prop after JS-Shim was removed by Meta#2100
Alicia Drummond (FalseLobster) merged 10 commits into
microsoft:0.73-stablefrom
FalseLobster:allyrole73

Conversation

@FalseLobster

Copy link
Copy Markdown

Summary:

Prior to 0.73, the new role prop was remapped to accessibilityRole on the JS-side. Starting with react#37304, the work needs to be completed on the native side. Since the new prop is ARIA inspired, the mappings are taking from the ARIA Core AAM which disagrees with some of the mappings used in the old accessibilityRole prop. Users of the old prop are unaffected, but the new prop will take the mappings from the spec.

Test Plan:

Tested a variety of permutations of accessibilityRole and role to confirm the behavior looks correct in Accessibility Inspector

Comment threadpackages/rn-tester/RNTesterPods.xcodeproj/project.pbxproj
Comment threadpackages/react-native/React/Base/RCTConvert.m Outdated
Comment threadpackages/react-native/React/Base/RCTConvert.m Outdated
Comment threadpackages/react-native/React/Base/RCTConvert.m Outdated
Comment threadpackages/react-native/React/Base/RCTConvert.m Outdated
Comment threadpackages/react-native/React/Base/RCTConvert.m Outdated
Comment threadpackages/react-native/React/Base/RCTConvert.h Outdated
Comment threadpackages/react-native/React/Base/RCTConvert.m Outdated
Comment threadpackages/react-native/React/Base/RCTConvert.m Outdated
Comment threadpackages/react-native/React/Base/RCTConvert.m Outdated
Comment threadpackages/react-native/React/Base/RCTConvert.m Outdated
Comment threadpackages/react-native/React/Views/RCTViewManager.m Outdated
Comment threadpackages/react-native/React/Views/RCTViewManager.m Outdated
@Saadnajmi

Copy link
Copy Markdown
Collaborator

An aside, we'll want to eventually also implement for Fabric, but not blocking for 0.73

@Saadnajmi

Copy link
Copy Markdown
Collaborator

Q: Would it make sense for our main branch version of the PR to use the aria mappings for the accessibilityRole slot, rather than the legacy mappings we defined? I I can imagine we keep the legacy mappings for one version (0.73) and disable it in the next (0.74). Alternatively, maybe put it behind a Feature flag via this JS file or this native file ?

@FalseLobster

Copy link
Copy Markdown
Author

Q: Would it make sense for our main branch version of the PR to use the aria mappings for the accessibilityRole slot, rather than the legacy mappings we defined? I I can imagine we keep the legacy mappings for one version (0.73) and disable it in the next (0.74). Alternatively, maybe put it behind a Feature flag via this JS file or this native file ?

Maybe, I'm open to it. I kinda suspect at some point Meta will drop accessibilityRole like they did accessibilityTraits (even though our impl still has accessibilityTraits). I think we run the risk internally of breaking partners who are using accessibilityRole if we silently change some mappings for them, whereas only applying the new mappings to the new values to the new prop feels safe, which is worth considering, but I suppose that's why you're calling out the feature flag :)

Comment threadpackages/react-native/React/Base/RCTConvert.h Outdated
Comment threadpackages/react-native/React/Base/RCTConvert.m Outdated
Comment threadpackages/react-native/React/Views/RCTViewManager.m Outdated
Comment threadpackages/react-native/React/Views/RCTViewManager.m
Comment threadpackages/react-native/React/Base/RCTConvert.m
@FalseLobster
Alicia Drummond (FalseLobster) merged commit bbe12e3 into microsoft:0.73-stableMar 22, 2024
Alicia Drummond (FalseLobster) added a commit that referenced this pull request Mar 22, 2024
## Summary:
main targeted PR of #2100 Prior to 0.73, the new `role` prop was remapped to `accessibilityRole`
on the JS-side. Starting with react#37304, the work needs to be completed on
the native side. Since the new prop is ARIA inspired, the mappings are
taking from the [ARIA Core
AAM](https://www.w3.org/TR/core-aam-1.2/#mapping_role_table) which
disagrees with some of the mappings used in the old `accessibilityRole`
prop. Users of the old prop are unaffected, but the new prop will take
the mappings from the spec.
## Test Plan:
Tested a variety of permutations of accessibilityRole and role to
confirm the behavior looks correct in Accessibility Inspector
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@FalseLobster@Saadnajmi@amgleitman