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

Change Accessible/Focusable Relationship to Default to #10569

Merged
merged 6 commits into from
Sep 16, 2022

Conversation

chiaramooney
Copy link
Contributor

@chiaramooney chiaramooney commented Sep 14, 2022

Description

Type of Change

  • Functionality change

Why

Following change to accessible/focusable prop relationship in #9840, request from partner to adjust logic in order to reduce number of code edits needed.

#9840 was needed in order to bring RNW up to Windows accessibility standards. In Windows accessibility focus and keyboard focus are the same. RNW should not allow developers to remove elements from the UIA while they still remain keyboard focusable.

What

Originally, the PR set the behavior to be if either accessible or focusable was set to false the control should not be focusable or accessible. The logic is now if either accessible or focusable is set to true the control should be focusable and accessible.

Microsoft Reviewers: Open in CodeFlow

@chiaramooney chiaramooney requested review from a team as code owners September 14, 2022 00:19
@AgneLukoseviciute
Copy link
Contributor

I feel like given the fact that almost all controls are focusable/accessible by default, this might be confusing for folks when they set focusable or accessible (but not both) to false, but the control remains focusable. @rozele would it be enough if we only have this behavior for the View component?

@AgneLukoseviciute
Copy link
Contributor

AgneLukoseviciute commented Sep 15, 2022

yellow box should help with any confusion for new developers, although I'm still worried this will be a breaking change. E.g. even for us, https://github.com/microsoft/react-native-windows/pull/9898/files would no longer work if focusable is set to true anywhere. There could be other places where we'd need to change/add logic to account for this.

@@ -601,9 +602,15 @@ void ViewViewManager::TryUpdateView(
pViewShadowNode->GetControl().Content(visualRoot);

if (useControl && pViewShadowNode->IsAccessible() != pViewShadowNode->IsFocusable()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (useControl && pViewShadowNode->IsAccessible() != pViewShadowNode->IsFocusable()) {
if (useControl && (pViewShadowNode->IsAccessible() != pViewShadowNode->IsFocusable())) {

Copy link
Contributor

@AgneLukoseviciute AgneLukoseviciute Sep 16, 2022

Choose a reason for hiding this comment

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

Maybe let's add a set of parentheses here for some clarity/to ensure proper order of operations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure that's needed here because the second expression is a comparison and not a & and | operator

Copy link
Contributor

@AgneLukoseviciute AgneLukoseviciute left a comment

Choose a reason for hiding this comment

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

nit: a comment before line 480 and 605 would be nice, took me a good minute to wrap my head around what we're doing here/why.

@chiaramooney chiaramooney merged commit 3cb6748 into microsoft:main Sep 16, 2022
@chiaramooney chiaramooney deleted the cm-alter-access branch September 16, 2022 22:13
@asklar
Copy link
Member

asklar commented Sep 16, 2022

@chiaramooney this sounds like a breaking change, so please make sure to call it out and document it in the release notes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change This PR will break existing apps and should be part of the known breaking changes for the release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants