Adding CheckBox control UIA accessibility by M-Lipin · Pull Request #3228 · dotnet/winforms

M-Lipin · 2020-05-06T18:58:29Z

Fixes #3227

Proposed changes

Adding UIA providers support for CheckBox control.
Moving CheckBox control accessible object to a separate file.

Customer Impact

Improving development experience for Button control accessibility.
Prerequisite for Narrator focus is not moving in proper logical order when user is navigating by using caps + arrow keys (tab index is ignored) #2164

Regression?

No

Risk

Minimal

Screenshots

Before

After

Test methodology

Manual testing.
Unit tests.
UI automation tests.

Accessibility testing

Inspect tool (need to ensure that Inspect properties are similar to what we had before switching to UIA as well as accessibility actions should work properly: Default action, Toggle action and so on)
Accessibility Insights
Narrator

Test environment(s)

.NET Core 5.0
Version: 5.0.100-alpha.1.20073.10
Commit: 29f4d693a9
Runtime Environment:
OS Name: Windows
OS Version: 10.0.18363
OS Platform: Windows
RID: win10-x64
Base Path: C:\Program Files\dotnet\sdk\5.0.100-alpha1-05536
Host (useful for support):
Version: 5.0.0-alpha.1.20072.3
Commit: c3dc1fdfdc

Microsoft Reviewers: Open in CodeFlow

src/System.Windows.Forms/src/System/Windows/Forms/CheckBox.CheckBoxAccessibleObject.cs

RussKie

Please add tests

src/System.Windows.Forms/src/System/Windows/Forms/CheckBox.CheckBoxAccessibleObject.cs

weltkante · 2020-06-01T14:39:34Z

src/System.Windows.Forms/src/System/Windows/Forms/CheckBox.CheckBoxAccessibleObject.cs

Argument(Null)Exception seems inappropriate for reading a property

Thanks, fixed.

RussKie · 2020-06-02T10:47:06Z

src/System.Windows.Forms/src/System/Windows/Forms/CheckBox.CheckBoxAccessibleObject.cs

I've looked at the available exceptions, and I think it should be ArgumentException

I suggest to also add an error message, something like Argument '{0}' cannot be converted to type '{1}'.
Similar to

winforms/src/Microsoft.VisualBasic.Forms/src/Resources/SR.resx

Lines 128 to 130 in 7ccf509

<data name="Argument_InvalidValueType2" xml:space="preserve">

<value>Argument '{0}' cannot be converted to type '{1}'.</value>

</data>

Suggested change

public CheckBoxAccessibleObject(CheckBox owner) : base(owner)

public CheckBoxAccessibleObject(Control owner)

: base((owner as CheckBox) ?? throw new ArgumentException(string.Format(SR.Argument_InvalidValueType, nameof(Owner), typeof(CheckBox)), nameof(Owner)))

Is there a reason why you don't want to let the compiler do the work of ensuring the type (by declaring the argument type CheckBox and just doing a null check) ?

Declaring the argument type Control but throwing on anything but a CheckBox seems weird.

Normally we would force CheckBox owner, however unlike many other accessibility API, this one is a public API, and changing type would be a breaking change. Granted it won't properly work with any other type, but this seems to be a lesser breaking change.

oh, didn't notice that, yes changing the signature of a public API will cause MethodNotFound when you run a 3.x program on the 5.0 runtime (a secenario that was declared as desirable in previous discussions)

Haha, I made the same mistake 3 hours earlier, and then Mikhail alerted me to the same fact.

There has to be a pretty important reason to break the API surface. I can't even come up with a hypothetical case where we would (security perhaps, but I can't imagine a way we couldn't address a security problem that requires us to change the surface area).

Thanks, fixed.

RussKie · 2020-06-02T11:11:57Z

Please rework the PR in the following manner:

First commit - move CheckBoxAccessibleObject to own file, no other changes
Add required changes to CheckBoxAccessibleObject. Please observe the file organisation, it is described here: https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1201.md (we'll turn this rule on at some point).

codecov · 2020-06-03T11:57:27Z

Codecov Report

Merging #3228 into master will decrease coverage by 27.96536%.
The diff coverage is 0.00000%.

@@                 Coverage Diff                  @@
##              master       #3228          +/-   ##
====================================================
- Coverage   62.14766%   34.18230%   -27.96537%     
====================================================
  Files           1257         891         -366     
  Lines         449428      253798      -195630     
  Branches       39227       36786        -2441     
====================================================
- Hits          279309       86754      -192555     
+ Misses        164638      162283        -2355     
+ Partials        5481        4761         -720

Flag	Coverage Δ
#Debug	`34.18230% <0.00000%> (-27.96537%)`	⬇️
#production	`34.18230% <0.00000%> (+0.84037%)`	⬆️
#test	`?`

RussKie

👍

RussKie · 2020-06-04T04:28:47Z

src/System.Windows.Forms/src/System/Windows/Forms/CheckBox.CheckBoxAccessibleObject.cs

OwningCheckBox can never be null
Same below

Thanks, fixed.

src/System.Windows.Forms/src/System/Windows/Forms/CheckBox.CheckBoxAccessibleObject.cs

src/System.Windows.Forms/tests/UnitTests/AccessibleObjects/CheckBoxAccessibleObjectTests.cs

src/System.Windows.Forms/src/System/Windows/Forms/CheckBox.CheckBoxAccessibleObject.cs

src/System.Windows.Forms/tests/UnitTests/AccessibleObjects/CheckBoxAccessibleObjectTests.cs

src/System.Windows.Forms/src/System/Windows/Forms/CheckBox.CheckBoxAccessibleObject.cs

RussKie · 2020-06-04T10:09:52Z

Please keep checks, just a comment. Once the bug is fixed we'll update the code. Please work with Mikhail through the issue to have a resolution plan. We'll sync up next week on this.

________________________________ From: Sergey Smirnov <notifications@github.com> Sent: Thursday, 4 June 2020 6:59 PM To: dotnet/winforms <winforms@noreply.github.com> Cc: Igor Velikorossov <Igor.Velikorossov@microsoft.com>; Review requested <review_requested@noreply.github.com> Subject: Re: [dotnet/winforms] Adding CheckBox control UIA accessibility (#3228) @SergeySmirnov-Akvelon commented on this pull request.

________________________________ In src/System.Windows.Forms/tests/UnitTests/AccessibleObjects/CheckBoxAccessibleObjectTests.cs<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdotnet%2Fwinforms%2Fpull%2F3228%23discussion_r435100223&data=02%7C01%7Cigor.velikorossov%40microsoft.com%7Cf1659f7e07c64ff5142108d80865a876%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637268580001823931&sdata=QsnKnvyBTpmc%2BmATknoteDj%2BH3jE3e34vIotmnGhjF8%3D&reserved=0>:

@@ -0,0 +1,54 @@

+// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System.Collections.Generic; +using Xunit; + +namespace System.Windows.Forms.Tests +{ + public class CheckBoxAccessibleObjectTests + { + [WinFormsFact] + public void CheckBoxAccessibleObject_GetPropertyValue_returns_correct_values() Currently we have issue with "IsHandleCreated" property. "AccessibleObject" class gets "Handle" control's property in constructor. As result, "IsHandleCreated" always will return "true" after creation of AccessibleObject. I removed checking of this property from tests. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdotnet%2Fwinforms%2Fpull%2F3228%23discussion_r435100223&data=02%7C01%7Cigor.velikorossov%40microsoft.com%7Cf1659f7e07c64ff5142108d80865a876%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637268580001823931&sdata=QsnKnvyBTpmc%2BmATknoteDj%2BH3jE3e34vIotmnGhjF8%3D&reserved=0>, or unsubscribe<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABBTEXSFNL7XFZ7WIMVHIDDRU5PA3ANCNFSM4M2WFHQQ&data=02%7C01%7Cigor.velikorossov%40microsoft.com%7Cf1659f7e07c64ff5142108d80865a876%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637268580001833924&sdata=6J0RBUfatMfOKHeoxtI2f7qtYGbmNxO1wINkt3Ww0PE%3D&reserved=0>.

RussKie

👍

src/System.Windows.Forms/tests/UnitTests/AccessibleObjects/CheckBoxAccessibleObjectTests.cs

src/System.Windows.Forms/src/System/Windows/Forms/CheckBox.cs

RussKie

MC, otherwise 👍

src/System.Windows.Forms/tests/UnitTests/AccessibleObjects/CheckBoxAccessibleObjectTests.cs

src/System.Windows.Forms/src/System/Windows/Forms/CheckBox.CheckBoxAccessibleObject.cs

codecov · 2020-06-18T07:38:56Z

Codecov Report

Merging #3228 into master will decrease coverage by 31.20954%.
The diff coverage is 56.41026%.

@@                 Coverage Diff                  @@
##              master       #3228          +/-   ##
====================================================
- Coverage   66.58991%   35.38037%   -31.20955%     
====================================================
  Files           1338         896         -442     
  Lines         501564      253553      -248011     
  Branches       40847       36764        -4083     
====================================================
- Hits          333991       89708      -244283     
+ Misses        162033      159012        -3021     
+ Partials        5540        4833         -707

Flag	Coverage Δ
#Debug	`35.38037% <56.41026%> (-31.20955%)`	⬇️
#production	`35.38037% <56.41026%> (+0.01720%)`	⬆️
#test	`?`

vladimir-krestov

Some code style issues

src/System.Windows.Forms/src/Resources/SR.resx

src/System.Windows.Forms/src/System/Windows/Forms/CheckBox.CheckBoxAccessibleObject.cs

...em.Windows.Forms/tests/UnitTests/AccessibleObjects/CheckBox.CheckBoxAccessibleObjectTests.cs

SergeySmirnov-Akvelon · 2020-06-29T11:15:54Z

Issue has been tested, but another one has been found in Narrator, so i will investigate it

SergeySmirnov-Akvelon · 2020-06-30T07:21:19Z

CTI approved

RussKie · 2020-06-30T10:28:52Z

/azp run

azure-pipelines · 2020-06-30T10:29:01Z

Azure Pipelines successfully started running 1 pipeline(s).

Issue is reproduced because we used incorrect propertyId. We should use the NamePropertyId instead of the ToggleToggleStatePropertyId.

codecov · 2020-06-30T12:03:26Z

Codecov Report

Merging #3228 into master will increase coverage by 31.50229%.
The diff coverage is 100.00000%.

@@                 Coverage Diff                  @@
##              master       #3228          +/-   ##
====================================================
+ Coverage   66.99234%   98.49463%   +31.50229%     
====================================================
  Files           1348         452         -896     
  Lines         505822      252829      -252993     
  Branches       40904        4167       -36737     
====================================================
- Hits          338862      249023       -89839     
+ Misses        161389        3092      -158297     
+ Partials        5571         714        -4857

Flag	Coverage Δ
#Debug	`98.49463% <100.00000%> (+31.50229%)`	⬆️
#production	`?`
#test	`98.49463% <100.00000%> (+0.00947%)`	⬆️

M-Lipin requested a review from a team as a code owner May 6, 2020 18:58

ghost assigned M-Lipin May 6, 2020

M-Lipin assigned Ryuugamine May 6, 2020

weltkante reviewed May 6, 2020

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/CheckBox.CheckBoxAccessibleObject.cs Outdated Show resolved Hide resolved

RussKie suggested changes May 7, 2020

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/CheckBox.CheckBoxAccessibleObject.cs Outdated Show resolved Hide resolved

ghost added the waiting-author-feedback The team requires more information from the author label May 7, 2020

weltkante reviewed Jun 1, 2020

View reviewed changes

ghost removed the waiting-author-feedback The team requires more information from the author label Jun 1, 2020

RussKie unassigned M-Lipin Jun 2, 2020

ghost assigned M-Lipin Jun 2, 2020

RussKie reviewed Jun 2, 2020

View reviewed changes

SergeySmirnov-Akvelon force-pushed the Issue_3227_CheckBox_control_UIA_accessibility branch from 733b49e to b4d6c65 Compare June 3, 2020 11:32

RussKie reviewed Jun 4, 2020

View reviewed changes

RussKie reviewed Jun 9, 2020

View reviewed changes

RussKie unassigned M-Lipin Jun 9, 2020

ghost assigned M-Lipin Jun 9, 2020

SergeySmirnov-Akvelon force-pushed the Issue_3227_CheckBox_control_UIA_accessibility branch from bd5a96f to 71a279d Compare June 9, 2020 12:31

SergeySmirnov-Akvelon mentioned this pull request Jun 10, 2020

The "RaiseAutomationEvent" method doesn't check "IsHandleCreated" flag. #3419

Closed

vladimir-krestov mentioned this pull request Jun 10, 2020

Epic - [Accessibility] Implement UIA provider for all controls #3421

Closed

70 tasks

RussKie approved these changes Jun 11, 2020

View reviewed changes

RussKie suggested changes Jun 11, 2020

View reviewed changes

src/System.Windows.Forms/tests/UnitTests/AccessibleObjects/CheckBoxAccessibleObjectTests.cs Outdated Show resolved Hide resolved

ghost added the waiting-author-feedback The team requires more information from the author label Jun 11, 2020

RussKie reviewed Jun 11, 2020

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/CheckBox.CheckBoxAccessibleObject.cs Outdated Show resolved Hide resolved

SergeySmirnov-Akvelon force-pushed the Issue_3227_CheckBox_control_UIA_accessibility branch 2 times, most recently from a641c62 to 38c86e4 Compare June 18, 2020 07:17

SergeySmirnov-Akvelon force-pushed the Issue_3227_CheckBox_control_UIA_accessibility branch from 38c86e4 to 9304099 Compare June 24, 2020 14:14

vladimir-krestov reviewed Jun 25, 2020

View reviewed changes

vladimir-krestov mentioned this pull request Jun 25, 2020

Fixng 3214 - UIA providers support for Button control #3215

Merged

SergeySmirnov-Akvelon force-pushed the Issue_3227_CheckBox_control_UIA_accessibility branch 4 times, most recently from d083f46 to 32070ac Compare June 26, 2020 14:18

SergeySmirnov-Akvelon added waiting-review This item is waiting on review by one or more members of team and removed waiting-author-feedback The team requires more information from the author labels Jun 26, 2020

RussKie added the waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Jun 29, 2020

RussKie previously approved these changes Jun 29, 2020

View reviewed changes

RussKie removed the waiting-review This item is waiting on review by one or more members of team label Jun 29, 2020

SergeySmirnov-Akvelon dismissed RussKie’s stale review via 6c25ae3 June 29, 2020 11:35

SergeySmirnov-Akvelon removed the waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Jun 30, 2020

SergeySmirnov-Akvelon force-pushed the Issue_3227_CheckBox_control_UIA_accessibility branch from 6c25ae3 to 2afb6ab Compare June 30, 2020 09:05

SergeySmirnov-Akvelon added 3 commits June 30, 2020 14:39

Moved CheckBoxAccessibleObject to own file

b88f867

Added CheckBox control UIA accessibility. Added unit tests

91b6b2e

Fixed issue in Narrator.

ca99ed0

Issue is reproduced because we used incorrect propertyId. We should use the NamePropertyId instead of the ToggleToggleStatePropertyId.

SergeySmirnov-Akvelon force-pushed the Issue_3227_CheckBox_control_UIA_accessibility branch from 2afb6ab to ca99ed0 Compare June 30, 2020 11:44

RussKie assigned SergeySmirnov-Akvelon and unassigned Ryuugamine Jun 30, 2020

RussKie approved these changes Jun 30, 2020

View reviewed changes

RussKie merged commit 4c1c9d6 into dotnet:master Jun 30, 2020

RussKie deleted the Issue_3227_CheckBox_control_UIA_accessibility branch June 30, 2020 12:05

ghost added this to the 5.0 Preview8 milestone Jun 30, 2020

ghost locked as resolved and limited conversation to collaborators Feb 1, 2022

	<data name="Argument_InvalidValueType2" xml:space="preserve">
	<value>Argument '{0}' cannot be converted to type '{1}'.</value>
	</data>

	public CheckBoxAccessibleObject(CheckBox owner) : base(owner)
	public CheckBoxAccessibleObject(Control owner)
	: base((owner as CheckBox) ?? throw new ArgumentException(string.Format(SR.Argument_InvalidValueType, nameof(Owner), typeof(CheckBox)), nameof(Owner)))

Conversation

M-Lipin commented May 6, 2020 • edited by RussKie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Customer Impact

Regression?

Risk

Screenshots

Before

After

Test methodology

Accessibility testing

Test environment(s)

Microsoft Reviewers: Open in CodeFlow

Uh oh!

Uh oh!

RussKie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

weltkante Jun 1, 2020

Choose a reason for hiding this comment

Uh oh!

SergeySmirnov-Akvelon Jun 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

RussKie Jun 2, 2020

Choose a reason for hiding this comment

Uh oh!

RussKie Jun 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weltkante Jun 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RussKie Jun 2, 2020

Choose a reason for hiding this comment

Uh oh!

weltkante Jun 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RussKie Jun 2, 2020

Choose a reason for hiding this comment

Uh oh!

JeremyKuhne Jun 2, 2020

Choose a reason for hiding this comment

Uh oh!

SergeySmirnov-Akvelon Jun 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RussKie commented Jun 2, 2020

Uh oh!

codecov bot commented Jun 3, 2020

Codecov Report

Uh oh!

RussKie left a comment

Choose a reason for hiding this comment

Uh oh!

RussKie Jun 4, 2020

Choose a reason for hiding this comment

Uh oh!

SergeySmirnov-Akvelon Jun 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RussKie commented Jun 4, 2020 via email

Uh oh!

M-Lipin commented May 6, 2020 •

edited by RussKie

Loading

SergeySmirnov-Akvelon Jun 8, 2020 •

edited

Loading

RussKie Jun 2, 2020 •

edited

Loading

weltkante Jun 2, 2020 •

edited

Loading

weltkante Jun 2, 2020 •

edited

Loading

SergeySmirnov-Akvelon Jun 8, 2020 •

edited

Loading

SergeySmirnov-Akvelon Jun 8, 2020 •

edited

Loading