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

WIP: Adding TextPattern providers to TextBox and UpDown controls #2701

Open
wants to merge 7 commits into
base: master
from

Conversation

@M-Lipin
Copy link
Member

M-Lipin commented Jan 13, 2020

Fixes #1588

Proposed changes

  • Adding ITextPatternProvider and ITextPatternProvider2 and related imported accessibility interfaces to WinForms Accessibility.
  • Implementing TextPattern Provider functionality to allow accessibility client apps interact and announce (screen readers) text content of the Text controls.

Customer Impact

  • Visually impaired users will be able to read and interact with the text content of the input text elements.
  • Text inputs and up-down inputs will be fully accessible.

Regression?

  • No

Risk

  • Minimal

Screenshots

Before

Narrator does not announce the text navigation and selection of the TextBox, MaskedTextBox and up-down controls.

After

Narrator announces the text content of the text input controls (navigation, selection)

Test methodology

  • Manual testing.
  • Unit testing (to be implemented).
  • Automation tests.

Accessibility testing

Test environment(s)

.NET Core SDK (reflecting any global.json):
Version: 5.0.100-alpha1-015763
Commit: 0d0c902b77

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-015763\

Host (useful for support):
Version: 5.0.0-alpha.1.19564.1
Commit: c77948d92a

.NET Core SDKs installed:
3.0.101 [C:\Program Files\dotnet\sdk]
3.1.100-preview3-014645 [C:\Program Files\dotnet\sdk]
5.0.100-alpha1-015763 [C:\Program Files\dotnet\sdk]

.NET Core runtimes installed:
Microsoft.AspNetCore.All 2.1.13 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
Microsoft.AspNetCore.App 2.1.13 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 3.0.1 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 3.1.0-preview3.19555.2 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 5.0.0-alpha1.19575.10 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.NETCore.App 2.1.13 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 3.0.1 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 3.1.0-preview3.19553.2 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 5.0.0-alpha.1.19564.1 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 5.0.0-alpha.1.19564.2 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.WindowsDesktop.App 3.0.1 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 3.1.0-preview3.19553.2 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 5.0.0-alpha.1.19564.2 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

