[A11y] Treat last character as 'end of buffer' by carlos-zamora · Pull Request #11122 · microsoft/terminal

carlos-zamora · 2021-09-02T18:19:32Z

Summary of the Pull Request

Updates our UiaTextRange to no longer treat the end of the buffer as the "document end". Instead, we consider the "document end" to be the line beneath the cursor or last legible character (whichever is further down). In the event where the last legible character is on the last line of the buffer, we use the "end exclusive" position (left-most point on a line one past the end of the buffer).

When movement of any kind occurs, we clamp each endpoint to the document end. Since the document end is an actual spot in the buffer (most of the time), this should improve stability because we shouldn't be pointing out-of-bounds anymore.

The biggest benefit is that this significantly improves the performance of word navigation because screen readers no longer have to take into account the whitespace following the end of the prompt.

Word navigation tests were added to the TestTableWriter (see #10886). 24 of the 85 tests were failing, however, they don't seem to interact with the document end, so I've marked them as skip and will fix them in a follow-up. This PR is large enough as-is, so I'm hoping I can take time in the follow-up to clean some things on the side (aka preventBoundary and allowBottomExclusive being used interchangeably).

References

#7000 - Epic
Closes #6986
Closes #10925

Validation Steps Performed

Tests pass
@codeofdusk has been personally testing this build (and others)

carlos-zamora · 2021-09-02T18:21:47Z

@lhecker you're generally really good about finding ways to simplify code down a lot. Mind taking a look here? I'm worried I overcomplicated some things, and a fresh pair of eyes would be good 😊

carlos-zamora · 2021-09-02T19:17:07Z

src/types/UiaTextRangeBase.cpp

    return Viewport::FromDimensions({ 0, 0 }, width, height);
 }

+const til::point UiaTextRangeBase::_getDocumentEnd() const noexcept


Idea: I could cache _getDocumentEnd(), and in the scope exit for (un)locking the console, I could reset the cache

Worth pursuing?

I might add the description from the body of this PR here, since this is kinda a load-bearing function

We consider the "document end" to be the line beneath the cursor or last legible character (whichever is further down). In the event where the last legible character is on the last line of the buffer, we use the "end exclusive" position (left-most point on a line one past the end of the buffer).

src/buffer/out/textBuffer.cpp

lhecker · 2021-09-02T19:41:42Z

I find this PR a bit hard to understand (at least in diff form).
I'll check it out soon and try to look at it locally. 🙂

codeofdusk · 2021-09-03T21:12:36Z

Please note that nvaccess/nvda#12808 applies, but I'm not sure if this is on your side or theirs.

zadjii-msft

Okay I've got nits, but these don't matter. I definitely didn't read UiaTests.csv because I trust you know that these do the right thing.

zadjii-msft · 2021-09-09T19:06:51Z

tools/TestTableWriter/UiaTests.csv

+TRUE,9,TextUnit_Word,-1,endExclusive,endExclusive,-1,segment4LmidDocEnd,segment4LmidDocEnd,TRUE
+TRUE,9,TextUnit_Word,0,endExclusive,endExclusive,0,docEnd,docEnd,FALSE
+TRUE,9,TextUnit_Word,1,endExclusive,endExclusive,0,docEnd,docEnd,FALSE
+TRUE,9,TextUnit_Word,5,endExclusive,endExclusive,0,docEnd,docEnd,FALSE


this is a crapload of tests

zadjii-msft · 2021-09-09T19:36:26Z

src/types/UiaTextRangeBase.cpp

+    const auto& buffer{ _pData->GetTextBuffer() };
+    const auto lastCharPos{ buffer.GetLastNonSpaceCharacter(optBufferSize) };
+    const auto cursorPos{ buffer.GetCursor().GetPosition() };
+    return { optBufferSize.Left(), std::max(lastCharPos.Y, cursorPos.Y) + 1 };


isn't optBufferSize.Left() always 0?

yeah. I've been trying to use .Left() instead of 0 hoping that it would make things more clear. But idk if there's a real benefit to that. Should I change it?

