Improving accessible object for label edit in ListView #6309

WPMGPRoSToTeMa · 2021-12-07T15:54:25Z

Fixes #4810. Implements an accessible object for label edit control in ListView. This object now is the last child of the ListView (the same behavior as in the Windows Explorer). The object now also has a non-empty name according to its value (Windows Explorer behavior). The original object wasn't producing automation events related to the text, it was done by the UIAutomationCore proxy -- this was also implemented along with the text pattern support for the new accessible object, so the Narrator announces its text correctly.

Proposed changes

Implemented native window wrapper and accessible object for label edit control in ListView
Implemented text pattern provider for label edit control (and added triggering automation events about label edit text)
Made label edit accessible object be the last child of ListView (Windows Explorer behavior)
Made label edit name value set according to its text value (Windows Explorer behavior)

Customer Impact

Narrator now announces the name of the label edit control

Regression?

No

Risk

Minimal

Screenshots

Before

After

Test methodology

Manual (using inspect and Narrator tools to verify hierarchy and text pattern)
Unit-tests (not currently presented)
CTI (planned)

Test environment(s)

Microsoft Windows [Version 10.0.19044.1348]
.NET 7.0.0-alpha.1.21602.1

Microsoft Reviewers: Open in CodeFlow

Tanya-Solyanik

Looks good. I added cleanup-y comments only.
I did not review text patterns and pinvoke definitions yet.

Tanya-Solyanik · 2021-12-08T02:47:45Z

src/System.Windows.Forms/src/System/Windows/Forms/ListView.cs

