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.

Sign up for GitHub

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

Jump to bottom

Refactor UiaTextRange For Improved Navigation and Reliability #4018

Merged
24 commits merged into from
Jan 31, 2020
Merged

Refactor UiaTextRange For Improved Navigation and Reliability #4018

24 commits merged into from
Jan 31, 2020

Conversation

carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented Dec 19, 2019 •
edited
Loading

Summary of the Pull Request

This pull request is intended to achieve the following goals...

  1. reduce duplicate code
  2. remove static functions
  3. improve readability
  4. improve reliability
  5. improve code-coverage for testing
  6. establish functioning text buffer navigation in Narrator and NVDA

This also required a change to the wrapper class XamlUiaTextRange that has been causing issues with Narrator and NVDA.

See below for additional context.

References

#3976 - I believe this might have been a result of improperly handling degenerate ranges. Fixed here.
#3895 - reduced the duplicate code. No need to separate into different files
#2160 - same as #3976 above
#1993 - I think just about everything is no longer static

PR Checklist

Detailed Description of the Pull Request / Additional comments

UiaTextRange

  • converted endpoints into the COORD system in the TextBuffer coordinate space
  • start is inclusive, end is exclusive. A degenerate range is when start == end.
  • all functions are no longer static
  • MoveByUnit() functions now rely on MoveEndpointByUnit() functions
  • removed unnecessary typedefs like Endpoint, ScreenInfoRow, etc..
  • relied more heavily on existing functionality from TextBuffer and Viewport

XamlUiaTextRange

  • GetAttributeValue() must return a special HRESULT that signifies that the requested attribute is not supported. This was the cause of a number of inconsistencies between Narrator and NVDA.
  • FindText() should return nullptr if nothing was found. Introduce UiaTextRangeBase::FindText() for Accessibility #4373 properly fixes this functionality now that Search is a shared module

TextBuffer

  • Word navigation functionality is entirely in TextBuffer for proper abstraction
  • a total of 6 functions are now dedicated to word navigation to get a good understanding of the differences between a "word" in Accessibility and a "word" in selection

As an example, consider a buffer with this text in it:
" word other "
In selection, a "word" is defined as the range between two delimiters, so the words in the example include [" ", "word", " ", "other", " "].
In accessibility , a "word" includes the delimiters after a range of readable characters, so the words in the example include ["word ", "other "].

Additionally, accessibility word navigation must be able to detect if it is on the first or last word. This resulted in a slight variant of word navigation functions that return a boolean instead of a COORD.

Ideally, these functions can be consolidated, but that is too risky for a PR of this size as it can have an effect on selection.

Viewport

  • the concept of EndExclusive is added. This is used by UiaTextRange's end anchor as it is exclusive. To signify that the last character in the buffer is included in this buffer, end must be one past the end of the buffer. This is EndExclusive
  • Since many functions check if the given COORD is in bounds, a flag must be set to allow EndExclusive as a valid COORD that is in bounds.

Testing

  • word navigation testing relies more heavily on TextBuffer tests
  • additional testing was created for non-movement focused functions of UiaTextRange
  • The results have been compared to Microsoft Word and some have been verified by UiAutomation/Narrator contacts as expected results.

Validation Steps Performed

Tests pass
Narrator works
NVDA works

carlos-zamora and others added 4 commits December 17, 2019 23:42
@carlos-zamora
utr refactor
719676c
@carlos-zamora
Refactor UiaTextRange:
e7555dc 
- remove _degenerate
- _start and _end are now COORDs
- _end is always exclusive
- de-static-fy functions
- all COORDs are in the text buffer coordinate system
@carlos-zamora
remove accidental file
76a26e3
@carlos-zamora
Add accessibilityMode to GetWordStart/End and Viewport functions.
d4c4e29 
Get some tests running again
@DHowett-MSFT
Copy link
Contributor

Make sure you say “closes x, closes y, closes z” to make sure GitHub can figure out what you mean!

carlos-zamora and others added 4 commits December 20, 2019 12:50
@carlos-zamora
Testing!!!!
3017aaa
@carlos-zamora
wtvr
f6b2ea2
Let's try doing word nav this way
9a40869
@carlos-zamora
Add testing for Compare, CompareEndpoint, Expand, and MoveEndpointByR…
ea22c8f 
…ange

Missing tests for...
- GetText
- Move
- MoveEndpointByUnit
Still need to think of how to approach Move and MoveEndpointByUnit since these should be very similar and should reuse a lot of code.
@carlos-zamora carlos-zamora force-pushed the dev/cazamor/utr-refactor-1 branch from bdd75bd to ea22c8f Compare January 17, 2020 23:48
@carlos-zamora
Copy link
Member Author

Hi @josephsl. You reached out last year about providing assistance with NVDA. We're experiencing an issue where NVDA is successfully getting the text from our TextRangeProvider, but, for some reason, it fails to actually read it. Think you could take a look or provide some guidance here?

We've updated NVDA to 2019.3beta2 and enabled the enhanced debugging logs. We're seeing this issue:

IO - inputCore.InputManager.executeGesture (14:49:22.266) - winInputHook (17428):
Input: kb(desktop):numpad7
DEBUG - NVDAObjects.UIA.UIATextInfo._getTextWithFieldsForUIARange (14:49:22.299) - MainThread (3932):
_getTextWithFieldsForUIARange
DEBUG - NVDAObjects.UIA.UIATextInfo._getTextWithFieldsForUIARange (14:49:22.301) - MainThread (3932):
rootElement: TerminalControl
DEBUG - NVDAObjects.UIA.UIATextInfo._getTextWithFieldsForUIARange (14:49:22.304) - MainThread (3932):
full text: -a----         1/16/2020  1:05 PM         106764 OpenConsole.sln
DEBUG - NVDAObjects.UIA.UIATextInfo._getTextWithFieldsForUIARange (14:49:22.306) - MainThread (3932):
Fetching parents starting from enclosingElement
DEBUG - NVDAObjects.UIA.UIATextInfo._getTextWithFieldsForUIARange (14:49:22.310) - MainThread (3932):
Hit root
DEBUG - NVDAObjects.UIA.UIATextInfo._getTextWithFieldsForUIARange (14:49:22.311) - MainThread (3932):
Done fetching parents
DEBUG - NVDAObjects.UIA.UIATextInfo._getTextWithFieldsForUIARange (14:49:22.311) - MainThread (3932):
Generating controlFields for parents
DEBUG - NVDAObjects.UIA.UIATextInfo._getTextWithFieldsForUIARange (14:49:22.312) - MainThread (3932):
Done generating controlFields for parents
DEBUG - NVDAObjects.UIA.UIATextInfo._getTextWithFieldsForUIARange (14:49:22.312) - MainThread (3932):
Yielding control starts for parents
DEBUG - NVDAObjects.UIA.UIATextInfo._getTextWithFieldsForUIARange (14:49:22.312) - MainThread (3932):
Done yielding control starts for parents
DEBUG - NVDAObjects.UIA.UIATextInfo._getTextWithFieldsForUIARange (14:49:22.313) - MainThread (3932):
Yielding balanced fields for textRange
DEBUG - NVDAObjects.UIA.UIATextInfo._getTextWithFieldsForUIARange (14:49:22.313) - MainThread (3932):
no children
DEBUG - NVDAObjects.UIA.UIATextInfo._getTextWithFieldsForUIARange (14:49:22.313) - MainThread (3932):
Yielding text
DEBUG - NVDAObjects.UIA.UIATextInfo._getTextWithFields_text (14:49:22.313) - MainThread (3932):
_getTextWithFields_text start
DEBUG - NVDAObjects.UIA.UIATextInfo._getTextWithFields_text (14:49:22.314) - MainThread (3932):
Walking by unit None, with further units of: [1, 2, 0]
DEBUG - NVDAObjects.UIA.UIATextInfo._getTextWithFields_text (14:49:22.317) - MainThread (3932):
Chunk has text. Fetching formatting
ERROR - scriptHandler.executeScript (14:49:22.320) - MainThread (3932):
error executing script: <bound method GlobalCommands.script_review_previousLine of <globalCommands.GlobalCommands object at 0x0D2CB9B0>> with gesture 'numpad 7'
Traceback (most recent call last):
  File "scriptHandler.pyc", line 205, in executeScript
  File "globalCommands.pyc", line 1038, in script_review_previousLine
  File "speech\__init__.pyc", line 952, in speakTextInfo
  File "NVDAObjects\UIA\__init__.pyc", line 704, in getTextWithFields
  File "NVDAObjects\UIA\__init__.pyc", line 692, in _getTextWithFieldsForUIARange
  File "NVDAObjects\UIA\__init__.pyc", line 482, in _getTextWithFields_text
  File "NVDAObjects\UIA\__init__.pyc", line 177, in _getFormatFieldAtRange
  File "UIAUtils.pyc", line 219, in __init__
  File "comtypesMonkeyPatches.pyc", line 26, in __call__
