Skip to content

Commit

Permalink
Fix UIA Word movement tests (#11253)
Browse files Browse the repository at this point in the history 
## Summary of the Pull Request
Fixes the 24 failing generated tests. 20 of them were fixed by enforcing the following rule: when moving backwards by word...
- a degenerate range moves to the beginning of the word, then to the word behind it.
- a non-degenerate range outright moves to the word behind it.

The fix was simple: if we're a degenerate range, check if we're at the beginning of the word. If not, move there. Otherwise, move to the word before it. See UiaTextRangeBase.cpp changes for implementation details.

Along the way, several misauthored tests were found:
- 2 generated tests:
   - Cause: MS Word considers a line break a word delimiter. We don't use line-wrapping to distinguish two separate words.
- `MovementAtExclusiveEnd` backwards word movement tests:
   - `end` will always be `writeTarget` because...
      - [degenerate range case] both `start` and `end` are moved to the beginning of the word (`writeTarget`)
      - [non-degenerate range case] from the `UiaTextRangeBase` bugfix, we should be moving to the word behind it.
   - this misauthored test was explicitly found by fixing the bug first explained here.

## References
#10925 Word navigation testing
  • Loading branch information
@carlos-zamora
carlos-zamora authored Sep 22, 2021
1 parent 168d28b commit 5deb332
Show file tree
Hide file tree
Showing 5 changed files with 87 additions and 56 deletions.
60 changes: 30 additions & 30 deletions src/interactivity/win32/ut_interactivity_win32/GeneratedUiaTextRangeMovementTests.g.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,9 @@ constexpr til::point segment1LmidHistory{ segment1, midHistory.y() };
constexpr til::point segment1LmidTop{ segment1, midTop.y() };
constexpr til::point segment1LmidTopP1L{ segment1, midTopP1L.y() };
constexpr til::point segment2LmidDocEnd{ segment2, midDocEnd.y() };
constexpr til::point segment2LmidDocEndM1L{ segment2, midDocEndM1L.y() };
constexpr til::point segment2LmidHistory{ segment2, midHistory.y() };
constexpr til::point segment2LmidHistoryM1L{ segment2, midHistoryM1L.y() };
constexpr til::point segment2LmidHistoryP1L{ segment2, midHistoryP1L.y() };
constexpr til::point segment2LmidTop{ segment2, midTop.y() };
constexpr til::point segment2LmidTopP1L{ segment2, midTopP1L.y() };
Expand All @@ -102,9 +104,7 @@ constexpr til::point segment3LmidTop{ segment3, midTop.y() };
constexpr til::point segment3LmidTopP1L{ segment3, midTopP1L.y() };
constexpr til::point segment4LlastCharPosM1L{ segment4, lastCharPosM1L.y() };
constexpr til::point segment4LmidDocEnd{ segment4, midDocEnd.y() };
constexpr til::point segment4LmidDocEndM1L{ segment4, midDocEndM1L.y() };
constexpr til::point segment4LmidHistory{ segment4, midHistory.y() };
constexpr til::point segment4LmidHistoryM1L{ segment4, midHistoryM1L.y() };
constexpr til::point segment4LmidTop{ segment4, midTop.y() };
struct GeneratedMovementTestInput
{
Expand Down Expand Up @@ -3319,7 +3319,7 @@ static constexpr std::array<GeneratedMovementTest, 340> s_movementTests{
-3,
origin,
origin },
true },
false },
GeneratedMovementTest{
L"Move degenerate range at position 2 -1 times by Word",
GeneratedMovementTestInput{
Expand All @@ -3331,7 +3331,7 @@ static constexpr std::array<GeneratedMovementTest, 340> s_movementTests{
-1,
segment2LmidTop,
segment2LmidTop },
true },
false },
GeneratedMovementTest{
L"Move degenerate range at position 2 0 times by Word",
GeneratedMovementTestInput{
Expand Down Expand Up @@ -3439,7 +3439,7 @@ static constexpr std::array<GeneratedMovementTest, 340> s_movementTests{
-5,
segment3LmidHistoryM1L,
segment3LmidHistoryM1L },
true },
false },
GeneratedMovementTest{
L"Move degenerate range at position 3 -1 times by Word",
GeneratedMovementTestInput{
Expand All @@ -3451,7 +3451,7 @@ static constexpr std::array<GeneratedMovementTest, 340> s_movementTests{
-1,
segment2LmidHistory,
segment2LmidHistory },
true },
false },
GeneratedMovementTest{
L"Move degenerate range at position 3 0 times by Word",
GeneratedMovementTestInput{
Expand Down Expand Up @@ -3497,9 +3497,9 @@ static constexpr std::array<GeneratedMovementTest, 340> s_movementTests{
midHistoryP1C },
GeneratedMovementTestExpected{
-5,
segment3LmidHistoryM1L,
segment4LmidHistoryM1L },
true },
segment2LmidHistoryM1L,
segment3LmidHistoryM1L },
false },
GeneratedMovementTest{
L"Move non-degenerate range at position 3 -1 times by Word",
GeneratedMovementTestInput{
Expand Down Expand Up @@ -3559,7 +3559,7 @@ static constexpr std::array<GeneratedMovementTest, 340> s_movementTests{
-5,
segment3LmidDocEndM1L,
segment3LmidDocEndM1L },
true },
false },
GeneratedMovementTest{
L"Move degenerate range at position 4 -1 times by Word",
GeneratedMovementTestInput{
Expand All @@ -3571,7 +3571,7 @@ static constexpr std::array<GeneratedMovementTest, 340> s_movementTests{
-1,
segment2LmidDocEnd,
segment2LmidDocEnd },
true },
false },
GeneratedMovementTest{
L"Move degenerate range at position 4 0 times by Word",
GeneratedMovementTestInput{
Expand Down Expand Up @@ -3617,9 +3617,9 @@ static constexpr std::array<GeneratedMovementTest, 340> s_movementTests{
midDocEndP1C },
GeneratedMovementTestExpected{
-5,
segment3LmidDocEndM1L,
segment4LmidDocEndM1L },
true },
segment2LmidDocEndM1L,
segment3LmidDocEndM1L },
false },
GeneratedMovementTest{
L"Move non-degenerate range at position 4 -1 times by Word",
GeneratedMovementTestInput{
Expand Down Expand Up @@ -3679,7 +3679,7 @@ static constexpr std::array<GeneratedMovementTest, 340> s_movementTests{
-5,
lastCharPosLeft,
lastCharPosLeft },
true },
false },
GeneratedMovementTest{
L"Move degenerate range at position 5 -1 times by Word",
GeneratedMovementTestInput{
Expand All @@ -3691,7 +3691,7 @@ static constexpr std::array<GeneratedMovementTest, 340> s_movementTests{
-1,
segment4LmidDocEnd,
segment4LmidDocEnd },
true },
false },
GeneratedMovementTest{
L"Move degenerate range at position 5 0 times by Word",
GeneratedMovementTestInput{
Expand Down Expand Up @@ -3799,7 +3799,7 @@ static constexpr std::array<GeneratedMovementTest, 340> s_movementTests{
-5,
midDocEndLeft,
midDocEndLeft },
true },
false },
GeneratedMovementTest{
L"Move degenerate range at position 6 -1 times by Word",
GeneratedMovementTestInput{
Expand All @@ -3811,7 +3811,7 @@ static constexpr std::array<GeneratedMovementTest, 340> s_movementTests{
-1,
segment4LmidDocEnd,
segment4LmidDocEnd },
true },
false },
GeneratedMovementTest{
L"Move degenerate range at position 6 0 times by Word",
GeneratedMovementTestInput{
Expand Down Expand Up @@ -3859,7 +3859,7 @@ static constexpr std::array<GeneratedMovementTest, 340> s_movementTests{
-5,
midDocEndLeft,
midDocEndLeft },
true },
false },
GeneratedMovementTest{
L"Move non-degenerate range at position 6 -1 times by Word",
GeneratedMovementTestInput{
Expand All @@ -3871,7 +3871,7 @@ static constexpr std::array<GeneratedMovementTest, 340> s_movementTests{
-1,
segment4LmidDocEnd,
segment4LmidDocEnd },
true },
false },
GeneratedMovementTest{
L"Move non-degenerate range at position 6 0 times by Word",
GeneratedMovementTestInput{
Expand Down Expand Up @@ -3919,7 +3919,7 @@ static constexpr std::array<GeneratedMovementTest, 340> s_movementTests{
-5,
midDocEndLeft,
midDocEndLeft },
true },
false },
GeneratedMovementTest{
L"Move degenerate range at position 7 -1 times by Word",
GeneratedMovementTestInput{
Expand All @@ -3931,7 +3931,7 @@ static constexpr std::array<GeneratedMovementTest, 340> s_movementTests{
-1,
segment4LmidDocEnd,
segment4LmidDocEnd },
true },
false },
GeneratedMovementTest{
L"Move degenerate range at position 7 0 times by Word",
GeneratedMovementTestInput{
Expand Down Expand Up @@ -3979,7 +3979,7 @@ static constexpr std::array<GeneratedMovementTest, 340> s_movementTests{
-5,
midDocEndLeft,
midDocEndLeft },
true },
false },
GeneratedMovementTest{
L"Move non-degenerate range at position 7 -1 times by Word",
GeneratedMovementTestInput{
Expand All @@ -3991,7 +3991,7 @@ static constexpr std::array<GeneratedMovementTest, 340> s_movementTests{
-1,
segment4LmidDocEnd,
segment4LmidDocEnd },
true },
false },
GeneratedMovementTest{
L"Move non-degenerate range at position 7 0 times by Word",
GeneratedMovementTestInput{
Expand Down Expand Up @@ -4039,7 +4039,7 @@ static constexpr std::array<GeneratedMovementTest, 340> s_movementTests{
-5,
midDocEndLeft,
midDocEndLeft },
true },
false },
GeneratedMovementTest{
L"Move degenerate range at position 8 -1 times by Word",
GeneratedMovementTestInput{
Expand All @@ -4051,7 +4051,7 @@ static constexpr std::array<GeneratedMovementTest, 340> s_movementTests{
-1,
segment4LmidDocEnd,
segment4LmidDocEnd },
true },
false },
GeneratedMovementTest{
L"Move degenerate range at position 8 0 times by Word",
GeneratedMovementTestInput{
Expand Down Expand Up @@ -4099,7 +4099,7 @@ static constexpr std::array<GeneratedMovementTest, 340> s_movementTests{
-5,
midDocEndLeft,
midDocEndLeft },
true },
false },
GeneratedMovementTest{
L"Move non-degenerate range at position 8 -1 times by Word",
GeneratedMovementTestInput{
Expand All @@ -4111,7 +4111,7 @@ static constexpr std::array<GeneratedMovementTest, 340> s_movementTests{
-1,
segment4LmidDocEnd,
segment4LmidDocEnd },
true },
false },
GeneratedMovementTest{
L"Move non-degenerate range at position 8 0 times by Word",
GeneratedMovementTestInput{
Expand Down Expand Up @@ -4159,7 +4159,7 @@ static constexpr std::array<GeneratedMovementTest, 340> s_movementTests{
-5,
midDocEndLeft,
midDocEndLeft },
true },
false },
GeneratedMovementTest{
L"Move degenerate range at position 9 -1 times by Word",
GeneratedMovementTestInput{
Expand All @@ -4171,7 +4171,7 @@ static constexpr std::array<GeneratedMovementTest, 340> s_movementTests{
-1,
segment4LmidDocEnd,
segment4LmidDocEnd },
true },
false },
GeneratedMovementTest{
L"Move degenerate range at position 9 0 times by Word",
GeneratedMovementTestInput{
Expand Down
6 changes: 4 additions & 2 deletions src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1417,11 +1417,13 @@ class UiaTextRangeTests
// reset the UTR
if (degenerate)
{
// UTR: (exclusive, exclusive) range
const auto utrStart{ atDocumentEnd ? documentEndExclusive : endExclusive };
THROW_IF_FAILED(Microsoft::WRL::MakeAndInitialize<UiaTextRange>(&utr, _pUiaData, &_dummyProvider, utrStart, utrEnd));
}
else
{
// UTR: (inclusive, exclusive) range
const auto utrStart{ atDocumentEnd ? documentEndInclusive : endInclusive };
THROW_IF_FAILED(Microsoft::WRL::MakeAndInitialize<UiaTextRange>(&utr, _pUiaData, &_dummyProvider, utrStart, utrEnd));
}
Expand All @@ -1443,8 +1445,8 @@ class UiaTextRangeTests
else if (textUnit <= TextUnit::TextUnit_Word)
{
VERIFY_ARE_EQUAL(-1, moveAmt);
VERIFY_ARE_EQUAL(origin, til::point{ utr->_start });
VERIFY_ARE_EQUAL(degenerate || !atDocumentEnd ? origin : writeTarget, til::point{ utr->_end });
VERIFY_ARE_EQUAL(degenerate || !atDocumentEnd ? writeTarget : origin, til::point{ utr->_start });
VERIFY_ARE_EQUAL(writeTarget, til::point{ utr->_end });
}
else if (textUnit <= TextUnit::TextUnit_Line)
{
Expand Down
28 changes: 28 additions & 0 deletions src/types/UiaTextRangeBase.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1522,6 +1522,14 @@ void UiaTextRangeBase::_moveEndpointByUnitWord(_In_ const int moveCount,
{
success = false;
}
else if (allowBottomExclusive && _tryMoveToWordStart(buffer, documentEnd, resultPos))
{
// IMPORTANT: _tryMoveToWordStart modifies resultPos if successful
// Degenerate ranges first move to the beginning of the word,
// but if we're already at the beginning of the word, we continue
// to the next branch and move to the previous word!
(*pAmountMoved)--;
}
else if (buffer.MoveToPreviousWord(nextPos, _wordDelimiters))
{
resultPos = nextPos;
Expand All @@ -1541,6 +1549,26 @@ void UiaTextRangeBase::_moveEndpointByUnitWord(_In_ const int moveCount,
SetEndpoint(endpoint, resultPos);
}

// Routine Description:
// - tries to move resultingPos to the beginning of the word
// Arguments:
// - buffer - the text buffer we're operating on
// - documentEnd - the document end of the buffer (see _getDocumentEnd())
// - resultingPos - the position we're starting from and modifying
// Return Value:
// - true --> we were not at the beginning of the word, and we updated resultingPos to be so
// - false --> otherwise (we're already at the beginning of the word)
bool UiaTextRangeBase::_tryMoveToWordStart(const TextBuffer& buffer, const til::point documentEnd, COORD& resultingPos) const
{
const auto wordStart{ buffer.GetWordStart(resultingPos, _wordDelimiters, true, documentEnd) };
if (resultingPos != wordStart)
{
resultingPos = wordStart;
return true;
}
return false;
}

// Routine Description:
// - moves the UTR's endpoint by moveCount times by line.
// - if endpoints crossed, the degenerate range is created and both endpoints are moved
Expand Down
1 change: 1 addition & 0 deletions src/types/UiaTextRangeBase.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ namespace Microsoft::Console::Types

std::optional<bool> _verifyAttr(TEXTATTRIBUTEID attributeId, VARIANT val, const TextAttribute& attr) const;
bool _initializeAttrQuery(TEXTATTRIBUTEID attributeId, VARIANT* pRetVal, const TextAttribute& attr) const;
bool _tryMoveToWordStart(const TextBuffer& buffer, const til::point documentEnd, COORD& resultingPos) const;

COORD _getInclusiveEnd() noexcept;

Expand Down
Loading

0 comments on commit 5deb332

Please sign in to comment.