Added talkback support for button accessibility: disabled prop#31001

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

Closed
huzaifaaak 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 numberDiff line numberDiff 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';

@lunaleapslunaleapsFeb 17, 2021

Copy link
Copy Markdown
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

@huzaifaaakhuzaifaaakFeb 17, 2021•
edited
Loading

Copy link
Copy Markdown
Author

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 DownExpand 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 DownExpand 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;

@lunaleapslunaleapsFeb 18, 2021

Copy link
Copy Markdown
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

@lunaleapslunaleapsFeb 18, 2021

Copy link
Copy Markdown
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

@lunaleapslunaleapsFeb 18, 2021

Copy link
Copy Markdown
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.

@huzaifaaakhuzaifaaakFeb 18, 2021

Copy link
Copy Markdown
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

@blavallablavallaFeb 18, 2021

Copy link
Copy Markdown
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.

@blavallablavallaFeb 18, 2021

Copy link
Copy Markdown
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.

@lunaleapslunaleapsFeb 19, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

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.

@huzaifaaakhuzaifaaakFeb 19, 2021

Copy link
Copy Markdown
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.

@lunaleapslunaleapsFeb 19, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

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 numberDiff line numberDiff 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
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Added talkback support for button accessibility: disabled prop#31001

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

 Closed

huzaifaaak
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 numberDiff line numberDiff 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';
@lunaleapslunaleapsFeb 17, 2021

Copy link


Copy Markdown

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
@huzaifaaakhuzaifaaakFeb 17, 2021•

edited

Loading

Copy link


Copy Markdown

Author
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 DownExpand 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 DownExpand 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;
@lunaleapslunaleapsFeb 18, 2021

Copy link


Copy Markdown

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
@lunaleapslunaleapsFeb 18, 2021

Copy link


Copy Markdown

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
@lunaleapslunaleapsFeb 18, 2021

Copy link


Copy Markdown

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.
@huzaifaaakhuzaifaaakFeb 18, 2021

Copy link


Copy Markdown

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
@blavallablavallaFeb 18, 2021

Copy link


Copy Markdown

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.
@blavallablavallaFeb 18, 2021

Copy link


Copy Markdown

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.
@lunaleapslunaleapsFeb 19, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
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.
@huzaifaaakhuzaifaaakFeb 19, 2021

Copy link


Copy Markdown

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.
@lunaleapslunaleapsFeb 19, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
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 numberDiff line numberDiff 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

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Added talkback support for button accessibility: disabled prop#31001

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

 Closed

huzaifaaak
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 numberDiff line numberDiff 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';
@lunaleapslunaleapsFeb 17, 2021

Copy link


Copy Markdown

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
@huzaifaaakhuzaifaaakFeb 17, 2021•

edited

Loading

Copy link


Copy Markdown

Author
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 DownExpand 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 DownExpand 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;
@lunaleapslunaleapsFeb 18, 2021

Copy link


Copy Markdown

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
@lunaleapslunaleapsFeb 18, 2021

Copy link


Copy Markdown

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
@lunaleapslunaleapsFeb 18, 2021

Copy link


Copy Markdown

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.
@huzaifaaakhuzaifaaakFeb 18, 2021

Copy link


Copy Markdown

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
@blavallablavallaFeb 18, 2021

Copy link


Copy Markdown

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.
@blavallablavallaFeb 18, 2021

Copy link


Copy Markdown

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.
@lunaleapslunaleapsFeb 19, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
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.
@huzaifaaakhuzaifaaakFeb 19, 2021

Copy link


Copy Markdown

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.
@lunaleapslunaleapsFeb 19, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
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 numberDiff line numberDiff 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

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Added talkback support for button accessibility: disabled prop#31001

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

 Closed

huzaifaaak
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 numberDiff line numberDiff 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';
@lunaleapslunaleapsFeb 17, 2021

Copy link


Copy Markdown

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
@huzaifaaakhuzaifaaakFeb 17, 2021•

edited

Loading

Copy link


Copy Markdown

Author
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 DownExpand 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 DownExpand 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;
@lunaleapslunaleapsFeb 18, 2021

Copy link


Copy Markdown

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
@lunaleapslunaleapsFeb 18, 2021

Copy link


Copy Markdown

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
@lunaleapslunaleapsFeb 18, 2021

Copy link


Copy Markdown

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.
@huzaifaaakhuzaifaaakFeb 18, 2021

Copy link


Copy Markdown

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
@blavallablavallaFeb 18, 2021

Copy link


Copy Markdown

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.
@blavallablavallaFeb 18, 2021

Copy link


Copy Markdown

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.
@lunaleapslunaleapsFeb 19, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
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.
@huzaifaaakhuzaifaaakFeb 19, 2021

Copy link


Copy Markdown

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.
@lunaleapslunaleapsFeb 19, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
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 numberDiff line numberDiff 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

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Added talkback support for button accessibility: disabled prop#31001

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

 Closed