OSError: exception: access violation reading 0x00000000

It successfully extracts the text but it seems to be getting a null pointer exception somehow. Prior to this PR, NVDA was able to read the text, so I'm definitely doing something in this PR that is causing the issue. What's particularly confusing is that Narrator is not experiencing this issue.

Please let us know if you have any ideas for next steps. We want to make sure we do this right. Thanks!

@josephsl
Copy link

josephsl commented Jan 18, 2020 via email

@codeofdusk
Copy link
Contributor

codeofdusk commented Jan 18, 2020 via email

@codeofdusk
Copy link
Contributor

Can a .exe build of this new console be provided for testing? I suspect this PR will fix a number of issues we had to work around in NVDA.

@DHowett-MSFT
Copy link
Contributor

I guess one important question is: does NVDA treat windows terminal the same way it treats conhost.exe? The implementation for UIA is shared between the two of them, but if the behavior is different that could be a good starting point.

@carlos-zamora: you might try running Host.EXE and doing NVDA against that to reduce the number of variables :)

@codeofdusk
Copy link
Contributor

I guess one important question is: does NVDA treat windows terminal the same way it treats conhost.exe?

No, not yet. See nvaccess/nvda#10305

@carlos-zamora
Turns out, the bug is in our wrapper class. Not supposed to return nu…
bcfc522 
…llptr when attribute not found. Don't know why this is an issue _now_
@carlos-zamora
Copy link
Member Author

I guess one important question is: does NVDA treat windows terminal the same way it treats conhost.exe? The implementation for UIA is shared between the two of them, but if the behavior is different that could be a good starting point.

@carlos-zamora: you might try running Host.EXE and doing NVDA against that to reduce the number of variables :)

Turns out, NVDA works on ConHost but not Windows Terminal with this change. @DHowett-MSFT and I used that to trace an issue to our wrapper class for the text ranges.

@codeofdusk This change should affect ConHost as well so we're hoping this will make things on your end easier. Let us know if you expect any different behavior though. Here's a build of ConHost.exe for you to play with.

NOTE: All I really have left on my radar for this PR is adding polish to word navigation. You can expect the following in the coming commit(s)

  • Tests on a buffer with words
  • Additional testing for TextBuffer changes

OpenConsole.zip

@carlos-zamora
- Code format
71d50cb 
- disable debug log
@codeofdusk
Copy link
Contributor

I guess one important question is: does NVDA treat windows terminal the same way it treats conhost.exe? The implementation for UIA is shared between the two of them, but if the behavior is different that could be a good starting point.
@carlos-zamora: you might try running Host.EXE and doing NVDA against that to reduce the number of variables :)

Turns out, NVDA works on ConHost but not Windows Terminal with this change. @DHowett-MSFT and I used that to trace an issue to our wrapper class for the text ranges.

@codeofdusk This change should affect ConHost as well so we're hoping this will make things on your end easier. Let us know if you expect any different behavior though. Here's a build of ConHost.exe for you to play with.

NOTE: All I really have left on my radar for this PR is adding polish to word navigation. You can expect the following in the coming commit(s)

  • Tests on a buffer with words
  • Additional testing for TextBuffer changes

OpenConsole.zip

I get an error stating that VCRUNTIME140_1D.dll is missing. Installing the 2019 X64 VC++ redistributable does not fix the issue.

