Skip to content
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.

Sign up for GitHub

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

Added talkback support for button accessibility: disabled prop #31001

Closed
wants to merge 12 commits into from
Closed

Added talkback support for button accessibility: disabled prop #31001

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
599cd81
Chores: added talkback for button accessibility: disabled prop
Feb 16, 2021
7266305
Merge branch 'master' into accessibility/button
Feb 16, 2021
eb7bda3
fix: fixed flow error by adding type and refactoring accessibility st…
Feb 16, 2021
eb3c9a0
Merge branch 'accessibility/button' of https://github.com/huzaifaaak/…
Feb 16, 2021
0024420
Chores: refactored condition and updated string in example
Feb 17, 2021
db1644f
chores: formatting reverted :apple:
Feb 17, 2021
d3fcdb6
Merge branch 'master' into accessibility/button
Feb 26, 2021
23a1f9b
Chores: updated disable to have higher precedence over accessibilitys…
Feb 26, 2021
e0ece83
Chores: refactored for tests
Mar 4, 2021
b499b2f
chores: added accState.disabled to apply styling too
Mar 9, 2021
7e6dfc8
Refactor: Avoid creating a new object
Mar 10, 2021
c25f8e8
Merge branch 'master' into accessibility/button
Mar 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions Libraries/Components/Button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ const Text = require('../Text/Text');
const TouchableNativeFeedback = require('./Touchable/TouchableNativeFeedback');
const TouchableOpacity = require('./Touchable/TouchableOpacity');
const View = require('./View/View');

import type {AccessibilityState} from './View/ViewAccessibility';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: should this also be grouped with line 24? I can also fix this on import

Copy link
Author

@huzaifaaak huzaifaaak Feb 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this should be grouped with other type imports 💯. Please fix this during import

const invariant = require('invariant');

import type {PressEvent} from '../Types/CoreEventTypes';
Expand Down Expand Up @@ -134,6 +134,11 @@ type ButtonProps = $ReadOnly<{|
Used to locate this view in end-to-end tests.
*/
testID?: ?string,

/**
* Accessibility props.
*/
accessibilityState?: ?AccessibilityState,
|}>;

/**
Expand Down Expand Up @@ -261,7 +266,6 @@ class Button extends React.Component<ButtonProps> {
nextFocusLeft,
nextFocusRight,
nextFocusUp,
disabled,
testID,
} = this.props;
const buttonStyles = [styles.button];
Expand All @@ -273,12 +277,22 @@ class Button extends React.Component<ButtonProps> {
buttonStyles.push({backgroundColor: color});
}
}
const accessibilityState = {};

const disabled =
this.props.disabled != null
? this.props.disabled
: this.props.accessibilityState?.disabled;

const accessibilityState =
disabled !== this.props.accessibilityState?.disabled
? {...this.props.accessibilityState, disabled}
: this.props.accessibilityState;

if (disabled) {
buttonStyles.push(styles.buttonDisabled);
textStyles.push(styles.textDisabled);
accessibilityState.disabled = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kacieb brought up a great point -- this PR doesn't seem to be fixing accessibility for talkback but rather adding an ability for accessibilityState -- I tested on an Android device and did notice that talkback does announce "disabled" for the button in your example where you solely use accessibilityState.

That raises the question of what was the original issue reporting? Was it reporting that accessibilityState={{disabled.. wasn't working -- which is indeed what your PR fixes -- but the fact that the property didn't even exist before has me puzzled

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @blavalla who reported the issue for clarification -- relevant to our discussion on disabled vs. accessibilityState.disabled

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to hold off landing this PR until we have some clarity about what the original issue is.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted!
Link to the accessibility issue: #30840

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lunaleaps, the original issue was that setting accessibilityState: disabled alone was not actually disabling the button for accessibility.

This snack (https://snack.expo.io/d1Ikg7nqo) from #30840 shows the issue well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify further, the issue was mostly that it is weird that you can actually set conflicting states for accessibility and non-accessibility users. Basically, there is nothing stopping someone from setting disabled="true" accessibilityState={{disabled: false}} or vice-versa.

If we are going to explicitly allow this though, they should at least work as expected, with the accessibilityState: disabled actually disabling the element for accessibility users.

Copy link
Contributor

@lunaleaps lunaleaps Feb 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right! accessibilityState is a View level prop so it should already exist on <Button />. My mistake! Okay that clarifies things! It sounds like this change is still worth pursuing -- to just sync the two values

Edit: Ah right because the snack does not have Flow enabled, it didn't warn by the fact that accessibiltyState isn't a prop on Button.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are going to explicitly allow this though, they should at least work as expected, with the accessibilityState: disabled actually disabling the element for accessibility users.

@lunaleaps I explicitly tested disabled="false" and accessibilityState={{disabled: true}} and it announces that button is disabled but it does not disables the onPress. I believe this needs to be done on the View level.

Having two different props is confusing. To disable the onPress we have to explicitly pass the disabled={true} prop.

Copy link
Contributor

@lunaleaps lunaleaps Feb 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and it announces that button is disabled but it does not disables the onPress. I believe this needs to be done on the View level.

Yea I think that's covered by #30943

I think we should set this down for a bit and circle back to how we should handle these two props

}

invariant(
typeof title === 'string',
'The title prop of a Button must be a string',
Expand All @@ -287,6 +301,7 @@ class Button extends React.Component<ButtonProps> {
Platform.OS === 'android' ? title.toUpperCase() : title;
const Touchable =
Platform.OS === 'android' ? TouchableNativeFeedback : TouchableOpacity;

return (
<Touchable
accessibilityLabel={accessibilityLabel}
Expand Down
23 changes: 23 additions & 0 deletions packages/rn-tester/js/examples/Button/ButtonExample.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,29 @@ exports.examples = [
);
},
},
{
title: 'Button with accessibilityState={{disabled: true}}',
description: ('Note: This prop will announce on TalkBack that the button is disabled. ' +
'The "disabled" prop has higher precedence on the state of the component': string),
render: function(): React.Node {
return (
<RNTesterThemeContext.Consumer>
{theme => {
return (
<Button
accessibilityState={{disabled: true}}
onPress={() => onButtonPress('submitted')}
testID="accessibilityState_button"
color={theme.LinkColor}
title="Submit Application"
accessibilityLabel="Press to submit your application!"
/>
);
}}
</RNTesterThemeContext.Consumer>
);
},
},
];

const styles = StyleSheet.create({
Expand Down