huzaifaaak
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 numberDiff line numberDiff 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';
@lunaleapslunaleapsFeb 17, 2021

Copy link


Copy Markdown

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
@huzaifaaakhuzaifaaakFeb 17, 2021•

edited

Loading

Copy link


Copy Markdown

Author
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 DownExpand 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 DownExpand 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;
@lunaleapslunaleapsFeb 18, 2021

Copy link


Copy Markdown

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
@lunaleapslunaleapsFeb 18, 2021

Copy link


Copy Markdown

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
@lunaleapslunaleapsFeb 18, 2021

Copy link


Copy Markdown

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.
@huzaifaaakhuzaifaaakFeb 18, 2021

Copy link


Copy Markdown

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
@blavallablavallaFeb 18, 2021

Copy link


Copy Markdown

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.
@blavallablavallaFeb 18, 2021

Copy link


Copy Markdown

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.
@lunaleapslunaleapsFeb 19, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
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.
@huzaifaaakhuzaifaaakFeb 19, 2021

Copy link


Copy Markdown

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.
@lunaleapslunaleapsFeb 19, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
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 numberDiff line numberDiff 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

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Added talkback support for button accessibility: disabled prop#31001

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

 Closed

huzaifaaak
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 numberDiff line numberDiff 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';
@lunaleapslunaleapsFeb 17, 2021

Copy link


Copy Markdown

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
@huzaifaaakhuzaifaaakFeb 17, 2021•

edited

Loading

Copy link


Copy Markdown

Author
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 DownExpand 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 DownExpand 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;
@lunaleapslunaleapsFeb 18, 2021

Copy link


Copy Markdown

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
@lunaleapslunaleapsFeb 18, 2021

Copy link


Copy Markdown

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
@lunaleapslunaleapsFeb 18, 2021

Copy link


Copy Markdown

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.
@huzaifaaakhuzaifaaakFeb 18, 2021

Copy link


Copy Markdown

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
@blavallablavallaFeb 18, 2021

Copy link


Copy Markdown

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.
@blavallablavallaFeb 18, 2021

Copy link


Copy Markdown

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.
@lunaleapslunaleapsFeb 19, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
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.
@huzaifaaakhuzaifaaakFeb 19, 2021

Copy link


Copy Markdown

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.
@lunaleapslunaleapsFeb 19, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
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 numberDiff line numberDiff 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

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Added talkback support for button accessibility: disabled prop#31001

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

 Closed

huzaifaaak
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 numberDiff line numberDiff 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';
@lunaleapslunaleapsFeb 17, 2021

Copy link


Copy Markdown

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
@huzaifaaakhuzaifaaakFeb 17, 2021•

edited

Loading

Copy link


Copy Markdown

Author
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 DownExpand 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 DownExpand 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;
@lunaleapslunaleapsFeb 18, 2021

Copy link


Copy Markdown

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
@lunaleapslunaleapsFeb 18, 2021

Copy link


Copy Markdown

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
@lunaleapslunaleapsFeb 18, 2021

Copy link


Copy Markdown

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.
@huzaifaaakhuzaifaaakFeb 18, 2021

Copy link


Copy Markdown

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
@blavallablavallaFeb 18, 2021

Copy link


Copy Markdown

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.
@blavallablavallaFeb 18, 2021

Copy link


Copy Markdown

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.
@lunaleapslunaleapsFeb 19, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
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.
@huzaifaaakhuzaifaaakFeb 19, 2021

Copy link


Copy Markdown

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.
@lunaleapslunaleapsFeb 19, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
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 numberDiff line numberDiff 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

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Added talkback support for button accessibility: disabled prop#31001

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

 Closed

huzaifaaak
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 numberDiff line numberDiff 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';
@lunaleapslunaleapsFeb 17, 2021

Copy link


Copy Markdown

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
@huzaifaaakhuzaifaaakFeb 17, 2021•

edited

Loading

Copy link


Copy Markdown

Author
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 DownExpand 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 DownExpand 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;
@lunaleapslunaleapsFeb 18, 2021

Copy link


Copy Markdown

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
@lunaleapslunaleapsFeb 18, 2021

Copy link


Copy Markdown

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
@lunaleapslunaleapsFeb 18, 2021

Copy link


Copy Markdown

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.
@huzaifaaakhuzaifaaakFeb 18, 2021

Copy link


Copy Markdown

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
@blavallablavallaFeb 18, 2021

Copy link


Copy Markdown

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.
@blavallablavallaFeb 18, 2021

Copy link


Copy Markdown

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.
@lunaleapslunaleapsFeb 19, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
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.
@huzaifaaakhuzaifaaakFeb 19, 2021

Copy link


Copy Markdown

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.
@lunaleapslunaleapsFeb 19, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
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 numberDiff line numberDiff 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