Properly handle and test a11y movement at end of buffer #7792

carlos-zamora · 2020-10-01T03:11:16Z

Summary of the Pull Request

The MovementAtExclusiveEnd test was improperly authored for the
following reasons:

it should have used TEST_METHOD_PROPERTY to cover all of the
TextUnits
TextUnit::Document (arguably one of the most important) was ommitted
accidentally (!= TextUnit_Document was used instead of <=)
The created range was not EndExclusive, but rather, the last cell in
the buffer (EndInclusive)

The first half of this PR fixes the test.

The second half of this PR expands the test and fixes any related issues
to make the test pass (i.e. #7771):

TEST_METHOD_PROPERTY was added for it to be degenerate (start/end at
EndExclusive) or not (last cell of buffer)
utr->_start is now also validated after moving backwards

NOTE: utr->_start was not validated when moving forwards because
moving forwards should always fail when at/past the last chell in the
buffer.

Closes #7771

DHowett

Thanks so much for doing this. I like the look of this, and just had some process questions!

DHowett · 2020-10-01T03:15:54Z

src/buffer/out/textBuffer.cpp

@@ -1359,9 +1359,15 @@ const til::point TextBuffer::GetGlyphEnd(const til::point pos) const
 bool TextBuffer::MoveToNextGlyph(til::point& pos, bool allowBottomExclusive) const
 {
    COORD resultPos = pos;
+    const auto bufferSize = GetSize();


is this the fix for the crash?

Yup. In general, I thought it was only possible for _end to be at EndExclusive(), so allowBottomExclusive is used to track if we are working with _end. That definitely wasn't the case because you can end up with _start at EndExclusive() by doing the repro steps.

So this check for EndExclusive() is basically thrown around everywhere to ensure we don't crash on an IncrementInBounds or DecrementInBounds check (which checks if the COORD is in the buffer, and EndExclusive isn't)

DHowett · 2020-10-01T03:17:56Z

src/types/UiaTextRangeBase.cpp

@@ -953,7 +953,7 @@ void UiaTextRangeBase::_moveEndpointByUnitCharacter(_In_ const int moveCount,
            }
            break;
        case MovementDirection::Backward:
-            success = buffer.MoveToPreviousGlyph(target, allowBottomExclusive);
+            success = buffer.MoveToPreviousGlyph(target);


this is because it is safe to move backwards off the exclusive end, and you're checking before loading the glyph data that it is a valid position?

Correct. It's safe to move backwards off the exclusive end. So let's not bother passing in allowBottomExclusive and always allow it.

src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp

DHowett · 2020-10-01T03:24:03Z

src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp

+        else // textUnit <= TextUnit::TextUnit_Document:
+        {
+            VERIFY_ARE_EQUAL(origin, utr->_start);
+            VERIFY_ARE_EQUAL(degenerate ? origin : endExclusive, utr->_end);


i know it might be a chore, but having @codeofdusk review this test case for sanity would be helpful

(a chore for him, not a chore for you. since this is a bunch of dense WEX C++)

zadjii-msft · 2020-10-01T17:50:03Z

In general, this seems fine to me, but I can't really say I understand the problem space well enough to give an official ✔️. Maybe if you came through and answered some of Dustin's comments, then I could make better sense of what's going on? Or if @codeofdusk thinks the tests are good, then I'd probably defer to his expertise in the area

carlos-zamora · 2020-10-01T23:29:20Z

@codeofdusk found that there was a crash when expanding at the end. So I added testing for that and fixed any failing tests.

DHowett · 2020-10-01T23:52:22Z

is there any way at all whatsoever that we can automatically do the things a human might do

codeofdusk · 2020-10-01T23:58:30Z

is there any way at all whatsoever that we can automatically do the things a human might do

Maybe, by adapting NVDA's system test framework? Cc @feerrenrut, @michaelDCurran.

codeofdusk

While I don't feel comfortable commenting on the robustness of the unit tests, this PR definitely fixes my reported UIA bugs!

zadjii-msft

Good enough for me!

miniksa

This looks fine to me. I appreciate all the extra tests thrown in to be more comprehensive than before.

DHowett · 2020-10-14T20:42:15Z

This fix was just released as part of Windows in Insider Build 20236!

The `MovementAtExclusiveEnd` test was improperly authored for the following reasons: - it should have used `TEST_METHOD_PROPERTY` to cover all of the TextUnits - TextUnit::Document (arguably one of the most important) was ommitted accidentally (`!= TextUnit_Document` was used instead of `<=`) - The created range was not `EndExclusive`, but rather, the last cell in the buffer (`EndInclusive`) The first half of this PR fixes the test. The second half of this PR expands the test and fixes any related issues to make the test pass (i.e. #7771): - `TEST_METHOD_PROPERTY` was added for it to be degenerate (start/end at `EndExclusive`) or not (last cell of buffer) - `utr->_start` is now also validated after moving backwards NOTE: `utr->_start` was not validated when moving forwards because moving forwards should always fail when at/past the last chell in the buffer. Closes #7771 (cherry picked from commit e401edf)

ghost · 2020-11-11T18:05:05Z

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

Handy links:

ghost · 2020-11-11T18:08:53Z

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

Handy links:

carlos-zamora added 2 commits September 30, 2020 17:19

Fix MovementAtExclusiveEnd test; use test_method_property

2470d33

Expand 'MovementAtExclusiveEnd' test; fix failing tests

5e87c50

carlos-zamora requested a review from DHowett October 1, 2020 03:11

DHowett approved these changes Oct 1, 2020

View reviewed changes

carlos-zamora added 2 commits October 1, 2020 11:59

remove dead code

e13ea08

fix and test expanding at buffer end

520ad92

carlos-zamora mentioned this pull request Oct 2, 2020

UIA: GetVisibleRanges() returning text beyond the visible range #7803

Closed

codeofdusk mentioned this pull request Oct 2, 2020

UI Automation in Windows Console: remove workaround at POSITION_LAST for 21H1 nvaccess/nvda#11713

Closed

codeofdusk approved these changes Oct 4, 2020

View reviewed changes

zadjii-msft approved these changes Oct 5, 2020

View reviewed changes

miniksa approved these changes Oct 5, 2020

View reviewed changes

DHowett merged commit e401edf into master Oct 5, 2020

DHowett deleted the dev/cazamor/a11y/move-at-exclusive-end branch October 5, 2020 22:11

codeofdusk mentioned this pull request Oct 9, 2020

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

Merged

ghost mentioned this pull request Nov 11, 2020

UIA: crash at very end of buffer #7771

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Properly handle and test a11y movement at end of buffer #7792

Properly handle and test a11y movement at end of buffer #7792

carlos-zamora commented Oct 1, 2020 •

edited by DHowett

Loading

DHowett left a comment

DHowett Oct 1, 2020

carlos-zamora Oct 1, 2020

DHowett Oct 1, 2020

carlos-zamora Oct 1, 2020

DHowett Oct 1, 2020

DHowett Oct 1, 2020

zadjii-msft commented Oct 1, 2020

carlos-zamora commented Oct 1, 2020

DHowett commented Oct 1, 2020

codeofdusk commented Oct 1, 2020

codeofdusk left a comment

zadjii-msft left a comment

miniksa left a comment

DHowett commented Oct 14, 2020

ghost commented Nov 11, 2020

ghost commented Nov 11, 2020

Properly handle and test a11y movement at end of buffer #7792

Properly handle and test a11y movement at end of buffer #7792

Conversation

carlos-zamora commented Oct 1, 2020 • edited by DHowett Loading

Summary of the Pull Request

DHowett left a comment

Choose a reason for hiding this comment

DHowett Oct 1, 2020

Choose a reason for hiding this comment

carlos-zamora Oct 1, 2020

Choose a reason for hiding this comment

DHowett Oct 1, 2020

Choose a reason for hiding this comment

carlos-zamora Oct 1, 2020

Choose a reason for hiding this comment

DHowett Oct 1, 2020

Choose a reason for hiding this comment

DHowett Oct 1, 2020

Choose a reason for hiding this comment

zadjii-msft commented Oct 1, 2020

carlos-zamora commented Oct 1, 2020

DHowett commented Oct 1, 2020

codeofdusk commented Oct 1, 2020

codeofdusk left a comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

miniksa left a comment

Choose a reason for hiding this comment

DHowett commented Oct 14, 2020

ghost commented Nov 11, 2020

ghost commented Nov 11, 2020

carlos-zamora commented Oct 1, 2020 •

edited by DHowett

Loading