Added talkback support for button accessibility: disabled prop - #31001

Closed
huzaifaaak wants to merge 12 commits into
react:masterfrom
huzaifaaak:accessibility/button
Closed

Added talkback support for button accessibility: disabled prop#31001
huzaifaaak wants to merge 12 commits into
react:masterfrom
huzaifaaak:accessibility/button

Conversation

@huzaifaaak

@huzaifaaakhuzaifaaak commented Feb 16, 2021

Copy link
Copy Markdown

Summary

Issue # #30934 .When using a screen reader disabled buttons do not announce that they are disabled.

Changelog

[Android] [Changed] - Passing accessibility state in button so it can announce disabled in talkback

Test Plan

I have added Button in Button Example with accessibiltyState prop that will announce button is disabled when testing with talkback.

Ios test

I am unable to run ios project on my machine. RNTesterPods.xcworkspace gives workspace integrity error :/

@facebook-github-bot

facebook-github-bot commented Feb 16, 2021

Copy link
Copy Markdown
Contributor

Hi @huzaifaaak!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@analysis-bot

analysis-bot commented Feb 16, 2021•
edited
Loading

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
ios-universaln/a--

Base commit: 4170726

@analysis-bot

analysis-bot commented Feb 16, 2021•
edited
Loading

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,908,772+366
androidhermesarmeabi-v7a8,407,852+375
androidhermesx869,399,329+376
androidhermesx86_649,343,247+368
androidjscarm64-v8a10,642,464+144
androidjscarmeabi-v7a10,124,638+159
androidjscx8610,694,515+149
androidjscx86_6411,278,700+142

Base commit: 4170726

@facebook-github-bot

facebook-github-bot commented Feb 16, 2021

Copy link
Copy Markdown
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@facebook-github-botfacebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 16, 2021
@facebook-github-bot

facebook-github-bot commented Feb 16, 2021

Copy link
Copy Markdown
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

analysis-bot
analysis-bot suggested changes Feb 16, 2021
View reviewed changes

@analysis-botanalysis-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code analysis results:

  • eslint found some issues. Run yarn lint --fix to automatically fix problems.

Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js Outdated
Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js Outdated
@facebook-github-bot

facebook-github-bot commented Feb 16, 2021

Copy link
Copy Markdown
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

4 similar comments
@facebook-github-bot

facebook-github-bot commented Feb 16, 2021

Copy link
Copy Markdown
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@facebook-github-bot

facebook-github-bot commented Feb 17, 2021

Copy link
Copy Markdown
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@facebook-github-bot

facebook-github-bot commented Feb 17, 2021

Copy link
Copy Markdown
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@facebook-github-bot

facebook-github-bot commented Feb 17, 2021

Copy link
Copy Markdown
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@huzaifaaak

huzaifaaak commented Feb 17, 2021•
edited
Loading

Copy link
Copy Markdown
Author

@kacieb Can you please review this and give your comments 🙏

lunaleaps
lunaleaps reviewed Feb 17, 2021
View reviewed changes
Comment threadpackages/rn-tester/js/examples/Button/ButtonExample.js Outdated
Comment threadLibraries/Components/Button.js Outdated
}
const accessibilityState = {};
const accessibilityState =
disabled != null

@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.

I'd suggest const accessibilityState = disabled === true ? ... : ... so it's accessiblityState.disabled | disabled.

@lunaleapslunaleapsMar 10, 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.

Sorry to follow up on this can we avoid creating a new object here?

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

Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js Outdated
@lunaleaps

lunaleaps commented Feb 17, 2021

Copy link
Copy Markdown
Contributor

Thanks for adding an example as well!

analysis-bot
analysis-bot suggested changes Feb 17, 2021
View reviewed changes

@analysis-botanalysis-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code analysis results:

  • eslint found some issues. Run yarn lint --fix to automatically fix problems.

Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js Outdated
onDismiss={() => {
if (this.state.action === 'onDismiss') alert(this.state.action);
if (this.state.action === 'onDismiss') {
alert(this.state.action);

@analysis-botanalysis-botFeb 17, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

no-alert: Unexpected alert.

Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js Outdated
onShow={() => {
if (this.state.action === 'onShow') alert(this.state.action);
if (this.state.action === 'onShow') {
alert(this.state.action);

@analysis-botanalysis-botFeb 17, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

no-alert: Unexpected alert.

lunaleaps
lunaleaps approved these changes Feb 17, 2021
View reviewed changes

@lunaleapslunaleaps left a comment

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.

Looks good to me!

Comment threadLibraries/Components/Button.js
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

facebook-github-bot
facebook-github-bot reviewed Feb 17, 2021
View reviewed changes

@facebook-github-botfacebook-github-bot left a comment

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 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

lunaleaps
lunaleaps reviewed Feb 18, 2021
View reviewed changes
Comment threadLibraries/Components/Button.js
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

lunaleaps
lunaleaps reviewed Mar 8, 2021
View reviewed changes
Comment threadLibraries/Components/Button.js Outdated
nextFocusUp={nextFocusUp}
testID={testID}
disabled={disabled}
disabled={disabled != null ? disabled : accessibilityState?.disabled} //This will sync accessibilityState={disabled} with disabled prop.

@lunaleapslunaleapsMar 8, 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 think this should be done on line 286? Otherwise we're missing the logic in that if statement if accessibilityState.disabled is true

@huzaifaaakhuzaifaaakMar 8, 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.

I was a bit confused on this. For now it's not appyling the disable style if only accessibilityState?.disabled is passed.

@huzaifaaakhuzaifaaakMar 8, 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.

I think this needs to be passed here and on line 286 so it applies style and disables the button aswell if accessibilityState.disabled is true

@lunaleapslunaleapsMar 8, 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.

Yea I think we should treat the button as disabled if accessibilityState.disabled is true in all manners --- behaviour and styling

@kaciebkaciebMar 8, 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.

Yea I think we should treat the button as disabled if accessibilityState.disabled is true in all manners --- behaviour and styling

I think I agree with you -I think if accessibilityState.disabled=true and disabled=false though, disabled should take precedence. If disabled is undefined, then I agree accessibilityState.disabled=true should disable the component.

@huzaifaaakhuzaifaaakMar 9, 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.

@lunaleaps@kacieb thank you for the explanation. I have updated the accessibilityState.disabled=true to update styling aswell.

Edit: The disabled prop has precedence and if it is not provided accessibilityState.disabled will take over.

lunaleaps
lunaleaps approved these changes Mar 10, 2021
View reviewed changes

@lunaleapslunaleaps left a comment

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.

Otherwise looks good to me

Comment threadLibraries/Components/Button.js Outdated
}
const accessibilityState = {};
const accessibilityState =
disabled != null

@lunaleapslunaleapsMar 10, 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.

Sorry to follow up on this can we avoid creating a new object here?

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

@huzaifaaak

huzaifaaak commented Mar 10, 2021

Copy link
Copy Markdown
Author

@lunaleaps I have updated it to avoid object creation. Do let me know if it can be done in a better way

facebook-github-bot
facebook-github-bot reviewed Mar 10, 2021
View reviewed changes

@facebook-github-botfacebook-github-bot left a comment

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 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-botfacebook-github-bot added the Merged This PR has been merged. label Mar 11, 2021
@facebook-github-bot

facebook-github-bot commented Mar 11, 2021

Copy link
Copy Markdown
Contributor

@lunaleaps merged this pull request in 5889cbe.

@kacieb

kacieb commented Mar 12, 2021

Copy link
Copy Markdown
Contributor

This is awesome, thank you so much for working on this and improving React Native's accessibility! I'd love to see a snapshot test for this component at some point, similar to this Pressable one added here, that ensures the accessibilityState values are correctly set as expected.

@amarletteamarlette mentioned this pull request Mar 15, 2021
Closed
This was referenced Mar 15, 2021
Closed
Closed
@kaciebkacieb mentioned this pull request Mar 16, 2021
Closed
@amarlette

amarlette commented Mar 17, 2021

Copy link
Copy Markdown

Hey @huzaifaaak I would like to give you a shout-out on Twitter and include you in our end-of-month issues update for your contribution. Do you have a Twitter we can tag and is your @ the same as your Github?

@huzaifaaak

huzaifaaak commented Mar 17, 2021

Copy link
Copy Markdown
Author

Hey @amarlette yes my twitter handle is the same huzaifaaak

@huzaifaaakhuzaifaaak mentioned this pull request Mar 18, 2021
Closed
@blavallablavalla mentioned this pull request Mar 25, 2021
Closed
@amarletteamarlette linked an issue Mar 29, 2021 that may be closed by this pull request
Selected State does not annonce when Button Component selected #30954
Closed
@amarletteamarlette mentioned this pull request Mar 29, 2021
Closed
facebook-github-bot pushed a commit that referenced this pull request Apr 2, 2021
Huzaifa@facebook-github-bot
Accessibility/button test (#31189)
b5e649f
Summary:
This PR aims to add test's for button.
Snapshot test for PR #31001 . This would make sure `accessibilityState` is properly set.
## Changelog
<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->
[General] [Added] - Test's for button
Pull Request resolved: #31189
Test Plan:
`npm test` to run the test's.
Since the disabled prop of button has precedence over `accessibilityState.disabled` the test's will make sure it remains this way.
Reviewed By: kacieb
Differential Revision: D27473082
Pulled By: lunaleaps
fbshipit-source-id: 65d82620e8c245c2a8e29c3e9a8252d3a4275b09
@anaskhrazaanaskhraza mentioned this pull request May 16, 2021
Closed
@martinivanalfonsomartinivanalfonso mentioned this pull request May 16, 2021
Closed
@amarletteamarlette linked an issue May 24, 2021 that may be closed by this pull request
Button Component does not announce Checked/Unchecked State #30959
Closed
This was referenced Jan 28, 2022
Closed
Merged
@coderabbitaicoderabbitaiBot mentioned this pull request Aug 16, 2026
Merged
4 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nadiianadiiaAwaiting requested review from nadiia
@kaciebkaciebAwaiting requested review from kacieb
4 more reviewers
@blavallablavallablavalla left review comments
@facebook-github-botfacebook-github-botfacebook-github-bot left review comments
@analysis-botanalysis-botanalysis-bot requested changes
@lunaleapslunaleapslunaleaps approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

AccessibilityCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Button Component does not announce Checked/Unchecked State Selected State does not annonce when Button Component selected

7 participants

@huzaifaaak@facebook-github-bot@analysis-bot@lunaleaps@kacieb@amarlette@blavalla
, '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
Closed
huzaifaaak wants to merge 12 commits into
react:masterfrom 
huzaifaaak:accessibility/button
Closed
Added talkback support for button accessibility: disabled prop#31001
huzaifaaak wants to merge 12 commits into
react:masterfrom 
huzaifaaak:accessibility/button
Conversation
@huzaifaaak
@huzaifaaakhuzaifaaak
commented
Feb 16, 2021

Copy link


Copy Markdown

Summary
Issue # #30934 .When using a screen reader disabled buttons do not announce that they are disabled.
Changelog
[Android] [Changed] - Passing accessibility state in button so it can announce disabled in talkback
Test Plan
I have added Button in Button Example with accessibiltyState prop that will announce button is disabled when testing with talkback.
Ios test
I am unable to run ios project on my machine. RNTesterPods.xcworkspace gives workspace integrity error :/
@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Hi @huzaifaaak!
Thank you for your pull request and welcome to our community.
Action Required
In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.
Process
In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.
Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.
If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!
@analysis-bot
analysis-bot
commented
Feb 16, 2021•

edited

Loading

Copy link


Copy Markdown

PlatformEngineArchSize (bytes)Diff
ios-universaln/a--
Base commit: 4170726
@analysis-bot
analysis-bot
commented
Feb 16, 2021•

edited

Loading

Copy link


Copy Markdown

PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,908,772+366
androidhermesarmeabi-v7a8,407,852+375
androidhermesx869,399,329+376
androidhermesx86_649,343,247+368
androidjscarm64-v8a10,642,464+144
androidjscarmeabi-v7a10,124,638+159
androidjscx8610,694,515+149
androidjscx86_6411,278,700+142
Base commit: 4170726
@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@facebook-github-botfacebook-github-bot
added
the 
CLA Signed
This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
label
Feb 16, 2021
@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
analysis-bot
analysis-bot
suggested changes
Feb 16, 2021
View reviewed changes
@analysis-botanalysis-bot
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Code analysis results:
  • eslint found some issues. Run yarn lint --fix to automatically fix problems.
Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

4 similar comments

@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@facebook-github-bot
facebook-github-bot
commented
Feb 17, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@facebook-github-bot
facebook-github-bot
commented
Feb 17, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@facebook-github-bot
facebook-github-bot
commented
Feb 17, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@huzaifaaak
huzaifaaak
commented
Feb 17, 2021•

edited

Loading

Copy link


Copy Markdown

Author
@kacieb Can you please review this and give your comments 🙏
lunaleaps
lunaleaps
reviewed
Feb 17, 2021
View reviewed changes
Comment threadpackages/rn-tester/js/examples/Button/ButtonExample.js
Outdated

Comment threadLibraries/Components/Button.js
Outdated

 }
 const accessibilityState = {};
 const accessibilityState =
 disabled != null
@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.

I'd suggest const accessibilityState = disabled === true ? ... : ... so it's accessiblityState.disabled | disabled.
@lunaleapslunaleapsMar 10, 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.

Sorry to follow up on this can we avoid creating a new object here?
const accessibilityState = disabled !== accessibilityState.disabled ? {...this.props.accessibilityState, disabled} : this.props.accessibilityState

Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

@lunaleaps
lunaleaps
commented
Feb 17, 2021

Copy link


Copy Markdown

Contributor
Thanks for adding an example as well!
analysis-bot
analysis-bot
suggested changes
Feb 17, 2021
View reviewed changes
@analysis-botanalysis-bot
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Code analysis results:
  • eslint found some issues. Run yarn lint --fix to automatically fix problems.
Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

 onDismiss={() => {
 if (this.state.action === 'onDismiss') alert(this.state.action);
 if (this.state.action === 'onDismiss') {
 alert(this.state.action);
@analysis-botanalysis-botFeb 17, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

no-alert: Unexpected alert.
Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

 onShow={() => {
 if (this.state.action === 'onShow') alert(this.state.action);
 if (this.state.action === 'onShow') {
 alert(this.state.action);
@analysis-botanalysis-botFeb 17, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

no-alert: Unexpected alert.
lunaleaps
lunaleaps
approved these changes
Feb 17, 2021
View reviewed changes
@lunaleapslunaleaps
left a comment


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.

Looks good to me!
Comment threadLibraries/Components/Button.js
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
facebook-github-bot
facebook-github-bot
reviewed
Feb 17, 2021
View reviewed changes
@facebook-github-botfacebook-github-bot
left a comment


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 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
lunaleaps
lunaleaps
reviewed
Feb 18, 2021
View reviewed changes
Comment threadLibraries/Components/Button.js
 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
lunaleaps
lunaleaps
reviewed
Mar 8, 2021
View reviewed changes
Comment threadLibraries/Components/Button.js
Outdated

 nextFocusUp={nextFocusUp}
 testID={testID}
 disabled={disabled}
 disabled={disabled != null ? disabled : accessibilityState?.disabled} //This will sync accessibilityState={disabled} with disabled prop.
@lunaleapslunaleapsMar 8, 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 think this should be done on line 286? Otherwise we're missing the logic in that if statement if accessibilityState.disabled is true
@huzaifaaakhuzaifaaakMar 8, 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.

I was a bit confused on this. For now it's not appyling the disable style if only accessibilityState?.disabled is passed.
@huzaifaaakhuzaifaaakMar 8, 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.

I think this needs to be passed here and on line 286 so it applies style and disables the button aswell if accessibilityState.disabled is true
@lunaleapslunaleapsMar 8, 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.

Yea I think we should treat the button as disabled if accessibilityState.disabled is true in all manners --- behaviour and styling
@kaciebkaciebMar 8, 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.

Yea I think we should treat the button as disabled if accessibilityState.disabled is true in all manners --- behaviour and styling
I think I agree with you -I think if accessibilityState.disabled=true and disabled=false though, disabled should take precedence. If disabled is undefined, then I agree accessibilityState.disabled=true should disable the component.
@huzaifaaakhuzaifaaakMar 9, 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.

@lunaleaps@kacieb thank you for the explanation. I have updated the accessibilityState.disabled=true to update styling aswell.
Edit: The disabled prop has precedence and if it is not provided accessibilityState.disabled will take over.
lunaleaps
lunaleaps
approved these changes
Mar 10, 2021
View reviewed changes
@lunaleapslunaleaps
left a comment


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.

Otherwise looks good to me
Comment threadLibraries/Components/Button.js
Outdated

 }
 const accessibilityState = {};
 const accessibilityState =
 disabled != null
@lunaleapslunaleapsMar 10, 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.

Sorry to follow up on this can we avoid creating a new object here?
const accessibilityState = disabled !== accessibilityState.disabled ? {...this.props.accessibilityState, disabled} : this.props.accessibilityState

@huzaifaaak
huzaifaaak
commented
Mar 10, 2021

Copy link


Copy Markdown

Author
@lunaleaps I have updated it to avoid object creation. Do let me know if it can be done in a better way
facebook-github-bot
facebook-github-bot
reviewed
Mar 10, 2021
View reviewed changes
@facebook-github-botfacebook-github-bot
left a comment


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 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@facebook-github-botfacebook-github-bot
added
the 
Merged
This PR has been merged.
label
Mar 11, 2021
@facebook-github-bot
facebook-github-bot
commented
Mar 11, 2021

Copy link


Copy Markdown

Contributor
@lunaleaps merged this pull request in 5889cbe.
@kacieb
kacieb
commented
Mar 12, 2021

Copy link


Copy Markdown

Contributor
This is awesome, thank you so much for working on this and improving React Native's accessibility! I'd love to see a snapshot test for this component at some point, similar to this Pressable one added here, that ensures the accessibilityState values are correctly set as expected.
@amarletteamarlette
mentioned this pull request
Mar 15, 2021

Closed


This was referenced Mar 15, 2021

Closed


Closed

@kaciebkacieb
mentioned this pull request
Mar 16, 2021
 Closed

@amarlette
amarlette
commented
Mar 17, 2021

Copy link


Copy Markdown

Hey @huzaifaaak I would like to give you a shout-out on Twitter and include you in our end-of-month issues update for your contribution. Do you have a Twitter we can tag and is your @ the same as your Github?
@huzaifaaak
huzaifaaak
commented
Mar 17, 2021

Copy link


Copy Markdown

Author
Hey @amarlette yes my twitter handle is the same huzaifaaak
@huzaifaaakhuzaifaaak
mentioned this pull request
Mar 18, 2021
 Closed

@blavallablavalla
mentioned this pull request
Mar 25, 2021

Closed

@amarletteamarlette
linked an issue
Mar 29, 2021
that may be
closed
by this pull request


Selected State does not annonce when Button Component selected
#30954

Closed

@amarletteamarlette
mentioned this pull request
Mar 29, 2021
 Closed

facebook-github-bot pushed a commit
that referenced
this pull request
Apr 2, 2021
Huzaifa@facebook-github-bot
Accessibility/button test (#31189)
b5e649f
Summary:
This PR aims to add test's for button.
Snapshot test for PR #31001 . This would make sure `accessibilityState` is properly set.
## Changelog
<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->
[General] [Added] - Test's for button
Pull Request resolved: #31189
Test Plan:
`npm test` to run the test's.
Since the disabled prop of button has precedence over `accessibilityState.disabled` the test's will make sure it remains this way.
Reviewed By: kacieb
Differential Revision: D27473082
Pulled By: lunaleaps
fbshipit-source-id: 65d82620e8c245c2a8e29c3e9a8252d3a4275b09
@anaskhrazaanaskhraza
mentioned this pull request
May 16, 2021

Closed

@martinivanalfonsomartinivanalfonso
mentioned this pull request
May 16, 2021

Closed

@amarletteamarlette
linked an issue
May 24, 2021
that may be
closed
by this pull request


Button Component does not announce Checked/Unchecked State
#30959

Closed


This was referenced Jan 28, 2022

Closed

 Merged

@coderabbitaicoderabbitaiBot
mentioned this pull request
Aug 16, 2026
 Merged

4 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@nadiianadiiaAwaiting requested review from nadiia
@kaciebkaciebAwaiting requested review from kacieb

4 more reviewers

@blavallablavallablavalla left review comments
@facebook-github-botfacebook-github-botfacebook-github-bot left review comments
@analysis-botanalysis-botanalysis-bot requested changes
@lunaleapslunaleapslunaleaps approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

AccessibilityCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Button Component does not announce Checked/Unchecked State

Selected State does not annonce when Button Component selected


7 participants

@huzaifaaak@facebook-github-bot@analysis-bot@lunaleaps@kacieb@amarlette@blavalla

, '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
Closed
huzaifaaak wants to merge 12 commits into
react:masterfrom 
huzaifaaak:accessibility/button
Closed
Added talkback support for button accessibility: disabled prop#31001
huzaifaaak wants to merge 12 commits into
react:masterfrom 
huzaifaaak:accessibility/button
Conversation
@huzaifaaak
@huzaifaaakhuzaifaaak
commented
Feb 16, 2021

Copy link


Copy Markdown

Summary
Issue # #30934 .When using a screen reader disabled buttons do not announce that they are disabled.
Changelog
[Android] [Changed] - Passing accessibility state in button so it can announce disabled in talkback
Test Plan
I have added Button in Button Example with accessibiltyState prop that will announce button is disabled when testing with talkback.
Ios test
I am unable to run ios project on my machine. RNTesterPods.xcworkspace gives workspace integrity error :/
@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Hi @huzaifaaak!
Thank you for your pull request and welcome to our community.
Action Required
In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.
Process
In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.
Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.
If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!
@analysis-bot
analysis-bot
commented
Feb 16, 2021•

edited

Loading

Copy link


Copy Markdown

PlatformEngineArchSize (bytes)Diff
ios-universaln/a--
Base commit: 4170726
@analysis-bot
analysis-bot
commented
Feb 16, 2021•

edited

Loading

Copy link


Copy Markdown

PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,908,772+366
androidhermesarmeabi-v7a8,407,852+375
androidhermesx869,399,329+376
androidhermesx86_649,343,247+368
androidjscarm64-v8a10,642,464+144
androidjscarmeabi-v7a10,124,638+159
androidjscx8610,694,515+149
androidjscx86_6411,278,700+142
Base commit: 4170726
@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@facebook-github-botfacebook-github-bot
added
the 
CLA Signed
This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
label
Feb 16, 2021
@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
analysis-bot
analysis-bot
suggested changes
Feb 16, 2021
View reviewed changes
@analysis-botanalysis-bot
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Code analysis results:
  • eslint found some issues. Run yarn lint --fix to automatically fix problems.
Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

4 similar comments

@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@facebook-github-bot
facebook-github-bot
commented
Feb 17, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@facebook-github-bot
facebook-github-bot
commented
Feb 17, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@facebook-github-bot
facebook-github-bot
commented
Feb 17, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@huzaifaaak
huzaifaaak
commented
Feb 17, 2021•

edited

Loading

Copy link


Copy Markdown

Author
@kacieb Can you please review this and give your comments 🙏
lunaleaps
lunaleaps
reviewed
Feb 17, 2021
View reviewed changes
Comment threadpackages/rn-tester/js/examples/Button/ButtonExample.js
Outdated

Comment threadLibraries/Components/Button.js
Outdated

 }
 const accessibilityState = {};
 const accessibilityState =
 disabled != null
@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.

I'd suggest const accessibilityState = disabled === true ? ... : ... so it's accessiblityState.disabled | disabled.
@lunaleapslunaleapsMar 10, 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.

Sorry to follow up on this can we avoid creating a new object here?
const accessibilityState = disabled !== accessibilityState.disabled ? {...this.props.accessibilityState, disabled} : this.props.accessibilityState

Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

@lunaleaps
lunaleaps
commented
Feb 17, 2021

Copy link


Copy Markdown

Contributor
Thanks for adding an example as well!
analysis-bot
analysis-bot
suggested changes
Feb 17, 2021
View reviewed changes
@analysis-botanalysis-bot
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Code analysis results:
  • eslint found some issues. Run yarn lint --fix to automatically fix problems.
Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

 onDismiss={() => {
 if (this.state.action === 'onDismiss') alert(this.state.action);
 if (this.state.action === 'onDismiss') {
 alert(this.state.action);
@analysis-botanalysis-botFeb 17, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

no-alert: Unexpected alert.
Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

 onShow={() => {
 if (this.state.action === 'onShow') alert(this.state.action);
 if (this.state.action === 'onShow') {
 alert(this.state.action);
@analysis-botanalysis-botFeb 17, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

no-alert: Unexpected alert.
lunaleaps
lunaleaps
approved these changes
Feb 17, 2021
View reviewed changes
@lunaleapslunaleaps
left a comment


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.

Looks good to me!
Comment threadLibraries/Components/Button.js
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
facebook-github-bot
facebook-github-bot
reviewed
Feb 17, 2021
View reviewed changes
@facebook-github-botfacebook-github-bot
left a comment


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 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
lunaleaps
lunaleaps
reviewed
Feb 18, 2021
View reviewed changes
Comment threadLibraries/Components/Button.js
 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
lunaleaps
lunaleaps
reviewed
Mar 8, 2021
View reviewed changes
Comment threadLibraries/Components/Button.js
Outdated

 nextFocusUp={nextFocusUp}
 testID={testID}
 disabled={disabled}
 disabled={disabled != null ? disabled : accessibilityState?.disabled} //This will sync accessibilityState={disabled} with disabled prop.
@lunaleapslunaleapsMar 8, 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 think this should be done on line 286? Otherwise we're missing the logic in that if statement if accessibilityState.disabled is true
@huzaifaaakhuzaifaaakMar 8, 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.

I was a bit confused on this. For now it's not appyling the disable style if only accessibilityState?.disabled is passed.
@huzaifaaakhuzaifaaakMar 8, 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.

I think this needs to be passed here and on line 286 so it applies style and disables the button aswell if accessibilityState.disabled is true
@lunaleapslunaleapsMar 8, 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.

Yea I think we should treat the button as disabled if accessibilityState.disabled is true in all manners --- behaviour and styling
@kaciebkaciebMar 8, 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.

Yea I think we should treat the button as disabled if accessibilityState.disabled is true in all manners --- behaviour and styling
I think I agree with you -I think if accessibilityState.disabled=true and disabled=false though, disabled should take precedence. If disabled is undefined, then I agree accessibilityState.disabled=true should disable the component.
@huzaifaaakhuzaifaaakMar 9, 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.

@lunaleaps@kacieb thank you for the explanation. I have updated the accessibilityState.disabled=true to update styling aswell.
Edit: The disabled prop has precedence and if it is not provided accessibilityState.disabled will take over.
lunaleaps
lunaleaps
approved these changes
Mar 10, 2021
View reviewed changes
@lunaleapslunaleaps
left a comment


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.

Otherwise looks good to me
Comment threadLibraries/Components/Button.js
Outdated

 }
 const accessibilityState = {};
 const accessibilityState =
 disabled != null
@lunaleapslunaleapsMar 10, 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.

Sorry to follow up on this can we avoid creating a new object here?
const accessibilityState = disabled !== accessibilityState.disabled ? {...this.props.accessibilityState, disabled} : this.props.accessibilityState

@huzaifaaak
huzaifaaak
commented
Mar 10, 2021

Copy link


Copy Markdown

Author
@lunaleaps I have updated it to avoid object creation. Do let me know if it can be done in a better way
facebook-github-bot
facebook-github-bot
reviewed
Mar 10, 2021
View reviewed changes
@facebook-github-botfacebook-github-bot
left a comment


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 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@facebook-github-botfacebook-github-bot
added
the 
Merged
This PR has been merged.
label
Mar 11, 2021
@facebook-github-bot
facebook-github-bot
commented
Mar 11, 2021

Copy link


Copy Markdown

Contributor
@lunaleaps merged this pull request in 5889cbe.
@kacieb
kacieb
commented
Mar 12, 2021

Copy link


Copy Markdown

Contributor
This is awesome, thank you so much for working on this and improving React Native's accessibility! I'd love to see a snapshot test for this component at some point, similar to this Pressable one added here, that ensures the accessibilityState values are correctly set as expected.
@amarletteamarlette
mentioned this pull request
Mar 15, 2021

Closed


This was referenced Mar 15, 2021

Closed


Closed

@kaciebkacieb
mentioned this pull request
Mar 16, 2021
 Closed

@amarlette
amarlette
commented
Mar 17, 2021

Copy link


Copy Markdown

Hey @huzaifaaak I would like to give you a shout-out on Twitter and include you in our end-of-month issues update for your contribution. Do you have a Twitter we can tag and is your @ the same as your Github?
@huzaifaaak
huzaifaaak
commented
Mar 17, 2021

Copy link


Copy Markdown

Author
Hey @amarlette yes my twitter handle is the same huzaifaaak
@huzaifaaakhuzaifaaak
mentioned this pull request
Mar 18, 2021
 Closed

@blavallablavalla
mentioned this pull request
Mar 25, 2021

Closed

@amarletteamarlette
linked an issue
Mar 29, 2021
that may be
closed
by this pull request


Selected State does not annonce when Button Component selected
#30954

Closed

@amarletteamarlette
mentioned this pull request
Mar 29, 2021
 Closed

facebook-github-bot pushed a commit
that referenced
this pull request
Apr 2, 2021
Huzaifa@facebook-github-bot
Accessibility/button test (#31189)
b5e649f
Summary:
This PR aims to add test's for button.
Snapshot test for PR #31001 . This would make sure `accessibilityState` is properly set.
## Changelog
<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->
[General] [Added] - Test's for button
Pull Request resolved: #31189
Test Plan:
`npm test` to run the test's.
Since the disabled prop of button has precedence over `accessibilityState.disabled` the test's will make sure it remains this way.
Reviewed By: kacieb
Differential Revision: D27473082
Pulled By: lunaleaps
fbshipit-source-id: 65d82620e8c245c2a8e29c3e9a8252d3a4275b09
@anaskhrazaanaskhraza
mentioned this pull request
May 16, 2021

Closed

@martinivanalfonsomartinivanalfonso
mentioned this pull request
May 16, 2021

Closed

@amarletteamarlette
linked an issue
May 24, 2021
that may be
closed
by this pull request


Button Component does not announce Checked/Unchecked State
#30959

Closed


This was referenced Jan 28, 2022

Closed

 Merged

@coderabbitaicoderabbitaiBot
mentioned this pull request
Aug 16, 2026
 Merged

4 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@nadiianadiiaAwaiting requested review from nadiia
@kaciebkaciebAwaiting requested review from kacieb

4 more reviewers

@blavallablavallablavalla left review comments
@facebook-github-botfacebook-github-botfacebook-github-bot left review comments
@analysis-botanalysis-botanalysis-bot requested changes
@lunaleapslunaleapslunaleaps approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

AccessibilityCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Button Component does not announce Checked/Unchecked State

Selected State does not annonce when Button Component selected


7 participants

@huzaifaaak@facebook-github-bot@analysis-bot@lunaleaps@kacieb@amarlette@blavalla

, '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
Closed
huzaifaaak wants to merge 12 commits into
react:masterfrom 
huzaifaaak:accessibility/button
Closed
Added talkback support for button accessibility: disabled prop#31001
huzaifaaak wants to merge 12 commits into
react:masterfrom 
huzaifaaak:accessibility/button
Conversation
@huzaifaaak
@huzaifaaakhuzaifaaak
commented
Feb 16, 2021

Copy link


Copy Markdown

Summary
Issue # #30934 .When using a screen reader disabled buttons do not announce that they are disabled.
Changelog
[Android] [Changed] - Passing accessibility state in button so it can announce disabled in talkback
Test Plan
I have added Button in Button Example with accessibiltyState prop that will announce button is disabled when testing with talkback.
Ios test
I am unable to run ios project on my machine. RNTesterPods.xcworkspace gives workspace integrity error :/
@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Hi @huzaifaaak!
Thank you for your pull request and welcome to our community.
Action Required
In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.
Process
In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.
Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.
If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!
@analysis-bot
analysis-bot
commented
Feb 16, 2021•

edited

Loading

Copy link


Copy Markdown

PlatformEngineArchSize (bytes)Diff
ios-universaln/a--
Base commit: 4170726
@analysis-bot
analysis-bot
commented
Feb 16, 2021•

edited

Loading

Copy link


Copy Markdown

PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,908,772+366
androidhermesarmeabi-v7a8,407,852+375
androidhermesx869,399,329+376
androidhermesx86_649,343,247+368
androidjscarm64-v8a10,642,464+144
androidjscarmeabi-v7a10,124,638+159
androidjscx8610,694,515+149
androidjscx86_6411,278,700+142
Base commit: 4170726
@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@facebook-github-botfacebook-github-bot
added
the 
CLA Signed
This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
label
Feb 16, 2021
@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
analysis-bot
analysis-bot
suggested changes
Feb 16, 2021
View reviewed changes
@analysis-botanalysis-bot
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Code analysis results:
  • eslint found some issues. Run yarn lint --fix to automatically fix problems.
Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

4 similar comments

@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@facebook-github-bot
facebook-github-bot
commented
Feb 17, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@facebook-github-bot
facebook-github-bot
commented
Feb 17, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@facebook-github-bot
facebook-github-bot
commented
Feb 17, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@huzaifaaak
huzaifaaak
commented
Feb 17, 2021•

edited

Loading

Copy link


Copy Markdown

Author
@kacieb Can you please review this and give your comments 🙏
lunaleaps
lunaleaps
reviewed
Feb 17, 2021
View reviewed changes
Comment threadpackages/rn-tester/js/examples/Button/ButtonExample.js
Outdated

Comment threadLibraries/Components/Button.js
Outdated

 }
 const accessibilityState = {};
 const accessibilityState =
 disabled != null
@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.

I'd suggest const accessibilityState = disabled === true ? ... : ... so it's accessiblityState.disabled | disabled.
@lunaleapslunaleapsMar 10, 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.

Sorry to follow up on this can we avoid creating a new object here?
const accessibilityState = disabled !== accessibilityState.disabled ? {...this.props.accessibilityState, disabled} : this.props.accessibilityState

Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

@lunaleaps
lunaleaps
commented
Feb 17, 2021

Copy link


Copy Markdown

Contributor
Thanks for adding an example as well!
analysis-bot
analysis-bot
suggested changes
Feb 17, 2021
View reviewed changes
@analysis-botanalysis-bot
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Code analysis results:
  • eslint found some issues. Run yarn lint --fix to automatically fix problems.
Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

 onDismiss={() => {
 if (this.state.action === 'onDismiss') alert(this.state.action);
 if (this.state.action === 'onDismiss') {
 alert(this.state.action);
@analysis-botanalysis-botFeb 17, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

no-alert: Unexpected alert.
Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

 onShow={() => {
 if (this.state.action === 'onShow') alert(this.state.action);
 if (this.state.action === 'onShow') {
 alert(this.state.action);
@analysis-botanalysis-botFeb 17, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

no-alert: Unexpected alert.
lunaleaps
lunaleaps
approved these changes
Feb 17, 2021
View reviewed changes
@lunaleapslunaleaps
left a comment


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.

Looks good to me!
Comment threadLibraries/Components/Button.js
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
facebook-github-bot
facebook-github-bot
reviewed
Feb 17, 2021
View reviewed changes
@facebook-github-botfacebook-github-bot
left a comment


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 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
lunaleaps
lunaleaps
reviewed
Feb 18, 2021
View reviewed changes
Comment threadLibraries/Components/Button.js
 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
lunaleaps
lunaleaps
reviewed
Mar 8, 2021
View reviewed changes
Comment threadLibraries/Components/Button.js
Outdated

 nextFocusUp={nextFocusUp}
 testID={testID}
 disabled={disabled}
 disabled={disabled != null ? disabled : accessibilityState?.disabled} //This will sync accessibilityState={disabled} with disabled prop.
@lunaleapslunaleapsMar 8, 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 think this should be done on line 286? Otherwise we're missing the logic in that if statement if accessibilityState.disabled is true
@huzaifaaakhuzaifaaakMar 8, 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.

I was a bit confused on this. For now it's not appyling the disable style if only accessibilityState?.disabled is passed.
@huzaifaaakhuzaifaaakMar 8, 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.

I think this needs to be passed here and on line 286 so it applies style and disables the button aswell if accessibilityState.disabled is true
@lunaleapslunaleapsMar 8, 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.

Yea I think we should treat the button as disabled if accessibilityState.disabled is true in all manners --- behaviour and styling
@kaciebkaciebMar 8, 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.

Yea I think we should treat the button as disabled if accessibilityState.disabled is true in all manners --- behaviour and styling
I think I agree with you -I think if accessibilityState.disabled=true and disabled=false though, disabled should take precedence. If disabled is undefined, then I agree accessibilityState.disabled=true should disable the component.
@huzaifaaakhuzaifaaakMar 9, 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.

@lunaleaps@kacieb thank you for the explanation. I have updated the accessibilityState.disabled=true to update styling aswell.
Edit: The disabled prop has precedence and if it is not provided accessibilityState.disabled will take over.
lunaleaps
lunaleaps
approved these changes
Mar 10, 2021
View reviewed changes
@lunaleapslunaleaps
left a comment


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.

Otherwise looks good to me
Comment threadLibraries/Components/Button.js
Outdated

 }
 const accessibilityState = {};
 const accessibilityState =
 disabled != null
@lunaleapslunaleapsMar 10, 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.

Sorry to follow up on this can we avoid creating a new object here?
const accessibilityState = disabled !== accessibilityState.disabled ? {...this.props.accessibilityState, disabled} : this.props.accessibilityState

@huzaifaaak
huzaifaaak
commented
Mar 10, 2021

Copy link


Copy Markdown

Author
@lunaleaps I have updated it to avoid object creation. Do let me know if it can be done in a better way
facebook-github-bot
facebook-github-bot
reviewed
Mar 10, 2021
View reviewed changes
@facebook-github-botfacebook-github-bot
left a comment


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 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@facebook-github-botfacebook-github-bot
added
the 
Merged
This PR has been merged.
label
Mar 11, 2021
@facebook-github-bot
facebook-github-bot
commented
Mar 11, 2021

Copy link


Copy Markdown

Contributor
@lunaleaps merged this pull request in 5889cbe.
@kacieb
kacieb
commented
Mar 12, 2021

Copy link


Copy Markdown

Contributor
This is awesome, thank you so much for working on this and improving React Native's accessibility! I'd love to see a snapshot test for this component at some point, similar to this Pressable one added here, that ensures the accessibilityState values are correctly set as expected.
@amarletteamarlette
mentioned this pull request
Mar 15, 2021

Closed


This was referenced Mar 15, 2021

Closed


Closed

@kaciebkacieb
mentioned this pull request
Mar 16, 2021
 Closed

@amarlette
amarlette
commented
Mar 17, 2021

Copy link


Copy Markdown

Hey @huzaifaaak I would like to give you a shout-out on Twitter and include you in our end-of-month issues update for your contribution. Do you have a Twitter we can tag and is your @ the same as your Github?
@huzaifaaak
huzaifaaak
commented
Mar 17, 2021

Copy link


Copy Markdown

Author
Hey @amarlette yes my twitter handle is the same huzaifaaak
@huzaifaaakhuzaifaaak
mentioned this pull request
Mar 18, 2021
 Closed

@blavallablavalla
mentioned this pull request
Mar 25, 2021

Closed

@amarletteamarlette
linked an issue
Mar 29, 2021
that may be
closed
by this pull request


Selected State does not annonce when Button Component selected
#30954

Closed

@amarletteamarlette
mentioned this pull request
Mar 29, 2021
 Closed

facebook-github-bot pushed a commit
that referenced
this pull request
Apr 2, 2021
Huzaifa@facebook-github-bot
Accessibility/button test (#31189)
b5e649f
Summary:
This PR aims to add test's for button.
Snapshot test for PR #31001 . This would make sure `accessibilityState` is properly set.
## Changelog
<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->
[General] [Added] - Test's for button
Pull Request resolved: #31189
Test Plan:
`npm test` to run the test's.
Since the disabled prop of button has precedence over `accessibilityState.disabled` the test's will make sure it remains this way.
Reviewed By: kacieb
Differential Revision: D27473082
Pulled By: lunaleaps
fbshipit-source-id: 65d82620e8c245c2a8e29c3e9a8252d3a4275b09
@anaskhrazaanaskhraza
mentioned this pull request
May 16, 2021

Closed

@martinivanalfonsomartinivanalfonso
mentioned this pull request
May 16, 2021

Closed

@amarletteamarlette
linked an issue
May 24, 2021
that may be
closed
by this pull request


Button Component does not announce Checked/Unchecked State
#30959

Closed


This was referenced Jan 28, 2022

Closed

 Merged

@coderabbitaicoderabbitaiBot
mentioned this pull request
Aug 16, 2026
 Merged

4 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@nadiianadiiaAwaiting requested review from nadiia
@kaciebkaciebAwaiting requested review from kacieb

4 more reviewers

@blavallablavallablavalla left review comments
@facebook-github-botfacebook-github-botfacebook-github-bot left review comments
@analysis-botanalysis-botanalysis-bot requested changes
@lunaleapslunaleapslunaleaps approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

AccessibilityCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Button Component does not announce Checked/Unchecked State

Selected State does not annonce when Button Component selected


7 participants

@huzaifaaak@facebook-github-bot@analysis-bot@lunaleaps@kacieb@amarlette@blavalla

, '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
Closed
huzaifaaak wants to merge 12 commits into
react:masterfrom 
huzaifaaak:accessibility/button
Closed
Added talkback support for button accessibility: disabled prop#31001
huzaifaaak wants to merge 12 commits into
react:masterfrom 
huzaifaaak:accessibility/button
Conversation
@huzaifaaak
@huzaifaaakhuzaifaaak
commented
Feb 16, 2021

Copy link


Copy Markdown

Summary
Issue # #30934 .When using a screen reader disabled buttons do not announce that they are disabled.
Changelog
[Android] [Changed] - Passing accessibility state in button so it can announce disabled in talkback
Test Plan
I have added Button in Button Example with accessibiltyState prop that will announce button is disabled when testing with talkback.
Ios test
I am unable to run ios project on my machine. RNTesterPods.xcworkspace gives workspace integrity error :/
@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Hi @huzaifaaak!
Thank you for your pull request and welcome to our community.
Action Required
In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.
Process
In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.
Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.
If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!
@analysis-bot
analysis-bot
commented
Feb 16, 2021•

edited

Loading

Copy link


Copy Markdown

PlatformEngineArchSize (bytes)Diff
ios-universaln/a--
Base commit: 4170726
@analysis-bot
analysis-bot
commented
Feb 16, 2021•

edited

Loading

Copy link


Copy Markdown

PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,908,772+366
androidhermesarmeabi-v7a8,407,852+375
androidhermesx869,399,329+376
androidhermesx86_649,343,247+368
androidjscarm64-v8a10,642,464+144
androidjscarmeabi-v7a10,124,638+159
androidjscx8610,694,515+149
androidjscx86_6411,278,700+142
Base commit: 4170726
@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@facebook-github-botfacebook-github-bot
added
the 
CLA Signed
This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
label
Feb 16, 2021
@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
analysis-bot
analysis-bot
suggested changes
Feb 16, 2021
View reviewed changes
@analysis-botanalysis-bot
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Code analysis results:
  • eslint found some issues. Run yarn lint --fix to automatically fix problems.
Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

4 similar comments

@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@facebook-github-bot
facebook-github-bot
commented
Feb 17, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@facebook-github-bot
facebook-github-bot
commented
Feb 17, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@facebook-github-bot
facebook-github-bot
commented
Feb 17, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@huzaifaaak
huzaifaaak
commented
Feb 17, 2021•

edited

Loading

Copy link


Copy Markdown

Author
@kacieb Can you please review this and give your comments 🙏
lunaleaps
lunaleaps
reviewed
Feb 17, 2021
View reviewed changes
Comment threadpackages/rn-tester/js/examples/Button/ButtonExample.js
Outdated

Comment threadLibraries/Components/Button.js
Outdated

 }
 const accessibilityState = {};
 const accessibilityState =
 disabled != null
@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.

I'd suggest const accessibilityState = disabled === true ? ... : ... so it's accessiblityState.disabled | disabled.
@lunaleapslunaleapsMar 10, 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.

Sorry to follow up on this can we avoid creating a new object here?
const accessibilityState = disabled !== accessibilityState.disabled ? {...this.props.accessibilityState, disabled} : this.props.accessibilityState

Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

@lunaleaps
lunaleaps
commented
Feb 17, 2021

Copy link


Copy Markdown

Contributor
Thanks for adding an example as well!
analysis-bot
analysis-bot
suggested changes
Feb 17, 2021
View reviewed changes
@analysis-botanalysis-bot
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Code analysis results:
  • eslint found some issues. Run yarn lint --fix to automatically fix problems.
Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

 onDismiss={() => {
 if (this.state.action === 'onDismiss') alert(this.state.action);
 if (this.state.action === 'onDismiss') {
 alert(this.state.action);
@analysis-botanalysis-botFeb 17, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

no-alert: Unexpected alert.
Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

 onShow={() => {
 if (this.state.action === 'onShow') alert(this.state.action);
 if (this.state.action === 'onShow') {
 alert(this.state.action);
@analysis-botanalysis-botFeb 17, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

no-alert: Unexpected alert.
lunaleaps
lunaleaps
approved these changes
Feb 17, 2021
View reviewed changes
@lunaleapslunaleaps
left a comment


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.

Looks good to me!
Comment threadLibraries/Components/Button.js
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
facebook-github-bot
facebook-github-bot
reviewed
Feb 17, 2021
View reviewed changes
@facebook-github-botfacebook-github-bot
left a comment


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 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
lunaleaps
lunaleaps
reviewed
Feb 18, 2021
View reviewed changes
Comment threadLibraries/Components/Button.js
 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
lunaleaps
lunaleaps
reviewed
Mar 8, 2021
View reviewed changes
Comment threadLibraries/Components/Button.js
Outdated

 nextFocusUp={nextFocusUp}
 testID={testID}
 disabled={disabled}
 disabled={disabled != null ? disabled : accessibilityState?.disabled} //This will sync accessibilityState={disabled} with disabled prop.
@lunaleapslunaleapsMar 8, 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 think this should be done on line 286? Otherwise we're missing the logic in that if statement if accessibilityState.disabled is true
@huzaifaaakhuzaifaaakMar 8, 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.

I was a bit confused on this. For now it's not appyling the disable style if only accessibilityState?.disabled is passed.
@huzaifaaakhuzaifaaakMar 8, 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.

I think this needs to be passed here and on line 286 so it applies style and disables the button aswell if accessibilityState.disabled is true
@lunaleapslunaleapsMar 8, 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.

Yea I think we should treat the button as disabled if accessibilityState.disabled is true in all manners --- behaviour and styling
@kaciebkaciebMar 8, 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.

Yea I think we should treat the button as disabled if accessibilityState.disabled is true in all manners --- behaviour and styling
I think I agree with you -I think if accessibilityState.disabled=true and disabled=false though, disabled should take precedence. If disabled is undefined, then I agree accessibilityState.disabled=true should disable the component.
@huzaifaaakhuzaifaaakMar 9, 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.

@lunaleaps@kacieb thank you for the explanation. I have updated the accessibilityState.disabled=true to update styling aswell.
Edit: The disabled prop has precedence and if it is not provided accessibilityState.disabled will take over.
lunaleaps
lunaleaps
approved these changes
Mar 10, 2021
View reviewed changes
@lunaleapslunaleaps
left a comment


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.

Otherwise looks good to me
Comment threadLibraries/Components/Button.js
Outdated

 }
 const accessibilityState = {};
 const accessibilityState =
 disabled != null
@lunaleapslunaleapsMar 10, 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.

Sorry to follow up on this can we avoid creating a new object here?
const accessibilityState = disabled !== accessibilityState.disabled ? {...this.props.accessibilityState, disabled} : this.props.accessibilityState

@huzaifaaak
huzaifaaak
commented
Mar 10, 2021

Copy link


Copy Markdown

Author
@lunaleaps I have updated it to avoid object creation. Do let me know if it can be done in a better way
facebook-github-bot
facebook-github-bot
reviewed
Mar 10, 2021
View reviewed changes
@facebook-github-botfacebook-github-bot
left a comment


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 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@facebook-github-botfacebook-github-bot
added
the 
Merged
This PR has been merged.
label
Mar 11, 2021
@facebook-github-bot
facebook-github-bot
commented
Mar 11, 2021

Copy link


Copy Markdown

Contributor
@lunaleaps merged this pull request in 5889cbe.
@kacieb
kacieb
commented
Mar 12, 2021

Copy link


Copy Markdown

Contributor
This is awesome, thank you so much for working on this and improving React Native's accessibility! I'd love to see a snapshot test for this component at some point, similar to this Pressable one added here, that ensures the accessibilityState values are correctly set as expected.
@amarletteamarlette
mentioned this pull request
Mar 15, 2021

Closed


This was referenced Mar 15, 2021

Closed


Closed

@kaciebkacieb
mentioned this pull request
Mar 16, 2021
 Closed

@amarlette
amarlette
commented
Mar 17, 2021

Copy link


Copy Markdown

Hey @huzaifaaak I would like to give you a shout-out on Twitter and include you in our end-of-month issues update for your contribution. Do you have a Twitter we can tag and is your @ the same as your Github?
@huzaifaaak
huzaifaaak
commented
Mar 17, 2021

Copy link


Copy Markdown

Author
Hey @amarlette yes my twitter handle is the same huzaifaaak
@huzaifaaakhuzaifaaak
mentioned this pull request
Mar 18, 2021
 Closed

@blavallablavalla
mentioned this pull request
Mar 25, 2021

Closed

@amarletteamarlette
linked an issue
Mar 29, 2021
that may be
closed
by this pull request


Selected State does not annonce when Button Component selected
#30954

Closed

@amarletteamarlette
mentioned this pull request
Mar 29, 2021
 Closed

facebook-github-bot pushed a commit
that referenced
this pull request
Apr 2, 2021
Huzaifa@facebook-github-bot
Accessibility/button test (#31189)
b5e649f
Summary:
This PR aims to add test's for button.
Snapshot test for PR #31001 . This would make sure `accessibilityState` is properly set.
## Changelog
<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->
[General] [Added] - Test's for button
Pull Request resolved: #31189
Test Plan:
`npm test` to run the test's.
Since the disabled prop of button has precedence over `accessibilityState.disabled` the test's will make sure it remains this way.
Reviewed By: kacieb
Differential Revision: D27473082
Pulled By: lunaleaps
fbshipit-source-id: 65d82620e8c245c2a8e29c3e9a8252d3a4275b09
@anaskhrazaanaskhraza
mentioned this pull request
May 16, 2021

Closed

@martinivanalfonsomartinivanalfonso
mentioned this pull request
May 16, 2021

Closed

@amarletteamarlette
linked an issue
May 24, 2021
that may be
closed
by this pull request


Button Component does not announce Checked/Unchecked State
#30959

Closed


This was referenced Jan 28, 2022

Closed

 Merged

@coderabbitaicoderabbitaiBot
mentioned this pull request
Aug 16, 2026
 Merged

4 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@nadiianadiiaAwaiting requested review from nadiia
@kaciebkaciebAwaiting requested review from kacieb

4 more reviewers

@blavallablavallablavalla left review comments
@facebook-github-botfacebook-github-botfacebook-github-bot left review comments
@analysis-botanalysis-botanalysis-bot requested changes
@lunaleapslunaleapslunaleaps approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

AccessibilityCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Button Component does not announce Checked/Unchecked State

Selected State does not annonce when Button Component selected


7 participants

@huzaifaaak@facebook-github-bot@analysis-bot@lunaleaps@kacieb@amarlette@blavalla

, '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
Closed
huzaifaaak wants to merge 12 commits into
react:masterfrom 
huzaifaaak:accessibility/button
Closed
Added talkback support for button accessibility: disabled prop#31001
huzaifaaak wants to merge 12 commits into
react:masterfrom 
huzaifaaak:accessibility/button
Conversation
@huzaifaaak
@huzaifaaakhuzaifaaak
commented
Feb 16, 2021

Copy link


Copy Markdown

Summary
Issue # #30934 .When using a screen reader disabled buttons do not announce that they are disabled.
Changelog
[Android] [Changed] - Passing accessibility state in button so it can announce disabled in talkback
Test Plan
I have added Button in Button Example with accessibiltyState prop that will announce button is disabled when testing with talkback.
Ios test
I am unable to run ios project on my machine. RNTesterPods.xcworkspace gives workspace integrity error :/
@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Hi @huzaifaaak!
Thank you for your pull request and welcome to our community.
Action Required
In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.
Process
In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.
Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.
If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!
@analysis-bot
analysis-bot
commented
Feb 16, 2021•

edited

Loading

Copy link


Copy Markdown

PlatformEngineArchSize (bytes)Diff
ios-universaln/a--
Base commit: 4170726
@analysis-bot
analysis-bot
commented
Feb 16, 2021•

edited

Loading

Copy link


Copy Markdown

PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,908,772+366
androidhermesarmeabi-v7a8,407,852+375
androidhermesx869,399,329+376
androidhermesx86_649,343,247+368
androidjscarm64-v8a10,642,464+144
androidjscarmeabi-v7a10,124,638+159
androidjscx8610,694,515+149
androidjscx86_6411,278,700+142
Base commit: 4170726
@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@facebook-github-botfacebook-github-bot
added
the 
CLA Signed
This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
label
Feb 16, 2021
@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
analysis-bot
analysis-bot
suggested changes
Feb 16, 2021
View reviewed changes
@analysis-botanalysis-bot
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Code analysis results:
  • eslint found some issues. Run yarn lint --fix to automatically fix problems.
Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

4 similar comments

@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@facebook-github-bot
facebook-github-bot
commented
Feb 17, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@facebook-github-bot
facebook-github-bot
commented
Feb 17, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@facebook-github-bot
facebook-github-bot
commented
Feb 17, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@huzaifaaak
huzaifaaak
commented
Feb 17, 2021•

edited

Loading

Copy link


Copy Markdown

Author
@kacieb Can you please review this and give your comments 🙏
lunaleaps
lunaleaps
reviewed
Feb 17, 2021
View reviewed changes
Comment threadpackages/rn-tester/js/examples/Button/ButtonExample.js
Outdated

Comment threadLibraries/Components/Button.js
Outdated

 }
 const accessibilityState = {};
 const accessibilityState =
 disabled != null
@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.

I'd suggest const accessibilityState = disabled === true ? ... : ... so it's accessiblityState.disabled | disabled.
@lunaleapslunaleapsMar 10, 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.

Sorry to follow up on this can we avoid creating a new object here?
const accessibilityState = disabled !== accessibilityState.disabled ? {...this.props.accessibilityState, disabled} : this.props.accessibilityState

Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

@lunaleaps
lunaleaps
commented
Feb 17, 2021

Copy link


Copy Markdown

Contributor
Thanks for adding an example as well!
analysis-bot
analysis-bot
suggested changes
Feb 17, 2021
View reviewed changes
@analysis-botanalysis-bot
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Code analysis results:
  • eslint found some issues. Run yarn lint --fix to automatically fix problems.
Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

 onDismiss={() => {
 if (this.state.action === 'onDismiss') alert(this.state.action);
 if (this.state.action === 'onDismiss') {
 alert(this.state.action);
@analysis-botanalysis-botFeb 17, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

no-alert: Unexpected alert.
Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

 onShow={() => {
 if (this.state.action === 'onShow') alert(this.state.action);
 if (this.state.action === 'onShow') {
 alert(this.state.action);
@analysis-botanalysis-botFeb 17, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

no-alert: Unexpected alert.
lunaleaps
lunaleaps
approved these changes
Feb 17, 2021
View reviewed changes
@lunaleapslunaleaps
left a comment


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.

Looks good to me!
Comment threadLibraries/Components/Button.js
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
facebook-github-bot
facebook-github-bot
reviewed
Feb 17, 2021
View reviewed changes
@facebook-github-botfacebook-github-bot
left a comment


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 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
lunaleaps
lunaleaps
reviewed
Feb 18, 2021
View reviewed changes
Comment threadLibraries/Components/Button.js
 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
lunaleaps
lunaleaps
reviewed
Mar 8, 2021
View reviewed changes
Comment threadLibraries/Components/Button.js
Outdated

 nextFocusUp={nextFocusUp}
 testID={testID}
 disabled={disabled}
 disabled={disabled != null ? disabled : accessibilityState?.disabled} //This will sync accessibilityState={disabled} with disabled prop.
@lunaleapslunaleapsMar 8, 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 think this should be done on line 286? Otherwise we're missing the logic in that if statement if accessibilityState.disabled is true
@huzaifaaakhuzaifaaakMar 8, 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.

I was a bit confused on this. For now it's not appyling the disable style if only accessibilityState?.disabled is passed.
@huzaifaaakhuzaifaaakMar 8, 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.

I think this needs to be passed here and on line 286 so it applies style and disables the button aswell if accessibilityState.disabled is true
@lunaleapslunaleapsMar 8, 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.

Yea I think we should treat the button as disabled if accessibilityState.disabled is true in all manners --- behaviour and styling
@kaciebkaciebMar 8, 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.

Yea I think we should treat the button as disabled if accessibilityState.disabled is true in all manners --- behaviour and styling
I think I agree with you -I think if accessibilityState.disabled=true and disabled=false though, disabled should take precedence. If disabled is undefined, then I agree accessibilityState.disabled=true should disable the component.
@huzaifaaakhuzaifaaakMar 9, 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.

@lunaleaps@kacieb thank you for the explanation. I have updated the accessibilityState.disabled=true to update styling aswell.
Edit: The disabled prop has precedence and if it is not provided accessibilityState.disabled will take over.
lunaleaps
lunaleaps
approved these changes
Mar 10, 2021
View reviewed changes
@lunaleapslunaleaps
left a comment


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.

Otherwise looks good to me
Comment threadLibraries/Components/Button.js
Outdated

 }
 const accessibilityState = {};
 const accessibilityState =
 disabled != null
@lunaleapslunaleapsMar 10, 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.

Sorry to follow up on this can we avoid creating a new object here?
const accessibilityState = disabled !== accessibilityState.disabled ? {...this.props.accessibilityState, disabled} : this.props.accessibilityState

@huzaifaaak
huzaifaaak
commented
Mar 10, 2021

Copy link


Copy Markdown

Author
@lunaleaps I have updated it to avoid object creation. Do let me know if it can be done in a better way
facebook-github-bot
facebook-github-bot
reviewed
Mar 10, 2021
View reviewed changes
@facebook-github-botfacebook-github-bot
left a comment


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 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@facebook-github-botfacebook-github-bot
added
the 
Merged
This PR has been merged.
label
Mar 11, 2021
@facebook-github-bot
facebook-github-bot
commented
Mar 11, 2021

Copy link


Copy Markdown

Contributor
@lunaleaps merged this pull request in 5889cbe.
@kacieb
kacieb
commented
Mar 12, 2021

Copy link


Copy Markdown

Contributor
This is awesome, thank you so much for working on this and improving React Native's accessibility! I'd love to see a snapshot test for this component at some point, similar to this Pressable one added here, that ensures the accessibilityState values are correctly set as expected.
@amarletteamarlette
mentioned this pull request
Mar 15, 2021

Closed


This was referenced Mar 15, 2021

Closed


Closed

@kaciebkacieb
mentioned this pull request
Mar 16, 2021
 Closed

@amarlette
amarlette
commented
Mar 17, 2021

Copy link


Copy Markdown

Hey @huzaifaaak I would like to give you a shout-out on Twitter and include you in our end-of-month issues update for your contribution. Do you have a Twitter we can tag and is your @ the same as your Github?
@huzaifaaak
huzaifaaak
commented
Mar 17, 2021

Copy link


Copy Markdown

Author
Hey @amarlette yes my twitter handle is the same huzaifaaak
@huzaifaaakhuzaifaaak
mentioned this pull request
Mar 18, 2021
 Closed

@blavallablavalla
mentioned this pull request
Mar 25, 2021

Closed

@amarletteamarlette
linked an issue
Mar 29, 2021
that may be
closed
by this pull request


Selected State does not annonce when Button Component selected
#30954

Closed

@amarletteamarlette
mentioned this pull request
Mar 29, 2021
 Closed

facebook-github-bot pushed a commit
that referenced
this pull request
Apr 2, 2021
Huzaifa@facebook-github-bot
Accessibility/button test (#31189)
b5e649f
Summary:
This PR aims to add test's for button.
Snapshot test for PR #31001 . This would make sure `accessibilityState` is properly set.
## Changelog
<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->
[General] [Added] - Test's for button
Pull Request resolved: #31189
Test Plan:
`npm test` to run the test's.
Since the disabled prop of button has precedence over `accessibilityState.disabled` the test's will make sure it remains this way.
Reviewed By: kacieb
Differential Revision: D27473082
Pulled By: lunaleaps
fbshipit-source-id: 65d82620e8c245c2a8e29c3e9a8252d3a4275b09
@anaskhrazaanaskhraza
mentioned this pull request
May 16, 2021

Closed

@martinivanalfonsomartinivanalfonso
mentioned this pull request
May 16, 2021

Closed

@amarletteamarlette
linked an issue
May 24, 2021
that may be
closed
by this pull request


Button Component does not announce Checked/Unchecked State
#30959

Closed


This was referenced Jan 28, 2022

Closed

 Merged

@coderabbitaicoderabbitaiBot
mentioned this pull request
Aug 16, 2026
 Merged

4 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@nadiianadiiaAwaiting requested review from nadiia
@kaciebkaciebAwaiting requested review from kacieb

4 more reviewers

@blavallablavallablavalla left review comments
@facebook-github-botfacebook-github-botfacebook-github-bot left review comments
@analysis-botanalysis-botanalysis-bot requested changes
@lunaleapslunaleapslunaleaps approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

AccessibilityCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Button Component does not announce Checked/Unchecked State

Selected State does not annonce when Button Component selected


7 participants

@huzaifaaak@facebook-github-bot@analysis-bot@lunaleaps@kacieb@amarlette@blavalla

, '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
Closed
huzaifaaak wants to merge 12 commits into
react:masterfrom 
huzaifaaak:accessibility/button
Closed
Added talkback support for button accessibility: disabled prop#31001
huzaifaaak wants to merge 12 commits into
react:masterfrom 
huzaifaaak:accessibility/button
Conversation
@huzaifaaak
@huzaifaaakhuzaifaaak
commented
Feb 16, 2021

Copy link


Copy Markdown

Summary
Issue # #30934 .When using a screen reader disabled buttons do not announce that they are disabled.
Changelog
[Android] [Changed] - Passing accessibility state in button so it can announce disabled in talkback
Test Plan
I have added Button in Button Example with accessibiltyState prop that will announce button is disabled when testing with talkback.
Ios test
I am unable to run ios project on my machine. RNTesterPods.xcworkspace gives workspace integrity error :/
@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Hi @huzaifaaak!
Thank you for your pull request and welcome to our community.
Action Required
In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.
Process
In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.
Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.
If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!
@analysis-bot
analysis-bot
commented
Feb 16, 2021•

edited

Loading

Copy link


Copy Markdown

PlatformEngineArchSize (bytes)Diff
ios-universaln/a--
Base commit: 4170726
@analysis-bot
analysis-bot
commented
Feb 16, 2021•

edited

Loading

Copy link


Copy Markdown

PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,908,772+366
androidhermesarmeabi-v7a8,407,852+375
androidhermesx869,399,329+376
androidhermesx86_649,343,247+368
androidjscarm64-v8a10,642,464+144
androidjscarmeabi-v7a10,124,638+159
androidjscx8610,694,515+149
androidjscx86_6411,278,700+142
Base commit: 4170726
@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@facebook-github-botfacebook-github-bot
added
the 
CLA Signed
This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
label
Feb 16, 2021
@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
analysis-bot
analysis-bot
suggested changes
Feb 16, 2021
View reviewed changes
@analysis-botanalysis-bot
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Code analysis results:
  • eslint found some issues. Run yarn lint --fix to automatically fix problems.
Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

4 similar comments

@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@facebook-github-bot
facebook-github-bot
commented
Feb 17, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@facebook-github-bot
facebook-github-bot
commented
Feb 17, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@facebook-github-bot
facebook-github-bot
commented
Feb 17, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@huzaifaaak
huzaifaaak
commented
Feb 17, 2021•

edited

Loading

Copy link


Copy Markdown

Author
@kacieb Can you please review this and give your comments 🙏
lunaleaps
lunaleaps
reviewed
Feb 17, 2021
View reviewed changes
Comment threadpackages/rn-tester/js/examples/Button/ButtonExample.js
Outdated

Comment threadLibraries/Components/Button.js
Outdated

 }
 const accessibilityState = {};
 const accessibilityState =
 disabled != null
@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.

I'd suggest const accessibilityState = disabled === true ? ... : ... so it's accessiblityState.disabled | disabled.
@lunaleapslunaleapsMar 10, 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.

Sorry to follow up on this can we avoid creating a new object here?
const accessibilityState = disabled !== accessibilityState.disabled ? {...this.props.accessibilityState, disabled} : this.props.accessibilityState

Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

@lunaleaps
lunaleaps
commented
Feb 17, 2021

Copy link


Copy Markdown

Contributor
Thanks for adding an example as well!
analysis-bot
analysis-bot
suggested changes
Feb 17, 2021
View reviewed changes
@analysis-botanalysis-bot
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Code analysis results:
  • eslint found some issues. Run yarn lint --fix to automatically fix problems.
Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

 onDismiss={() => {
 if (this.state.action === 'onDismiss') alert(this.state.action);
 if (this.state.action === 'onDismiss') {
 alert(this.state.action);
@analysis-botanalysis-botFeb 17, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

no-alert: Unexpected alert.
Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

 onShow={() => {
 if (this.state.action === 'onShow') alert(this.state.action);
 if (this.state.action === 'onShow') {
 alert(this.state.action);
@analysis-botanalysis-botFeb 17, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

no-alert: Unexpected alert.
lunaleaps
lunaleaps
approved these changes
Feb 17, 2021
View reviewed changes
@lunaleapslunaleaps
left a comment


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.

Looks good to me!
Comment threadLibraries/Components/Button.js
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
facebook-github-bot
facebook-github-bot
reviewed
Feb 17, 2021
View reviewed changes
@facebook-github-botfacebook-github-bot
left a comment


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 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
lunaleaps
lunaleaps
reviewed
Feb 18, 2021
View reviewed changes
Comment threadLibraries/Components/Button.js
 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
lunaleaps
lunaleaps
reviewed
Mar 8, 2021
View reviewed changes
Comment threadLibraries/Components/Button.js
Outdated

 nextFocusUp={nextFocusUp}
 testID={testID}
 disabled={disabled}
 disabled={disabled != null ? disabled : accessibilityState?.disabled} //This will sync accessibilityState={disabled} with disabled prop.
@lunaleapslunaleapsMar 8, 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 think this should be done on line 286? Otherwise we're missing the logic in that if statement if accessibilityState.disabled is true
@huzaifaaakhuzaifaaakMar 8, 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.

I was a bit confused on this. For now it's not appyling the disable style if only accessibilityState?.disabled is passed.
@huzaifaaakhuzaifaaakMar 8, 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.

I think this needs to be passed here and on line 286 so it applies style and disables the button aswell if accessibilityState.disabled is true
@lunaleapslunaleapsMar 8, 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.

Yea I think we should treat the button as disabled if accessibilityState.disabled is true in all manners --- behaviour and styling
@kaciebkaciebMar 8, 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.

Yea I think we should treat the button as disabled if accessibilityState.disabled is true in all manners --- behaviour and styling
I think I agree with you -I think if accessibilityState.disabled=true and disabled=false though, disabled should take precedence. If disabled is undefined, then I agree accessibilityState.disabled=true should disable the component.
@huzaifaaakhuzaifaaakMar 9, 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.

@lunaleaps@kacieb thank you for the explanation. I have updated the accessibilityState.disabled=true to update styling aswell.
Edit: The disabled prop has precedence and if it is not provided accessibilityState.disabled will take over.
lunaleaps
lunaleaps
approved these changes
Mar 10, 2021
View reviewed changes
@lunaleapslunaleaps
left a comment


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.

Otherwise looks good to me
Comment threadLibraries/Components/Button.js
Outdated

 }
 const accessibilityState = {};
 const accessibilityState =
 disabled != null
@lunaleapslunaleapsMar 10, 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.

Sorry to follow up on this can we avoid creating a new object here?
const accessibilityState = disabled !== accessibilityState.disabled ? {...this.props.accessibilityState, disabled} : this.props.accessibilityState

@huzaifaaak
huzaifaaak
commented
Mar 10, 2021

Copy link


Copy Markdown

Author
@lunaleaps I have updated it to avoid object creation. Do let me know if it can be done in a better way
facebook-github-bot
facebook-github-bot
reviewed
Mar 10, 2021
View reviewed changes
@facebook-github-botfacebook-github-bot
left a comment


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 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@facebook-github-botfacebook-github-bot
added
the 
Merged
This PR has been merged.
label
Mar 11, 2021
@facebook-github-bot
facebook-github-bot
commented
Mar 11, 2021

Copy link


Copy Markdown

Contributor
@lunaleaps merged this pull request in 5889cbe.
@kacieb
kacieb
commented
Mar 12, 2021

Copy link


Copy Markdown

Contributor
This is awesome, thank you so much for working on this and improving React Native's accessibility! I'd love to see a snapshot test for this component at some point, similar to this Pressable one added here, that ensures the accessibilityState values are correctly set as expected.
@amarletteamarlette
mentioned this pull request
Mar 15, 2021

Closed


This was referenced Mar 15, 2021

Closed


Closed

@kaciebkacieb
mentioned this pull request
Mar 16, 2021
 Closed

@amarlette
amarlette
commented
Mar 17, 2021

Copy link


Copy Markdown

Hey @huzaifaaak I would like to give you a shout-out on Twitter and include you in our end-of-month issues update for your contribution. Do you have a Twitter we can tag and is your @ the same as your Github?
@huzaifaaak
huzaifaaak
commented
Mar 17, 2021

Copy link


Copy Markdown

Author
Hey @amarlette yes my twitter handle is the same huzaifaaak
@huzaifaaakhuzaifaaak
mentioned this pull request
Mar 18, 2021
 Closed

@blavallablavalla
mentioned this pull request
Mar 25, 2021

Closed

@amarletteamarlette
linked an issue
Mar 29, 2021
that may be
closed
by this pull request


Selected State does not annonce when Button Component selected
#30954

Closed

@amarletteamarlette
mentioned this pull request
Mar 29, 2021
 Closed

facebook-github-bot pushed a commit
that referenced
this pull request
Apr 2, 2021
Huzaifa@facebook-github-bot
Accessibility/button test (#31189)
b5e649f
Summary:
This PR aims to add test's for button.
Snapshot test for PR #31001 . This would make sure `accessibilityState` is properly set.
## Changelog
<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->
[General] [Added] - Test's for button
Pull Request resolved: #31189
Test Plan:
`npm test` to run the test's.
Since the disabled prop of button has precedence over `accessibilityState.disabled` the test's will make sure it remains this way.
Reviewed By: kacieb
Differential Revision: D27473082
Pulled By: lunaleaps
fbshipit-source-id: 65d82620e8c245c2a8e29c3e9a8252d3a4275b09
@anaskhrazaanaskhraza
mentioned this pull request
May 16, 2021

Closed

@martinivanalfonsomartinivanalfonso
mentioned this pull request
May 16, 2021

Closed

@amarletteamarlette
linked an issue
May 24, 2021
that may be
closed
by this pull request


Button Component does not announce Checked/Unchecked State
#30959

Closed


This was referenced Jan 28, 2022

Closed

 Merged

@coderabbitaicoderabbitaiBot
mentioned this pull request
Aug 16, 2026
 Merged

4 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@nadiianadiiaAwaiting requested review from nadiia
@kaciebkaciebAwaiting requested review from kacieb

4 more reviewers

@blavallablavallablavalla left review comments
@facebook-github-botfacebook-github-botfacebook-github-bot left review comments
@analysis-botanalysis-botanalysis-bot requested changes
@lunaleapslunaleapslunaleaps approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

AccessibilityCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Button Component does not announce Checked/Unchecked State

Selected State does not annonce when Button Component selected


7 participants

@huzaifaaak@facebook-github-bot@analysis-bot@lunaleaps@kacieb@amarlette@blavalla

, '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
Closed
huzaifaaak wants to merge 12 commits into
react:masterfrom 
huzaifaaak:accessibility/button
Closed
Added talkback support for button accessibility: disabled prop#31001
huzaifaaak wants to merge 12 commits into
react:masterfrom 
huzaifaaak:accessibility/button
Conversation
@huzaifaaak
@huzaifaaakhuzaifaaak
commented
Feb 16, 2021

Copy link


Copy Markdown

Summary
Issue # #30934 .When using a screen reader disabled buttons do not announce that they are disabled.
Changelog
[Android] [Changed] - Passing accessibility state in button so it can announce disabled in talkback
Test Plan
I have added Button in Button Example with accessibiltyState prop that will announce button is disabled when testing with talkback.
Ios test
I am unable to run ios project on my machine. RNTesterPods.xcworkspace gives workspace integrity error :/
@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Hi @huzaifaaak!
Thank you for your pull request and welcome to our community.
Action Required
In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.
Process
In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.
Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.
If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!
@analysis-bot
analysis-bot
commented
Feb 16, 2021•

edited

Loading

Copy link


Copy Markdown

PlatformEngineArchSize (bytes)Diff
ios-universaln/a--
Base commit: 4170726
@analysis-bot
analysis-bot
commented
Feb 16, 2021•

edited

Loading

Copy link


Copy Markdown

PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,908,772+366
androidhermesarmeabi-v7a8,407,852+375
androidhermesx869,399,329+376
androidhermesx86_649,343,247+368
androidjscarm64-v8a10,642,464+144
androidjscarmeabi-v7a10,124,638+159
androidjscx8610,694,515+149
androidjscx86_6411,278,700+142
Base commit: 4170726
@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@facebook-github-botfacebook-github-bot
added
the 
CLA Signed
This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
label
Feb 16, 2021
@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
analysis-bot
analysis-bot
suggested changes
Feb 16, 2021
View reviewed changes
@analysis-botanalysis-bot
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Code analysis results:
  • eslint found some issues. Run yarn lint --fix to automatically fix problems.
Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

4 similar comments

@facebook-github-bot
facebook-github-bot
commented
Feb 16, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@facebook-github-bot
facebook-github-bot
commented
Feb 17, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@facebook-github-bot
facebook-github-bot
commented
Feb 17, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@facebook-github-bot
facebook-github-bot
commented
Feb 17, 2021

Copy link


Copy Markdown

Contributor
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
@huzaifaaak
huzaifaaak
commented
Feb 17, 2021•

edited

Loading

Copy link


Copy Markdown

Author
@kacieb Can you please review this and give your comments 🙏
lunaleaps
lunaleaps
reviewed
Feb 17, 2021
View reviewed changes
Comment threadpackages/rn-tester/js/examples/Button/ButtonExample.js
Outdated

Comment threadLibraries/Components/Button.js
Outdated

 }
 const accessibilityState = {};
 const accessibilityState =
 disabled != null
@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.

I'd suggest const accessibilityState = disabled === true ? ... : ... so it's accessiblityState.disabled | disabled.
@lunaleapslunaleapsMar 10, 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.

Sorry to follow up on this can we avoid creating a new object here?
const accessibilityState = disabled !== accessibilityState.disabled ? {...this.props.accessibilityState, disabled} : this.props.accessibilityState

Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

@lunaleaps
lunaleaps
commented
Feb 17, 2021

Copy link


Copy Markdown

Contributor
Thanks for adding an example as well!
analysis-bot
analysis-bot
suggested changes
Feb 17, 2021
View reviewed changes
@analysis-botanalysis-bot
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

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

Code analysis results:
  • eslint found some issues. Run yarn lint --fix to automatically fix problems.
Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

 onDismiss={() => {
 if (this.state.action === 'onDismiss') alert(this.state.action);
 if (this.state.action === 'onDismiss') {
 alert(this.state.action);
@analysis-botanalysis-botFeb 17, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

no-alert: Unexpected alert.
Comment threadpackages/rn-tester/js/examples/Modal/ModalExample.js
Outdated

 onShow={() => {
 if (this.state.action === 'onShow') alert(this.state.action);
 if (this.state.action === 'onShow') {
 alert(this.state.action);
@analysis-botanalysis-botFeb 17, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

no-alert: Unexpected alert.
lunaleaps
lunaleaps
approved these changes
Feb 17, 2021
View reviewed changes
@lunaleapslunaleaps
left a comment


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.

Looks good to me!
Comment threadLibraries/Components/Button.js
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
facebook-github-bot
facebook-github-bot
reviewed
Feb 17, 2021
View reviewed changes
@facebook-github-botfacebook-github-bot
left a comment


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 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
lunaleaps
lunaleaps
reviewed
Feb 18, 2021
View reviewed changes
Comment threadLibraries/Components/Button.js
 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
lunaleaps
lunaleaps
reviewed
Mar 8, 2021
View reviewed changes
Comment threadLibraries/Components/Button.js
Outdated

 nextFocusUp={nextFocusUp}
 testID={testID}
 disabled={disabled}
 disabled={disabled != null ? disabled : accessibilityState?.disabled} //This will sync accessibilityState={disabled} with disabled prop.
@lunaleapslunaleapsMar 8, 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 think this should be done on line 286? Otherwise we're missing the logic in that if statement if accessibilityState.disabled is true
@huzaifaaakhuzaifaaakMar 8, 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.

I was a bit confused on this. For now it's not appyling the disable style if only accessibilityState?.disabled is passed.
@huzaifaaakhuzaifaaakMar 8, 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.

I think this needs to be passed here and on line 286 so it applies style and disables the button aswell if accessibilityState.disabled is true
@lunaleapslunaleapsMar 8, 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.

Yea I think we should treat the button as disabled if accessibilityState.disabled is true in all manners --- behaviour and styling
@kaciebkaciebMar 8, 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.

Yea I think we should treat the button as disabled if accessibilityState.disabled is true in all manners --- behaviour and styling
I think I agree with you -I think if accessibilityState.disabled=true and disabled=false though, disabled should take precedence. If disabled is undefined, then I agree accessibilityState.disabled=true should disable the component.
@huzaifaaakhuzaifaaakMar 9, 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.

@lunaleaps@kacieb thank you for the explanation. I have updated the accessibilityState.disabled=true to update styling aswell.
Edit: The disabled prop has precedence and if it is not provided accessibilityState.disabled will take over.
lunaleaps
lunaleaps
approved these changes
Mar 10, 2021
View reviewed changes
@lunaleapslunaleaps
left a comment


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.

Otherwise looks good to me
Comment threadLibraries/Components/Button.js
Outdated

 }
 const accessibilityState = {};
 const accessibilityState =
 disabled != null
@lunaleapslunaleapsMar 10, 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.

Sorry to follow up on this can we avoid creating a new object here?
const accessibilityState = disabled !== accessibilityState.disabled ? {...this.props.accessibilityState, disabled} : this.props.accessibilityState

@huzaifaaak
huzaifaaak
commented
Mar 10, 2021

Copy link


Copy Markdown

Author
@lunaleaps I have updated it to avoid object creation. Do let me know if it can be done in a better way
facebook-github-bot
facebook-github-bot
reviewed
Mar 10, 2021
View reviewed changes
@facebook-github-botfacebook-github-bot
left a comment


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 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@facebook-github-botfacebook-github-bot
added
the 
Merged
This PR has been merged.
label
Mar 11, 2021
@facebook-github-bot
facebook-github-bot
commented
Mar 11, 2021

Copy link


Copy Markdown

Contributor
@lunaleaps merged this pull request in 5889cbe.
@kacieb
kacieb
commented
Mar 12, 2021

Copy link


Copy Markdown

Contributor
This is awesome, thank you so much for working on this and improving React Native's accessibility! I'd love to see a snapshot test for this component at some point, similar to this Pressable one added here, that ensures the accessibilityState values are correctly set as expected.
@amarletteamarlette
mentioned this pull request
Mar 15, 2021

Closed


This was referenced Mar 15, 2021

Closed


Closed

@kaciebkacieb
mentioned this pull request
Mar 16, 2021
 Closed

@amarlette
amarlette
commented
Mar 17, 2021

Copy link


Copy Markdown

Hey @huzaifaaak I would like to give you a shout-out on Twitter and include you in our end-of-month issues update for your contribution. Do you have a Twitter we can tag and is your @ the same as your Github?
@huzaifaaak
huzaifaaak
commented
Mar 17, 2021

Copy link


Copy Markdown

Author
Hey @amarlette yes my twitter handle is the same huzaifaaak
@huzaifaaakhuzaifaaak
mentioned this pull request
Mar 18, 2021
 Closed

@blavallablavalla
mentioned this pull request
Mar 25, 2021

Closed

@amarletteamarlette
linked an issue
Mar 29, 2021
that may be
closed
by this pull request


Selected State does not annonce when Button Component selected
#30954

Closed

@amarletteamarlette
mentioned this pull request
Mar 29, 2021
 Closed

facebook-github-bot pushed a commit
that referenced
this pull request
Apr 2, 2021
Huzaifa@facebook-github-bot
Accessibility/button test (#31189)
b5e649f
Summary:
This PR aims to add test's for button.
Snapshot test for PR #31001 . This would make sure `accessibilityState` is properly set.
## Changelog
<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->
[General] [Added] - Test's for button
Pull Request resolved: #31189
Test Plan:
`npm test` to run the test's.
Since the disabled prop of button has precedence over `accessibilityState.disabled` the test's will make sure it remains this way.
Reviewed By: kacieb
Differential Revision: D27473082
Pulled By: lunaleaps
fbshipit-source-id: 65d82620e8c245c2a8e29c3e9a8252d3a4275b09
@anaskhrazaanaskhraza
mentioned this pull request
May 16, 2021

Closed

@martinivanalfonsomartinivanalfonso
mentioned this pull request
May 16, 2021

Closed

@amarletteamarlette
linked an issue
May 24, 2021
that may be
closed
by this pull request


Button Component does not announce Checked/Unchecked State
#30959

Closed


This was referenced Jan 28, 2022

Closed

 Merged

@coderabbitaicoderabbitaiBot
mentioned this pull request
Aug 16, 2026
 Merged

4 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@nadiianadiiaAwaiting requested review from nadiia
@kaciebkaciebAwaiting requested review from kacieb

4 more reviewers

@blavallablavallablavalla left review comments
@facebook-github-botfacebook-github-botfacebook-github-bot left review comments
@analysis-botanalysis-botanalysis-bot requested changes
@lunaleapslunaleapslunaleaps approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

AccessibilityCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Button Component does not announce Checked/Unchecked State

Selected State does not annonce when Button Component selected


7 participants

@huzaifaaak@facebook-github-bot@analysis-bot@lunaleaps@kacieb@amarlette@blavalla