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

Fix accessibilityState to support states not being set #11792

Merged
merged 12 commits into from
Jul 28, 2023

Conversation

jonthysell
Copy link
Contributor

@jonthysell jonthysell commented Jun 21, 2023

Description

Our existing implementation for accessibilityState does not support that the values of a state can be null. This means every View has every unset state with the default false value.

The problem stems from implementing the short-lived accessibilityStates prop at the same time as accessibilityState, but not removing the deprecated API right away.

This PR fixes our implementation to only include the states when explicitly set, and to otherwise clear them. This PR also removed the no longer required "opposite" states like Collapsed and Unchecked.

Resolves #11791

Type of Change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Why

Properly implement accessibility in preparation for doing the work correctly for Fabric.

Resolves #11791

What

Removed the old unused states and updated FrameworkElementViewManager to allow for unset values rather than forcing the default false. Updated AccessbilityStateChecked to accept an enum of Unchecked, Mixed, or Checked. Updated the DynamicAutomationPeer to only express a View has the appropriate provider iff the matching state is present on the view.

Screenshots

N/A

Testing

Updated E2E test snapshots to include the AccessibilityRole and AccessibilityState properties in the XAML tree dumps.

Microsoft Reviewers: codeflow:open?pullrequest=#11792

Our existing implementation for `accessibilityState` does not support
that the values of a state can be `null`. This means *every* View has
*every* unset state with the default `false` value.

The problem stems from implementing the short-lived `accessibilityStates`
prop at the same time.

This PR fixes our implementation to only include the states when
explicitly set.

Resolves microsoft#11791
@jonthysell
Copy link
Contributor Author

Need to get XamlTreeDump updated to test this properly, see asklar/XamlTreeDump#10

jonthysell added a commit to asklar/XamlTreeDump that referenced this pull request Jun 28, 2023
…anup

This PR accomplishes several tasks to refresh `XamlTreeDump`, which is still used by [React Native for Windows](https://aka.ms/reactnative).

1. Added an `ExcludeIfValueIsUnset` boolean to `AttachedProperty`, which allows callers to specify that they don't want the attached property included in the dump if it's not explicitly set on the element. This specifically fixes the use case of adding more attached properties without necessarily polluting dumps with default values on every element (and allowing for checking that actual values were set).
2. Added a `PropertyValueConverter` delegate to `AttachedProperty`, which lets callers perform custom transformations on the values of attached properties. This specifically fixes the use case where custom WinRT enum types are stored in attached properties and the typing isn't available to XamlTreeDump, causing values to be returned as `System.__COMObject` types. Callers (with access to the correct typing) can do the transformation on XamlTreeDump's behalf.
3. Fixes an issue where boolean property values were not returned in a valid JSON format ('true' instead of 'True').
4. Fixes the LastVersion to the actual last published version (1.0.9).
5. Adds copyright and license headers to every source file
6. Cleans up line endings and code formatting

All changes have added unit tests and have been validated by this PR consuming the new features: microsoft/react-native-windows#11792
@jonthysell jonthysell changed the title [WIP] Fix accessibilityState to support states not being set Fix accessibilityState to support states not being set Jun 28, 2023
@jonthysell jonthysell marked this pull request as ready for review June 28, 2023 21:41
@jonthysell jonthysell requested review from a team as code owners June 28, 2023 21:41
@jonthysell jonthysell added the Breaking Change This PR will break existing apps and should be part of the known breaking changes for the release label Jun 28, 2023
@jonthysell
Copy link
Contributor Author

Tagging as a breaking change since I am changing idls.

Copy link
Contributor

@chiaramooney chiaramooney left a comment

Choose a reason for hiding this comment

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

Looks good to me

@jonthysell jonthysell enabled auto-merge (squash) July 28, 2023 18:31
@jonthysell jonthysell disabled auto-merge July 28, 2023 20:23
@jonthysell jonthysell merged commit 13c1223 into microsoft:main Jul 28, 2023
44 checks passed
@jonthysell jonthysell deleted the accStateFix branch July 28, 2023 20:24
Copy link
Contributor

@chiaramooney chiaramooney left a comment

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Accessibility 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.

UIA showing Expand/Collapsed property option on non-expandable components
3 participants