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

[Fabric] Implement IExpandCollapseProvider #13892

Merged
merged 6 commits into from
Oct 9, 2024

Conversation

chiaramooney
Copy link
Contributor

@chiaramooney chiaramooney commented Sep 24, 2024

Description

Type of Change

  • New feature (non-breaking change which adds functionality)

Why

Implement ExpandCollapse provider so controls can display correct control state to UIA and custom controls can write expand collapse functionality.

Resolves #13352
Resolves #13067
Resolves #13063
Resolves #13059
Resolves #13055
Resolves #13051
Resolves #13047
Resolves #13043
Resolves #13039
Resolves #13035

#13316
#13327

What

  • Add support for IExpandCollapseProvider to CompositionDynamicAutomationProvider (Expand, Collapse, get_ExpandCollapseState)
  • ExpandCollapseProvider will be implemented for a control when it meets the conditions laid out in <>
  • Add dump of ExpandCollapse pattern information to E2E tests
  • Adjust example in RNTester to include expand/collapse functionality

Screenshots

Add any relevant screen captures here from before or after your changes.

Testing

Tested the following:

  • accessibilityState: expanded=true, expanded=false, empty accessibilityState, accessibilityState not specified
  • aria-expanded: set to true, false, undefined
  • accessibilityActions: set/unset 'expand' and 'collapse' actions

Changelog

Should this change be included in the release notes: Yes

Implement UIA ExpandCollapse Pattern allowing custom controls to implement 'expand' and 'collapse' actions.

Microsoft Reviewers: Open in CodeFlow

@chiaramooney chiaramooney requested a review from a team as a code owner September 24, 2024 23:12
@microsoft-github-policy-service microsoft-github-policy-service bot added Area: Accessibility Area: ActivityIndicator Area: Component Views Area: Fabric Support Facebook Fabric Area: Image Area: Modal Area: RefreshControl Area: ScrollView Area: Switch Area: Text Area: TextInput Area: View Area: View Props https://reactnative.dev/docs/view#props Parity: Fabric vs. Paper RNW Fabric does not look or behave like RNW Paper New Architecture Broad category for issues that apply to the RN "new" architecture of Turbo Modules + Fabric labels Sep 24, 2024
@@ -181,6 +193,15 @@ HRESULT __stdcall CompositionDynamicAutomationProvider::GetPatternProvider(PATTE
AddRef();
}

