[Accessibility] Fix Image does not announce "disabled" - #31252

Closed
fabOnReact wants to merge 11 commits into
react:masterfrom
fabOnReact:fix/image-accessibility
Closed

[Accessibility] Fix Image does not announce "disabled"#31252
fabOnReact wants to merge 11 commits into
react:masterfrom
fabOnReact:fix/image-accessibility

Conversation

@fabOnReact

@fabOnReactfabOnReact commented Mar 26, 2021

Copy link
Copy Markdown
Contributor

Summary

This issue fixes#30935fixes#30936 screenreader does not announce Image disabled accessibilityState.

As stated in AOSP View.java, the framework will handle routine focus movement, views indicate their willingness to take focus through the isFocusable method https://bit.ly/3dCnyHb

* <p>The framework will handle routine focus movement in response to user input. This includes
* changing the focus as views are removed or hidden, or as new views become available. Views
* indicate their willingness to take focus through the {@link #isFocusable} method. To change
* whether a view can take focus, call {@link #setFocusable(boolean)}.

The property is updated through its shadow node ReactImageManager method setAccessiblehttps://bit.ly/3dDuK5L

 * <p>InstancesofthisclassreceivepropertyupdatesfromJSvia@{linkUIManagerModule}.
* Subclassesmayuse {@link #updateShadowNode} topersistsomeoftheupdatedfieldsinthenode
* instancethatcorrespondstoaparticularviewtype.

Changelog

[Android] [Fixed] - adding setAccessible to ReactImageManager to allow screenreader announce Image accessibilityState of "disabled"

Test Plan

CLICK TO OPEN TESTS RESULTS

Enable audio to hear the screenreader

TEST SCENARIO

  • The user moves the screenreader focus to an image and the screenreader reads the Image accessibilityLabel "plain network image"

RESULT

  • The screenreader announces the accessibilityState disabled after reading the Image accessibilityLabel "plain network image"
<Imageaccessible={true}accessibilityLabel="plain network image"accessibilityState={{disabled: true}}source={fullImage}style={styles.base}/>
2021-03-26.18-27-36.mp4

@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 Mar 26, 2021
@fabOnReact

fabOnReact commented Mar 26, 2021

Copy link
Copy Markdown
ContributorAuthor

@analysis-bot

analysis-bot commented Mar 26, 2021

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

Base commit: da899c0

@amarlette

amarlette commented Mar 26, 2021

Copy link
Copy Markdown

Thank you for working on this issue @fabriziobertoglio1987! Excited to see your finished PR.

Got you on the project board and will assign a reviewer when you are out of drafts.

@fabOnReact
fabOnReact marked this pull request as ready for review April 6, 2021 16:33
@lunaleaps

lunaleaps commented Apr 22, 2021

Copy link
Copy Markdown
Contributor

@fabriziobertoglio1987is this ready for review? Ah sorry looks like it is, I'll move it to the right column then

@fabOnReact

Copy link
Copy Markdown
ContributorAuthor

sorry @lunaleaps, I did set this as ready for review. Currently I'm away from office, but I will be doing opensource again in 1-2 weeks. Thanks a lot. I wish you a good day

@kacieb

Copy link
Copy Markdown
Contributor

cc @blavalla for Android review!

},
},
{
title: 'Check if these properties are enabled',

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: It would be good to specify what properties we're testing here - in this case it's image focusability and disabled state.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@kacieb thanks for the code review. I experienced some issues with flipper after the last rebase and I published pr #31468 which includes solution to the runtime in the rn-tester app. I added the changes you required and tested them using the fix #31468. The functionality works like before. Thanks a lot 🙏

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

Android code looks good to me!

const mixedCheckboxImageSource = require('./mixed.png');
const {createRef} = require('react');
const fullImage = {
uri: 'https://www.facebook.com/ads/pics/successstories.png',

@lunaleapslunaleapsApr 26, 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.

Do you think we should reference a local asset here instead of fetching over network? I know there are some assets under js/assets/..

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

thanks a lot for the code review @lunaleaps. I added the changes required. More info in comment #31252 (comment) 🙏

@analysis-bot

analysis-bot commented Apr 27, 2021

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,864,454-122,281
androidhermesarmeabi-v7a8,386,258-107,725
androidhermesx869,321,886-117,136
androidhermesx86_649,264,852-115,931
androidjscarm64-v8a10,593,478-11,656
androidjscarmeabi-v7a10,098,377-7,030
androidjscx8610,612,415-11,729
androidjscx86_6411,195,638-12,174

Base commit: da899c0

image: {
width: 20,
height: 20,
width: 120,

@lunaleapslunaleapsMay 3, 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.

@fabriziobertoglio1987 Would this break other accessibility examples that use styles.image? Could create a separate styles.disabledImage? Did you want me to add during import? Otherwise I think this looks good!

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@lunaleaps thanks a lot for the codereview.. sorry for this. I just updated the code with commit f5e4a16 I remain available 🙏

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

Great! Thank you for your help! Importing this now :)

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@lunaleaps has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@lunaleaps merged this pull request in 333b46c.

@facebook-github-botfacebook-github-bot added the Merged This PR has been merged. label May 4, 2021
@amarlette

Copy link
Copy Markdown

Thank you @fabriziobertoglio1987 for another great contribution to a more accessible React Native! 😄

I'll highlight you with the same info from your other PR.

@fabOnReact

Copy link
Copy Markdown
ContributorAuthor

@lunaleaps thanks a lot for merging this pr. I will look for my next issues in the Accessibility Project 🙏
@amarlette thanks a lot for the mention 🙏

facebook-github-bot pushed a commit that referenced this pull request Feb 15, 2022
…ality when disabled (#33076)
Summary:
This issue fixes#30937fixes#30947fixes#30840 ([Test Case 7.1][7.1], [Test Case 7.3][7.3], [Test Case 7.5][7.5]) .
The issue is caused by:
1) The missing javascript logic on the `accessibilityState` in the Text component fabOnReact@6ab7ab3 (as previously implemented in [Button][20]).
2) The missing setter for prop `accessible` in `ReactTextAnchorViewManager` fabOnReact@17095c6 (More information in previous PR #31252)
Related PR #33070 PR callstack/react-native-slider#354
[20]: https://github.com/facebook/react-native/pull/31001/files#diff-4f225d043edf4cf5b8288285b6a957e2187fc0242f240bde396e41c4c25e4124R281-R289
## Changelog
[Android] [Fixed] - Text Component does not announce disabled and disables click functionality when disabled
Pull Request resolved: #33076
Test Plan:
[1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][1])
[2]. Text has `disabled` ([link][2])
[3]. Text has `accessibilityState={{disabled: true}}` ([link][3])
[4]. Text has `accessibilityState={{disabled:false}}` ([link][4])
[5]. Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][5])
[6]. Text has `accessibilityState={{disabled:true}}` and method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [b4cd8][10]) ([link][6])
7. Test Cases on the main branch
[7.1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][7.1])
[7.3] Text has `accessibilityState={{disabled: true}}` ([link][7.3])
[7.5] Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][7.5])
[7.6] Text has `onPress callback` and `accessibilityState={{disabled: true}}` ([link][7.6])
[7.7] Text has `accessibilityState={{disabled:true}}` and no method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [c4f98dd][11]) ([link][7.7])
[1]: fabOnReact/react-native-notes#1 (comment)
[2]: fabOnReact/react-native-notes#1 (comment)
[3]: fabOnReact/react-native-notes#1 (comment)
[4]: fabOnReact/react-native-notes#1 (comment)
[5]: fabOnReact/react-native-notes#1 (comment)
[6]: fabOnReact/react-native-notes#1 (comment)
[7.1]: fabOnReact/react-native-notes#1 (comment)
[7.3]: fabOnReact/react-native-notes#1 (comment)
[7.5]: fabOnReact/react-native-notes#1 (comment)
[7.6]: fabOnReact/react-native-notes#1 (comment)
[7.7]: fabOnReact/react-native-notes#1 (comment)
[10]: 17095c6
[11]: 6ab7ab3
Reviewed By: blavalla
Differential Revision: D34211793
Pulled By: ShikaSD
fbshipit-source-id: e153fb48c194f5884e30beb9172e66aca7ce1a41
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2023
…ality when disabled (react#33076)
Summary:
This issue fixesreact#30937fixesreact#30947fixesreact#30840 ([Test Case 7.1][7.1], [Test Case 7.3][7.3], [Test Case 7.5][7.5]) .
The issue is caused by:
1) The missing javascript logic on the `accessibilityState` in the Text component fabOnReact@6ab7ab3 (as previously implemented in [Button][20]).
2) The missing setter for prop `accessible` in `ReactTextAnchorViewManager` fabOnReact@17095c6 (More information in previous PR react#31252)
Related PR react#33070 PR callstack/react-native-slider#354
[20]: https://github.com/facebook/react-native/pull/31001/files#diff-4f225d043edf4cf5b8288285b6a957e2187fc0242f240bde396e41c4c25e4124R281-R289
[Android] [Fixed] - Text Component does not announce disabled and disables click functionality when disabled
Pull Request resolved: react#33076
Test Plan:
[1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][1])
[2]. Text has `disabled` ([link][2])
[3]. Text has `accessibilityState={{disabled: true}}` ([link][3])
[4]. Text has `accessibilityState={{disabled:false}}` ([link][4])
[5]. Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][5])
[6]. Text has `accessibilityState={{disabled:true}}` and method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [b4cd8][10]) ([link][6])
7. Test Cases on the main branch
[7.1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][7.1])
[7.3] Text has `accessibilityState={{disabled: true}}` ([link][7.3])
[7.5] Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][7.5])
[7.6] Text has `onPress callback` and `accessibilityState={{disabled: true}}` ([link][7.6])
[7.7] Text has `accessibilityState={{disabled:true}}` and no method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [c4f98dd][11]) ([link][7.7])
[1]: fabOnReact/react-native-notes#1 (comment)
[2]: fabOnReact/react-native-notes#1 (comment)
[3]: fabOnReact/react-native-notes#1 (comment)
[4]: fabOnReact/react-native-notes#1 (comment)
[5]: fabOnReact/react-native-notes#1 (comment)
[6]: fabOnReact/react-native-notes#1 (comment)
[7.1]: fabOnReact/react-native-notes#1 (comment)
[7.3]: fabOnReact/react-native-notes#1 (comment)
[7.5]: fabOnReact/react-native-notes#1 (comment)
[7.6]: fabOnReact/react-native-notes#1 (comment)
[7.7]: fabOnReact/react-native-notes#1 (comment)
[10]: react@17095c6
[11]: react@6ab7ab3
Reviewed By: blavalla
Differential Revision: D34211793
Pulled By: ShikaSD
fbshipit-source-id: e153fb48c194f5884e30beb9172e66aca7ce1a41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

ImageBackground does not announce "disabled" Image does not announce "disabled"

7 participants

@fabOnReact@analysis-bot@amarlette@lunaleaps@kacieb@facebook-github-bot@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

