Skip to content

Commit

Permalink
Properly handle and test a11y movement at end of buffer (#7792)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@carlos-zamora
carlos-zamora authored Oct 5, 2020
1 parent 4a11497 commit e401edf
Show file tree
Hide file tree
Showing 4 changed files with 123 additions and 50 deletions.
22 changes: 16 additions & 6 deletions src/buffer/out/textBuffer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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())
{
Expand All @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion src/buffer/out/textBuffer.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<SMALL_RECT> GetTextRects(COORD start, COORD end, bool blockSelection = false) const;

Expand Down
126 changes: 89 additions & 37 deletions src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(static_cast<TextUnit>(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<UiaTextRange>(&utr, _pUiaData, &_dummyProvider, endExclusive, endExclusive));
}
else
{
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(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<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))));
// 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<UiaTextRange>(&utr, _pUiaData, &_dummyProvider, endExclusive, endExclusive));
}
else
{
THROW_IF_FAILED(Microsoft::WRL::MakeAndInitialize<UiaTextRange>(&utr, _pUiaData, &_dummyProvider, endInclusive, endExclusive));
THROW_IF_FAILED(utr->Move(static_cast<TextUnit>(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);
}
}

Expand Down
23 changes: 17 additions & 6 deletions src/types/UiaTextRangeBase.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -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)--;
Expand Down Expand Up @@ -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)
{
Expand Down

0 comments on commit e401edf

Please sign in to comment.