Skip to content

Implement accessibilityState #4617

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

Merged
merged 8 commits into from
Apr 16, 2020
Merged

Conversation

kmelmon
Copy link
Contributor

@kmelmon kmelmon commented Apr 15, 2020

Fixes #4042

In version 0.61, accessibilityState was introduced and accessibilityStates was marked as deprecated.
In version 0.62, accessibilityStates was removed.

This change implements accessibilityState and removes accessibilityStates.

The change is straightforward - instead of an array, the property is an object with fields.

Microsoft Reviewers: Open in CodeFlow

@kmelmon kmelmon requested a review from a team as a code owner April 15, 2020 22:28
@NickGerleman
Copy link
Contributor

NickGerleman commented Apr 15, 2020

Could we split this into two changes? We want to backport accessibilityState to 0.61, but don't want to remove accessibilityStates from that branch? #Resolved

@@ -140,7 +140,6 @@ const ReactNativeViewConfig = {
accessibilityLabel: true,
accessibilityLiveRegion: true,
accessibilityRole: true,
accessibilityStates: true, // TODO: Can be removed after next release
Copy link
Contributor

@NickGerleman NickGerleman Apr 15, 2020

Choose a reason for hiding this comment

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

This and the comment were stock 0.62. We should probably leave this as is. #Resolved

Copy link
Contributor Author

@kmelmon kmelmon Apr 15, 2020

Choose a reason for hiding this comment

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

This is weird - I don't see any mention of accessibilityStates in the RN docs. Is this property not actually removed yet? #Closed

Copy link
Contributor

@NickGerleman NickGerleman Apr 15, 2020

Choose a reason for hiding this comment

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

It's removed to my understanding. You could quickly search their view managers in their 0.62 branch though. My guess it that fb has a view manager internally that still allows it, or just forgot to regenerate #Closed

Copy link
Contributor

@NickGerleman NickGerleman Apr 15, 2020

Choose a reason for hiding this comment

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

There were other places in the 0.62 upgrade where it seemed like support was truly removed. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I undid this to prepare for backporting to 61


In reply to: 409173188 [](ancestors = 409173188)

@kmelmon
Copy link
Contributor Author

kmelmon commented Apr 15, 2020

OK I brought back accessibilityStates so we can backport this to 61


In reply to: 614312986 [](ancestors = 614312986)

props.update(folly::dynamic::object("selected", "boolean")("disabled", "boolean")("checked", "boolean")(
"busy", "boolean")("expanded", "boolean"));
return props;
}
folly::dynamic FrameworkElementViewManager::GetNativeProps() const {
folly::dynamic props = Super::GetNativeProps();
Copy link
Member

@asklar asklar Apr 15, 2020

Choose a reason for hiding this comment

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

nit add newline between methods :) #Resolved

@kmelmon kmelmon added the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label Apr 16, 2020
@ghost
Copy link

ghost commented Apr 16, 2020

Hello @kmelmon!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@kmelmon kmelmon merged commit 9718f84 into microsoft:master Apr 16, 2020
kmelmon added a commit to kmelmon/react-native-windows that referenced this pull request Apr 16, 2020
* Implement AccessibilityState

* formatting

* Change files

* bring back accessibilityStates to port back to 0.61

* formatting

* handle mixed state

* update test

* prettier fixes
ghost pushed a commit that referenced this pull request Apr 17, 2020
* Implement accessibilityState (#4617)

* Implement AccessibilityState

* formatting

* Change files

* bring back accessibilityStates to port back to 0.61

* formatting

* handle mixed state

* update test

* prettier fixes

* resolve merge conflict

* idunno what the heck I'm doing but apparently need to change this

* f***ing clang

* fix merge conflict
asklar pushed a commit to asklar/react-native-windows that referenced this pull request Apr 19, 2020
* Implement AccessibilityState

* formatting

* Change files

* bring back accessibilityStates to port back to 0.61

* formatting

* handle mixed state

* update test

* prettier fixes
NickGerleman added a commit to NickGerleman/react-native-windows that referenced this pull request Apr 24, 2020
NickGerleman pushed a commit to NickGerleman/react-native-windows that referenced this pull request Apr 24, 2020
…yStates

* Implement AccessibilityState

* formatting

* Change files

* bring back accessibilityStates to port back to 0.61

* formatting

* handle mixed state

* update test

* prettier fixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to 61: View.accessibilityState API change
3 participants