@codeofdusk
Copy link
Contributor

I guess one important question is: does NVDA treat windows terminal the same way it treats conhost.exe? The implementation for UIA is shared between the two of them, but if the behavior is different that could be a good starting point.
@carlos-zamora: you might try running Host.EXE and doing NVDA against that to reduce the number of variables :)

Turns out, NVDA works on ConHost but not Windows Terminal with this change. @DHowett-MSFT and I used that to trace an issue to our wrapper class for the text ranges.
@codeofdusk This change should affect ConHost as well so we're hoping this will make things on your end easier. Let us know if you expect any different behavior though. Here's a build of ConHost.exe for you to play with.
NOTE: All I really have left on my radar for this PR is adding polish to word navigation. You can expect the following in the coming commit(s)

  • Tests on a buffer with words
  • Additional testing for TextBuffer changes

OpenConsole.zip

I get an error stating that VCRUNTIME140_1D.dll is missing. Installing the 2019 X64 VC++ redistributable does not fix the issue.

Never mind, copying the file to %systemroot%\system32 and running from there works. Yes, this PR provides major improvement. Is it targetted for Windows 10 version 2004?
Cc @LeonarddeR @michaelDCurran

@codeofdusk codeofdusk mentioned this pull request Jan 23, 2020
Closed
@codeofdusk
Copy link
Contributor

codeofdusk commented Jan 24, 2020 •
edited
Loading

This build of NVDA is good for testing these changes – UIA in consoles is preferred when available and our workarounds and word navigation have been disabled. Internally, it is identical to nvaccess/nvda#10716 with a Windows version check that always returns True.

With changes applied, NVDA and the console hang when using the new native word navigation across lines.

@carlos-zamora
Copy link
Member Author

This build of NVDA is good for testing these changes – UIA in consoles is preferred when available and our workarounds and word navigation have been disabled. Internally, it is identical to nvaccess/nvda#10716 with a Windows version check that always returns True.

With changes applied, NVDA and the console hang when using the new native word navigation across lines.

I'm glad to hear that this fixes some of your workarounds! If there's any more workarounds that you're still going through or find, let me know and I'll see if I can fix them in this PR too (or at least track them on the repo).

As for the new word navigation, I'm currently working on optimizing that properly here. That should come in in the upcoming commits. Right now it's scanning the entire buffer to decide if it is on the last word and I'm sure there's a better approach haha.

With regards to the timeline, this is where things get tricky. This change will affect Windows Terminal in the upcoming release. Though this code is shared with ConHost (the regular Console), the in-box console will not have these changes in Windows 10 version 2004. However, if somebody builds/deploys the ConHost from GitHub or somehow uses the one that comes with Windows Terminal, then it'll still have the changes. @DHowett-MSFT can confirm or provide more clarity on that and correct me if I'm wrong.

@codeofdusk
Copy link
Contributor

codeofdusk commented Jan 24, 2020 via email

@carlos-zamora
Copy link
Member Author

Yes – IUIAutomationTextPattern::GetVisibleRanges in consoles returns junk ranges when the window is maximized and subsequently restored or when it receives a lot of new text.
…
Sent from my iPhone
On Jan 24, 2020, at 13:44, Carlos Zamora @.***> wrote:  This build of NVDA is good for testing these changes – UIA in consoles is preferred when available and our workarounds and word navigation have been disabled. Internally, it is identical to nvaccess/nvda#10716 with a Windows version check that always returns True. With changes applied, NVDA and the console hang when using the new native word navigation across lines. I'm glad to hear that this fixes some of your workarounds! If there's any more workarounds that you're still going through or find, let me know and I'll see if I can fix them in this PR too (or at least track them on the repo). As for the new word navigation, I'm currently working on optimizing that properly here. That should come in in the upcoming commits. Right now it's scanning the entire buffer to decide if it is on the last word and I'm sure there's a better approach haha. With regards to the timeline, this is where things get tricky. This change will affect Windows Terminal in the upcoming release. Though this code is shared with ConHost (the regular Console), the in-box console will not have these changes in Windows 10 version 2004. However, if somebody builds/deploys the ConHost from GitHub or somehow uses the one that comes with Windows Terminal, then it'll still have the changes. @DHowett-MSFT can confirm or provide more clarity on that and correct me if I'm wrong. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

