Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Accessibility states API. #24608

Closed
wants to merge 10 commits into from

Conversation

@marcmulcahy
Copy link

commented Apr 25, 2019

Summary

As currently defined, accessibilityStates is an array of strings, which represents the state of an object. The array of strings notion doesn't well encapsulate how various states are related, nor enforce any level of correctness.

This PR converts accessibilityStates to an object with a specific definition. So, rather than:

<View
...
accessibilityStates={['unchecked']}>

We have:

<View
accessibilityStates={{'checked': false}}>

And specifically define the checked state to either take a boolean or the "mixed" string (to represent mixed checkboxes).

We feel this API is easier to understand an implement, and provides better semantic definition of the states themselves, and how states are related to one another.

Changelog

[general] [change] - Convert accessibilityStates to an object instead of an array of strings.

Test Plan

Converted code in RNTester to use the new accessibilityStates API, and modified the checkbox example to support the "mixed" state, which was one of the limitations of the current API which prompted this work.

@analysis-bot
Copy link

left a comment

Code analysis results:

  • eslint found some issues. Run yarn lint --fix to automatically fix problems.
if (disabled) {
buttonStyles.push(styles.buttonDisabled);
textStyles.push(styles.textDisabled);
accessibilityStates.push('disabled');
accessibilityStates['enabled'] = false;

This comment has been minimized.

Copy link
@analysis-bot

analysis-bot Apr 25, 2019

dot-notation: ["enabled"] is better written in dot notation.

@pull-bot

This comment has been minimized.

Copy link

commented Apr 25, 2019

Messages
📖

📋 Verify Changelog Format - A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

CATEGORY may be:
  • General
  • iOS
  • Android
  • JavaScript
  • Internal (for changes that do not need to be called out in the release notes)

TYPE may be:

  • Added, for new features.
  • Changed, for changes in existing functionality.
  • Deprecated, for soon-to-be removed features.
  • Removed, for now removed features.
  • Fixed, for any bug fixes.
  • Security, in case of vulnerabilities.

MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

Generated by 🚫 dangerJS against 780d6f5

@marcmulcahy marcmulcahy changed the title A11y states api New Accessibility states API. Apr 25, 2019

@necolas

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

For context, this is a change I proposed here to provide an API that works for platforms where attributes must be present on host nodes to communicate their purpose to the native accessibility system

@necolas
Copy link
Contributor

left a comment

This might need to support but deprecate the old API to give people time to migrate

@marcmulcahy

This comment has been minimized.

Copy link
Author

commented Apr 25, 2019

@necolas Any thoughts about how to deprecate the older API, while strongly encouraging developers to move to the new one?

@necolas

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

Any thoughts about how to deprecate the older API, while strongly encouraging developers to move to the new one?

Hopefully someone from RN Core will be able to advise

@cpojer

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

Ideally we make this a non-breaking change for now by supporting both arrays or objects for a while, then we’ll add a warning in the release after, and then release a codemod (I can help with that) and then remove it.

@marcmulcahy marcmulcahy force-pushed the marcmulcahy:a11y-states-api branch from 600eb59 to 44bec03 May 3, 2019

@marcmulcahy

This comment has been minimized.

Copy link
Author

commented May 3, 2019

@cpojer Not sure how to allow the native code to accept both an array of strings as well as an object. Any ideas?

@cpojer

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

Unfortunately we can't make a breaking change like that in a single pull request. We could wrap <View /> in JS, which would be slow and which we can't do. It seems like the only option here is to introduce a new prop and deprecate the existing one (in a separate PR later) which I don't think we want to do. I'll ask at FB if there are other possibilities but unless we want to introduce a new prop name this may not easily be possible atm.

@cpojer

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

@marcmulcahy do you have any idea for a new prop that we could introduce so that we could deprecate the existing one? I know this is unfortunate but it seems like the only way. If not, please close this PR.

@marcmulcahy

This comment has been minimized.

Copy link
Author

commented May 9, 2019

@cpojer I suppose we could simply add a new prop called something like accessibilityStates2 that's an object instead of an array of strings. Switch all the RNTester samples and components to use accessibilityStates2, and then eventually deprecate accessibilityStates, or rename accessibilityStates2 to accessibilityStates.

I do think this object-based API is much clearer, in that the component author must enumerate the states that "matter", and it makes much clearer how states are related to each other (expanded/collapsed, checked/unchecked, etc.).

I'm also happy to close the PR if this isn't something we want to tackle ATM. Let me know what FB prefers to do.

@cpojer

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

Alright, yeah I think we can do this. Can you change this PR to do the following:

  • Add deprecatedAccessibilityStates and make the native code work with that
  • Make it so accessibilityStates still keeps working exactly the same after your PR lands.

In a month from now (sorry, this is some fb internal issue we have to deal with) we can then change how accessibilityStates works as long as we don't have internal users for it any longer. A month after that we can bring back the accessibilityStates prop with the new API and kill the deprecatedAccessibilityStates API.

I'm sorry for this slow multi process step but I think it's worth doing it if you are up for it :)

