-
Notifications
You must be signed in to change notification settings - Fork 24.8k
Fix: a11y crash when an accessible link is ellipsized away #37050
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Base commit: 01d80c1 |
b5ea6cb
to
eb5d823
Compare
if (bounds == null) { | ||
node.setContentDescription(""); | ||
node.setBoundsInParent(new Rect(0, 0, 1, 1)); | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, why we're not adding the "action" AccessibilityNodeInfoCompat.ACTION_CLICK
as well; here?
if (bounds == null) { | |
node.setContentDescription(""); | |
node.setBoundsInParent(new Rect(0, 0, 1, 1)); | |
return; | |
} | |
if (bounds == null) { | |
node.setContentDescription(""); | |
node.addAction(AccessibilityNodeInfoCompat.ACTION_CLICK); | |
node.setBoundsInParent(new Rect(0, 0, 1, 1)); | |
return; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The link has been ellipsized away so it's not visible (IE would not actually be tappable). The two checks above don't include it when there isn't a valid link, etc; that seemed to apply in this case, so I mirrored that approach.
@NickGerleman seems like no one reviewed this PR? |
@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@NickGerleman merged this pull request in d54f486. |
Summary: I got notified that a recent change I made was causing a crash. This would happen when the endOffset of the link is larger than the end offset of the line. This can be repro'd with ``` /** * (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. * * flow strict-local * format */ import {useState} from 'react'; import {Platform, StyleSheet, Text, View} from 'react-native'; export default function Playground() { const [state, setState] = useState(false); function toggle() { setState(old => !old); } return ( <View style={styles.container}> <Text style={[styles.paragraph, state && {backgroundColor: 'red'}]} numberOfLines={3}> <Text>Bacon Ipsum{'\n'}</Text> <Text>Dolor sit amet{'\n'}</Text> <Text>{'\n'}</Text> <Text accessibilityRole="link" onPress={toggle}> http://www.google.com </Text> </Text> </View> ); } const styles = StyleSheet.create({ container: { flex: 1, justifyContent: 'center', paddingTop: 24, backgroundColor: '#ecf0f1', padding: 8, }, paragraph: { margin: 24, fontSize: 18, fontWeight: 'bold', textAlign: 'center', }, }); ``` This was encountered already by NickGerleman in D46206673 (facebook#37050) and the fix is fairly straightforward. I modified that fix to check for the endOffset as well. Changelog: [Internal] Differential Revision: D74823458
Summary: Pull Request resolved: #51372 I got notified that a recent change I made was causing a crash. This would happen when the endOffset of the link is larger than the end offset of the line. This can be repro'd with ``` /** * (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. * * flow strict-local * format */ import {useState} from 'react'; import {Platform, StyleSheet, Text, View} from 'react-native'; export default function Playground() { const [state, setState] = useState(false); function toggle() { setState(old => !old); } return ( <View style={styles.container}> <Text style={[styles.paragraph, state && {backgroundColor: 'red'}]} numberOfLines={3}> <Text>Bacon Ipsum{'\n'}</Text> <Text>Dolor sit amet{'\n'}</Text> <Text>{'\n'}</Text> <Text accessibilityRole="link" onPress={toggle}> http://www.google.com </Text> </Text> </View> ); } const styles = StyleSheet.create({ container: { flex: 1, justifyContent: 'center', paddingTop: 24, backgroundColor: '#ecf0f1', padding: 8, }, paragraph: { margin: 24, fontSize: 18, fontWeight: 'bold', textAlign: 'center', }, }); ``` This was encountered already by NickGerleman in D46206673 (#37050) and the fix is fairly straightforward. I modified that fix to check for the endOffset as well. Changelog: [Internal] Reviewed By: zeyap Differential Revision: D74823458 fbshipit-source-id: 08fe81893f1c41d1934215515f88a6e0e546e705
Summary:
If an accessible link is ellipsized out of being rendered, the AccessibilityDelegate will still attempt to populate an accessibility node for it; doing so results in an invalid request to a TextLayout API, however, causing a crash. This crash occurs as soon as the element is rendered, so long as a Screen Reader (or app using similar a11y APIs) is enabled. This change uses a technique similar to those existing to make the node "blank" in such cases, so Talkback can filter it out—and, more importantly, not crash.
Changelog:
[Android] [Fixed] - Fix links hidden via ellipsis crashing screen readers
Test Plan:
ReactAccessibilityDelegate
, this component crashes the app; with the changes, the component renders without problem.