maxmustermann/terminal
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

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

Browse source 
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
This commit is contained in:
Carlos Zamora 2020-10-05 15:11:47 -07:00 committed by GitHub
parent 4a114971f9
commit e401edf9ef
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 125 additions and 52 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

22
src/buffer/out/textBuffer.cpp
View file

@ -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;

2
src/buffer/out/textBuffer.hpp
View file

@ -137,7 +137,7 @@ public:
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<SMALL_RECT> GetTextRects(COORD start, COORD end, bool blockSelection = false) const;

130
src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp
View file

@ -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<TextUnit>(unit) };
Microsoft::WRL::ComPtr<UiaTextRange> utr;
int moveAmt;
for (int unit = TextUnit::TextUnit_Character; unit != TextUnit::TextUnit_Document; ++unit)
Log::Comment(NoThrowString().Format(L"Forward by %s", toString(textUnit)));
// Create an UTR at EndExclusive
if (degenerate)
{
THROW_IF_FAILED(Microsoft::WRL::MakeAndInitialize<UiaTextRange>(&utr, _pUiaData, &_dummyProvider, endExclusive, endExclusive));
}
else
{
Log::Comment(NoThrowString().Format(L"Forward by %s", toString(static_cast<TextUnit>(unit))));
// Create a degenerate UTR at EndExclusive
THROW_IF_FAILED(Microsoft::WRL::MakeAndInitialize<UiaTextRange>(&utr, _pUiaData, &_dummyProvider, endInclusive, endExclusive));
THROW_IF_FAILED(utr->Move(static_cast<TextUnit>(unit), 1, &moveAmt));
}
THROW_IF_FAILED(utr->Move(textUnit, 1, &moveAmt));
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);
// 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);
VERIFY_ARE_EQUAL(0, moveAmt);
}
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);
}
// reset the UTR
if (degenerate)
{
THROW_IF_FAILED(Microsoft::WRL::MakeAndInitialize<UiaTextRange>(&utr, _pUiaData, &_dummyProvider, endExclusive, endExclusive));
}
else
{
THROW_IF_FAILED(Microsoft::WRL::MakeAndInitialize<UiaTextRange>(&utr, _pUiaData, &_dummyProvider, endInclusive, endExclusive));
}
// Verify that moving backwards still works properly
const COORD writeTarget{ 2, 2 };
_pTextBuffer->Write({ L"temp" }, writeTarget);
for (int unit = TextUnit::TextUnit_Character; unit != TextUnit::TextUnit_Document; ++unit)
Log::Comment(NoThrowString().Format(L"Backwards by %s", toString(textUnit)));
THROW_IF_FAILED(utr->Move(textUnit, -1, &moveAmt));
VERIFY_ARE_EQUAL(-1, moveAmt);
// NOTE: If the range is degenerate, _start == _end before AND after the move.
if (textUnit <= TextUnit::TextUnit_Character)
{
COORD expectedEnd;
switch (static_cast<TextUnit>(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<TextUnit>(unit))));
// Create a degenerate UTR at EndExclusive
THROW_IF_FAILED(Microsoft::WRL::MakeAndInitialize<UiaTextRange>(&utr, _pUiaData, &_dummyProvider, endInclusive, endExclusive));
THROW_IF_FAILED(utr->Move(static_cast<TextUnit>(unit), -1, &moveAmt));
VERIFY_ARE_EQUAL(expectedEnd, utr->_end);
VERIFY_ARE_EQUAL(-1, moveAmt);
// 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);
}
}

23
src/types/UiaTextRangeBase.cpp
View file

@ -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)
{