I don't follow. What's the repro steps and the expected behavior? Are you callingGetVisibleRanges, then resizing the Terminal Window? I haven't been able to dispatch UIAutomation Events in Windows Terminal (other than selection), so that's definitely not going to work yet.

@codeofdusk
Copy link
Contributor

  1. Open Windows Console.
  2. Call GetVisibleRanges. The visible portion of the buffer is returned.
  3. Maximize the window and call GetVisibleRanges again. The (larger) visible range should be returned.
  4. Restore the window and call GetVisibleRanges again. The original visible range should be returned.
  5. Fill the window with text, such as the numbers 1 through 100000 printed in a loop. Call GetVisibleRanges while the text is printing. The visible ranges should change as the console scrolls.

@carlos-zamora
Finish fixing NVDA issues with UiaTextRange (scoped for this PR)
c075ab2 
Narrator's word navigation has regressed. This PR will not be ready until that is fixed.
@carlos-zamora
Merge branch 'master' into dev/cazamor/utr-refactor-1
d11a7b6
@carlos-zamora carlos-zamora mentioned this pull request Jan 31, 2020
Closed
4 tasks
@carlos-zamora carlos-zamora restored the dev/cazamor/utr-refactor-1 branch January 31, 2020 21:18
@carlos-zamora carlos-zamora deleted the dev/cazamor/utr-refactor-1 branch January 31, 2020 21:18
ghost pushed a commit that referenced this pull request Jan 31, 2020
@carlos-zamora
Introduce UiaTextRangeBase::FindText() for Accessibility (#4373)
55b6388 
Moved `FindText` to `UiaTextRangeBase`. Now that Search is a shared component (thanks #3279), I can just reuse it basically as-is.

#3279 - Make Search a shared component
#4018 - UiaTextRange Refactor

I removed it from the two different kinds of UiaTextRange and put it in the base class.

I needed a very minor change to ensure we convert from an inclusive end (from Search) to an exclusive end (in UTR).

Worked with `FindText` was globally messed with in windows.h. So we had to do a few weird things there (thanks Michael).

No need for additional tests because it _literally_ just sets up a Searcher and calls it.
@codeofdusk
Copy link
Contributor

Glad to see that this was merged!

Any idea when this will ship to Windows 10 conhost?

@DHowett-MSFT
Copy link
Contributor

@codeofdusk unfortunately, this one missed the train for this year’s Windows releases. We’ll see what we can do about that, but it’s looking like it won’t be out for desktop users for a while.

@codeofdusk
Copy link
Contributor

@codeofdusk unfortunately, this one missed the train for this year’s Windows releases. We’ll see what we can do about that, but it’s looking like it won’t be out for desktop users for a while.

Thanks for looking into it – I'd really appreciate seeing this sooner rather than later, as it's blocking an NVDA feature (see nvaccess/nvda#10716).

@ghost
Copy link

ghost commented Feb 13, 2020

🎉Windows Terminal Preview v0.9.433.0 has been released which incorporates this pull request.:tada:

Handy links:

This was referenced Feb 13, 2020
Closed
Closed
Closed
Closed
@codeofdusk codeofdusk mentioned this pull request Feb 13, 2020
Merged
This was referenced Apr 2, 2020
Closed
Closed
@codeofdusk
Copy link
Contributor

@carlos-zamora @DHowett-MSFT About when will this PR make it into conhost in insider builds? 30 April?

@DHowett-MSFT
Copy link
Contributor

"Soon", but I can't officially give any dates. 😄

@DHowett-MSFT
Copy link
Contributor

@codeofdusk well, lol, it's our lucky day. This was just released for the inbox console in insider build 19603!

@josephsl
Copy link

josephsl commented Apr 8, 2020 via email •
edited by ghost
Loading

This was referenced Apr 8, 2020
Merged
Closed
@codeofdusk codeofdusk mentioned this pull request Jul 20, 2020
Closed
@codeofdusk codeofdusk mentioned this pull request Nov 6, 2020
Closed
@carlos-zamora carlos-zamora mentioned this pull request Feb 26, 2021
Open
@codeofdusk codeofdusk mentioned this pull request May 5, 2021
Merged
7 tasks
feerrenrut pushed a commit to nvaccess/nvda that referenced this pull request May 17, 2021
@codeofdusk
UIA in Windows Console: rename is21H1Plus (PR #12094)
5c157ba 
Fix for microsoft/terminal#9239

Summary of the issue:
Windows 10 21H1 has been released to insiders with significantly reduced scope (i.e. excluding the UIA changes it was originally set to include), making our internal names misleading. Additionally, there are plans to "undock" conhost from Windows, removing the guarantee that a given Windows version will run a particular version of the console (it's technically already possible to run newer console on older Windows, I do it for testing).

Description of how this pull request fixes the issue:
Renames:
- NVDAObjects.UIA.winConsoleUIA.is21H1Plus -> NVDAObjects.UIA.winConsoleUIA.isImprovedTextRangeAvailable
- NVDAObjects.UIA.winConsoleUIA.consoleUIATextInfo -> NVDAObjects.UIA.winConsoleUIA.ConsoleUIATextInfo (Start class name with upper case)
- NVDAObjects.UIA.winConsoleUIA.consoleUIATextInfoPre21H1 -> NVDAObjects.UIA.winConsoleUIA.ConsoleUIATextInfoWorkaroundEndInclusive
  - The implementation works around both end points being inclusive (in text ranges) before microsoft/terminal#4018
  - Workarounds for expand, collapse, compareEndPoints, setEndPoint, etc

Also moves the text override to the workaround textInfo as #10036 is no longer reproducible in new console.
Co-authored-by: Reef Turner <reef@nvaccess.org>
@codeofdusk codeofdusk mentioned this pull request Sep 23, 2021
Open
@codeofdusk codeofdusk mentioned this pull request Oct 25, 2021
Open
@codeofdusk codeofdusk mentioned this pull request Feb 9, 2022
Open
seanbudd pushed a commit to nvaccess/nvda that referenced this pull request Jun 21, 2022
@codeofdusk
UI Automation in Windows Console: make default in FORMATTED consoles …
b817b03 
…(Windows 11 Sun Valley 2) (#10964)

Supersedes #9771 and #10716. Closes #1682. Closes #8653. Closes #9867. Closes #11172. Closes #11554.

Summary of the issue:

Microsoft has significantly improved performance and reliability of UIA console:
* microsoft/terminal#4018 is an almost complete rewrite of the UIA code which makes the console's UIA implementation more closely align with the API specification.
* microsoft/terminal#10886, microsoft/terminal#10925, and microsoft/terminal#11253 form a robust testing methodology for the UIA implementation, including bug fixes in response to newly added tests based on Word's UIA implementation.
* microsoft/terminal#11122 removes the thousands of empty lines at the end of the console buffer, significantly improving performance and stability. Since all console text ranges are now within the text buffer's bounds, it is no longer possible for console to crash due to the manipulation by UIA clients of an out-of-bounds text range.
* Countless other accessibility-related PRs too numerous to list here.

We should enable UIA support on new Windows Console builds by default for performance improvement and controllable password suppression.

Description of how this pull request fixes the issue:

This PR:
* Exposes all three options for the UIA console feature flag in the UI (replaces the UIA check box with a combo box).
* Adds a runtime check to test if `apiLevel >= FORMATTED`, and use UIA in this case when the user preference is auto. This is the case on Windows 11 Sun Valley 2 (SV2) available now in beta and set for release in the second half of 2022.
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zadjii-msft zadjii-msft zadjii-msft left review comments

@miniksa miniksa miniksa approved these changes

@DHowett-MSFT DHowett-MSFT DHowett-MSFT approved these changes

Assignees
No one assigned
Labels
Area-Accessibility Issues related to accessibility Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. AutoMerge Marked for automatic merge by the bot when requirements are met Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
7 participants
@carlos-zamora @DHowett-MSFT @josephsl @codeofdusk @rioshin @miniksa @zadjii-msft