[Accessibility] Fix Image does not announce "disabled" - #31252

Closed
fabOnReact wants to merge 11 commits into
react:masterfrom
fabOnReact:fix/image-accessibility
Closed

[Accessibility] Fix Image does not announce "disabled"#31252
fabOnReact wants to merge 11 commits into
react:masterfrom
fabOnReact:fix/image-accessibility

Conversation

@fabOnReact

@fabOnReactfabOnReact commented Mar 26, 2021

Copy link
Copy Markdown
Contributor

Summary

This issue fixes#30935fixes#30936 screenreader does not announce Image disabled accessibilityState.

As stated in AOSP View.java, the framework will handle routine focus movement, views indicate their willingness to take focus through the isFocusable method https://bit.ly/3dCnyHb

* <p>The framework will handle routine focus movement in response to user input. This includes
* changing the focus as views are removed or hidden, or as new views become available. Views
* indicate their willingness to take focus through the {@link #isFocusable} method. To change
* whether a view can take focus, call {@link #setFocusable(boolean)}.

The property is updated through its shadow node ReactImageManager method setAccessiblehttps://bit.ly/3dDuK5L

 * <p>InstancesofthisclassreceivepropertyupdatesfromJSvia@{linkUIManagerModule}.
* Subclassesmayuse {@link #updateShadowNode} topersistsomeoftheupdatedfieldsinthenode
* instancethatcorrespondstoaparticularviewtype.

Changelog

[Android] [Fixed] - adding setAccessible to ReactImageManager to allow screenreader announce Image accessibilityState of "disabled"

Test Plan

CLICK TO OPEN TESTS RESULTS

Enable audio to hear the screenreader

TEST SCENARIO

  • The user moves the screenreader focus to an image and the screenreader reads the Image accessibilityLabel "plain network image"

RESULT

  • The screenreader announces the accessibilityState disabled after reading the Image accessibilityLabel "plain network image"
<Imageaccessible={true}accessibilityLabel="plain network image"accessibilityState={{disabled: true}}source={fullImage}style={styles.base}/>
2021-03-26.18-27-36.mp4

@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 Mar 26, 2021
@fabOnReact

fabOnReact commented Mar 26, 2021

Copy link
Copy Markdown
ContributorAuthor

@analysis-bot

analysis-bot commented Mar 26, 2021

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

Base commit: da899c0

@amarlette

amarlette commented Mar 26, 2021

Copy link
Copy Markdown

Thank you for working on this issue @fabriziobertoglio1987! Excited to see your finished PR.

Got you on the project board and will assign a reviewer when you are out of drafts.

@fabOnReact
fabOnReact marked this pull request as ready for review April 6, 2021 16:33
@lunaleaps

lunaleaps commented Apr 22, 2021

Copy link
Copy Markdown
Contributor

@fabriziobertoglio1987is this ready for review? Ah sorry looks like it is, I'll move it to the right column then

@fabOnReact

Copy link
Copy Markdown
ContributorAuthor

sorry @lunaleaps, I did set this as ready for review. Currently I'm away from office, but I will be doing opensource again in 1-2 weeks. Thanks a lot. I wish you a good day

@kacieb

Copy link
Copy Markdown
Contributor

cc @blavalla for Android review!

},
},
{
title: 'Check if these properties are enabled',

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: It would be good to specify what properties we're testing here - in this case it's image focusability and disabled state.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@kacieb thanks for the code review. I experienced some issues with flipper after the last rebase and I published pr #31468 which includes solution to the runtime in the rn-tester app. I added the changes you required and tested them using the fix #31468. The functionality works like before. Thanks a lot 🙏

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

Android code looks good to me!

const mixedCheckboxImageSource = require('./mixed.png');
const {createRef} = require('react');
const fullImage = {
uri: 'https://www.facebook.com/ads/pics/successstories.png',

@lunaleapslunaleapsApr 26, 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.

Do you think we should reference a local asset here instead of fetching over network? I know there are some assets under js/assets/..

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

thanks a lot for the code review @lunaleaps. I added the changes required. More info in comment #31252 (comment) 🙏

@analysis-bot

analysis-bot commented Apr 27, 2021

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,864,454-122,281
androidhermesarmeabi-v7a8,386,258-107,725
androidhermesx869,321,886-117,136
androidhermesx86_649,264,852-115,931
androidjscarm64-v8a10,593,478-11,656
androidjscarmeabi-v7a10,098,377-7,030
androidjscx8610,612,415-11,729
androidjscx86_6411,195,638-12,174

Base commit: da899c0

image: {
width: 20,
height: 20,
width: 120,

@lunaleapslunaleapsMay 3, 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.

@fabriziobertoglio1987 Would this break other accessibility examples that use styles.image? Could create a separate styles.disabledImage? Did you want me to add during import? Otherwise I think this looks good!

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@lunaleaps thanks a lot for the codereview.. sorry for this. I just updated the code with commit f5e4a16 I remain available 🙏

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

Great! Thank you for your help! Importing this now :)

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@lunaleaps has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@lunaleaps merged this pull request in 333b46c.

@facebook-github-botfacebook-github-bot added the Merged This PR has been merged. label May 4, 2021
@amarlette

Copy link
Copy Markdown

Thank you @fabriziobertoglio1987 for another great contribution to a more accessible React Native! 😄

I'll highlight you with the same info from your other PR.

@fabOnReact

Copy link
Copy Markdown
ContributorAuthor

@lunaleaps thanks a lot for merging this pr. I will look for my next issues in the Accessibility Project 🙏
@amarlette thanks a lot for the mention 🙏

facebook-github-bot pushed a commit that referenced this pull request Feb 15, 2022
…ality when disabled (#33076)
Summary:
This issue fixes#30937fixes#30947fixes#30840 ([Test Case 7.1][7.1], [Test Case 7.3][7.3], [Test Case 7.5][7.5]) .
The issue is caused by:
1) The missing javascript logic on the `accessibilityState` in the Text component fabOnReact@6ab7ab3 (as previously implemented in [Button][20]).
2) The missing setter for prop `accessible` in `ReactTextAnchorViewManager` fabOnReact@17095c6 (More information in previous PR #31252)
Related PR #33070 PR callstack/react-native-slider#354
[20]: https://github.com/facebook/react-native/pull/31001/files#diff-4f225d043edf4cf5b8288285b6a957e2187fc0242f240bde396e41c4c25e4124R281-R289
## Changelog
[Android] [Fixed] - Text Component does not announce disabled and disables click functionality when disabled
Pull Request resolved: #33076
Test Plan:
[1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][1])
[2]. Text has `disabled` ([link][2])
[3]. Text has `accessibilityState={{disabled: true}}` ([link][3])
[4]. Text has `accessibilityState={{disabled:false}}` ([link][4])
[5]. Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][5])
[6]. Text has `accessibilityState={{disabled:true}}` and method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [b4cd8][10]) ([link][6])
7. Test Cases on the main branch
[7.1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][7.1])
[7.3] Text has `accessibilityState={{disabled: true}}` ([link][7.3])
[7.5] Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][7.5])
[7.6] Text has `onPress callback` and `accessibilityState={{disabled: true}}` ([link][7.6])
[7.7] Text has `accessibilityState={{disabled:true}}` and no method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [c4f98dd][11]) ([link][7.7])
[1]: fabOnReact/react-native-notes#1 (comment)
[2]: fabOnReact/react-native-notes#1 (comment)
[3]: fabOnReact/react-native-notes#1 (comment)
[4]: fabOnReact/react-native-notes#1 (comment)
[5]: fabOnReact/react-native-notes#1 (comment)
[6]: fabOnReact/react-native-notes#1 (comment)
[7.1]: fabOnReact/react-native-notes#1 (comment)
[7.3]: fabOnReact/react-native-notes#1 (comment)
[7.5]: fabOnReact/react-native-notes#1 (comment)
[7.6]: fabOnReact/react-native-notes#1 (comment)
[7.7]: fabOnReact/react-native-notes#1 (comment)
[10]: 17095c6
[11]: 6ab7ab3
Reviewed By: blavalla
Differential Revision: D34211793
Pulled By: ShikaSD
fbshipit-source-id: e153fb48c194f5884e30beb9172e66aca7ce1a41
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2023
…ality when disabled (react#33076)
Summary:
This issue fixesreact#30937fixesreact#30947fixesreact#30840 ([Test Case 7.1][7.1], [Test Case 7.3][7.3], [Test Case 7.5][7.5]) .
The issue is caused by:
1) The missing javascript logic on the `accessibilityState` in the Text component fabOnReact@6ab7ab3 (as previously implemented in [Button][20]).
2) The missing setter for prop `accessible` in `ReactTextAnchorViewManager` fabOnReact@17095c6 (More information in previous PR react#31252)
Related PR react#33070 PR callstack/react-native-slider#354
[20]: https://github.com/facebook/react-native/pull/31001/files#diff-4f225d043edf4cf5b8288285b6a957e2187fc0242f240bde396e41c4c25e4124R281-R289
[Android] [Fixed] - Text Component does not announce disabled and disables click functionality when disabled
Pull Request resolved: react#33076
Test Plan:
[1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][1])
[2]. Text has `disabled` ([link][2])
[3]. Text has `accessibilityState={{disabled: true}}` ([link][3])
[4]. Text has `accessibilityState={{disabled:false}}` ([link][4])
[5]. Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][5])
[6]. Text has `accessibilityState={{disabled:true}}` and method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [b4cd8][10]) ([link][6])
7. Test Cases on the main branch
[7.1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][7.1])
[7.3] Text has `accessibilityState={{disabled: true}}` ([link][7.3])
[7.5] Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][7.5])
[7.6] Text has `onPress callback` and `accessibilityState={{disabled: true}}` ([link][7.6])
[7.7] Text has `accessibilityState={{disabled:true}}` and no method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [c4f98dd][11]) ([link][7.7])
[1]: fabOnReact/react-native-notes#1 (comment)
[2]: fabOnReact/react-native-notes#1 (comment)
[3]: fabOnReact/react-native-notes#1 (comment)
[4]: fabOnReact/react-native-notes#1 (comment)
[5]: fabOnReact/react-native-notes#1 (comment)
[6]: fabOnReact/react-native-notes#1 (comment)
[7.1]: fabOnReact/react-native-notes#1 (comment)
[7.3]: fabOnReact/react-native-notes#1 (comment)
[7.5]: fabOnReact/react-native-notes#1 (comment)
[7.6]: fabOnReact/react-native-notes#1 (comment)
[7.7]: fabOnReact/react-native-notes#1 (comment)
[10]: react@17095c6
[11]: react@6ab7ab3
Reviewed By: blavalla
Differential Revision: D34211793
Pulled By: ShikaSD
fbshipit-source-id: e153fb48c194f5884e30beb9172e66aca7ce1a41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

ImageBackground does not announce "disabled" Image does not announce "disabled"

7 participants

@fabOnReact@analysis-bot@amarlette@lunaleaps@kacieb@facebook-github-bot@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

[Accessibility] Fix Image does not announce "disabled" - #31252

Closed
fabOnReact wants to merge 11 commits into
react:masterfrom
fabOnReact:fix/image-accessibility
Closed

[Accessibility] Fix Image does not announce "disabled"#31252
fabOnReact wants to merge 11 commits into
react:masterfrom
fabOnReact:fix/image-accessibility

Conversation

@fabOnReact

@fabOnReactfabOnReact commented Mar 26, 2021

Copy link
Copy Markdown
Contributor

Summary

This issue fixes#30935fixes#30936 screenreader does not announce Image disabled accessibilityState.

As stated in AOSP View.java, the framework will handle routine focus movement, views indicate their willingness to take focus through the isFocusable method https://bit.ly/3dCnyHb

* <p>The framework will handle routine focus movement in response to user input. This includes
* changing the focus as views are removed or hidden, or as new views become available. Views
* indicate their willingness to take focus through the {@link #isFocusable} method. To change
* whether a view can take focus, call {@link #setFocusable(boolean)}.

The property is updated through its shadow node ReactImageManager method setAccessiblehttps://bit.ly/3dDuK5L

 * <p>InstancesofthisclassreceivepropertyupdatesfromJSvia@{linkUIManagerModule}.
* Subclassesmayuse {@link #updateShadowNode} topersistsomeoftheupdatedfieldsinthenode
* instancethatcorrespondstoaparticularviewtype.

Changelog

[Android] [Fixed] - adding setAccessible to ReactImageManager to allow screenreader announce Image accessibilityState of "disabled"

Test Plan

CLICK TO OPEN TESTS RESULTS

Enable audio to hear the screenreader

TEST SCENARIO

  • The user moves the screenreader focus to an image and the screenreader reads the Image accessibilityLabel "plain network image"

RESULT

  • The screenreader announces the accessibilityState disabled after reading the Image accessibilityLabel "plain network image"
<Imageaccessible={true}accessibilityLabel="plain network image"accessibilityState={{disabled: true}}source={fullImage}style={styles.base}/>
2021-03-26.18-27-36.mp4

@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 Mar 26, 2021
@fabOnReact

fabOnReact commented Mar 26, 2021

Copy link
Copy Markdown
ContributorAuthor

@analysis-bot

analysis-bot commented Mar 26, 2021

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

Base commit: da899c0

@amarlette

amarlette commented Mar 26, 2021

Copy link
Copy Markdown

Thank you for working on this issue @fabriziobertoglio1987! Excited to see your finished PR.

Got you on the project board and will assign a reviewer when you are out of drafts.

@fabOnReact
fabOnReact marked this pull request as ready for review April 6, 2021 16:33
@lunaleaps

lunaleaps commented Apr 22, 2021

Copy link
Copy Markdown
Contributor

@fabriziobertoglio1987is this ready for review? Ah sorry looks like it is, I'll move it to the right column then

@fabOnReact

Copy link
Copy Markdown
ContributorAuthor

sorry @lunaleaps, I did set this as ready for review. Currently I'm away from office, but I will be doing opensource again in 1-2 weeks. Thanks a lot. I wish you a good day

@kacieb

Copy link
Copy Markdown
Contributor

cc @blavalla for Android review!

},
},
{
title: 'Check if these properties are enabled',

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: It would be good to specify what properties we're testing here - in this case it's image focusability and disabled state.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@kacieb thanks for the code review. I experienced some issues with flipper after the last rebase and I published pr #31468 which includes solution to the runtime in the rn-tester app. I added the changes you required and tested them using the fix #31468. The functionality works like before. Thanks a lot 🙏

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

Android code looks good to me!

const mixedCheckboxImageSource = require('./mixed.png');
const {createRef} = require('react');
const fullImage = {
uri: 'https://www.facebook.com/ads/pics/successstories.png',

@lunaleapslunaleapsApr 26, 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.

Do you think we should reference a local asset here instead of fetching over network? I know there are some assets under js/assets/..

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

thanks a lot for the code review @lunaleaps. I added the changes required. More info in comment #31252 (comment) 🙏

@analysis-bot

analysis-bot commented Apr 27, 2021

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,864,454-122,281
androidhermesarmeabi-v7a8,386,258-107,725
androidhermesx869,321,886-117,136
androidhermesx86_649,264,852-115,931
androidjscarm64-v8a10,593,478-11,656
androidjscarmeabi-v7a10,098,377-7,030
androidjscx8610,612,415-11,729
androidjscx86_6411,195,638-12,174

Base commit: da899c0

image: {
width: 20,
height: 20,
width: 120,

@lunaleapslunaleapsMay 3, 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.

@fabriziobertoglio1987 Would this break other accessibility examples that use styles.image? Could create a separate styles.disabledImage? Did you want me to add during import? Otherwise I think this looks good!

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@lunaleaps thanks a lot for the codereview.. sorry for this. I just updated the code with commit f5e4a16 I remain available 🙏

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

Great! Thank you for your help! Importing this now :)

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@lunaleaps has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@lunaleaps merged this pull request in 333b46c.

@facebook-github-botfacebook-github-bot added the Merged This PR has been merged. label May 4, 2021
@amarlette

Copy link
Copy Markdown

Thank you @fabriziobertoglio1987 for another great contribution to a more accessible React Native! 😄

I'll highlight you with the same info from your other PR.

@fabOnReact

Copy link
Copy Markdown
ContributorAuthor

@lunaleaps thanks a lot for merging this pr. I will look for my next issues in the Accessibility Project 🙏
@amarlette thanks a lot for the mention 🙏

facebook-github-bot pushed a commit that referenced this pull request Feb 15, 2022
…ality when disabled (#33076)
Summary:
This issue fixes#30937fixes#30947fixes#30840 ([Test Case 7.1][7.1], [Test Case 7.3][7.3], [Test Case 7.5][7.5]) .
The issue is caused by:
1) The missing javascript logic on the `accessibilityState` in the Text component fabOnReact@6ab7ab3 (as previously implemented in [Button][20]).
2) The missing setter for prop `accessible` in `ReactTextAnchorViewManager` fabOnReact@17095c6 (More information in previous PR #31252)
Related PR #33070 PR callstack/react-native-slider#354
[20]: https://github.com/facebook/react-native/pull/31001/files#diff-4f225d043edf4cf5b8288285b6a957e2187fc0242f240bde396e41c4c25e4124R281-R289
## Changelog
[Android] [Fixed] - Text Component does not announce disabled and disables click functionality when disabled
Pull Request resolved: #33076
Test Plan:
[1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][1])
[2]. Text has `disabled` ([link][2])
[3]. Text has `accessibilityState={{disabled: true}}` ([link][3])
[4]. Text has `accessibilityState={{disabled:false}}` ([link][4])
[5]. Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][5])
[6]. Text has `accessibilityState={{disabled:true}}` and method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [b4cd8][10]) ([link][6])
7. Test Cases on the main branch
[7.1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][7.1])
[7.3] Text has `accessibilityState={{disabled: true}}` ([link][7.3])
[7.5] Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][7.5])
[7.6] Text has `onPress callback` and `accessibilityState={{disabled: true}}` ([link][7.6])
[7.7] Text has `accessibilityState={{disabled:true}}` and no method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [c4f98dd][11]) ([link][7.7])
[1]: fabOnReact/react-native-notes#1 (comment)
[2]: fabOnReact/react-native-notes#1 (comment)
[3]: fabOnReact/react-native-notes#1 (comment)
[4]: fabOnReact/react-native-notes#1 (comment)
[5]: fabOnReact/react-native-notes#1 (comment)
[6]: fabOnReact/react-native-notes#1 (comment)
[7.1]: fabOnReact/react-native-notes#1 (comment)
[7.3]: fabOnReact/react-native-notes#1 (comment)
[7.5]: fabOnReact/react-native-notes#1 (comment)
[7.6]: fabOnReact/react-native-notes#1 (comment)
[7.7]: fabOnReact/react-native-notes#1 (comment)
[10]: 17095c6
[11]: 6ab7ab3
Reviewed By: blavalla
Differential Revision: D34211793
Pulled By: ShikaSD
fbshipit-source-id: e153fb48c194f5884e30beb9172e66aca7ce1a41
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2023
…ality when disabled (react#33076)
Summary:
This issue fixesreact#30937fixesreact#30947fixesreact#30840 ([Test Case 7.1][7.1], [Test Case 7.3][7.3], [Test Case 7.5][7.5]) .
The issue is caused by:
1) The missing javascript logic on the `accessibilityState` in the Text component fabOnReact@6ab7ab3 (as previously implemented in [Button][20]).
2) The missing setter for prop `accessible` in `ReactTextAnchorViewManager` fabOnReact@17095c6 (More information in previous PR react#31252)
Related PR react#33070 PR callstack/react-native-slider#354
[20]: https://github.com/facebook/react-native/pull/31001/files#diff-4f225d043edf4cf5b8288285b6a957e2187fc0242f240bde396e41c4c25e4124R281-R289
[Android] [Fixed] - Text Component does not announce disabled and disables click functionality when disabled
Pull Request resolved: react#33076
Test Plan:
[1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][1])
[2]. Text has `disabled` ([link][2])
[3]. Text has `accessibilityState={{disabled: true}}` ([link][3])
[4]. Text has `accessibilityState={{disabled:false}}` ([link][4])
[5]. Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][5])
[6]. Text has `accessibilityState={{disabled:true}}` and method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [b4cd8][10]) ([link][6])
7. Test Cases on the main branch
[7.1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][7.1])
[7.3] Text has `accessibilityState={{disabled: true}}` ([link][7.3])
[7.5] Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][7.5])
[7.6] Text has `onPress callback` and `accessibilityState={{disabled: true}}` ([link][7.6])
[7.7] Text has `accessibilityState={{disabled:true}}` and no method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [c4f98dd][11]) ([link][7.7])
[1]: fabOnReact/react-native-notes#1 (comment)
[2]: fabOnReact/react-native-notes#1 (comment)
[3]: fabOnReact/react-native-notes#1 (comment)
[4]: fabOnReact/react-native-notes#1 (comment)
[5]: fabOnReact/react-native-notes#1 (comment)
[6]: fabOnReact/react-native-notes#1 (comment)
[7.1]: fabOnReact/react-native-notes#1 (comment)
[7.3]: fabOnReact/react-native-notes#1 (comment)
[7.5]: fabOnReact/react-native-notes#1 (comment)
[7.6]: fabOnReact/react-native-notes#1 (comment)
[7.7]: fabOnReact/react-native-notes#1 (comment)
[10]: react@17095c6
[11]: react@6ab7ab3
Reviewed By: blavalla
Differential Revision: D34211793
Pulled By: ShikaSD
fbshipit-source-id: e153fb48c194f5884e30beb9172e66aca7ce1a41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

ImageBackground does not announce "disabled" Image does not announce "disabled"

7 participants

@fabOnReact@analysis-bot@amarlette@lunaleaps@kacieb@facebook-github-bot@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

[Accessibility] Fix Image does not announce "disabled" - #31252

Closed
fabOnReact wants to merge 11 commits into
react:masterfrom
fabOnReact:fix/image-accessibility
Closed

[Accessibility] Fix Image does not announce "disabled"#31252
fabOnReact wants to merge 11 commits into
react:masterfrom
fabOnReact:fix/image-accessibility

Conversation

@fabOnReact

@fabOnReactfabOnReact commented Mar 26, 2021

Copy link
Copy Markdown
Contributor

Summary

This issue fixes#30935fixes#30936 screenreader does not announce Image disabled accessibilityState.

As stated in AOSP View.java, the framework will handle routine focus movement, views indicate their willingness to take focus through the isFocusable method https://bit.ly/3dCnyHb

* <p>The framework will handle routine focus movement in response to user input. This includes
* changing the focus as views are removed or hidden, or as new views become available. Views
* indicate their willingness to take focus through the {@link #isFocusable} method. To change
* whether a view can take focus, call {@link #setFocusable(boolean)}.

The property is updated through its shadow node ReactImageManager method setAccessiblehttps://bit.ly/3dDuK5L

 * <p>InstancesofthisclassreceivepropertyupdatesfromJSvia@{linkUIManagerModule}.
* Subclassesmayuse {@link #updateShadowNode} topersistsomeoftheupdatedfieldsinthenode
* instancethatcorrespondstoaparticularviewtype.

Changelog

[Android] [Fixed] - adding setAccessible to ReactImageManager to allow screenreader announce Image accessibilityState of "disabled"

Test Plan

CLICK TO OPEN TESTS RESULTS

Enable audio to hear the screenreader

TEST SCENARIO

  • The user moves the screenreader focus to an image and the screenreader reads the Image accessibilityLabel "plain network image"

RESULT

  • The screenreader announces the accessibilityState disabled after reading the Image accessibilityLabel "plain network image"
<Imageaccessible={true}accessibilityLabel="plain network image"accessibilityState={{disabled: true}}source={fullImage}style={styles.base}/>
2021-03-26.18-27-36.mp4

@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 Mar 26, 2021
@fabOnReact

fabOnReact commented Mar 26, 2021

Copy link
Copy Markdown
ContributorAuthor

@analysis-bot

analysis-bot commented Mar 26, 2021

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

Base commit: da899c0

@amarlette

amarlette commented Mar 26, 2021

Copy link
Copy Markdown

Thank you for working on this issue @fabriziobertoglio1987! Excited to see your finished PR.

Got you on the project board and will assign a reviewer when you are out of drafts.

@fabOnReact
fabOnReact marked this pull request as ready for review April 6, 2021 16:33
@lunaleaps

lunaleaps commented Apr 22, 2021

Copy link
Copy Markdown
Contributor

@fabriziobertoglio1987is this ready for review? Ah sorry looks like it is, I'll move it to the right column then

@fabOnReact

Copy link
Copy Markdown
ContributorAuthor

sorry @lunaleaps, I did set this as ready for review. Currently I'm away from office, but I will be doing opensource again in 1-2 weeks. Thanks a lot. I wish you a good day

@kacieb

Copy link
Copy Markdown
Contributor

cc @blavalla for Android review!

},
},
{
title: 'Check if these properties are enabled',

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: It would be good to specify what properties we're testing here - in this case it's image focusability and disabled state.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@kacieb thanks for the code review. I experienced some issues with flipper after the last rebase and I published pr #31468 which includes solution to the runtime in the rn-tester app. I added the changes you required and tested them using the fix #31468. The functionality works like before. Thanks a lot 🙏

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

Android code looks good to me!

const mixedCheckboxImageSource = require('./mixed.png');
const {createRef} = require('react');
const fullImage = {
uri: 'https://www.facebook.com/ads/pics/successstories.png',

@lunaleapslunaleapsApr 26, 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.

Do you think we should reference a local asset here instead of fetching over network? I know there are some assets under js/assets/..

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

thanks a lot for the code review @lunaleaps. I added the changes required. More info in comment #31252 (comment) 🙏

@analysis-bot

analysis-bot commented Apr 27, 2021

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,864,454-122,281
androidhermesarmeabi-v7a8,386,258-107,725
androidhermesx869,321,886-117,136
androidhermesx86_649,264,852-115,931
androidjscarm64-v8a10,593,478-11,656
androidjscarmeabi-v7a10,098,377-7,030
androidjscx8610,612,415-11,729
androidjscx86_6411,195,638-12,174

Base commit: da899c0

image: {
width: 20,
height: 20,
width: 120,

@lunaleapslunaleapsMay 3, 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.

@fabriziobertoglio1987 Would this break other accessibility examples that use styles.image? Could create a separate styles.disabledImage? Did you want me to add during import? Otherwise I think this looks good!

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@lunaleaps thanks a lot for the codereview.. sorry for this. I just updated the code with commit f5e4a16 I remain available 🙏

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

Great! Thank you for your help! Importing this now :)

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@lunaleaps has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@lunaleaps merged this pull request in 333b46c.

@facebook-github-botfacebook-github-bot added the Merged This PR has been merged. label May 4, 2021
@amarlette

Copy link
Copy Markdown

Thank you @fabriziobertoglio1987 for another great contribution to a more accessible React Native! 😄

I'll highlight you with the same info from your other PR.

@fabOnReact

Copy link
Copy Markdown
ContributorAuthor

@lunaleaps thanks a lot for merging this pr. I will look for my next issues in the Accessibility Project 🙏
@amarlette thanks a lot for the mention 🙏

facebook-github-bot pushed a commit that referenced this pull request Feb 15, 2022
…ality when disabled (#33076)
Summary:
This issue fixes#30937fixes#30947fixes#30840 ([Test Case 7.1][7.1], [Test Case 7.3][7.3], [Test Case 7.5][7.5]) .
The issue is caused by:
1) The missing javascript logic on the `accessibilityState` in the Text component fabOnReact@6ab7ab3 (as previously implemented in [Button][20]).
2) The missing setter for prop `accessible` in `ReactTextAnchorViewManager` fabOnReact@17095c6 (More information in previous PR #31252)
Related PR #33070 PR callstack/react-native-slider#354
[20]: https://github.com/facebook/react-native/pull/31001/files#diff-4f225d043edf4cf5b8288285b6a957e2187fc0242f240bde396e41c4c25e4124R281-R289
## Changelog
[Android] [Fixed] - Text Component does not announce disabled and disables click functionality when disabled
Pull Request resolved: #33076
Test Plan:
[1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][1])
[2]. Text has `disabled` ([link][2])
[3]. Text has `accessibilityState={{disabled: true}}` ([link][3])
[4]. Text has `accessibilityState={{disabled:false}}` ([link][4])
[5]. Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][5])
[6]. Text has `accessibilityState={{disabled:true}}` and method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [b4cd8][10]) ([link][6])
7. Test Cases on the main branch
[7.1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][7.1])
[7.3] Text has `accessibilityState={{disabled: true}}` ([link][7.3])
[7.5] Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][7.5])
[7.6] Text has `onPress callback` and `accessibilityState={{disabled: true}}` ([link][7.6])
[7.7] Text has `accessibilityState={{disabled:true}}` and no method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [c4f98dd][11]) ([link][7.7])
[1]: fabOnReact/react-native-notes#1 (comment)
[2]: fabOnReact/react-native-notes#1 (comment)
[3]: fabOnReact/react-native-notes#1 (comment)
[4]: fabOnReact/react-native-notes#1 (comment)
[5]: fabOnReact/react-native-notes#1 (comment)
[6]: fabOnReact/react-native-notes#1 (comment)
[7.1]: fabOnReact/react-native-notes#1 (comment)
[7.3]: fabOnReact/react-native-notes#1 (comment)
[7.5]: fabOnReact/react-native-notes#1 (comment)
[7.6]: fabOnReact/react-native-notes#1 (comment)
[7.7]: fabOnReact/react-native-notes#1 (comment)
[10]: 17095c6
[11]: 6ab7ab3
Reviewed By: blavalla
Differential Revision: D34211793
Pulled By: ShikaSD
fbshipit-source-id: e153fb48c194f5884e30beb9172e66aca7ce1a41
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2023
…ality when disabled (react#33076)
Summary:
This issue fixesreact#30937fixesreact#30947fixesreact#30840 ([Test Case 7.1][7.1], [Test Case 7.3][7.3], [Test Case 7.5][7.5]) .
The issue is caused by:
1) The missing javascript logic on the `accessibilityState` in the Text component fabOnReact@6ab7ab3 (as previously implemented in [Button][20]).
2) The missing setter for prop `accessible` in `ReactTextAnchorViewManager` fabOnReact@17095c6 (More information in previous PR react#31252)
Related PR react#33070 PR callstack/react-native-slider#354
[20]: https://github.com/facebook/react-native/pull/31001/files#diff-4f225d043edf4cf5b8288285b6a957e2187fc0242f240bde396e41c4c25e4124R281-R289
[Android] [Fixed] - Text Component does not announce disabled and disables click functionality when disabled
Pull Request resolved: react#33076
Test Plan:
[1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][1])
[2]. Text has `disabled` ([link][2])
[3]. Text has `accessibilityState={{disabled: true}}` ([link][3])
[4]. Text has `accessibilityState={{disabled:false}}` ([link][4])
[5]. Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][5])
[6]. Text has `accessibilityState={{disabled:true}}` and method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [b4cd8][10]) ([link][6])
7. Test Cases on the main branch
[7.1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][7.1])
[7.3] Text has `accessibilityState={{disabled: true}}` ([link][7.3])
[7.5] Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][7.5])
[7.6] Text has `onPress callback` and `accessibilityState={{disabled: true}}` ([link][7.6])
[7.7] Text has `accessibilityState={{disabled:true}}` and no method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [c4f98dd][11]) ([link][7.7])
[1]: fabOnReact/react-native-notes#1 (comment)
[2]: fabOnReact/react-native-notes#1 (comment)
[3]: fabOnReact/react-native-notes#1 (comment)
[4]: fabOnReact/react-native-notes#1 (comment)
[5]: fabOnReact/react-native-notes#1 (comment)
[6]: fabOnReact/react-native-notes#1 (comment)
[7.1]: fabOnReact/react-native-notes#1 (comment)
[7.3]: fabOnReact/react-native-notes#1 (comment)
[7.5]: fabOnReact/react-native-notes#1 (comment)
[7.6]: fabOnReact/react-native-notes#1 (comment)
[7.7]: fabOnReact/react-native-notes#1 (comment)
[10]: react@17095c6
[11]: react@6ab7ab3
Reviewed By: blavalla
Differential Revision: D34211793
Pulled By: ShikaSD
fbshipit-source-id: e153fb48c194f5884e30beb9172e66aca7ce1a41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

ImageBackground does not announce "disabled" Image does not announce "disabled"

7 participants

@fabOnReact@analysis-bot@amarlette@lunaleaps@kacieb@facebook-github-bot@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

[Accessibility] Fix Image does not announce "disabled" - #31252

Closed
fabOnReact wants to merge 11 commits into
react:masterfrom
fabOnReact:fix/image-accessibility
Closed

[Accessibility] Fix Image does not announce "disabled"#31252
fabOnReact wants to merge 11 commits into
react:masterfrom
fabOnReact:fix/image-accessibility

Conversation

@fabOnReact

@fabOnReactfabOnReact commented Mar 26, 2021

Copy link
Copy Markdown
Contributor

Summary

This issue fixes#30935fixes#30936 screenreader does not announce Image disabled accessibilityState.

As stated in AOSP View.java, the framework will handle routine focus movement, views indicate their willingness to take focus through the isFocusable method https://bit.ly/3dCnyHb

* <p>The framework will handle routine focus movement in response to user input. This includes
* changing the focus as views are removed or hidden, or as new views become available. Views
* indicate their willingness to take focus through the {@link #isFocusable} method. To change
* whether a view can take focus, call {@link #setFocusable(boolean)}.

The property is updated through its shadow node ReactImageManager method setAccessiblehttps://bit.ly/3dDuK5L

 * <p>InstancesofthisclassreceivepropertyupdatesfromJSvia@{linkUIManagerModule}.
* Subclassesmayuse {@link #updateShadowNode} topersistsomeoftheupdatedfieldsinthenode
* instancethatcorrespondstoaparticularviewtype.

Changelog

[Android] [Fixed] - adding setAccessible to ReactImageManager to allow screenreader announce Image accessibilityState of "disabled"

Test Plan

CLICK TO OPEN TESTS RESULTS

Enable audio to hear the screenreader

TEST SCENARIO

  • The user moves the screenreader focus to an image and the screenreader reads the Image accessibilityLabel "plain network image"

RESULT

  • The screenreader announces the accessibilityState disabled after reading the Image accessibilityLabel "plain network image"
<Imageaccessible={true}accessibilityLabel="plain network image"accessibilityState={{disabled: true}}source={fullImage}style={styles.base}/>
2021-03-26.18-27-36.mp4

@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 Mar 26, 2021
@fabOnReact

fabOnReact commented Mar 26, 2021

Copy link
Copy Markdown
ContributorAuthor

@analysis-bot

analysis-bot commented Mar 26, 2021

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

Base commit: da899c0

@amarlette

amarlette commented Mar 26, 2021

Copy link
Copy Markdown

Thank you for working on this issue @fabriziobertoglio1987! Excited to see your finished PR.

Got you on the project board and will assign a reviewer when you are out of drafts.

@fabOnReact
fabOnReact marked this pull request as ready for review April 6, 2021 16:33
@lunaleaps

lunaleaps commented Apr 22, 2021

Copy link
Copy Markdown
Contributor

@fabriziobertoglio1987is this ready for review? Ah sorry looks like it is, I'll move it to the right column then

@fabOnReact

Copy link
Copy Markdown
ContributorAuthor

sorry @lunaleaps, I did set this as ready for review. Currently I'm away from office, but I will be doing opensource again in 1-2 weeks. Thanks a lot. I wish you a good day

@kacieb

Copy link
Copy Markdown
Contributor

cc @blavalla for Android review!

},
},
{
title: 'Check if these properties are enabled',

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: It would be good to specify what properties we're testing here - in this case it's image focusability and disabled state.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@kacieb thanks for the code review. I experienced some issues with flipper after the last rebase and I published pr #31468 which includes solution to the runtime in the rn-tester app. I added the changes you required and tested them using the fix #31468. The functionality works like before. Thanks a lot 🙏

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

Android code looks good to me!

const mixedCheckboxImageSource = require('./mixed.png');
const {createRef} = require('react');
const fullImage = {
uri: 'https://www.facebook.com/ads/pics/successstories.png',

@lunaleapslunaleapsApr 26, 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.

Do you think we should reference a local asset here instead of fetching over network? I know there are some assets under js/assets/..

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

thanks a lot for the code review @lunaleaps. I added the changes required. More info in comment #31252 (comment) 🙏

@analysis-bot

analysis-bot commented Apr 27, 2021

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,864,454-122,281
androidhermesarmeabi-v7a8,386,258-107,725
androidhermesx869,321,886-117,136
androidhermesx86_649,264,852-115,931
androidjscarm64-v8a10,593,478-11,656
androidjscarmeabi-v7a10,098,377-7,030
androidjscx8610,612,415-11,729
androidjscx86_6411,195,638-12,174

Base commit: da899c0

image: {
width: 20,
height: 20,
width: 120,

@lunaleapslunaleapsMay 3, 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.

@fabriziobertoglio1987 Would this break other accessibility examples that use styles.image? Could create a separate styles.disabledImage? Did you want me to add during import? Otherwise I think this looks good!

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@lunaleaps thanks a lot for the codereview.. sorry for this. I just updated the code with commit f5e4a16 I remain available 🙏

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

Great! Thank you for your help! Importing this now :)

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@lunaleaps has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@lunaleaps merged this pull request in 333b46c.

@facebook-github-botfacebook-github-bot added the Merged This PR has been merged. label May 4, 2021
@amarlette

Copy link
Copy Markdown

Thank you @fabriziobertoglio1987 for another great contribution to a more accessible React Native! 😄

I'll highlight you with the same info from your other PR.

@fabOnReact

Copy link
Copy Markdown
ContributorAuthor

@lunaleaps thanks a lot for merging this pr. I will look for my next issues in the Accessibility Project 🙏
@amarlette thanks a lot for the mention 🙏

facebook-github-bot pushed a commit that referenced this pull request Feb 15, 2022
…ality when disabled (#33076)
Summary:
This issue fixes#30937fixes#30947fixes#30840 ([Test Case 7.1][7.1], [Test Case 7.3][7.3], [Test Case 7.5][7.5]) .
The issue is caused by:
1) The missing javascript logic on the `accessibilityState` in the Text component fabOnReact@6ab7ab3 (as previously implemented in [Button][20]).
2) The missing setter for prop `accessible` in `ReactTextAnchorViewManager` fabOnReact@17095c6 (More information in previous PR #31252)
Related PR #33070 PR callstack/react-native-slider#354
[20]: https://github.com/facebook/react-native/pull/31001/files#diff-4f225d043edf4cf5b8288285b6a957e2187fc0242f240bde396e41c4c25e4124R281-R289
## Changelog
[Android] [Fixed] - Text Component does not announce disabled and disables click functionality when disabled
Pull Request resolved: #33076
Test Plan:
[1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][1])
[2]. Text has `disabled` ([link][2])
[3]. Text has `accessibilityState={{disabled: true}}` ([link][3])
[4]. Text has `accessibilityState={{disabled:false}}` ([link][4])
[5]. Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][5])
[6]. Text has `accessibilityState={{disabled:true}}` and method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [b4cd8][10]) ([link][6])
7. Test Cases on the main branch
[7.1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][7.1])
[7.3] Text has `accessibilityState={{disabled: true}}` ([link][7.3])
[7.5] Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][7.5])
[7.6] Text has `onPress callback` and `accessibilityState={{disabled: true}}` ([link][7.6])
[7.7] Text has `accessibilityState={{disabled:true}}` and no method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [c4f98dd][11]) ([link][7.7])
[1]: fabOnReact/react-native-notes#1 (comment)
[2]: fabOnReact/react-native-notes#1 (comment)
[3]: fabOnReact/react-native-notes#1 (comment)
[4]: fabOnReact/react-native-notes#1 (comment)
[5]: fabOnReact/react-native-notes#1 (comment)
[6]: fabOnReact/react-native-notes#1 (comment)
[7.1]: fabOnReact/react-native-notes#1 (comment)
[7.3]: fabOnReact/react-native-notes#1 (comment)
[7.5]: fabOnReact/react-native-notes#1 (comment)
[7.6]: fabOnReact/react-native-notes#1 (comment)
[7.7]: fabOnReact/react-native-notes#1 (comment)
[10]: 17095c6
[11]: 6ab7ab3
Reviewed By: blavalla
Differential Revision: D34211793
Pulled By: ShikaSD
fbshipit-source-id: e153fb48c194f5884e30beb9172e66aca7ce1a41
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2023
…ality when disabled (react#33076)
Summary:
This issue fixesreact#30937fixesreact#30947fixesreact#30840 ([Test Case 7.1][7.1], [Test Case 7.3][7.3], [Test Case 7.5][7.5]) .
The issue is caused by:
1) The missing javascript logic on the `accessibilityState` in the Text component fabOnReact@6ab7ab3 (as previously implemented in [Button][20]).
2) The missing setter for prop `accessible` in `ReactTextAnchorViewManager` fabOnReact@17095c6 (More information in previous PR react#31252)
Related PR react#33070 PR callstack/react-native-slider#354
[20]: https://github.com/facebook/react-native/pull/31001/files#diff-4f225d043edf4cf5b8288285b6a957e2187fc0242f240bde396e41c4c25e4124R281-R289
[Android] [Fixed] - Text Component does not announce disabled and disables click functionality when disabled
Pull Request resolved: react#33076
Test Plan:
[1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][1])
[2]. Text has `disabled` ([link][2])
[3]. Text has `accessibilityState={{disabled: true}}` ([link][3])
[4]. Text has `accessibilityState={{disabled:false}}` ([link][4])
[5]. Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][5])
[6]. Text has `accessibilityState={{disabled:true}}` and method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [b4cd8][10]) ([link][6])
7. Test Cases on the main branch
[7.1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][7.1])
[7.3] Text has `accessibilityState={{disabled: true}}` ([link][7.3])
[7.5] Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][7.5])
[7.6] Text has `onPress callback` and `accessibilityState={{disabled: true}}` ([link][7.6])
[7.7] Text has `accessibilityState={{disabled:true}}` and no method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [c4f98dd][11]) ([link][7.7])
[1]: fabOnReact/react-native-notes#1 (comment)
[2]: fabOnReact/react-native-notes#1 (comment)
[3]: fabOnReact/react-native-notes#1 (comment)
[4]: fabOnReact/react-native-notes#1 (comment)
[5]: fabOnReact/react-native-notes#1 (comment)
[6]: fabOnReact/react-native-notes#1 (comment)
[7.1]: fabOnReact/react-native-notes#1 (comment)
[7.3]: fabOnReact/react-native-notes#1 (comment)
[7.5]: fabOnReact/react-native-notes#1 (comment)
[7.6]: fabOnReact/react-native-notes#1 (comment)
[7.7]: fabOnReact/react-native-notes#1 (comment)
[10]: react@17095c6
[11]: react@6ab7ab3
Reviewed By: blavalla
Differential Revision: D34211793
Pulled By: ShikaSD
fbshipit-source-id: e153fb48c194f5884e30beb9172e66aca7ce1a41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

ImageBackground does not announce "disabled" Image does not announce "disabled"

7 participants

@fabOnReact@analysis-bot@amarlette@lunaleaps@kacieb@facebook-github-bot@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

[Accessibility] Fix Image does not announce "disabled" - #31252

