From e401edf9ef35f8ffea3420bfaea2c8e80d9c081f Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Mon, 5 Oct 2020 15:11:47 -0700 Subject: [PATCH] Properly handle and test a11y movement at end of buffer (#7792) 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 --- src/buffer/out/textBuffer.cpp | 22 ++- src/buffer/out/textBuffer.hpp | 2 +- .../UiaTextRangeTests.cpp | 126 +++++++++++++----- src/types/UiaTextRangeBase.cpp | 23 +++- 4 files changed, 123 insertions(+), 50 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index cd417dd7034..b99ae9feda1 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1317,8 +1317,13 @@ bool TextBuffer::MoveToPreviousWord(COORD& pos, std::wstring_view wordDelimiters const til::point TextBuffer::GetGlyphStart(const til::point pos) const { COORD resultPos = pos; - const auto bufferSize = GetSize(); + + if (resultPos == bufferSize.EndExclusive()) + { + bufferSize.DecrementInBounds(resultPos, true); + } + if (resultPos != bufferSize.EndExclusive() && GetCellDataAt(resultPos)->DbcsAttr().IsTrailing()) { bufferSize.DecrementInBounds(resultPos, true); @@ -1359,9 +1364,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(); + + if (resultPos == GetSize().EndExclusive()) + { + // we're already at the end + return false; + } // try to move. If we can't, we're done. - const auto bufferSize = GetSize(); const bool success = bufferSize.IncrementInBounds(resultPos, allowBottomExclusive); if (resultPos != bufferSize.EndExclusive() && GetCellDataAt(resultPos)->DbcsAttr().IsTrailing()) { @@ -1376,20 +1387,19 @@ bool TextBuffer::MoveToNextGlyph(til::point& pos, bool allowBottomExclusive) con // - Update pos to be the beginning of the previous glyph/character. This is used for accessibility // Arguments: // - pos - a COORD on the word you are currently on -// - allowBottomExclusive - allow the nonexistent end-of-buffer cell to be encountered // Return Value: // - true, if successfully updated pos. False, if we are unable to move (usually due to a buffer boundary) // - pos - The COORD for the first cell of the previous glyph (inclusive) -bool TextBuffer::MoveToPreviousGlyph(til::point& pos, bool allowBottomExclusive) const +bool TextBuffer::MoveToPreviousGlyph(til::point& pos) const { COORD resultPos = pos; // try to move. If we can't, we're done. const auto bufferSize = GetSize(); - const bool success = bufferSize.DecrementInBounds(resultPos, allowBottomExclusive); + const bool success = bufferSize.DecrementInBounds(resultPos, true); if (resultPos != bufferSize.EndExclusive() && GetCellDataAt(resultPos)->DbcsAttr().IsLeading()) { - bufferSize.DecrementInBounds(resultPos, allowBottomExclusive); + bufferSize.DecrementInBounds(resultPos, true); } pos = resultPos; diff --git a/src/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp index 451a6e0ac01..a17382b2ff5 100644 --- a/src/buffer/out/textBuffer.hpp +++ b/src/buffer/out/textBuffer.hpp @@ -137,7 +137,7 @@ class TextBuffer final const til::point GetGlyphStart(const til::point pos) const; const til::point GetGlyphEnd(const til::point pos) const; bool MoveToNextGlyph(til::point& pos, bool allowBottomExclusive = false) const; - bool MoveToPreviousGlyph(til::point& pos, bool allowBottomExclusive = false) const; + bool MoveToPreviousGlyph(til::point& pos) const; const std::vector GetTextRects(COORD start, COORD end, bool blockSelection = false) const; diff --git a/src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp b/src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp index d0ee72fa53c..9faff4c0a64 100644 --- a/src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp +++ b/src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp @@ -1136,58 +1136,110 @@ class UiaTextRangeTests // the UTR should refuse to move past the end. const auto bufferSize{ _pTextBuffer->GetSize() }; - const til::point endInclusive = { bufferSize.RightInclusive(), bufferSize.BottomInclusive() }; - const auto endExclusive{ bufferSize.EndExclusive() }; + const COORD origin{ bufferSize.Origin() }; + const COORD lastLineStart{ bufferSize.Left(), bufferSize.BottomInclusive() }; + const COORD secondToLastCharacterPos{ bufferSize.RightInclusive() - 1, bufferSize.BottomInclusive() }; + const COORD endInclusive{ bufferSize.RightInclusive(), bufferSize.BottomInclusive() }; + const COORD endExclusive{ bufferSize.EndExclusive() }; - // Iterate over each TextUnit. If the we don't support + // Iterate over each TextUnit. If we don't support // the given TextUnit, we're supposed to fallback // to the last one that was defined anyways. + BEGIN_TEST_METHOD_PROPERTIES() + TEST_METHOD_PROPERTY(L"Data:textUnit", L"{0, 1, 2, 3, 4, 5, 6}") + TEST_METHOD_PROPERTY(L"Data:degenerate", L"{false, true}") + END_TEST_METHOD_PROPERTIES(); + + int unit; + bool degenerate; + VERIFY_SUCCEEDED(TestData::TryGetValue(L"textUnit", unit), L"Get TextUnit variant"); + VERIFY_SUCCEEDED(TestData::TryGetValue(L"degenerate", degenerate), L"Get degenerate variant"); + TextUnit textUnit{ static_cast(unit) }; + Microsoft::WRL::ComPtr utr; int moveAmt; - for (int unit = TextUnit::TextUnit_Character; unit != TextUnit::TextUnit_Document; ++unit) - { - Log::Comment(NoThrowString().Format(L"Forward by %s", toString(static_cast(unit)))); + Log::Comment(NoThrowString().Format(L"Forward by %s", toString(textUnit))); - // Create a degenerate UTR at EndExclusive + // Create an UTR at EndExclusive + if (degenerate) + { + THROW_IF_FAILED(Microsoft::WRL::MakeAndInitialize(&utr, _pUiaData, &_dummyProvider, endExclusive, endExclusive)); + } + else + { THROW_IF_FAILED(Microsoft::WRL::MakeAndInitialize(&utr, _pUiaData, &_dummyProvider, endInclusive, endExclusive)); - THROW_IF_FAILED(utr->Move(static_cast(unit), 1, &moveAmt)); - - VERIFY_ARE_EQUAL(endExclusive, utr->_end); - VERIFY_ARE_EQUAL(0, moveAmt); } + THROW_IF_FAILED(utr->Move(textUnit, 1, &moveAmt)); - // Verify that moving backwards still works properly + VERIFY_ARE_EQUAL(endExclusive, utr->_end); + VERIFY_ARE_EQUAL(0, moveAmt); + + // write "temp" at (2,2) const COORD writeTarget{ 2, 2 }; _pTextBuffer->Write({ L"temp" }, writeTarget); - for (int unit = TextUnit::TextUnit_Character; unit != TextUnit::TextUnit_Document; ++unit) - { - COORD expectedEnd; - switch (static_cast(unit)) - { - case TextUnit::TextUnit_Character: - expectedEnd = endInclusive; - break; - case TextUnit::TextUnit_Word: - expectedEnd = writeTarget; - break; - case TextUnit::TextUnit_Line: - expectedEnd = endExclusive; - break; - case TextUnit::TextUnit_Document: - expectedEnd = bufferSize.Origin(); - break; - default: - continue; - } - Log::Comment(NoThrowString().Format(L"Backwards by %s", toString(static_cast(unit)))); + // Verify expansion works properly + Log::Comment(NoThrowString().Format(L"Expand by %s", toString(textUnit))); + THROW_IF_FAILED(utr->ExpandToEnclosingUnit(textUnit)); + if (textUnit <= TextUnit::TextUnit_Character) + { + VERIFY_ARE_EQUAL(endInclusive, utr->_start); + VERIFY_ARE_EQUAL(endExclusive, utr->_end); + } + else if (textUnit <= TextUnit::TextUnit_Word) + { + VERIFY_ARE_EQUAL(writeTarget, utr->_start); + VERIFY_ARE_EQUAL(endExclusive, utr->_end); + } + else if (textUnit <= TextUnit::TextUnit_Line) + { + VERIFY_ARE_EQUAL(lastLineStart, utr->_start); + VERIFY_ARE_EQUAL(endExclusive, utr->_end); + } + else // textUnit <= TextUnit::TextUnit_Document: + { + VERIFY_ARE_EQUAL(origin, utr->_start); + VERIFY_ARE_EQUAL(endExclusive, utr->_end); + } - // Create a degenerate UTR at EndExclusive + // reset the UTR + if (degenerate) + { + THROW_IF_FAILED(Microsoft::WRL::MakeAndInitialize(&utr, _pUiaData, &_dummyProvider, endExclusive, endExclusive)); + } + else + { THROW_IF_FAILED(Microsoft::WRL::MakeAndInitialize(&utr, _pUiaData, &_dummyProvider, endInclusive, endExclusive)); - THROW_IF_FAILED(utr->Move(static_cast(unit), -1, &moveAmt)); + } + + // Verify that moving backwards still works properly + Log::Comment(NoThrowString().Format(L"Backwards by %s", toString(textUnit))); + THROW_IF_FAILED(utr->Move(textUnit, -1, &moveAmt)); + VERIFY_ARE_EQUAL(-1, moveAmt); - VERIFY_ARE_EQUAL(expectedEnd, utr->_end); - VERIFY_ARE_EQUAL(-1, moveAmt); + // NOTE: If the range is degenerate, _start == _end before AND after the move. + if (textUnit <= TextUnit::TextUnit_Character) + { + // Special case: _end will always be endInclusive, because... + // - degenerate --> it moves with _start to stay degenerate + // - !degenerate --> it excludes the last char, to select the second to last char + VERIFY_ARE_EQUAL(degenerate ? endInclusive : secondToLastCharacterPos, utr->_start); + VERIFY_ARE_EQUAL(endInclusive, utr->_end); + } + else if (textUnit <= TextUnit::TextUnit_Word) + { + VERIFY_ARE_EQUAL(origin, utr->_start); + VERIFY_ARE_EQUAL(degenerate ? origin : writeTarget, utr->_end); + } + else if (textUnit <= TextUnit::TextUnit_Line) + { + VERIFY_ARE_EQUAL(lastLineStart, utr->_start); + VERIFY_ARE_EQUAL(degenerate ? lastLineStart : endExclusive, utr->_end); + } + else // textUnit <= TextUnit::TextUnit_Document: + { + VERIFY_ARE_EQUAL(origin, utr->_start); + VERIFY_ARE_EQUAL(degenerate ? origin : endExclusive, utr->_end); } } diff --git a/src/types/UiaTextRangeBase.cpp b/src/types/UiaTextRangeBase.cpp index 506b25cb104..e264159997d 100644 --- a/src/types/UiaTextRangeBase.cpp +++ b/src/types/UiaTextRangeBase.cpp @@ -279,10 +279,21 @@ IFACEMETHODIMP UiaTextRangeBase::ExpandToEnclosingUnit(_In_ TextUnit unit) noexc } else if (unit <= TextUnit_Line) { - // expand to line - _start.X = 0; - _end.X = 0; - _end.Y = base::ClampAdd(_start.Y, 1); + if (_start == bufferEnd) + { + // Special case: if we are at the bufferEnd, + // move _start back one, instead of _end forward + _start.X = 0; + _start.Y = base::ClampSub(_start.Y, 1); + _end = bufferEnd; + } + else + { + // expand to line + _start.X = 0; + _end.X = 0; + _end.Y = base::ClampAdd(_start.Y, 1); + } } else { @@ -953,7 +964,7 @@ void UiaTextRangeBase::_moveEndpointByUnitCharacter(_In_ const int moveCount, } break; case MovementDirection::Backward: - success = buffer.MoveToPreviousGlyph(target, allowBottomExclusive); + success = buffer.MoveToPreviousGlyph(target); if (success) { (*pAmountMoved)--; @@ -1123,7 +1134,7 @@ void UiaTextRangeBase::_moveEndpointByUnitLine(_In_ const int moveCount, } // NOTE: Automatically detects if we are trying to move past origin - success = bufferSize.DecrementInBounds(nextPos, allowBottomExclusive); + success = bufferSize.DecrementInBounds(nextPos, true); if (success) {