@@ -6481,6 +6490,13 @@ private unsafe void WmReflectNotify(ref Message m)

                case (int)LVN.ENDLABELEDITW:
                    {
+                        Debug.Assert(_childLabelEdit is not null);


Consider a symmetric assert in BEGINLABELEDIT case

Though I would throw here if _childLabelEdit is null.

src/System.Windows.Forms/src/System/Windows/Forms/ListView.cs

src/System.Windows.Forms/src/System/Windows/Forms/ListView.ListViewAccessibleObject.cs

src/System.Windows.Forms/src/System/Windows/Forms/ListViewChildLabelEditNativeWindow.cs

Tanya-Solyanik · 2021-12-08T04:27:12Z

src/System.Windows.Forms/src/System/Windows/Forms/ListViewChildLabelEditNativeWindow.cs

+
+        public ListViewChildLabelEditNativeWindow(ListView owner)
+        {
+            _owner = owner;


THrow is argument is null.

src/System.Windows.Forms/src/System/Windows/Forms/ListViewChildLabelEditNativeWindow.cs

src/System.Windows.Forms.Primitives/src/Interop/User32/Interop.SetWinEventHook.cs

src/System.Windows.Forms/src/System/Windows/Forms/ListViewChildLabelEditNativeWindow.cs

Tanya-Solyanik

I did not finish the review

src/System.Windows.Forms/src/System/Windows/Forms/ListViewChildLabelEditUiaTextProvider.cs

Tanya-Solyanik · 2021-12-14T17:55:42Z

src/System.Windows.Forms/src/System/Windows/Forms/ListViewChildLabelEditUiaTextProvider.cs

+            int index = (int)SendMessageW(_owningChildEdit, (WM)EM.CHARFROMPOS, 0, PARAM.FromPoint(pt));
+            index = PARAM.LOWORD(index);
+
+            if (index < 0)


This is the case when the point is outside the control, I don't see why we are returning the first char.

src/System.Windows.Forms/src/System/Windows/Forms/ListViewChildLabelEditUiaTextProvider.cs

RussKie · 2021-12-15T01:40:43Z

src/System.Windows.Forms.Primitives/src/Interop/User32/Interop.SetWinEventHook.cs

+    internal static partial class User32
+    {
+        [DllImport(Libraries.User32, ExactSpelling = true, SetLastError = true)]
+        public static extern nint SetWinEventHook(


Is this method used outside this file?

It indeed doesn't so I made it private.

src/System.Windows.Forms.Primitives/src/Interop/User32/Interop.SetWinEventHook.cs

src/System.Windows.Forms.Primitives/src/Interop/User32/Interop.WINEVENT.cs

src/System.Windows.Forms/src/System/Windows/Forms/ListViewChildLabelEditUiaTextProvider.cs

RussKie · 2021-12-15T01:49:39Z

src/System.Windows.Forms/src/System/Windows/Forms/ListViewChildLabelEditUiaTextProvider.cs

+
+        private readonly ListView _owningListView;
+
+        public ListViewChildLabelEditUiaTextProvider(ListView owner, IHandle childEdit, AccessibleObject childEditAccessibilityObject)


childEditAccessibilityObject must either be NRT annotated or checked for null below.
NRT annotations as such do not enforce nullability, they only act as compiler hints. This means it is possible to pass a null here and have the app crash at runtime, hence we still need to enforce the contract.

Whilst this is a non public API (and hence we control the input parameters) we had a number of NRE related issues in the accessibility implementations, and @vladimir-krestov and @SergeySmirnov-Akvelon spent a significant amount of time fixing our implementations. So in the interest of consistency and to reduce the maintenance overhead of these already complex API please continue null checking .ctor input args.

src/System.Windows.Forms/src/System/Windows/Forms/ListViewChildLabelEditUiaTextProvider.cs

RussKie · 2021-12-15T01:54:13Z

src/System.Windows.Forms/src/System/Windows/Forms/ListViewLabelEditAccessibleObject.cs

+        public ListViewLabelEditAccessibleObject(ListView owner, IHandle handle)
+        {
+            _owner = owner;


owner must either be NRT annotated or checked for null. Please make sure to provide test coverage for all such scenarios.

RussKie · 2021-12-15T01:55:16Z

src/System.Windows.Forms/src/System/Windows/Forms/ListViewLabelEditNativeWindow.cs

+        public ListViewLabelEditNativeWindow(ListView owner)
+        {
+            _owner = owner;
+        }


Ditto here.

src/System.Windows.Forms/src/System/Windows/Forms/ListViewLabelEditNativeWindow.cs

RussKie · 2022-01-18T04:58:32Z

src/System.Windows.Forms/src/System/Windows/Forms/ListViewLabelEditNativeWindow.cs

+        private ListView _owner;
+
+        private AccessibleObject? _accessibilityObject;
+
+        private User32.WINEVENTPROC? _winEventProc;
+
+        private nint _valueChangeHook;
+        private nint _textSelectionChangedHook;
+
+        private bool _winEventHooksInstalled;


Please keep these together

RussKie · 2022-01-18T04:58:49Z

src/System.Windows.Forms/src/System/Windows/Forms/ListViewLabelEditAccessibleObject.cs

+        private IntPtr _handle;
+        private int[]? _runtimeId;
+
+        public ListViewLabelEditAccessibleObject(ListView owner, IHandle handle)


I vaguely remember a discussion with @vladimir-krestov and @SergeySmirnov-Akvelon about prefixing the first parameter on non public types with "owning", e.g. owningListView.

Yes, we decided to make every owning field in one style - _owning[Control], eg. _owningListView

src/System.Windows.Forms/src/System/Windows/Forms/ListViewLabelEditAccessibleObject.cs

RussKie · 2022-01-18T05:06:23Z

src/System.Windows.Forms/src/System/Windows/Forms/ListViewLabelEditNativeWindow.cs

+            // Install winevent hooks only in the case when the parent listview already has an accessible object created.
+            // If we won't install hooks at the label edit startup then Narrator won't announce the text pattern for it.
+            // If we will just check for UiaClientsAreListening then we will have hooks installed even if Narrator isn't currently run.


Did I get correctly the intent?

Suggested change

// Install winevent hooks only in the case when the parent listview already has an accessible object created.

// If we won't install hooks at the label edit startup then Narrator won't announce the text pattern for it.

// If we will just check for UiaClientsAreListening then we will have hooks installed even if Narrator isn't currently run.

// Install winevent hooks *only* when the parent listview has an existing accessible object.

// If we don't install hooks at the label edit startup then assistive tech (e.g. Narrator) won't announce the text pattern for it.

// By invoking UiaClientsAreListening we will have hooks installed even if the assistive tech isn't currently run.

Also "...then assistive tech (e.g. Narrator) won't announce the text pattern for it." What is "it" in this context?

Thanks for cleaning up the texts. I assume "it" means the label edit control.

RussKie · 2022-01-18T05:09:48Z

src/System.Windows.Forms/src/System/Windows/Forms/ListViewLabelEditNativeWindow.cs

+
+        private AccessibleObject RequestAccessibilityObject()
+        {
+            AccessibleObject result = AccessibilityObject;


Why is it called result?

RussKie · 2022-01-18T05:14:04Z

src/System.Windows.Forms/src/System/Windows/Forms/ListViewLabelEditNativeWindow.cs

+            _winEventHooksInstalled = true;
+        }
+
+        private AccessibleObject RequestAccessibilityObject()


I am finding the method name at odds with its implementation. The name suggests it sends or initiate a request somewhere to get an accessibility object. In reality it just deferences AccessibilityObject property and install winhooks. The last bit is a side effect, which has an element of surprise.

I renamed it and also made it a local function inside WmGetObject because it's used only there, and the comment makes more sense within WmGetObject.

src/System.Windows.Forms/src/System/Windows/Forms/ListViewLabelEditNativeWindow.cs

vladimir-krestov

Good work!

src/System.Windows.Forms.Primitives/src/Interop/User32/Interop.SetWinEventHook.cs

vladimir-krestov · 2022-01-31T19:05:44Z

src/System.Windows.Forms/src/System/Windows/Forms/ListViewLabelEditAccessibleObject.cs

+        private IntPtr _handle;
+        private int[]? _runtimeId;
+
+        public ListViewLabelEditAccessibleObject(ListView owner, IHandle handle)


Yes, we decided to make every owning field in one style - _owning[Control], eg. _owningListView

src/System.Windows.Forms/src/System/Windows/Forms/ListViewLabelEditAccessibleObject.cs

vladimir-krestov · 2022-01-31T19:18:54Z

src/System.Windows.Forms/src/System/Windows/Forms/ListViewLabelEditNativeWindow.cs

+                _winEventHooksInstalled = false;
+            }
+
+            if (_accessibilityObject is not null)


I would create IsAccessibilityObjectCreated created for readability, like we did for ControlAO

vladimir-krestov · 2022-01-31T19:21:58Z

src/System.Windows.Forms/src/System/Windows/Forms/ListViewLabelEditNativeWindow.cs

+
+        private void WinEventProc(nint hWinEventHook, uint eventId, nint hwnd, int idObject, int idChild, uint idEventThread, uint dwmsEventTime)
+        {
+            if (hwnd != Handle || idObject != User32.OBJID.CLIENT)


Do we need to check if AO is created before?

Added check for IsAccessibilityObjectCreated

src/System.Windows.Forms/src/System/Windows/Forms/ListViewLabelEditUiaTextProvider.cs

...Windows.Forms/tests/UnitTests/System/Windows/Forms/ListViewLabelEditAccessibleObjectTests.cs

Tanya-Solyanik · 2022-06-06T20:45:10Z

@dmitrii-drobotov - the placement of edit box seems incorrect to me in the file explorer (and this PR). DGV placement looks more logical. However priority-wise this is not as high as other issues, since File Explorer had not fixed it in win11. Please open a new issue with this info - #6309 (comment) and we'll triage it.

src/System.Windows.Forms/src/System/Windows/Forms/ListView.cs

Tanya-Solyanik

Please address comment about the uint in the enum type. When had this been tested by the CTIs, should it be re-tested or was the delta since then minor?

src/System.Windows.Forms/src/System/Windows/Forms/ListViewLabelEditNativeWindow.cs

src/System.Windows.Forms.Primitives/src/Interop/User32/Interop.WINEVENT.cs

dmitrii-drobotov · 2022-08-04T17:08:33Z

Please address comment about the uint in the enum type. When had this been tested by the CTIs, should it be re-tested or was the delta since then minor?

This PR actually hasn't been tested by the CTI yet. I'll make a request once I address these code review points.

Fixes dotnet#4810. Implements an accessible object for label edit control in ListView. This object now is the last child of the ListView (the same behavior as in the Windows Explorer). The object now also has a non-empty name according to its value (Windows Explorer behavior). The original object wasn't producing automation events related to the text, it was done by the UIAutomationCore proxy -- this was also implemented along with the text pattern support for the new accessible object, so the Narrator announces its text correctly.

* Update ListViewLabelEditAccessibleObject after TextProvider refactoring * Add new unit tests for constructor arguments

dmitrii-drobotov · 2022-08-08T14:31:34Z

There were 2 issues in the CTI test report, but both of them are related to Inspect not updating its UIA tree automatically and would be resolved after making a refresh. It's also the same behavior as in other Winforms controls with edit like TreeView or DataGridView and in Windows file explorer. Therefore I can say that both issues are expected and don't need to be fixed.

There are no other issues found, so this PR passes the testing.

RussKie · 2022-08-09T02:22:16Z

@dmitrii-drobotov this is causing the main to fail with

D:\a\_work\1\s\src\System.Windows.Forms\src\System\Windows\Forms\ListViewLabelEditUiaTextProvider.cs(12,20): error CS0534: 'ListViewLabelEditUiaTextProvider' does not implement inherited abstract member 'UiaTextProvider.RectangleToScreen(Rectangle)' [D:\a\_work\1\s\src\System.Windows.Forms\src\System.Windows.Forms.csproj]
##[error]src\System.Windows.Forms\src\System\Windows\Forms\ListViewLabelEditUiaTextProvider.cs(12,20): error CS0534: (NETCORE_ENGINEERING_TELEMETRY=Build) 'ListViewLabelEditUiaTextProvider' does not implement inherited abstract member 'UiaTextProvider.RectangleToScreen(Rectangle)'

Build FAILED.

ghost assigned WPMGPRoSToTeMa Dec 7, 2021

ghost added the draft draft PR label Dec 7, 2021

WPMGPRoSToTeMa added the waiting-review This item is waiting on review by one or more members of team label Dec 7, 2021

WPMGPRoSToTeMa force-pushed the Issue_4810_Improving_ListView_LabelEdit_AccessibleObject branch 3 times, most recently from 7dc0326 to 16670aa Compare December 8, 2021 02:14

Tanya-Solyanik reviewed Dec 8, 2021

View reviewed changes

Tanya-Solyanik requested review from SergeySmirnov-Akvelon, ArtemTatarinov, vladimir-krestov and lisoenot December 8, 2021 04:35

Tanya-Solyanik reviewed Dec 8, 2021

View reviewed changes

src/System.Windows.Forms.Primitives/src/Interop/User32/Interop.SetWinEventHook.cs Show resolved Hide resolved

Tanya-Solyanik reviewed Dec 8, 2021

View reviewed changes

src/System.Windows.Forms.Primitives/src/Interop/User32/Interop.SetWinEventHook.cs Outdated Show resolved Hide resolved

Tanya-Solyanik reviewed Dec 8, 2021

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/ListViewChildLabelEditNativeWindow.cs Outdated Show resolved Hide resolved

WPMGPRoSToTeMa force-pushed the Issue_4810_Improving_ListView_LabelEdit_AccessibleObject branch from 16670aa to 7a046e6 Compare December 14, 2021 14:15

Tanya-Solyanik reviewed Dec 14, 2021

View reviewed changes

RussKie reviewed Dec 15, 2021

View reviewed changes

WPMGPRoSToTeMa removed the waiting-review This item is waiting on review by one or more members of team label Jan 3, 2022

WPMGPRoSToTeMa force-pushed the Issue_4810_Improving_ListView_LabelEdit_AccessibleObject branch 3 times, most recently from 70da02f to 58ab17c Compare January 11, 2022 16:04

WPMGPRoSToTeMa marked this pull request as ready for review January 11, 2022 16:07

WPMGPRoSToTeMa requested a review from a team as a code owner January 11, 2022 16:07

WPMGPRoSToTeMa removed the draft draft PR label Jan 11, 2022

WPMGPRoSToTeMa force-pushed the Issue_4810_Improving_ListView_LabelEdit_AccessibleObject branch 2 times, most recently from fc5fe56 to 434d304 Compare January 11, 2022 20:08

RussKie reviewed Jan 18, 2022

View reviewed changes

RussKie added the waiting-author-feedback The team requires more information from the author label Jan 18, 2022

vladimir-krestov reviewed Jan 31, 2022

View reviewed changes

RussKie assigned vladimir-krestov Apr 5, 2022

Tanya-Solyanik reviewed Jun 6, 2022

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/ListView.cs Show resolved Hide resolved

src/System.Windows.Forms/src/System/Windows/Forms/ListView.cs Outdated Show resolved Hide resolved

dmitrii-drobotov mentioned this pull request Jun 7, 2022

Label edit in ListView has incorrect position in accessibility tree #7263

Closed

dmitrii-drobotov force-pushed the Issue_4810_Improving_ListView_LabelEdit_AccessibleObject branch from c9a3481 to 166e165 Compare June 7, 2022 16:42

dmitrii-drobotov force-pushed the Issue_4810_Improving_ListView_LabelEdit_AccessibleObject branch from 166e165 to b691179 Compare June 23, 2022 13:40

runfoapp bot mentioned this pull request Jun 23, 2022

Flaky System.Windows.Forms.UITests.ButtonTests.Button_Anchor_ResizeOnWindowSizeWiderAsync #7297

Closed

dmitrii-drobotov force-pushed the Issue_4810_Improving_ListView_LabelEdit_AccessibleObject branch from b691179 to 0001879 Compare June 30, 2022 09:11

Tanya-Solyanik reviewed Aug 4, 2022

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/ListViewLabelEditNativeWindow.cs Show resolved Hide resolved

src/System.Windows.Forms.Primitives/src/Interop/User32/Interop.WINEVENT.cs Outdated Show resolved Hide resolved

Tanya-Solyanik added waiting-author-feedback The team requires more information from the author and removed waiting-review This item is waiting on review by one or more members of team labels Aug 4, 2022

WPMGPRoSToTeMa and others added 2 commits August 5, 2022 10:17

Refactor and make fixes after code review

b9b4c7e

* Update ListViewLabelEditAccessibleObject after TextProvider refactoring * Add new unit tests for constructor arguments

dmitrii-drobotov force-pushed the Issue_4810_Improving_ListView_LabelEdit_AccessibleObject branch from 0001879 to b9b4c7e Compare August 5, 2022 12:21

dmitrii-drobotov added waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) and removed waiting-author-feedback The team requires more information from the author labels Aug 5, 2022

dmitrii-drobotov 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 Aug 8, 2022

Tanya-Solyanik approved these changes Aug 8, 2022

View reviewed changes

Tanya-Solyanik added the ready-to-merge PRs that are ready to merge but worth notifying the internal team. label Aug 8, 2022

RussKie removed the ready-to-merge PRs that are ready to merge but worth notifying the internal team. label Aug 8, 2022

RussKie merged commit 12f6993 into dotnet:main Aug 8, 2022

ghost added this to the 7.0 RC1 milestone Aug 8, 2022

RussKie unassigned WPMGPRoSToTeMa Aug 9, 2022

ghost assigned WPMGPRoSToTeMa Aug 9, 2022

ghost locked as resolved and limited conversation to collaborators Sep 8, 2022


		private readonly ListView _owningListView;

		public ListViewChildLabelEditUiaTextProvider(ListView owner, IHandle childEdit, AccessibleObject childEditAccessibilityObject)

Improving accessible object for label edit in ListView #6309

Improving accessible object for label edit in ListView #6309

Uh oh!

Conversation

WPMGPRoSToTeMa commented Dec 7, 2021 • edited by ghost 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

Test environment(s)

Microsoft Reviewers: Open in CodeFlow

Uh oh!

Tanya-Solyanik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Tanya-Solyanik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RussKie Dec 15, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

WPMGPRoSToTeMa commented Dec 7, 2021 •

edited by ghost

Loading

RussKie Dec 15, 2021 •

edited

Loading

Tanya-Solyanik left a comment •

edited

Loading

RussKie commented Aug 9, 2022 •

edited

Loading