if (patternId == UIA_ExpandCollapsePatternId &&
(accessibilityRole == "combobox" || accessibilityRole == "splitbutton" || accessibilityRole == "treeitem" ||
(expandableControl(props) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we do this pattern elsewhere, but do we really need to gate on accessibilityRole if expandable is set? Is there an actual limitation in UIA where a JS component author can't define say, a switch with expandable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

UIA would not stop us. We would be able to specify a Switch control type that implements the ExpandCollapse provider, but that would have the wrong accessibility conventions. i.e. a control like that would not meet "correct" accessibility rules. The role checks here were mostly added as "guard rails" for JS developers who aren't as familiar with UIA accessibility expectations. They help prevent JS developers from building apps with wonky accessibility implementations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a case where we are potentially blocking something, which while unusual, might be desired.

What if I have a switch which expands/collapses a bunch of UI based on its state. Maybe I want it to report both expand state and the rest of the switch state.

I always worry about these kinds of restrictive policies. What if I'm designing some completely new kind of control type, where expandable very much makes sense, but I dont want UIA reporting that I'm a combobox (or any of these other options). If I set accessibilityState.expanded - I'd expect that to report to the accessibility tools. It would be completely non-obvious that actually for that to work I need to set the role to one of several special values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we've followed this pattern for other providers so far I don't want to merge this PR with a different behavior pattern. I'm happy to rediscuss this concern, but let's do it in a separate issue and if we decide to remove the guard rails we can do so for all providers in one PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue filed #13934

@chiaramooney chiaramooney merged commit bba47ee into microsoft:main Oct 9, 2024
59 checks passed
acoates-ms pushed a commit to acoates-ms/react-native-windows that referenced this pull request Oct 16, 2024
* Implement IExpandCollapseProvider

* Change files

* Adjust Example

* Format + Update Snapshots
acoates-ms pushed a commit to acoates-ms/react-native-windows that referenced this pull request Oct 16, 2024
* Implement IExpandCollapseProvider

* Change files

* Adjust Example

* Format + Update Snapshots
acoates-ms pushed a commit to acoates-ms/react-native-windows that referenced this pull request Oct 16, 2024
* Implement IExpandCollapseProvider

* Change files

* Adjust Example

* Format + Update Snapshots
acoates-ms added a commit that referenced this pull request Oct 16, 2024
* [Fabric] Implement IExpandCollapseProvider  (#13892)

* Implement IExpandCollapseProvider

* Change files

* Adjust Example

* Format + Update Snapshots

* [Fabric] implement tooltip property (#13941)

* [Fabric] implement view tooltip property

* format

* Change files

* update

* Fix lingering tooltip if component is unmounted while tooltip showing

* snapshot

---------

Co-authored-by: Jon Thysell <jthysell@microsoft.com>

* Fix RootComponentView leak (#13959)

* Fix RootComponentView leak

* Change files

* format

---------

Co-authored-by: React-Native-Windows Bot <53619745+rnbot@users.noreply.github.com>

* Add Support for AccessibilityState:Busy (#13952)

* Support AccessibilityState: Busy

* Change files

* Add Testing

* Update Snapshots

* Update for Leak

---------

Co-authored-by: Chiara Mooney <34109996+chiaramooney@users.noreply.github.com>
Co-authored-by: Jon Thysell <jthysell@microsoft.com>
Co-authored-by: React-Native-Windows Bot <53619745+rnbot@users.noreply.github.com>
acoates-ms added a commit that referenced this pull request Oct 16, 2024
* [Fabric] Implement IExpandCollapseProvider  (#13892)

* Implement IExpandCollapseProvider

* Change files

* Adjust Example

* Format + Update Snapshots

* [Fabric] implement tooltip property (#13941)

* [Fabric] implement view tooltip property

* format

* Change files

* update

* Fix lingering tooltip if component is unmounted while tooltip showing

* snapshot

---------

Co-authored-by: Jon Thysell <jthysell@microsoft.com>

* Fix RootComponentView leak (#13959)

* Fix RootComponentView leak

* Change files

* format

---------

Co-authored-by: React-Native-Windows Bot <53619745+rnbot@users.noreply.github.com>

* Add Support for AccessibilityState:Busy (#13952)

* Support AccessibilityState: Busy

* Change files

* Add Testing

* Update Snapshots

* Update for Leak

* Update change files to use patch instead of prerelease

* fix

* Update snapshot

---------

Co-authored-by: Chiara Mooney <34109996+chiaramooney@users.noreply.github.com>
Co-authored-by: Jon Thysell <jthysell@microsoft.com>
Co-authored-by: React-Native-Windows Bot <53619745+rnbot@users.noreply.github.com>
acoates-ms added a commit that referenced this pull request Oct 16, 2024
* Add Support for AccessibilityState:Busy (#13952)

* Support AccessibilityState: Busy

* Change files

* Add Testing

* Update Snapshots

* Update for Leak

* Fix RootComponentView leak (#13959)

* Fix RootComponentView leak

* Change files

* format

---------

Co-authored-by: React-Native-Windows Bot <53619745+rnbot@users.noreply.github.com>

* [Fabric] implement tooltip property (#13941)

* [Fabric] implement view tooltip property

* format

* Change files

* update

* Fix lingering tooltip if component is unmounted while tooltip showing

* snapshot

---------

Co-authored-by: Jon Thysell <jthysell@microsoft.com>

* [Fabric] Implement IExpandCollapseProvider  (#13892)

* Implement IExpandCollapseProvider

* Change files

* Adjust Example

* Format + Update Snapshots

* Change files prerelease->patch

* update snapshots

---------

Co-authored-by: Chiara Mooney <34109996+chiaramooney@users.noreply.github.com>
Co-authored-by: React-Native-Windows Bot <53619745+rnbot@users.noreply.github.com>
Co-authored-by: Jon Thysell <jthysell@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment