Additional Accessibility Roles and States #24095
Conversation
facebook-github-bot
added
CLA Signed
|
RNTester/js/AccessibilityExample.js
Outdated
| <View | ||
| accessibilityLabel="element 13" | ||
| accessibilityRole="switch" | ||
| accessibilityStates={['checked']} |
There was a problem hiding this comment.
FWIW on Web this is how ARIA switches are meant to be created, using checked/unchecked states - https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/Switch_role#Associated_ARIA_roles_states_and_properties. Is it necessary for React Native to have separate on/off states?
There was a problem hiding this comment.
Probably not, but semanticly, it's weird to say a switch is "checked". When describing the state of a switch verbally, we would usually say it is "on" or "off".
There was a problem hiding this comment.
We end up using 'on/off' quite a bit on native, especially for slide toggles.
There was a problem hiding this comment.
The question I have is whether on/off and checked/unchecked are redundant. If so, I think we don't need both. And the example here that uses a switch with checked/unchecked highlights the point of confusion
There was a problem hiding this comment.
@necolas the restriction, as I understand it, is on iOS where the trait value is simply a string, not a token. So the announcement of state won't be automatically varied with the semantic role of the control. There is an argument to be made that once we introduce "on/off", we can't take it back. But it makes sense to me in the content of a switch role in contrast to a checkbox role.
| | 'alert' | ||
| | 'checkbox' | ||
| | 'combobox' | ||
| | 'editabletext' |
There was a problem hiding this comment.
The web ARIA equivalent is textbox. Introducing this role might require other states to be added, such as multiline and readOnly, so people can build accessible text inputs
There was a problem hiding this comment.
Good point. Would you prefer to remove this role for now and add "editabletext" or "textbox" as a separate PR?
There was a problem hiding this comment.
Feel free to move it to a separate PR if you prefer
| 'alert', | ||
| 'checkbox', | ||
| 'combobox', | ||
| 'editabletext', |
There was a problem hiding this comment.
Same issue as the textbox comment above.
| } | ||
| } | ||
|
|
||
| private void updateViewContentDescription(@Nonnull T view) { |
There was a problem hiding this comment.
Background: Chris McMeeking, Mobile Architect Deque Systems, mobile accessibility expert, member W3C Mobile Accessibility Task Force, Accessibility researcher with U of M Mott's Children's Hospital, etc, etc.
I love everything about this pull request except this function. Appending things to the content description is icky. Akin to Off Screen text in the web world. I'd rather see a feature request sent to Google. Programmatic association with Roles and States is crazy important. It allows technologies like Switch Control, Braille Boards, Joysticks, etc, to respond accordingly. Content Description hacks only fix things for TalkBack. If a role or state does not exist programmatically that is the Operating System's problem. Adding it to the Content Description only serves to complicate an already complicated ecosystem.
Not only that but it adds minimal value. The value in roles and states is consistency of behavior, this is why programmatic association is so important. Very seldom does the context of a control not hint at its purpose. Appending information to a content description is adding context that is likely gathered from the surrounding controls. It's a form of hand holding that research shows has minimal value to AT users. The value is in consistency of the communication of a thing and in how it is interacted with by an assisstive technology. This doesn't exist when you hack things onto a content description.
In fact, all React Apps behaving one way and all native apps behaving in others would serve the reverse purpose. Every other app in the Android Store would be made less Accessible, because it would fail to communicate these things. For a platform as powerful as react native, this could actually cause significant damage to the collective Accessibility of the Android platform.
A solution like this will fragment the Android Accessibility Ecosystem even further than it already is. IMHO an effort like this should stick within the Operating Systems capabilities, and if those capabilities lead us to the conclusion that the platform is not Accessible, we should unitedly and vocally raise a fuss about this. But, appending a bunch of crud into Off Screen text and pretend like that fixes things, is a bad idea. We'd be better to embrace Android's features and refrain from fragmenting the Operating System even more than it already is.
There was a problem hiding this comment.
I respectfully disagree. We're dealing with more than simply Android support with this pull request-- we're thinking about iOS and Web as well. And we can't afford to "dumb down" a toolkit like React Native to the least common denominator. If we do that, then we have no checkbox role, no checked state, etc. since iOS doesn't natively support those.
I wholeheartedly agree that this solution doesn't work well in braille, or for single switch access. But IMHO, improvements in speech access and support are better than no improvements at all.
The way this is architected, Javascript components expose the states and roles semantically, so if someday the situation on Android or iOS changes, we can re-implement the platform support layer to use native roles and states, which I think is the right approach.
There was a problem hiding this comment.
You can have roles and traits that you don't communicate. Off-Screen text is a known non-solution in the Web Sphere, and that is all that this is.
Having a trait in react native, and admitting that the Android operating system doesn't respect it, seems perfectly reasonable to me.
There was a problem hiding this comment.
But IMHO, improvements in speech access and support are better than no improvements at all.
This is assuming that speaking roles is valuable. It frequently isn't. It is the other, programmatic behavioral responses to traits that matter much more.
This is a sympathetic, but not empathic thought process.
There was a problem hiding this comment.
Quote from Android Accessibility Best Practices Page:
Many accessibility services, such as TalkBack and BrailleBack, automatically announce an element's type after announcing its label, so you shouldn't include element types in your labels. For example, "submit" is a good label for a Button object, but "submitButton" isn't a good label.
If Android's best practices are wrong, we should ask them to change them. But a platform like React Native deciding that those best practices are wrong... bad plan.
There was a problem hiding this comment.
@chriscm2006, I think there may be some confusion here. This method only adds concatenated text for hints (which Android only supports if they are associated with an action, not just a generic string like iOS or web), and 5 different states (defined in the sStateDescription map). These states (on, off, expanded, collapsed, and busy) are not ones that allow users to change navigation modes to easily find. Roles and States that can be semantically expressed in Android are handled correctly in the accessibility delegate here:
I understand your concern though, as this definitely isn't a best practice approach when it can be avoided. But if the choice was between not telling a user something is expanded (which is usually obvious visually), versus the user not having control to disable this announcement (as they can do by disabling state information), I'd err on the side of providing too much information over not enough. While this could be annoying for power users, the vast majority of users are not adjusting their settings from the default, and would simply be missing this state information which could be important to the understanding of the UI.
All that being said, I do think that on/off could be removed from this list, as semantically it used in the same way as "checked" and "unchecked". In fact, when Talkback sees an element with the "Switch" role (the only one I know to use "on" and "off" as its labels), it uses the value of the checked property in the AccessibilityNodeInfo to append "on" or "off" to its readback. Logic in the talkback compositor here:
If we want to keep the ReactNative API to use the state of "on" and "off", we can do so, but we should map this to setting the checkable and checked properties in the AccessibilityDelegate rather than appending them to the content description here.
There was a problem hiding this comment.
I think if I saw some of the items handled in the semantic way when it can be, I would feel better. It's one thing to create things that aren't there... I'll go quiet on that point. It's another to ignore Operating System support that is there. The two examples that I can't get over.
- CollectionItemInfo for menu items.
- Ensuring that everything that is a control is either "mocked" as such, or throws exceptions or warnings when it isn't used properly.
There was a problem hiding this comment.
Given this lack of clear purpose, such a role would be used very arbitrarily across the react-native ecosystem
The React Native ecosystem is not limited to mobile and includes desktop apps
I'm not asking to drop those roles, I'm asking to ignore or mark up those roles properly for Android. Not all Reac Native apps need to behave the same way. In fact they shouldn't. They should behave the way the platform would normally behave.
There was a problem hiding this comment.
To flip this argument around-- iOS doesn't have a checkbox trait or a checked trait. It's essential to know that something is a checkbox, and whether it is checked or unchedked. Android supports both a checkbox role convention (className == "android.widget.CheckBox" and the isChecked() method). So does that mean we shouldn't surface the checkbox role and associated checked/unchecked states on iOS because it doesn't support them?
We have used Android conventions where they exist to annotate the roles and states they represent, so setting class name as you suggest, as well as setting node metadata such as isChecked().
Happy to remove on/off in favor of checked/unchedked.
You can reasonably argue that menu and menuitem are not useful as roles on mobile, but that's a different argument than saying the way in which we support them is wrong. Using CollectionInfo and CollectionItemInfo only surfaces to the user that this is a list/grid, not that it's a menu. Knowing whether something is a menu or a standard list could be important IMHO.
We're using the roleDescription paradigm to annotate roles that don't have Android native analogs (equivalent Java class names). This is a paradigm introduced by Google for Chromium accessibility on Android where everything has a class name of "android.view.View".
Only states without AccessibilityNodeInfo analogs are concatenated to the content description, where we deemed such state information was useful.
I'm in favor of using Android-specific markup where it exists. If we've missed some standard markup, let us know and we'll fix it.
In summary:
- roles never get appended to the content description, they either cause the AccessibilityNodeInfo's class name to be set to the closest native analog, or a string description of the role to be placed in the roleDescription extra.
- States set the AccessibilityNodeInfo property where it exists, otherwise a string description of the state is appended to the content description.
There was a problem hiding this comment.
Checked/unchecked in favor of on/off I think is a good idea. Though you could also look into:
setTextOn(), setTextOff()
For scenarios when you want to be more expressive about the state of a given toggle-able trait.
I'd have to look closer at the roles where you add states, but the best Android Markup for times when you need to convey State AND Name, is to associate the stateful control and a TextView with a labelFor attribute.
This may or may not be a solution in this case, I feel like that may turn this effort into something larger than it is. But it does take care of the equivalency of visual and spoken data issue.
This should result in Android reading out something like:
STATE, for, Text of the TextView
React/Views/RCTView.m
Outdated
| @"radiogroup" : @"radio group", | ||
| @"scrollbar" : @"scroll bar", | ||
| @"spinbutton" : @"spin button", | ||
| @"switch" : @"switch", |
There was a problem hiding this comment.
First of all, thanks for working on it.
- What do you think of using
UISwitch().accessibilityTraitsfor it? It would bring the default and localizable trait forswitchrole, andon/offstate.
I was able to make it work (in my local react-native fork), based on this example https://github.com/jldailey13/24-Accessibility-Article-Examples/blob/master/24A11y%20Article/Name%20Examples/AccessibleSwitchView.swift
- Switch component will benefit from this role 👍
There was a problem hiding this comment.
Interestingly, this does seem correct, but we can't get it to work in our local trees. Switches are still spoken as buttons with a value of 1 or 0. Not sure what we're doing wrong...
There was a problem hiding this comment.
Hey @jldailey13, take a look @marcmulcahy has an interesting issue here! Is this related to the class name of the control?
There was a problem hiding this comment.
I was able to get the right trait by instantiating a new UISwitch
@synthesize a11ySwitch = _a11ySwitch;
....
_a11ySwitch = [UISwitch new];
view.reactAccessibilityElement.accessibilityTraits = [_a11ySwitch accessibilityTraits];
view.reactAccessibilityElement.accessibilityValue = isOn ? @"1" : @"0";
There was a problem hiding this comment.
It looks like UISwitch is simply setting traits to Button. We tried something similar, but VoiceOver doesn't present the switch as a switch-- it says "button 1". Did you spin this up with VoiceOver and confirm that it properly describes the switch?
There was a problem hiding this comment.
I looked a bit closer at this. @elucaswork, how exactly did you test? I know you used VoiceOver-- which components did you play with?
The React Native switch component already behaved itself, since it uses a UISwitch underneath, so if that's what you used to test, you wouldn't see any change. It used to work before this change, and still does.
To validate the actual role, we created generic views (UIView underneath) and set the role to "switch". after applying what I think is a change similar to yours, VoiceOver says "label 1 button", which isn't what we want.
What I fear is going on is that VoiceOver is special case handling components based on their class names, not just their accessibility traits.
I hope I'm wrong, and we just missed a little detail. Let me know.
There was a problem hiding this comment.
I've tested it with a TouchableOpacity (UIView as well):
<TouchableOpacity
onPress={() => this.setState({switchOn: !this.state.switchOn})}
accessibilityRole="switch"
accessibilityStates={this.state.switchOn ? ['on'] : ['off']}>
<Text>
The switch value is {this.state.switchOn ? 'on' : 'off'}
</Text>
</TouchableOpacity>
I'm gonna try later to play with this PR and see if I can replicate the same result that I got on my local branch. I let you know.
There was a problem hiding this comment.
Interesting. Good to know. We can test with your patch here too... Although it conflicts with this PR, so we'll need to do a bit of surgery. :)
There was a problem hiding this comment.
There's actually a non-public trait that UISwitches use and it's UIAccessibilityTraitToggle. It's defined as 0x0020000000000000 ( 1 << 53 ).
So you could do something like switch.accessibilityTraits = UIAccessibilityTraitButton | 0x0020000000000000 (or define a constant) to get this behavior instead and avoid the cost of creating a UISwitch
There was a problem hiding this comment.
As an aside, while this constant isn't in the header, it's in the source so you can print out this symbol in lldb:
(lldb) po UIAccessibilityTraitToggle
0x0020000000000000
| <View | ||
| accessibilityLabel="element 1" | ||
| accessibilityRole="alert" | ||
| accessible={true}> |
There was a problem hiding this comment.
@marcmulcahy apologies if you've explained this in the past.
What would happen if the developer supplied accessibleLabel and accessibleRole, but set accessible to false? Is this a necessary orthogonal toggle? I can't think if a use case where one would supply a label and a role but not want the element to be "accessible".
There was a problem hiding this comment.
@jessebeach I don't think this is a concern, as I think accessible=false would hide the component from accessibility services, so the label, roles, and states would be ignored.
React/Views/RCTView.m
Outdated
| @"radiogroup" : @"radio group", | ||
| @"scrollbar" : @"scroll bar", | ||
| @"spinbutton" : @"spin button", | ||
| @"switch" : @"switch", |
There was a problem hiding this comment.
There's actually a non-public trait that UISwitches use and it's UIAccessibilityTraitToggle. It's defined as 0x0020000000000000 ( 1 << 53 ).
So you could do something like switch.accessibilityTraits = UIAccessibilityTraitButton | 0x0020000000000000 (or define a constant) to get this behavior instead and avoid the cost of creating a UISwitch
React/Views/RCTViewManager.m
Outdated
| if ([state isEqualToString:@"selected"]) { | ||
| view.reactAccessibilityElement.accessibilityTraits |= UIAccessibilityTraitSelected; | ||
| } | ||
| else if ([state isEqualToString:@"disabled"]) { |
There was a problem hiding this comment.
nit: keep the else + else if in the same line as the previous }.
React/Views/RCTView.m
Outdated
| - (NSString *)accessibilityValue | ||
| { | ||
| NSMutableString *value = [NSMutableString stringWithString:@""]; | ||
| NSDictionary *roleDescriptions = @{ |
There was a problem hiding this comment.
can you mark this and stateDescriptions as static so you're not creating these every time accessibilityValue is called? Or use a static constant defined outside of this method.
React/Views/RCTView.m
Outdated
| @@ -184,6 +184,53 @@ - (BOOL)didActivateAccessibilityCustomAction:(UIAccessibilityCustomAction *)acti | |||
| return YES; | |||
| } | |||
|
|
|||
| - (NSString *)accessibilityValue | |||
| { | |||
| NSMutableString *value = [NSMutableString stringWithString:@""]; | |||
There was a problem hiding this comment.
Rather than mutate a NSMutableString, build up a NSMutableArray and use componentsJoinedByString: at the end.
React/Views/RCTView.m
Outdated
| for (NSString *state in _accessibilityStates) { | ||
| NSString *stateDescription =state ? stateDescriptions[state] : nil; | ||
| if (stateDescription) { | ||
| [value appendString:@" "]; |
There was a problem hiding this comment.
separator here should be a "," since VoiceOver will make it sound more natural when reading a list of words.
There was a problem hiding this comment.
Inserted comment in the wrong field... I made this a space due to eventual localization concerns (this whole chunk of code needs a rethink to support localizing role and state descriptions). Seems like the right separator is likely different depending on language.
There was a problem hiding this comment.
The separator here doesn't need to be localized, it should be a comma regardless of localization. VoiceOver will insert pauses when it encounters commas, which will help this list of states sound more natural. The list of localized states should be comma separated in any translation. Otherwise, this list of states would be space separated and VoiceOver may read them out like a sentence.
e.g. the final string should be "combobox, radio, alert" instead of "combobox radio alert". Even when localized, it would be "<state A>, <state B>, <state C>"
|
I didn't use a comma for eventual localization concerns (this whole chunk of code needs a rethink to allow for role and state descriptions to be localized). |
There was a problem hiding this comment.
Thanks @ikenwoo and @blavalla for the feedback! The FB AX team is on board with this code, pending any remaining responses or updates to our comments.
🎉 🎉 @marcmulcahy 🎉 🎉
|
Thanks all for the great feedback. We're working on a final set of changes to add native iOS switch support, as well as make some of the suggested code cleanups recommended by @ikenwoo I expect to have another revision posted by end of day. |
|
@ikenwoo could you please take another quick look to ensure we've adequately addressed your comments? Also, would love your feedback on how we can correctly support localization of the role and state description strings in iOS. Thanks for all the great feedback! |
| ); | ||
| } | ||
| } | ||
|
|
| accessibilityRole="spinbutton" | ||
| accessible={true}> | ||
| <Text>Spin button example</Text> | ||
| </View> |
| export type AccessibilityStates = $ReadOnlyArray<'disabled' | 'selected'>; | ||
| export type AccessibilityStates = $ReadOnlyArray< | ||
| | 'disabled' | ||
| | 'selected' |
There was a problem hiding this comment.
One issue with this accessibilityStates API in general is that to support states on web the aria-* property must always be set. For example, if a screenreader is to announce that a button is selectable then the DOM must include the aria-selected prop whether its value is true or false.
This means most states need a value equivalent to the false state (although there are tri-state states too), like we have with checked and unchecked. Or we could also support objects as values in the array of states, e.g., [ 'checked', false ] or { checked: 'mixed' }.
There was a problem hiding this comment.
@necolas, are you proposing a specific change to this PR?
There was a problem hiding this comment.
I'm bringing up an issue with the accessibilityStates API / cc @jessebeach
There was a problem hiding this comment.
The only alternate solution I can think of ATM would be to have the value of a state be a defined enumeration. So, define a state type:
BooleanState = {'true', 'false'}
TriState = {'true', 'false', 'mixed'}
... some others
then each state (checked, selected, enabled) defines its type.
But this seems like a significant complication. What benefit does it add for component developers and AT builders?
There was a problem hiding this comment.
But this seems like a significant complication. What benefit does it add for component developers and AT builders?
@necolas brings up a really important point. It would be impossible to build a tri-state checkbox with this API. @necolas might it be sufficient to introduce a 'mixedChecked' value to the state enum?
if a screenreader is to announce that a button is selectable then the DOM must include the aria-selected prop whether its value is true or false.
@necolas the concept "selectableness" only applies to rows, tabs, options and cells. Could we address your concern by providing unselected and selected in the states enum? If unselected is passed to an element with a row class, then the web platform implementation would add aria-selected="false".
I think there are few enough of these exceptions that we can address them with the current state enum and some logic on the platform-specific target implementation.
There was a problem hiding this comment.
(This is definitely not directly related to the PR but something to think about in terms of where we could steer RN accessibility APIs.)
I think we'd have to do it for quite a few states including pressed, expanded, etc. It's also a little inconvenient to work with strings in conditions (isPressed ? 'pressed' : 'unpressed') compared to using booleans with ARIA attributes directly. From the perspective of multi-platform apps, it also seems like an enum makes it easier to accidentally not list "false" states and unwittingly produce poor AX for web (and perhaps other platforms). If accessibilityStates accepted an object some of these issues might be less likely, as the syntax encourages use of booleans and defining the attributes for every render.
const [ disabled, setDisabled ] = useState(false);
const [ checked, setChecked ] = useState(false);
// ...
return <View accessibilityStates={{ disabled, checked }} />
There was a problem hiding this comment.
@necolas, I don't disagree, but am struggling to come up with a workable API. This maps reasonably for states like selected, enabled, pressed, checked, which are clearly booleans. Mixed checkboxes are a little trickier-- do they have three values (true, false, and mixed)? If so, the value isn't a boolean, is it a string?
If the value of a state isn't always a boolean, how do we define/enforce what the value of a state should be? And how does a developer know what type of value a particular state should be if they're all potentially different?
Seems like boolean states make sense in most of the cases I can think of-- maybe we just need to solve for mixed checkboxes? Are there other states that would need to be strings or some other value type?
There was a problem hiding this comment.
Properties of the object that are of different or types can be annotated as such. I think most properties would also need to accept null/undefined values to cover the case where the platform property should be unset. Overall an API probably quite similar to some ideas we discussed for accessibilityRelationships.
type AccessibilityStates = {
checked?: ?boolean | 'mixed',
disabled?: ?boolean,
pressed?: ?boolean,
}
const [ disabled, setDisabled ] = useState(false);
const [ checked, setChecked ] = useState(false);
const [ pressed, setPressed ] = useState(false);
// ...do something that requires mixed checked state
setChecked('mixed');
// ...do something that means element should not be announced as pressable
setPressed(null);
return <View accessibilityStates={{ disabled, checked, pressed }} />
…1y) (#24344) Summary: Closes: #24016 React Native 0.57 introduced cross-platform `accessibilityRole` and `accessibilityStates` props in order to replace `accessibilityComponentType` (for android) and `accessibilityTraits` (for iOS). With #24095 `accessibilityRole` and `accessibilityStates` will increase, receiving more options, which seems to be a good moment to remove deprecated props. Remove deprecated `accessibilityComponentType` and `accessibilityTraits` props. [General] [Removed] - Remove accessibilityComponentType and accessibilityTraits props Pull Request resolved: #24344 Reviewed By: rickhanlonii Differential Revision: D14842214 Pulled By: cpojer fbshipit-source-id: 279945e503d8a23bfee7a49d42f5db490c5f6069
|
Can someone help me understand the purpose of DeprecatedViewAccessibility.js and DeprecatedViewPropTypes.js. these are causing me problems when converting accessibilityStates from an array of strings to an object. Several components, including Button and touchableWithoutFeedback explicitly declare accessibilityStates as an array of deprecated types. Can we safely remove DeprecatedAccessibility altogether and simply use the definitions in ViewAccessibility instead? |
@marcmulcahy I don't think that's necessary for this PR (as mentioned here) or a change that has been requested yet. Changing the |
|
OK, I was under the impression that this change was under consideration for this PR, and have the iOS implementation done, and am working on the Android implementation. What outstanding issues remain on this PR for us to address? |
|
What's the status of this PR? It is fairly large and I realize there is additional work for the future but is the current iteration shippable or are there still certain actions that need to be taken? |
|
To resolve conflicts, is it acceptable to simply rebase this code on top of master and force push this branch? |
|
Yep, that’s the preferred way to do it.
…________________________________
From: marcmulcahy <notifications@github.com>
Sent: Wednesday, April 24, 2019 6:17:22 PM
To: facebook/react-native
Cc: Christoph Nakazawa; Manual
Subject: Re: [facebook/react-native] Additional Accessibility Roles and States (#24095)
To resolve conflicts, is it acceptable to simply rebase this code on top of master and force push this branch?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub<#24095 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAADIKGOUUINRG3IIYO3XUTPSCI2FANCNFSM4HAK7TLA>.
|
|
This pull request was successfully merged by Marc Mulcahy in 1aeac1c. When will my fix make it into a release? | Upcoming Releases |
|
OK to delete the a11y-roles-states branch in my fork? Will the code and conversation history be preserved? |
|
Yeah, go ahead! |
|
@marcmulcahy are you planning to make a documentation PR with those updates? (https://github.com/facebook/react-native-website) Please let me know if you need any help on it. |
|
@marcmulcahy @elucaswork After merging I got an internal test failure that a If you can verify that this is still working as expected, then it may be an issue with our e2e framework, in which case I can look into it. |
|
@cpojer Sure, we'll take a look. I don't think we touched any non-a11y paths, but we'll double check. |
|
@elucaswork Happy to do that, but would like some feedback on #24608 first, as it changes the a11y states API, and is probably close to where we want the API to land longer term. |
|
Thanks Marc,
the failure definitely bisected to a change in this PR but it’s unclear to me whether the behavior actually changed or the test framework needs a change.
…________________________________
From: marcmulcahy <notifications@github.com>
Sent: Thursday, April 25, 2019 6:39:34 PM
To: facebook/react-native
Cc: Christoph Nakazawa; Mention
Subject: Re: [facebook/react-native] Additional Accessibility Roles and States (#24095)
@elucaswork<https://github.com/elucaswork> Happy to do that, but would like some feedback on #24608<#24608> first, as it changes the a11y states API, and is probably close to where we want the API to land longer term.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#24095 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAADIKFMUMMOMT4L6O6HUS3PSHUFNANCNFSM4HAK7TLA>.
|
|
@cpojer Is there a set of tests in the public repo we should be using, or is it as simple as creating a disabled button that shouldn't be clickable on Android? |
|
It should be enough to check whether it works or not on master, yes!
…________________________________
From: marcmulcahy <notifications@github.com>
Sent: Thursday, April 25, 2019 7:04:07 PM
To: facebook/react-native
Cc: Christoph Nakazawa; Mention
Subject: Re: [facebook/react-native] Additional Accessibility Roles and States (#24095)
@cpojer<https://github.com/cpojer> Is there a set of tests in the public repo we should be using, or is it as simple as creating a disabled button that shouldn't be clickable on Android?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#24095 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAADIKB4LBHWSRTAXUSLT5DPSHXBPANCNFSM4HAK7TLA>.
|
|
@cpojer We were unable to reproduce the problem when we did the following:
The button doesn't appear to click either with or without this patch applied. Happy to troubleshoot this if you can help us re-create the failing test. |
|
Thank you so much! Can you confirm that the disabled state also works correctly on other elements?
I’ll check the internal test again tomorrow and see if we need to fix something up or if it’s the test.
…________________________________
From: marcmulcahy <notifications@github.com>
Sent: Thursday, April 25, 2019 9:23:42 PM
To: facebook/react-native
Cc: Christoph Nakazawa; Mention
Subject: Re: [facebook/react-native] Additional Accessibility Roles and States (#24095)
@cpojer<https://github.com/cpojer> We were unable to reproduce the problem when we did the following:
* OPen RNTester
* Select Button module
* Attempt to click the disabled button
The button doesn't appear to click either with or without this patch applied.
Happy to troubleshoot this if you can help us re-create the failing test.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#24095 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAADIKEPSUF2U7TBS44UKUDPSIHM5ANCNFSM4HAK7TLA>.
|
|
@cpojer We have the test of disabled TouchableOpacity in AccessibilityExamples.js. Cannot repro the problem. |
| view.setTag(R.id.accessibility_role, AccessibilityRole.fromValue(accessibilityRole)); | ||
| } | ||
|
|
||
| @ReactProp(name = PROP_ACCESSIBILITY_STATES) | ||
| public void setViewStates(@Nonnull T view, @Nullable ReadableArray accessibilityStates) { | ||
| view.setSelected(false); | ||
| view.setEnabled(true); |
There was a problem hiding this comment.
@cpojer I suspect this is the culprit. This code is broken, which is why we proposed removing it. That is, setting accessibilityStates shouldn't change the view state like this. But you could imagine that if a test passed an empty state set, as I think Button does:
That causes the view to be enabled. Likewise, if you pass:
<Button <accessibilityStates={['disabled']}
This erroneously disables the view.
IMHO, setting accessibility states shouldn't affect the state of the underlying view.
In an ideal world, the accessibility "disabled" and "selected" states simply mirror the underlying view states.
Not sure what the right fix is here...
There was a problem hiding this comment.
I was able to repro the bug in the AccessibilityExample.js by only setting the accessibility state to be disabled not the disable property itself. I guess the internal test of disabled button does the same thing.
There was a problem hiding this comment.
I have been digging into the test failure as well, and found that the test is only looking at the View's "enabled" property, and checking if it is false. By setting the accessibilityState to disabled we are not longer setting the view's "enabled" property to false, which means the test now fails.
It seems to me like we should be keeping the accessibilityState's and the viewState's in sync wherever possible (selected, enabled, focusable, clickable, longClickable, etc.) otherwise you could have a view that is disabled visually, but not disabled in the AccessibilityNodeInfo, or disabled in the AccessibilityNodeInfo, but not disabled for users interacting without an AT.
The other option is to not even have these as specific accessibilityStates, but have them only exist as viewStates that we then set the accessibilityStates to automatically. I'm not sure if this would work cross platform however, as not every view on iOS can be "Selected" or "enabled" like it can be on Android.
There was a problem hiding this comment.
@blavalla I completely agree-- the goal is to have the view states and accessibility states in sync. But it seems to me that the test is broken if it relies on calling setAccessibilityStates with no states set to ensure that the view itself is disabled. Clearly, we wouldn't remove the view enabled property, and the accessibility enabled state should definitely match the view enabled state.
Can the test be modified to explicitly set the view state to disabled, which seems more correct, rather than relying on setting the accessibility states of the view to disable it?
I could perhaps be persuaded that setting the accessibility state of a view to disabled, should propagate to the view and disable the entire view as well, although this doesn't feel quite right to me...
There was a problem hiding this comment.
@marcmulcahy the current test is testing for <View disabled={true} /> and that seems to trigger the failure.
There was a problem hiding this comment.
disabled is a valid View prop?
There was a problem hiding this comment.
Sorry, it was late on Friday and I didn't verify. I meant <Text disabled={true} /> is not working properly for testing after this PR. This prop is Android only and is only used for testing: https://github.com/facebook/react-native/blob/master/Libraries/Text/TextPropTypes.js#L136
@marcmulcahy would you be able to send a PR to restore the previous behavior on Android?
There was a problem hiding this comment.
@cpojer Of course I'll submit a PR to fix it. I spent some time looking at this again this morning, and I'm not sure how to correctly fix it. As I see it, the issue is that we have three uses of enabled/disabled:
- This testing-only use in -- perhaps other places?
- the View state itself (Checkbox, Slider, etc.).
- The accessibility disable state.
As best I can understand this, the view's enabled state should be the master state-- I.E., all other states should derive from that. So however the Android view's enabled state gets set, both the testing only states, as well as the accessibility disabled state should reflect that.
Presumably, React Native has a way to specify that components are visible, but can't be interacted with, (disabled, greyed, etc.). When this state is set, the accessibility and testing disabled states should also reflect that change.
I think it would be reasonable for the testing disabled state to also serve as a master state-- that is, view disabled and accessibility disabled are synced to that state as well.
What I'm not convinced of, is that when the accessibility disabled state is set, the view state should be changed. This seems backwards.
From an API point of view, we don't really want components setting the accessibility disabled state to "disable" the component. This is what's happening, and the test code seems to be relying on this peculiarity of how things are implemented.
What I can't work out is how the test, by setting one of these "disabled" states, was causing the accessibilityStates['disabled'] to be set, which in turn caused the view.setEnabled(false) to be called.
If you can help me work that out, I think we can propose a fix that will both fix this problem and be correct.
We can revert the accessibility enabled state code, but I think this propagates undesirable uses of the accessibility API and apps relying on it to control main view state, which IMHO, isn't desirable.
There was a problem hiding this comment.
Regarding your proposed fix, I think that makes a lot of sense, if you can send a PR to that effect, I think that'll solve our issues.
I'm not entirely sure about how this all works, though. I'm sorry I'm not an expert. Our internal testing framework as I understand it, simply calls this code: https://github.com/selendroid/selendroid/blob/44118d68955047a0645c707a4e021c5de5d53b99/selendroid-server/src/main/java/io/selendroid/server/model/AndroidNativeElement.java#L654
Is that enough for you to reverse engineer the necessary change?
| [newStates addObject:state]; | ||
| } | ||
| } | ||
| if (newStates.count > 0) { |
There was a problem hiding this comment.
I'm getting a crash here with react-native-gesture-handler. Basically it uses RCTViewManager with a UIControl instead of a RCTView so the cast here is not safe. Maybe we should add a isKindOfClass check before casting here?
There was a problem hiding this comment.
Good idea. Do you wanna send a PR?
|
PR to add additional a11y roles and states to the documentation is here: |
) Summary: In #24095, we removed the code that changes the underlying Android view's enabled state to false when "disabled" is included in the accessibility states, which seems correct. The Android view's state shouldn't mirror the accessibility state, it should be the other way round-- the accessibility state should represent the state of the view. It seems that the existing test is brokenly setting the disabled state in the Javascript object, and then querying the Android view's enabled state to confirm the change. If the Button implementation is actually the culprit, then IMHO, the correct fix would be to have the Button implementation manipulate the Android View's enabled state, not the accessibilityStates code. [android] [fix] - Fix internal test case around disabled state of buttons Pull Request resolved: #24678 Differential Revision: D15237590 Pulled By: cpojer fbshipit-source-id: d7ebefbcba9d4d9425da4285175302e4b6435df7
…Additional Accessibility Roles and States (facebook#24095)
Summary
Assistive technologies use the accessibility role of a component to tell the disabled user what the component is, and provide hints about how to use it. Many important roles do not have analog AccessibilityTraits on iOS. This PR adds many critical roles, such as editabletext, checkbox, menu, and switch to name a few.
Accessibility states are used to convey the current state of a component. This PR adds several critical states such as checked, unchecked, on and off.
Changelog
[general] [change] - Adds critical accessibility roles and states.
Test Plan