Closed
fabOnReact wants to merge 11 commits into
react:masterfrom
fabOnReact:fix/image-accessibility
Closed

[Accessibility] Fix Image does not announce "disabled"#31252
fabOnReact wants to merge 11 commits into
react:masterfrom
fabOnReact:fix/image-accessibility

Conversation

@fabOnReact

@fabOnReactfabOnReact commented Mar 26, 2021

Copy link
Copy Markdown
Contributor

Summary

This issue fixes#30935fixes#30936 screenreader does not announce Image disabled accessibilityState.

As stated in AOSP View.java, the framework will handle routine focus movement, views indicate their willingness to take focus through the isFocusable method https://bit.ly/3dCnyHb

* <p>The framework will handle routine focus movement in response to user input. This includes
* changing the focus as views are removed or hidden, or as new views become available. Views
* indicate their willingness to take focus through the {@link #isFocusable} method. To change
* whether a view can take focus, call {@link #setFocusable(boolean)}.

The property is updated through its shadow node ReactImageManager method setAccessiblehttps://bit.ly/3dDuK5L

 * <p>InstancesofthisclassreceivepropertyupdatesfromJSvia@{linkUIManagerModule}.
* Subclassesmayuse {@link #updateShadowNode} topersistsomeoftheupdatedfieldsinthenode
* instancethatcorrespondstoaparticularviewtype.

Changelog

[Android] [Fixed] - adding setAccessible to ReactImageManager to allow screenreader announce Image accessibilityState of "disabled"

Test Plan

CLICK TO OPEN TESTS RESULTS

Enable audio to hear the screenreader

TEST SCENARIO

  • The user moves the screenreader focus to an image and the screenreader reads the Image accessibilityLabel "plain network image"

RESULT

  • The screenreader announces the accessibilityState disabled after reading the Image accessibilityLabel "plain network image"
<Imageaccessible={true}accessibilityLabel="plain network image"accessibilityState={{disabled: true}}source={fullImage}style={styles.base}/>
2021-03-26.18-27-36.mp4

@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 Mar 26, 2021
@fabOnReact

fabOnReact commented Mar 26, 2021

Copy link
Copy Markdown
ContributorAuthor

@analysis-bot

analysis-bot commented Mar 26, 2021

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

Base commit: da899c0

@amarlette

amarlette commented Mar 26, 2021

Copy link
Copy Markdown

Thank you for working on this issue @fabriziobertoglio1987! Excited to see your finished PR.

Got you on the project board and will assign a reviewer when you are out of drafts.

@fabOnReact
fabOnReact marked this pull request as ready for review April 6, 2021 16:33
@lunaleaps

lunaleaps commented Apr 22, 2021

Copy link
Copy Markdown
Contributor

@fabriziobertoglio1987is this ready for review? Ah sorry looks like it is, I'll move it to the right column then

@fabOnReact

Copy link
Copy Markdown
ContributorAuthor

sorry @lunaleaps, I did set this as ready for review. Currently I'm away from office, but I will be doing opensource again in 1-2 weeks. Thanks a lot. I wish you a good day

@kacieb

Copy link
Copy Markdown
Contributor

cc @blavalla for Android review!

},
},
{
title: 'Check if these properties are enabled',

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: It would be good to specify what properties we're testing here - in this case it's image focusability and disabled state.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@kacieb thanks for the code review. I experienced some issues with flipper after the last rebase and I published pr #31468 which includes solution to the runtime in the rn-tester app. I added the changes you required and tested them using the fix #31468. The functionality works like before. Thanks a lot 🙏

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

Android code looks good to me!

const mixedCheckboxImageSource = require('./mixed.png');
const {createRef} = require('react');
const fullImage = {
uri: 'https://www.facebook.com/ads/pics/successstories.png',

@lunaleapslunaleapsApr 26, 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.

Do you think we should reference a local asset here instead of fetching over network? I know there are some assets under js/assets/..

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

thanks a lot for the code review @lunaleaps. I added the changes required. More info in comment #31252 (comment) 🙏

@analysis-bot

analysis-bot commented Apr 27, 2021

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,864,454-122,281
androidhermesarmeabi-v7a8,386,258-107,725
androidhermesx869,321,886-117,136
androidhermesx86_649,264,852-115,931
androidjscarm64-v8a10,593,478-11,656
androidjscarmeabi-v7a10,098,377-7,030
androidjscx8610,612,415-11,729
androidjscx86_6411,195,638-12,174

Base commit: da899c0

image: {
width: 20,
height: 20,
width: 120,

@lunaleapslunaleapsMay 3, 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.

@fabriziobertoglio1987 Would this break other accessibility examples that use styles.image? Could create a separate styles.disabledImage? Did you want me to add during import? Otherwise I think this looks good!

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@lunaleaps thanks a lot for the codereview.. sorry for this. I just updated the code with commit f5e4a16 I remain available 🙏

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

Great! Thank you for your help! Importing this now :)

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@lunaleaps has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@lunaleaps merged this pull request in 333b46c.

@facebook-github-botfacebook-github-bot added the Merged This PR has been merged. label May 4, 2021
@amarlette

Copy link
Copy Markdown

Thank you @fabriziobertoglio1987 for another great contribution to a more accessible React Native! 😄

I'll highlight you with the same info from your other PR.

@fabOnReact

Copy link
Copy Markdown
ContributorAuthor

@lunaleaps thanks a lot for merging this pr. I will look for my next issues in the Accessibility Project 🙏
@amarlette thanks a lot for the mention 🙏

facebook-github-bot pushed a commit that referenced this pull request Feb 15, 2022
…ality when disabled (#33076)
Summary:
This issue fixes#30937fixes#30947fixes#30840 ([Test Case 7.1][7.1], [Test Case 7.3][7.3], [Test Case 7.5][7.5]) .
The issue is caused by:
1) The missing javascript logic on the `accessibilityState` in the Text component fabOnReact@6ab7ab3 (as previously implemented in [Button][20]).
2) The missing setter for prop `accessible` in `ReactTextAnchorViewManager` fabOnReact@17095c6 (More information in previous PR #31252)
Related PR #33070 PR callstack/react-native-slider#354
[20]: https://github.com/facebook/react-native/pull/31001/files#diff-4f225d043edf4cf5b8288285b6a957e2187fc0242f240bde396e41c4c25e4124R281-R289
## Changelog
[Android] [Fixed] - Text Component does not announce disabled and disables click functionality when disabled
Pull Request resolved: #33076
Test Plan:
[1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][1])
[2]. Text has `disabled` ([link][2])
[3]. Text has `accessibilityState={{disabled: true}}` ([link][3])
[4]. Text has `accessibilityState={{disabled:false}}` ([link][4])
[5]. Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][5])
[6]. Text has `accessibilityState={{disabled:true}}` and method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [b4cd8][10]) ([link][6])
7. Test Cases on the main branch
[7.1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][7.1])
[7.3] Text has `accessibilityState={{disabled: true}}` ([link][7.3])
[7.5] Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][7.5])
[7.6] Text has `onPress callback` and `accessibilityState={{disabled: true}}` ([link][7.6])
[7.7] Text has `accessibilityState={{disabled:true}}` and no method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [c4f98dd][11]) ([link][7.7])
[1]: fabOnReact/react-native-notes#1 (comment)
[2]: fabOnReact/react-native-notes#1 (comment)
[3]: fabOnReact/react-native-notes#1 (comment)
[4]: fabOnReact/react-native-notes#1 (comment)
[5]: fabOnReact/react-native-notes#1 (comment)
[6]: fabOnReact/react-native-notes#1 (comment)
[7.1]: fabOnReact/react-native-notes#1 (comment)
[7.3]: fabOnReact/react-native-notes#1 (comment)
[7.5]: fabOnReact/react-native-notes#1 (comment)
[7.6]: fabOnReact/react-native-notes#1 (comment)
[7.7]: fabOnReact/react-native-notes#1 (comment)
[10]: 17095c6
[11]: 6ab7ab3
Reviewed By: blavalla
Differential Revision: D34211793
Pulled By: ShikaSD
fbshipit-source-id: e153fb48c194f5884e30beb9172e66aca7ce1a41
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2023
…ality when disabled (react#33076)
Summary:
This issue fixesreact#30937fixesreact#30947fixesreact#30840 ([Test Case 7.1][7.1], [Test Case 7.3][7.3], [Test Case 7.5][7.5]) .
The issue is caused by:
1) The missing javascript logic on the `accessibilityState` in the Text component fabOnReact@6ab7ab3 (as previously implemented in [Button][20]).
2) The missing setter for prop `accessible` in `ReactTextAnchorViewManager` fabOnReact@17095c6 (More information in previous PR react#31252)
Related PR react#33070 PR callstack/react-native-slider#354
[20]: https://github.com/facebook/react-native/pull/31001/files#diff-4f225d043edf4cf5b8288285b6a957e2187fc0242f240bde396e41c4c25e4124R281-R289
[Android] [Fixed] - Text Component does not announce disabled and disables click functionality when disabled
Pull Request resolved: react#33076
Test Plan:
[1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][1])
[2]. Text has `disabled` ([link][2])
[3]. Text has `accessibilityState={{disabled: true}}` ([link][3])
[4]. Text has `accessibilityState={{disabled:false}}` ([link][4])
[5]. Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][5])
[6]. Text has `accessibilityState={{disabled:true}}` and method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [b4cd8][10]) ([link][6])
7. Test Cases on the main branch
[7.1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][7.1])
[7.3] Text has `accessibilityState={{disabled: true}}` ([link][7.3])
[7.5] Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][7.5])
[7.6] Text has `onPress callback` and `accessibilityState={{disabled: true}}` ([link][7.6])
[7.7] Text has `accessibilityState={{disabled:true}}` and no method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [c4f98dd][11]) ([link][7.7])
[1]: fabOnReact/react-native-notes#1 (comment)
[2]: fabOnReact/react-native-notes#1 (comment)
[3]: fabOnReact/react-native-notes#1 (comment)
[4]: fabOnReact/react-native-notes#1 (comment)
[5]: fabOnReact/react-native-notes#1 (comment)
[6]: fabOnReact/react-native-notes#1 (comment)
[7.1]: fabOnReact/react-native-notes#1 (comment)
[7.3]: fabOnReact/react-native-notes#1 (comment)
[7.5]: fabOnReact/react-native-notes#1 (comment)
[7.6]: fabOnReact/react-native-notes#1 (comment)
[7.7]: fabOnReact/react-native-notes#1 (comment)
[10]: react@17095c6
[11]: react@6ab7ab3
Reviewed By: blavalla
Differential Revision: D34211793
Pulled By: ShikaSD
fbshipit-source-id: e153fb48c194f5884e30beb9172e66aca7ce1a41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

ImageBackground does not announce "disabled" Image does not announce "disabled"

7 participants

@fabOnReact@analysis-bot@amarlette@lunaleaps@kacieb@facebook-github-bot@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

[Accessibility] Fix Image does not announce "disabled" - #31252

Closed
fabOnReact wants to merge 11 commits into
react:masterfrom
fabOnReact:fix/image-accessibility
Closed

[Accessibility] Fix Image does not announce "disabled"#31252
fabOnReact wants to merge 11 commits into
react:masterfrom
fabOnReact:fix/image-accessibility

Conversation

@fabOnReact

@fabOnReactfabOnReact commented Mar 26, 2021

Copy link
Copy Markdown
Contributor

Summary

This issue fixes#30935fixes#30936 screenreader does not announce Image disabled accessibilityState.

As stated in AOSP View.java, the framework will handle routine focus movement, views indicate their willingness to take focus through the isFocusable method https://bit.ly/3dCnyHb

* <p>The framework will handle routine focus movement in response to user input. This includes
* changing the focus as views are removed or hidden, or as new views become available. Views
* indicate their willingness to take focus through the {@link #isFocusable} method. To change
* whether a view can take focus, call {@link #setFocusable(boolean)}.

The property is updated through its shadow node ReactImageManager method setAccessiblehttps://bit.ly/3dDuK5L

 * <p>InstancesofthisclassreceivepropertyupdatesfromJSvia@{linkUIManagerModule}.
* Subclassesmayuse {@link #updateShadowNode} topersistsomeoftheupdatedfieldsinthenode
* instancethatcorrespondstoaparticularviewtype.

Changelog

[Android] [Fixed] - adding setAccessible to ReactImageManager to allow screenreader announce Image accessibilityState of "disabled"

Test Plan

CLICK TO OPEN TESTS RESULTS

Enable audio to hear the screenreader

TEST SCENARIO

  • The user moves the screenreader focus to an image and the screenreader reads the Image accessibilityLabel "plain network image"

RESULT

  • The screenreader announces the accessibilityState disabled after reading the Image accessibilityLabel "plain network image"
<Imageaccessible={true}accessibilityLabel="plain network image"accessibilityState={{disabled: true}}source={fullImage}style={styles.base}/>
2021-03-26.18-27-36.mp4

@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 Mar 26, 2021
@fabOnReact

fabOnReact commented Mar 26, 2021

Copy link
Copy Markdown
ContributorAuthor

@analysis-bot

analysis-bot commented Mar 26, 2021

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

Base commit: da899c0

@amarlette

amarlette commented Mar 26, 2021

Copy link
Copy Markdown

Thank you for working on this issue @fabriziobertoglio1987! Excited to see your finished PR.

Got you on the project board and will assign a reviewer when you are out of drafts.

@fabOnReact
fabOnReact marked this pull request as ready for review April 6, 2021 16:33
@lunaleaps

lunaleaps commented Apr 22, 2021

Copy link
Copy Markdown
Contributor

@fabriziobertoglio1987is this ready for review? Ah sorry looks like it is, I'll move it to the right column then

@fabOnReact

Copy link
Copy Markdown
ContributorAuthor

sorry @lunaleaps, I did set this as ready for review. Currently I'm away from office, but I will be doing opensource again in 1-2 weeks. Thanks a lot. I wish you a good day

@kacieb

Copy link
Copy Markdown
Contributor

cc @blavalla for Android review!

},
},
{
title: 'Check if these properties are enabled',

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: It would be good to specify what properties we're testing here - in this case it's image focusability and disabled state.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@kacieb thanks for the code review. I experienced some issues with flipper after the last rebase and I published pr #31468 which includes solution to the runtime in the rn-tester app. I added the changes you required and tested them using the fix #31468. The functionality works like before. Thanks a lot 🙏

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

Android code looks good to me!

const mixedCheckboxImageSource = require('./mixed.png');
const {createRef} = require('react');
const fullImage = {
uri: 'https://www.facebook.com/ads/pics/successstories.png',

@lunaleapslunaleapsApr 26, 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.

Do you think we should reference a local asset here instead of fetching over network? I know there are some assets under js/assets/..

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

thanks a lot for the code review @lunaleaps. I added the changes required. More info in comment #31252 (comment) 🙏

@analysis-bot

analysis-bot commented Apr 27, 2021

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,864,454-122,281
androidhermesarmeabi-v7a8,386,258-107,725
androidhermesx869,321,886-117,136
androidhermesx86_649,264,852-115,931
androidjscarm64-v8a10,593,478-11,656
androidjscarmeabi-v7a10,098,377-7,030
androidjscx8610,612,415-11,729
androidjscx86_6411,195,638-12,174

Base commit: da899c0

image: {
width: 20,
height: 20,
width: 120,

@lunaleapslunaleapsMay 3, 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.

@fabriziobertoglio1987 Would this break other accessibility examples that use styles.image? Could create a separate styles.disabledImage? Did you want me to add during import? Otherwise I think this looks good!

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@lunaleaps thanks a lot for the codereview.. sorry for this. I just updated the code with commit f5e4a16 I remain available 🙏

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

Great! Thank you for your help! Importing this now :)

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@lunaleaps has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@lunaleaps merged this pull request in 333b46c.

@facebook-github-botfacebook-github-bot added the Merged This PR has been merged. label May 4, 2021
@amarlette

Copy link
Copy Markdown

Thank you @fabriziobertoglio1987 for another great contribution to a more accessible React Native! 😄

I'll highlight you with the same info from your other PR.

@fabOnReact

Copy link
Copy Markdown
ContributorAuthor

@lunaleaps thanks a lot for merging this pr. I will look for my next issues in the Accessibility Project 🙏
@amarlette thanks a lot for the mention 🙏

facebook-github-bot pushed a commit that referenced this pull request Feb 15, 2022
…ality when disabled (#33076)
Summary:
This issue fixes#30937fixes#30947fixes#30840 ([Test Case 7.1][7.1], [Test Case 7.3][7.3], [Test Case 7.5][7.5]) .
The issue is caused by:
1) The missing javascript logic on the `accessibilityState` in the Text component fabOnReact@6ab7ab3 (as previously implemented in [Button][20]).
2) The missing setter for prop `accessible` in `ReactTextAnchorViewManager` fabOnReact@17095c6 (More information in previous PR #31252)
Related PR #33070 PR callstack/react-native-slider#354
[20]: https://github.com/facebook/react-native/pull/31001/files#diff-4f225d043edf4cf5b8288285b6a957e2187fc0242f240bde396e41c4c25e4124R281-R289
## Changelog
[Android] [Fixed] - Text Component does not announce disabled and disables click functionality when disabled
Pull Request resolved: #33076
Test Plan:
[1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][1])
[2]. Text has `disabled` ([link][2])
[3]. Text has `accessibilityState={{disabled: true}}` ([link][3])
[4]. Text has `accessibilityState={{disabled:false}}` ([link][4])
[5]. Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][5])
[6]. Text has `accessibilityState={{disabled:true}}` and method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [b4cd8][10]) ([link][6])
7. Test Cases on the main branch
[7.1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][7.1])
[7.3] Text has `accessibilityState={{disabled: true}}` ([link][7.3])
[7.5] Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][7.5])
[7.6] Text has `onPress callback` and `accessibilityState={{disabled: true}}` ([link][7.6])
[7.7] Text has `accessibilityState={{disabled:true}}` and no method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [c4f98dd][11]) ([link][7.7])
[1]: fabOnReact/react-native-notes#1 (comment)
[2]: fabOnReact/react-native-notes#1 (comment)
[3]: fabOnReact/react-native-notes#1 (comment)
[4]: fabOnReact/react-native-notes#1 (comment)
[5]: fabOnReact/react-native-notes#1 (comment)
[6]: fabOnReact/react-native-notes#1 (comment)
[7.1]: fabOnReact/react-native-notes#1 (comment)
[7.3]: fabOnReact/react-native-notes#1 (comment)
[7.5]: fabOnReact/react-native-notes#1 (comment)
[7.6]: fabOnReact/react-native-notes#1 (comment)
[7.7]: fabOnReact/react-native-notes#1 (comment)
[10]: 17095c6
[11]: 6ab7ab3
Reviewed By: blavalla
Differential Revision: D34211793
Pulled By: ShikaSD
fbshipit-source-id: e153fb48c194f5884e30beb9172e66aca7ce1a41
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2023
…ality when disabled (react#33076)
Summary:
This issue fixesreact#30937fixesreact#30947fixesreact#30840 ([Test Case 7.1][7.1], [Test Case 7.3][7.3], [Test Case 7.5][7.5]) .
The issue is caused by:
1) The missing javascript logic on the `accessibilityState` in the Text component fabOnReact@6ab7ab3 (as previously implemented in [Button][20]).
2) The missing setter for prop `accessible` in `ReactTextAnchorViewManager` fabOnReact@17095c6 (More information in previous PR react#31252)
Related PR react#33070 PR callstack/react-native-slider#354
[20]: https://github.com/facebook/react-native/pull/31001/files#diff-4f225d043edf4cf5b8288285b6a957e2187fc0242f240bde396e41c4c25e4124R281-R289
[Android] [Fixed] - Text Component does not announce disabled and disables click functionality when disabled
Pull Request resolved: react#33076
Test Plan:
[1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][1])
[2]. Text has `disabled` ([link][2])
[3]. Text has `accessibilityState={{disabled: true}}` ([link][3])
[4]. Text has `accessibilityState={{disabled:false}}` ([link][4])
[5]. Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][5])
[6]. Text has `accessibilityState={{disabled:true}}` and method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [b4cd8][10]) ([link][6])
7. Test Cases on the main branch
[7.1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][7.1])
[7.3] Text has `accessibilityState={{disabled: true}}` ([link][7.3])
[7.5] Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][7.5])
[7.6] Text has `onPress callback` and `accessibilityState={{disabled: true}}` ([link][7.6])
[7.7] Text has `accessibilityState={{disabled:true}}` and no method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [c4f98dd][11]) ([link][7.7])
[1]: fabOnReact/react-native-notes#1 (comment)
[2]: fabOnReact/react-native-notes#1 (comment)
[3]: fabOnReact/react-native-notes#1 (comment)
[4]: fabOnReact/react-native-notes#1 (comment)
[5]: fabOnReact/react-native-notes#1 (comment)
[6]: fabOnReact/react-native-notes#1 (comment)
[7.1]: fabOnReact/react-native-notes#1 (comment)
[7.3]: fabOnReact/react-native-notes#1 (comment)
[7.5]: fabOnReact/react-native-notes#1 (comment)
[7.6]: fabOnReact/react-native-notes#1 (comment)
[7.7]: fabOnReact/react-native-notes#1 (comment)
[10]: react@17095c6
[11]: react@6ab7ab3
Reviewed By: blavalla
Differential Revision: D34211793
Pulled By: ShikaSD
fbshipit-source-id: e153fb48c194f5884e30beb9172e66aca7ce1a41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

ImageBackground does not announce "disabled" Image does not announce "disabled"

7 participants

@fabOnReact@analysis-bot@amarlette@lunaleaps@kacieb@facebook-github-bot@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

[Accessibility] Fix Image does not announce "disabled" - #31252