@cpojer

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

Alternatively, if we can think of a better name than accessibilityStates, we can also just add the new prop with a new name and then remove the old one in a month from now. That is way less involved and faster. The process is:

  • Add a new prop with the new API
  • I will migrate all internal callsites at FB
  • Send a PR with the removal for the old accessibilityStates that can be merged in a month from now.

What would be a reasonable name for a new prop?

@necolas

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

What would be a reasonable name for a new prop?

accessibilityState? Since the value is an object and the name is close enough. And thinking ahead to Marc's other proposal around relationships, that prop could be accessibilityRelationship (no s either).

@cpojer

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

I think that’s a great idea. Let’s do it!

@marcmulcahy marcmulcahy force-pushed the marcmulcahy:a11y-states-api branch from 44bec03 to 3e8420c May 22, 2019

@marcmulcahy

This comment has been minimized.

Copy link
Author

commented May 22, 2019

As we discussed, this version moves the new API to a new property called "accessibilityState" and leaves "accessibilityStates" in tact.

To verify that the existing prop was still working, I applied all the patches which add the new property, and left RNTester unchanged, and verified that RNTester behaves as before on both Android and iOS. I then modified RNTester to use "accessibilityState" instead of "accessibilityStates" and re-tested.

@necolas

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

Looks good to me

@cpojer

cpojer approved these changes May 23, 2019

Copy link
Contributor

left a comment

That's fantastic. Thank you so much @marcmulcahy and I'm sorry for putting you through so much pain with this one but I really appreciate you were sticking with us :)

I made a few small inline reverts to ensure the current props keep working at FB during the migration phase. Once this PR is landed, do you mind sending a PR that removes accessibilityStates altogether? I can merge that one (in about a month from now) and then we'll be in a good state again :)

@cpojer cpojer force-pushed the marcmulcahy:a11y-states-api branch from 780d6f5 to 998faa4 May 23, 2019

@facebook-github-bot
Copy link

left a comment

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@pull-bot

This comment has been minimized.

Copy link

commented May 23, 2019

Messages
📖

📋 Verify Changelog Format - A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

CATEGORY may be:
  • General
  • iOS
  • Android
  • JavaScript
  • Internal (for changes that do not need to be called out in the release notes)

TYPE may be:

  • Added, for new features.
  • Changed, for changes in existing functionality.
  • Deprecated, for soon-to-be removed features.
  • Removed, for now removed features.
  • Fixed, for any bug fixes.
  • Security, in case of vulnerabilities.

MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

Generated by 🚫 dangerJS against 998faa4

@facebook-github-bot
Copy link

left a comment

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

@react-native-bot

This comment has been minimized.

Copy link
Collaborator

commented May 23, 2019

This pull request was successfully merged by Marc Mulcahy in 099be9b.

When will my fix make it into a release? | Upcoming Releases

@marcmulcahy marcmulcahy deleted the marcmulcahy:a11y-states-api branch May 31, 2019

@ahimberg ahimberg referenced this pull request Jul 4, 2019
7 of 9 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.