src/types/UiaTextRangeBase.cpp

zadjii-msft · 2021-09-09T19:37:49Z

src/types/UiaTextRangeBase.cpp

    return Viewport::FromDimensions({ 0, 0 }, width, height);
 }

+const til::point UiaTextRangeBase::_getDocumentEnd() const noexcept


I might add the description from the body of this PR here, since this is kinda a load-bearing function

We consider the "document end" to be the line beneath the cursor or last legible character (whichever is further down). In the event where the last legible character is on the last line of the buffer, we use the "end exclusive" position (left-most point on a line one past the end of the buffer).

src/buffer/out/textBuffer.cpp

tools/TestTableWriter/UiaTests.csv

src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp

miniksa · 2021-09-14T23:12:03Z

src/types/UiaTextRangeBase.cpp

+    if (bufferSize.CompareInBounds(_start, documentEnd, true) > 0)
+    {
+        _start = documentEnd;
+    }


Man.... we really need to have these things using til::rectangle and have a reasonable clamp method don't we. :(

Yeah, I'd love to beef up til::rectangle, til::size, and til::point to be able to clamp and walk through bounds more easily. But unfortunately a lot of that work is blocked by making Viewport a til::rectangle.

Actually, are we tracking that anywhere? Just about every time I work on a11y, I find new things I want to add to these TIL structs.

#4015 is my record of "we still need to kill Viewport"

ghost · 2021-09-16T20:43:47Z

Hello @carlos-zamora!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost · 2021-10-20T17:03:22Z

🎉Windows Terminal Preview v1.12.2922.0 has been released which incorporates this pull request.:tada:

Handy links:

…(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.

carlos-zamora added 2 commits September 1, 2021 16:13

[A11y] Treat last character as 'end of buffer'

b6e2da4

fix tests

dae317b

ghost added Area-Accessibility Issues related to accessibility Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. labels Sep 2, 2021

carlos-zamora requested a review from lhecker September 2, 2021 18:21

carlos-zamora commented Sep 2, 2021

View reviewed changes

src/buffer/out/textBuffer.cpp Outdated Show resolved Hide resolved

codeofdusk mentioned this pull request Sep 3, 2021

Some UIA text ranges have no apparent bottom nvaccess/nvda#12808

Closed

zadjii-msft approved these changes Sep 9, 2021

View reviewed changes

zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Sep 9, 2021

ghost requested review from DHowett, PankajBhojwani, leonMSFT and miniksa September 9, 2021 19:49

carlos-zamora added 3 commits September 13, 2021 12:55

fix some variable/function names

aa4f7d5

static analysis

8c40303

fix failing GetWordEnd test

17c414d

miniksa reviewed Sep 14, 2021

View reviewed changes

address miniksa feedback

531c6de

carlos-zamora assigned miniksa Sep 15, 2021

miniksa approved these changes Sep 16, 2021

View reviewed changes

carlos-zamora unassigned miniksa Sep 16, 2021

carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 16, 2021

ghost merged commit d08afc4 into main Sep 16, 2021

ghost deleted the dev/cazamor/a11y-7000/document-end branch September 16, 2021 20:44

This was referenced Oct 20, 2021

Add more UIA word navigation tests #10925

Closed

Properly handle and test "end of buffer" in UiaTextRange #6986

Closed

codeofdusk mentioned this pull request Oct 22, 2021

UI Automation in Windows Console: make default in FORMATTED consoles (Windows 11 Sun Valley 2) nvaccess/nvda#10964

Merged

This pull request was closed.

Conversation

carlos-zamora commented Sep 2, 2021

Summary of the Pull Request

References

Validation Steps Performed

Uh oh!

carlos-zamora commented Sep 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lhecker commented Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codeofdusk commented Sep 3, 2021

Uh oh!

zadjii-msft 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!

Choose a reason for hiding this comment

Uh oh!

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!

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!

ghost commented Sep 16, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

ghost commented Oct 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lhecker commented Sep 2, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.