Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Oct 5, 2020
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2470d33
Fix MovementAtExclusiveEnd test; use test_method_property
carlos-zamora Oct 1, 2020
5e87c50
Expand 'MovementAtExclusiveEnd' test; fix failing tests
carlos-zamora Oct 1, 2020
e13ea08
remove dead code
carlos-zamora Oct 1, 2020
520ad92
fix and test expanding at buffer end
carlos-zamora Oct 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions src/buffer/out/textBuffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the fix for the crash?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


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 +1382,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
Open in desktop
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
97 changes: 61 additions & 36 deletions src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1136,57 +1136,82 @@ 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();
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved

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_ARE_EQUAL(endExclusive, utr->_end);
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
VERIFY_ARE_EQUAL(0, moveAmt);

// Verify that moving backwards still works properly

// 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))));
//if (textUnit == TextUnit::TextUnit_Character && !degenerate)
// DebugBreak();
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved

// 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));
Log::Comment(NoThrowString().Format(L"Backwards by %s", toString(textUnit)));
THROW_IF_FAILED(utr->Move(textUnit, -1, &moveAmt));
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved

VERIFY_ARE_EQUAL(expectedEnd, utr->_end);
// 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);
VERIFY_ARE_EQUAL(-1, moveAmt);
}
else if (textUnit <= TextUnit::TextUnit_Word)
{
VERIFY_ARE_EQUAL(origin, utr->_start);
VERIFY_ARE_EQUAL(degenerate ? origin : writeTarget, utr->_end);
VERIFY_ARE_EQUAL(-1, moveAmt);
}
else if (textUnit <= TextUnit::TextUnit_Line)
{
VERIFY_ARE_EQUAL(lastLineStart, utr->_start);
VERIFY_ARE_EQUAL(degenerate ? lastLineStart : endExclusive, utr->_end);
VERIFY_ARE_EQUAL(-1, moveAmt);
}
else // textUnit <= TextUnit::TextUnit_Document:
{
VERIFY_ARE_EQUAL(origin, utr->_start);
VERIFY_ARE_EQUAL(degenerate ? origin : endExclusive, utr->_end);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

VERIFY_ARE_EQUAL(-1, moveAmt);
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/types/UiaTextRangeBase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -953,7 +953,7 @@ void UiaTextRangeBase::_moveEndpointByUnitCharacter(_In_ const int moveCount,
}
break;
case MovementDirection::Backward:
success = buffer.MoveToPreviousGlyph(target, allowBottomExclusive);
success = buffer.MoveToPreviousGlyph(target);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (success)
{
(*pAmountMoved)--;
Expand Down Expand Up @@ -1123,7 +1123,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