Closed
fabOnReact wants to merge 11 commits into
react:masterfrom
fabOnReact:fix/image-accessibility
Closed

[Accessibility] Fix Image does not announce "disabled"#31252
fabOnReact wants to merge 11 commits into
react:masterfrom
fabOnReact:fix/image-accessibility

Conversation

@fabOnReact

@fabOnReactfabOnReact commented Mar 26, 2021

Copy link
Copy Markdown
Contributor

Summary

This issue fixes#30935fixes#30936 screenreader does not announce Image disabled accessibilityState.

As stated in AOSP View.java, the framework will handle routine focus movement, views indicate their willingness to take focus through the isFocusable method https://bit.ly/3dCnyHb

* <p>The framework will handle routine focus movement in response to user input. This includes
* changing the focus as views are removed or hidden, or as new views become available. Views
* indicate their willingness to take focus through the {@link #isFocusable} method. To change
* whether a view can take focus, call {@link #setFocusable(boolean)}.

The property is updated through its shadow node ReactImageManager method setAccessiblehttps://bit.ly/3dDuK5L

 * <p>InstancesofthisclassreceivepropertyupdatesfromJSvia@{linkUIManagerModule}.
* Subclassesmayuse {@link #updateShadowNode} topersistsomeoftheupdatedfieldsinthenode
* instancethatcorrespondstoaparticularviewtype.

Changelog

[Android] [Fixed] - adding setAccessible to ReactImageManager to allow screenreader announce Image accessibilityState of "disabled"

Test Plan

CLICK TO OPEN TESTS RESULTS

Enable audio to hear the screenreader

TEST SCENARIO

  • The user moves the screenreader focus to an image and the screenreader reads the Image accessibilityLabel "plain network image"

RESULT

  • The screenreader announces the accessibilityState disabled after reading the Image accessibilityLabel "plain network image"
<Imageaccessible={true}accessibilityLabel="plain network image"accessibilityState={{disabled: true}}source={fullImage}style={styles.base}/>
2021-03-26.18-27-36.mp4

@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 Mar 26, 2021
@fabOnReact

fabOnReact commented Mar 26, 2021

Copy link
Copy Markdown
ContributorAuthor

@analysis-bot

analysis-bot commented Mar 26, 2021

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

Base commit: da899c0

@amarlette

amarlette commented Mar 26, 2021

Copy link
Copy Markdown

Thank you for working on this issue @fabriziobertoglio1987! Excited to see your finished PR.

Got you on the project board and will assign a reviewer when you are out of drafts.

@fabOnReact
fabOnReact marked this pull request as ready for review April 6, 2021 16:33
@lunaleaps

lunaleaps commented Apr 22, 2021

Copy link
Copy Markdown
Contributor

@fabriziobertoglio1987is this ready for review? Ah sorry looks like it is, I'll move it to the right column then

@fabOnReact

Copy link
Copy Markdown
ContributorAuthor

sorry @lunaleaps, I did set this as ready for review. Currently I'm away from office, but I will be doing opensource again in 1-2 weeks. Thanks a lot. I wish you a good day

@kacieb

Copy link
Copy Markdown
Contributor

cc @blavalla for Android review!

},
},
{
title: 'Check if these properties are enabled',

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: It would be good to specify what properties we're testing here - in this case it's image focusability and disabled state.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@kacieb thanks for the code review. I experienced some issues with flipper after the last rebase and I published pr #31468 which includes solution to the runtime in the rn-tester app. I added the changes you required and tested them using the fix #31468. The functionality works like before. Thanks a lot 🙏

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

Android code looks good to me!

const mixedCheckboxImageSource = require('./mixed.png');
const {createRef} = require('react');
const fullImage = {
uri: 'https://www.facebook.com/ads/pics/successstories.png',

@lunaleapslunaleapsApr 26, 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.

Do you think we should reference a local asset here instead of fetching over network? I know there are some assets under js/assets/..

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

thanks a lot for the code review @lunaleaps. I added the changes required. More info in comment #31252 (comment) 🙏

@analysis-bot

analysis-bot commented Apr 27, 2021

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,864,454-122,281
androidhermesarmeabi-v7a8,386,258-107,725
androidhermesx869,321,886-117,136
androidhermesx86_649,264,852-115,931
androidjscarm64-v8a10,593,478-11,656
androidjscarmeabi-v7a10,098,377-7,030
androidjscx8610,612,415-11,729
androidjscx86_6411,195,638-12,174

Base commit: da899c0

image: {
width: 20,
height: 20,
width: 120,

@lunaleapslunaleapsMay 3, 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.

@fabriziobertoglio1987 Would this break other accessibility examples that use styles.image? Could create a separate styles.disabledImage? Did you want me to add during import? Otherwise I think this looks good!

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@lunaleaps thanks a lot for the codereview.. sorry for this. I just updated the code with commit f5e4a16 I remain available 🙏

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

Great! Thank you for your help! Importing this now :)

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@lunaleaps has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@lunaleaps merged this pull request in 333b46c.

@facebook-github-botfacebook-github-bot added the Merged This PR has been merged. label May 4, 2021
@amarlette

Copy link
Copy Markdown

Thank you @fabriziobertoglio1987 for another great contribution to a more accessible React Native! 😄

I'll highlight you with the same info from your other PR.

@fabOnReact

Copy link
Copy Markdown
ContributorAuthor

@lunaleaps thanks a lot for merging this pr. I will look for my next issues in the Accessibility Project 🙏
@amarlette thanks a lot for the mention 🙏

facebook-github-bot pushed a commit that referenced this pull request Feb 15, 2022
…ality when disabled (#33076)
Summary:
This issue fixes#30937fixes#30947fixes#30840 ([Test Case 7.1][7.1], [Test Case 7.3][7.3], [Test Case 7.5][7.5]) .
The issue is caused by:
1) The missing javascript logic on the `accessibilityState` in the Text component fabOnReact@6ab7ab3 (as previously implemented in [Button][20]).
2) The missing setter for prop `accessible` in `ReactTextAnchorViewManager` fabOnReact@17095c6 (More information in previous PR #31252)
Related PR #33070 PR callstack/react-native-slider#354
[20]: https://github.com/facebook/react-native/pull/31001/files#diff-4f225d043edf4cf5b8288285b6a957e2187fc0242f240bde396e41c4c25e4124R281-R289
## Changelog
[Android] [Fixed] - Text Component does not announce disabled and disables click functionality when disabled
Pull Request resolved: #33076
Test Plan:
[1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][1])
[2]. Text has `disabled` ([link][2])
[3]. Text has `accessibilityState={{disabled: true}}` ([link][3])
[4]. Text has `accessibilityState={{disabled:false}}` ([link][4])
[5]. Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][5])
[6]. Text has `accessibilityState={{disabled:true}}` and method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [b4cd8][10]) ([link][6])
7. Test Cases on the main branch
[7.1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][7.1])
[7.3] Text has `accessibilityState={{disabled: true}}` ([link][7.3])
[7.5] Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][7.5])
[7.6] Text has `onPress callback` and `accessibilityState={{disabled: true}}` ([link][7.6])
[7.7] Text has `accessibilityState={{disabled:true}}` and no method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [c4f98dd][11]) ([link][7.7])
[1]: fabOnReact/react-native-notes#1 (comment)
[2]: fabOnReact/react-native-notes#1 (comment)
[3]: fabOnReact/react-native-notes#1 (comment)
[4]: fabOnReact/react-native-notes#1 (comment)
[5]: fabOnReact/react-native-notes#1 (comment)
[6]: fabOnReact/react-native-notes#1 (comment)
[7.1]: fabOnReact/react-native-notes#1 (comment)
[7.3]: fabOnReact/react-native-notes#1 (comment)
[7.5]: fabOnReact/react-native-notes#1 (comment)
[7.6]: fabOnReact/react-native-notes#1 (comment)
[7.7]: fabOnReact/react-native-notes#1 (comment)
[10]: 17095c6
[11]: 6ab7ab3
Reviewed By: blavalla
Differential Revision: D34211793
Pulled By: ShikaSD
fbshipit-source-id: e153fb48c194f5884e30beb9172e66aca7ce1a41
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2023
…ality when disabled (react#33076)
Summary:
This issue fixesreact#30937fixesreact#30947fixesreact#30840 ([Test Case 7.1][7.1], [Test Case 7.3][7.3], [Test Case 7.5][7.5]) .
The issue is caused by:
1) The missing javascript logic on the `accessibilityState` in the Text component fabOnReact@6ab7ab3 (as previously implemented in [Button][20]).
2) The missing setter for prop `accessible` in `ReactTextAnchorViewManager` fabOnReact@17095c6 (More information in previous PR react#31252)
Related PR react#33070 PR callstack/react-native-slider#354
[20]: https://github.com/facebook/react-native/pull/31001/files#diff-4f225d043edf4cf5b8288285b6a957e2187fc0242f240bde396e41c4c25e4124R281-R289
[Android] [Fixed] - Text Component does not announce disabled and disables click functionality when disabled
Pull Request resolved: react#33076
Test Plan:
[1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][1])
[2]. Text has `disabled` ([link][2])
[3]. Text has `accessibilityState={{disabled: true}}` ([link][3])
[4]. Text has `accessibilityState={{disabled:false}}` ([link][4])
[5]. Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][5])
[6]. Text has `accessibilityState={{disabled:true}}` and method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [b4cd8][10]) ([link][6])
7. Test Cases on the main branch
[7.1]. Text has `disabled` and `accessibilityState={{disabled: false}}` ([link][7.1])
[7.3] Text has `accessibilityState={{disabled: true}}` ([link][7.3])
[7.5] Text has `disabled={false}` and `accessibilityState={{disabled:true}}` ([link][7.5])
[7.6] Text has `onPress callback` and `accessibilityState={{disabled: true}}` ([link][7.6])
[7.7] Text has `accessibilityState={{disabled:true}}` and no method `setAccessible` in `ReactTextAnchorViewManager` (tested on commit [c4f98dd][11]) ([link][7.7])
[1]: fabOnReact/react-native-notes#1 (comment)
[2]: fabOnReact/react-native-notes#1 (comment)
[3]: fabOnReact/react-native-notes#1 (comment)
[4]: fabOnReact/react-native-notes#1 (comment)
[5]: fabOnReact/react-native-notes#1 (comment)
[6]: fabOnReact/react-native-notes#1 (comment)
[7.1]: fabOnReact/react-native-notes#1 (comment)
[7.3]: fabOnReact/react-native-notes#1 (comment)
[7.5]: fabOnReact/react-native-notes#1 (comment)
[7.6]: fabOnReact/react-native-notes#1 (comment)
[7.7]: fabOnReact/react-native-notes#1 (comment)
[10]: react@17095c6
[11]: react@6ab7ab3
Reviewed By: blavalla
Differential Revision: D34211793
Pulled By: ShikaSD
fbshipit-source-id: e153fb48c194f5884e30beb9172e66aca7ce1a41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

ImageBackground does not announce "disabled" Image does not announce "disabled"

7 participants

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