Microsoft Reviewers: Open in CodeFlow
@M-Lipin M-Lipin requested a review from dotnet/dotnet-winforms as a code owner Jan 13, 2020
@M-Lipin M-Lipin changed the title Adding TextPattern providers to TextBox and UpDown controls WIP: Adding TextPattern providers to TextBox and UpDown controls Jan 13, 2020
internal partial class Kernel32
{
[DllImport(Libraries.Kernel32, CharSet = CharSet.Unicode)]
public static extern short GlobalAddAtom(string atomName);

This comment has been minimized.

Copy link
@hughbe

hughbe Jan 13, 2020

Contributor

Should be GlobalAddAtomW and ExactSpelling = true

This comment has been minimized.

Copy link
@RussKie

RussKie Jan 20, 2020

Member

It should probably be:

[DllImport(Libraries.Kernel32, ExactSpelling = true, CharSet = CharSet.Unicode)]
public static extern byte GlobalAddAtomW(char* lpString);

https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-globaladdatomw

The callsite should look like:

fixed (char* pAtomText = atomText) {
	GlobalAddAtomW(pAtomText);
}

Also worth noting, the string is limited to 255 chars.

{
internal partial class Kernel32
{
[DllImport(Libraries.Kernel32, ExactSpelling = true, CharSet = CharSet.Auto)]

This comment has been minimized.

Copy link
@hughbe

hughbe Jan 13, 2020

Contributor

don't need CharSet.Auto

internal static partial class UiaCore
{
[ComVisible(true)]
[ComImport()]

This comment has been minimized.

Copy link
@hughbe

hughbe Jan 13, 2020

Contributor

nit: redundant parens

{
internal static partial class UiaCore
{

This comment has been minimized.

Copy link
@hughbe

hughbe Jan 13, 2020

Contributor

nit


#endregion Public Methods

#region Public Properties

This comment has been minimized.

Copy link
@hughbe

hughbe Jan 13, 2020

Contributor

remove regions

[StructLayout(LayoutKind.Sequential)]
public struct GUITHREADINFO
{
internal int cbSize;

This comment has been minimized.

Copy link
@hughbe

hughbe Jan 13, 2020

Contributor

make these public

internal static partial class User32
{
[DllImport(ExternDll.User32, SetLastError = true)]
internal static extern bool GetGUIThreadInfo(uint idThread, ref GUITHREADINFO guiThreadInfo);

This comment has been minimized.

Copy link
@hughbe

hughbe Jan 13, 2020

Contributor

these should be public and use Libraries.User32. Should also be ExactSpelling = true

@@ -85,6 +85,7 @@ internal static class NativeMethods
public const int CW_USEDEFAULT = (unchecked((int)0x80000000)),
CWP_SKIPINVISIBLE = 0x0001,
COLOR_WINDOW = 5,
COLOR_WINDOWTEXT = 8,

This comment has been minimized.

Copy link
@hughbe

hughbe Jan 13, 2020

Contributor

can extract these to enum while we're here?

public static readonly AutomationTextAttribute UnderlineColorAttribute = new AutomationTextAttribute(Interop.UiaCore.TextAttributeIdentifier.UnderlineColorAttributeId);
public static readonly AutomationTextAttribute UnderlineStyleAttribute = new AutomationTextAttribute(Interop.UiaCore.TextAttributeIdentifier.UnderlineStyleAttributeId);

//public static readonly AutomationEvent TextChangedEvent = new AutomationEvent(Interop.UiaCore.UIA.TextChangedEventId);

This comment has been minimized.

Copy link
@hughbe

hughbe Jan 13, 2020

Contributor

??

[Fact]
public void TextBoxAccessibleObject_SupportsTextPatterns()
{
TextBox textBox = new TextBox();
Comment on lines +11 to +14

This comment has been minimized.

Copy link
@RussKie
Assert.True((bool)textBoxAccessibleObject.GetPropertyValue(Interop.UiaCore.UIA.IsTextPatternAvailablePropertyId));
Assert.True((bool)textBoxAccessibleObject.GetPropertyValue(Interop.UiaCore.UIA.IsTextPattern2AvailablePropertyId));

Assert.True(textBoxAccessibleObject.IsPatternSupported(Interop.UiaCore.UIA.TextPatternId));
Assert.True(textBoxAccessibleObject.IsPatternSupported(Interop.UiaCore.UIA.TextPattern2Id));
Comment on lines +17 to +21

This comment has been minimized.

Copy link
@RussKie

RussKie Jan 16, 2020

Member

Technically there are 4 tests here - 2 for GetPropertyValue() and 2 for IsPatternSupported(). Both cases should presented as Theory with InlineData.

public class UiaTextProviderTests
{
[Fact]
public void UiaTextProvider_IsReadingRTL_ReturnsCorrectValue()

This comment has been minimized.

Copy link
@RussKie

RussKie Jan 16, 2020

Member

Please convert to a theory tests with inputs (RightToLeft given, bool expected)

Added a TextBox test app.
get
{
int style = GetWindowStyle();
return IsBitSet(style, NativeMethods.ES_AUTOHSCROLL) || IsBitSet(style, NativeMethods.ES_AUTOHSCROLL);

This comment has been minimized.

Copy link
@vladimir-krestov

vladimir-krestov Jan 17, 2020

Contributor

These are the same conditions. Which bit should be set? ES_AUTOVSCROLL?

This comment has been minimized.

Copy link
@RussKie

RussKie Jan 17, 2020

Member

Good spot! I think it it a typo, and should be "V" in one of them.
Could you investigate whether it is a recent regression or it has been like this in .NET Framework as well?

return _owner.GetLineIndex(line);
}

public override int GetLinesPerPage()

This comment has been minimized.

Copy link
@vladimir-krestov

vladimir-krestov Jan 17, 2020

Contributor

LinesPerPage property is already implemented above.
This method is used once so I propose to remove this method and use LinesPerPage property

Copy link
Member

RussKie left a comment

@@ -2823,5 +2602,215 @@ protected override void WndProc(ref Message m)
break;
}
}

internal class TextBoxBaseUiaTextProvider : UiaTextProvider2

This comment has been minimized.

Copy link
@RussKie

RussKie Jan 20, 2020

Member

Please move to own file

return new UiaTextRange(_owner.AccessibilityObject, this, _owner.SelectionStart, _owner.SelectionStart);
}

public override int GetFirstVisibleLine()

This comment has been minimized.

Copy link
@RussKie

RussKie Jan 20, 2020

Member

This and other members like this can be converted to expression body.

internal partial class Kernel32
{
[DllImport(Libraries.Kernel32, CharSet = CharSet.Unicode)]
public static extern short GlobalAddAtom(string atomName);

This comment has been minimized.

Copy link
@RussKie

RussKie Jan 20, 2020

Member

It should probably be:

[DllImport(Libraries.Kernel32, ExactSpelling = true, CharSet = CharSet.Unicode)]
public static extern byte GlobalAddAtomW(char* lpString);

https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-globaladdatomw

The callsite should look like:

fixed (char* pAtomText = atomText) {
	GlobalAddAtomW(pAtomText);
}

Also worth noting, the string is limited to 255 chars.


ITextRangeProvider RangeFromAnnotation(IRawElementProviderSimple annotation);

ITextRangeProvider GetCaretRange([MarshalAs(UnmanagedType.Bool)] out bool isActive);

This comment has been minimized.

Copy link
@RussKie

RussKie Jan 20, 2020

Member
Suggested change
ITextRangeProvider GetCaretRange([MarshalAs(UnmanagedType.Bool)] out bool isActive);
ITextRangeProvider GetCaretRange(out BOOL isActive);
{
public enum TextAttributeIdentifier
{
AfterParagraphSpacingAttributeId = 40042,

This comment has been minimized.

Copy link
@RussKie

RussKie Jan 20, 2020

Member

Sort in value asc order

[DllImport(Interop.Libraries.UiaCore, EntryPoint = "UiaGetReservedNotSupportedValue", CharSet = CharSet.Unicode)]
private static extern int RawUiaGetReservedNotSupportedValue([MarshalAs(UnmanagedType.IUnknown)] out object notSupportedValue);
Comment on lines +23 to +24

This comment has been minimized.

Copy link
@RussKie

RussKie Jan 20, 2020

Member
        [DllImport(Interop.Libraries.UiaCore, ExactSpelling = true)]
        private static extern int UiaGetReservedNotSupportedValue([MarshalAs(UnmanagedType.IUnknown)] out object notSupportedValue);

        public static object UiaGetReservedNotSupportedValue()
        {
            if (notSupportedValue == null)
            {
                UiaGetReservedNotSupportedValue(out notSupportedValue);
            }

            return notSupportedValue;
        }
internal static partial class User32
{
[DllImport(ExternDll.User32, CharSet = CharSet.Unicode, SetLastError = true)]
internal static extern uint RealGetWindowClass(IntPtr hwnd, StringBuilder className, uint maxCount);

This comment has been minimized.

Copy link
@RussKie

RussKie Jan 20, 2020

Member

This signature must reflect the Win32 API and use char*.
Then have a public method that takes a string and marshals it to the interop, e.g.

        [DllImport(ExternDll.User32, ExactSpelling = true, CharSet = CharSet.Unicode, SetLastError = true)]
        private static extern uint RealGetWindowClassW(IntPtr hwnd, char* ptszClassName, uint maxCount);

        public static unsafe uint RealGetWindowClassW(IntPtr hwnd, string className, uint maxCount)
		{
			fixed (char* pClassName = className)
			{
				return RealGetWindowClassW(hwnd, pClassName, maxCount);
			}
		}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.