From b02e1302a1e2b23f15a26379b439bd4c3973e88a Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Mon, 28 Sep 2020 15:30:51 -0700 Subject: [PATCH 1/4] Fix and test a11y movement by word --- src/buffer/out/textBuffer.cpp | 100 +++++------------- src/buffer/out/textBuffer.hpp | 4 +- src/host/ut_host/TextBufferTests.cpp | 82 ++++++++++++++ .../UiaTextRangeTests.cpp | 42 ++++++++ 4 files changed, 155 insertions(+), 73 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 9bd34412fd5..f99215f8260 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1118,15 +1118,10 @@ const COORD TextBuffer::GetWordEnd(const COORD target, const std::wstring_view w // so the words in the example include ["word ", "other "] // NOTE: the end anchor (this one) is exclusive, whereas the start anchor (GetWordStart) is inclusive - // Already at the end. Can't move forward. - if (target == GetSize().EndExclusive()) - { - return target; - } - if (accessibilityMode) { - return _GetWordEndForAccessibility(target, wordDelimiters); + const auto lastCharPos{ GetLastNonSpaceCharacter() }; + return _GetWordEndForAccessibility(target, wordDelimiters, lastCharPos); } else { @@ -1139,13 +1134,20 @@ const COORD TextBuffer::GetWordEnd(const COORD target, const std::wstring_view w // Arguments: // - target - a COORD on the word you are currently on // - wordDelimiters - what characters are we considering for the separation of words +// - lastCharPos - the position of the last nonspace character in the text buffer (to improve performance) // Return Value: // - The COORD for the first character of the next readable "word". If no next word, return one past the end of the buffer -const COORD TextBuffer::_GetWordEndForAccessibility(const COORD target, const std::wstring_view wordDelimiters) const +const COORD TextBuffer::_GetWordEndForAccessibility(const COORD target, const std::wstring_view wordDelimiters, const COORD lastCharPos) const { const auto bufferSize = GetSize(); COORD result = target; + // Check if we're already on/past the last RegularChar + if (bufferSize.CompareInBounds(result, lastCharPos, true) >= 0) + { + return bufferSize.EndExclusive(); + } + // ignore right boundary. Continue through readable text found while (_GetDelimiterClassAt(result, wordDelimiters) == DelimiterClass::RegularChar) { @@ -1155,6 +1157,12 @@ const COORD TextBuffer::_GetWordEndForAccessibility(const COORD target, const st } } + // we are already on/past the last RegularChar + if (bufferSize.CompareInBounds(result, lastCharPos, true) >= 0) + { + return bufferSize.EndExclusive(); + } + // make sure we expand to the beginning of the NEXT word while (_GetDelimiterClassAt(result, wordDelimiters) != DelimiterClass::RegularChar) { @@ -1256,44 +1264,17 @@ void TextBuffer::_PruneHyperlinks() // - pos - The COORD for the first character on the "word" (inclusive) bool TextBuffer::MoveToNextWord(COORD& pos, const std::wstring_view wordDelimiters, COORD lastCharPos) const { - auto copy = pos; - const auto bufferSize = GetSize(); + // move to the beginning of the next word + // NOTE: _GetWordEnd...() returns the exclusive position of the "end of the word" + // This is also the inclusive start of the next word. + auto copy{ _GetWordEndForAccessibility(pos, wordDelimiters, lastCharPos) }; - // Already at the end. Can't move forward. - if (pos == bufferSize.EndExclusive()) + if (copy == GetSize().EndExclusive()) { + // there is no next word return false; } - // started on a word, continue until the end of the word - while (_GetDelimiterClassAt(copy, wordDelimiters) == DelimiterClass::RegularChar) - { - if (!bufferSize.IncrementInBounds(copy)) - { - // last char in buffer is a RegularChar - // thus there is no next word - return false; - } - } - - // we are already on/past the last RegularChar - if (bufferSize.CompareInBounds(copy, lastCharPos) >= 0) - { - return false; - } - - // on whitespace, continue until the beginning of the next word - while (_GetDelimiterClassAt(copy, wordDelimiters) != DelimiterClass::RegularChar) - { - if (!bufferSize.IncrementInBounds(copy)) - { - // last char in buffer is a DelimiterChar or ControlChar - // there is no next word - return false; - } - } - - // successful move, copy result out pos = copy; return true; } @@ -1308,40 +1289,17 @@ bool TextBuffer::MoveToNextWord(COORD& pos, const std::wstring_view wordDelimite // - pos - The COORD for the first character on the "word" (inclusive) bool TextBuffer::MoveToPreviousWord(COORD& pos, std::wstring_view wordDelimiters) const { - auto copy = pos; - const auto bufferSize{ GetSize() }; - - // GH#7663: Treat EndExclusive as EndInclusive so - // that it actually points to a space in the buffer - if (pos == bufferSize.EndExclusive()) - { - copy = { bufferSize.RightInclusive(), bufferSize.BottomInclusive() }; - } - - // started on whitespace/delimiter, continue until the end of the previous word - while (_GetDelimiterClassAt(copy, wordDelimiters) != DelimiterClass::RegularChar) - { - if (!bufferSize.DecrementInBounds(copy)) - { - // first char in buffer is a DelimiterChar or ControlChar - // there is no previous word - return false; - } - } + // move to the beginning of the current word + auto copy{ GetWordStart(pos, wordDelimiters, true) }; - // on a word, continue until the beginning of the word - while (_GetDelimiterClassAt(copy, wordDelimiters) == DelimiterClass::RegularChar) + if (!GetSize().DecrementInBounds(copy, true)) { - if (!bufferSize.DecrementInBounds(copy)) - { - // first char in buffer is a RegularChar - // there is no previous word - return false; - } + // can't move behind current word + return false; } - // successful move, copy result out - pos = copy; + // move to the beginning of the previous word + pos = GetWordStart(copy, wordDelimiters, true); return true; } diff --git a/src/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp index 56a3c59f3b0..d81c0327d0b 100644 --- a/src/buffer/out/textBuffer.hpp +++ b/src/buffer/out/textBuffer.hpp @@ -131,7 +131,7 @@ class TextBuffer final const COORD GetWordStart(const COORD target, const std::wstring_view wordDelimiters, bool accessibilityMode = false) const; const COORD GetWordEnd(const COORD target, const std::wstring_view wordDelimiters, bool accessibilityMode = false) const; - bool MoveToNextWord(COORD& pos, const std::wstring_view wordDelimiters, COORD lastCharPos) const; + bool MoveToNextWord(COORD& pos, const std::wstring_view wordDelimiters, const COORD lastCharPos) const; bool MoveToPreviousWord(COORD& pos, const std::wstring_view wordDelimiters) const; const til::point GetGlyphStart(const til::point pos) const; @@ -224,7 +224,7 @@ class TextBuffer final const DelimiterClass _GetDelimiterClassAt(const COORD pos, const std::wstring_view wordDelimiters) const; const COORD _GetWordStartForAccessibility(const COORD target, const std::wstring_view wordDelimiters) const; const COORD _GetWordStartForSelection(const COORD target, const std::wstring_view wordDelimiters) const; - const COORD _GetWordEndForAccessibility(const COORD target, const std::wstring_view wordDelimiters) const; + const COORD _GetWordEndForAccessibility(const COORD target, const std::wstring_view wordDelimiters, COORD lastCharPos) const; const COORD _GetWordEndForSelection(const COORD target, const std::wstring_view wordDelimiters) const; void _PruneHyperlinks(); diff --git a/src/host/ut_host/TextBufferTests.cpp b/src/host/ut_host/TextBufferTests.cpp index 638e15c1fe4..6be6daa79a9 100644 --- a/src/host/ut_host/TextBufferTests.cpp +++ b/src/host/ut_host/TextBufferTests.cpp @@ -148,6 +148,7 @@ class TextBufferTests void WriteLinesToBuffer(const std::vector& text, TextBuffer& buffer); TEST_METHOD(GetWordBoundaries); + TEST_METHOD(MoveByWord); TEST_METHOD(GetGlyphBoundaries); TEST_METHOD(GetTextRects); @@ -2132,6 +2133,87 @@ void TextBufferTests::GetWordBoundaries() } } +void TextBufferTests::MoveByWord() +{ + COORD bufferSize{ 80, 9001 }; + UINT cursorSize = 12; + TextAttribute attr{ 0x7f }; + auto _buffer = std::make_unique(bufferSize, attr, cursorSize, _renderTarget); + + // Setup: Write lines of text to the buffer + const std::vector text = { L"word other", + L" more words" }; + WriteLinesToBuffer(text, *_buffer); + + // Test Data: + // - COORD - starting position + // - COORD - expected result (moving forwards) + // - COORD - expected result (moving backwards) + struct ExpectedResult + { + COORD moveForwards; + COORD moveBackwards; + }; + + struct Test + { + COORD startPos; + ExpectedResult expected; + }; + + // Set testData for GetWordStart tests + // clang-format off + std::vector testData = { + // tests for first line of text + { { 0, 0 }, {{ 5, 0 }, { 0, 0 }} }, + { { 1, 0 }, {{ 5, 0 }, { 1, 0 }} }, + { { 3, 0 }, {{ 5, 0 }, { 3, 0 }} }, + { { 4, 0 }, {{ 5, 0 }, { 4, 0 }} }, + { { 5, 0 }, {{ 2, 1 }, { 0, 0 }} }, + { { 6, 0 }, {{ 2, 1 }, { 0, 0 }} }, + { { 20, 0 }, {{ 2, 1 }, { 0, 0 }} }, + { { 79, 0 }, {{ 2, 1 }, { 0, 0 }} }, + + // tests for second line of text + { { 0, 1 }, {{ 2, 1 }, { 0, 0 }} }, + { { 1, 1 }, {{ 2, 1 }, { 0, 0 }} }, + { { 2, 1 }, {{ 9, 1 }, { 5, 0 }} }, + { { 3, 1 }, {{ 9, 1 }, { 5, 0 }} }, + { { 5, 1 }, {{ 9, 1 }, { 5, 0 }} }, + { { 6, 1 }, {{ 9, 1 }, { 5, 0 }} }, + { { 7, 1 }, {{ 9, 1 }, { 5, 0 }} }, + { { 9, 1 }, {{ 9, 1 }, { 2, 1 }} }, + { { 10, 1 }, {{10, 1 }, { 2, 1 }} }, + { { 20, 1 }, {{20, 1 }, { 2, 1 }} }, + { { 79, 1 }, {{79, 1 }, { 2, 1 }} }, + }; + // clang-format on + + BEGIN_TEST_METHOD_PROPERTIES() + TEST_METHOD_PROPERTY(L"Data:movingForwards", L"{false, true}") + END_TEST_METHOD_PROPERTIES(); + + bool movingForwards; + VERIFY_SUCCEEDED(TestData::TryGetValue(L"movingForwards", movingForwards), L"Get movingForwards variant"); + + const std::wstring_view delimiters = L" "; + const COORD lastCharPos = _buffer->GetLastNonSpaceCharacter(); + for (const auto& test : testData) + { + Log::Comment(NoThrowString().Format(L"COORD (%hd, %hd)", test.startPos.X, test.startPos.Y)); + auto pos{ test.startPos }; + const auto result = movingForwards ? + _buffer->MoveToNextWord(pos, delimiters, lastCharPos) : + _buffer->MoveToPreviousWord(pos, delimiters); + const auto expected = movingForwards ? test.expected.moveForwards : test.expected.moveBackwards; + VERIFY_ARE_EQUAL(expected, pos); + + // if we moved, result is true and pos != startPos. + // otherwise, result is false and pos == startPos. + VERIFY_ARE_EQUAL(result, pos != test.startPos); + } +} + void TextBufferTests::GetGlyphBoundaries() { struct ExpectedResult diff --git a/src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp b/src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp index 50e36e66f5c..d0ee72fa53c 100644 --- a/src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp +++ b/src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp @@ -1190,4 +1190,46 @@ class UiaTextRangeTests VERIFY_ARE_EQUAL(-1, moveAmt); } } + + TEST_METHOD(MoveToPreviousWord) + { + // See GH#7742 for more details. + + const auto bufferSize{ _pTextBuffer->GetSize() }; + const COORD origin{ bufferSize.Origin() }; + const COORD originExclusive{ origin.X, origin.Y + 1 }; + + _pTextBuffer->Write({ L"My name is Carlos" }, origin); + + // Create degenerate UTR at origin + Microsoft::WRL::ComPtr utr; + THROW_IF_FAILED(Microsoft::WRL::MakeAndInitialize(&utr, _pUiaData, &_dummyProvider, origin, origin)); + + // move forward by a word + int moveAmt; + THROW_IF_FAILED(utr->Move(TextUnit::TextUnit_Word, 1, &moveAmt)); + VERIFY_ARE_EQUAL(1, moveAmt); + VERIFY_IS_TRUE(utr->IsDegenerate()); + + // Expand by word + BSTR text; + THROW_IF_FAILED(utr->ExpandToEnclosingUnit(TextUnit::TextUnit_Word)); + THROW_IF_FAILED(utr->GetText(-1, &text)); + VERIFY_ARE_EQUAL(L"name ", std::wstring_view{ text }); + + // Collapse utr (move end to start) + const COORD expectedStart{ 3, 0 }; + THROW_IF_FAILED(utr->MoveEndpointByRange(TextPatternRangeEndpoint::TextPatternRangeEndpoint_End, utr.Get(), TextPatternRangeEndpoint::TextPatternRangeEndpoint_Start)); + VERIFY_ARE_EQUAL(expectedStart, utr->_start); + VERIFY_IS_TRUE(utr->IsDegenerate()); + + // Move back by a word + THROW_IF_FAILED(utr->Move(TextUnit::TextUnit_Word, -1, &moveAmt)); + VERIFY_ARE_EQUAL(-1, moveAmt); + + // Expand by character + THROW_IF_FAILED(utr->ExpandToEnclosingUnit(TextUnit::TextUnit_Character)); + THROW_IF_FAILED(utr->GetText(-1, &text)); + VERIFY_ARE_EQUAL(L"M", std::wstring_view{ text }); + } }; From 3102da77002026171c84dca051e178b80a50d655 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Mon, 28 Sep 2020 17:03:28 -0700 Subject: [PATCH 2/4] remove optimization for MoveToNextWord() --- src/buffer/out/textBuffer.cpp | 63 +++++++++++++++++++++++------------ src/buffer/out/textBuffer.hpp | 2 +- 2 files changed, 42 insertions(+), 23 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index f99215f8260..b49cf8031ce 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1118,10 +1118,15 @@ const COORD TextBuffer::GetWordEnd(const COORD target, const std::wstring_view w // so the words in the example include ["word ", "other "] // NOTE: the end anchor (this one) is exclusive, whereas the start anchor (GetWordStart) is inclusive + // Already at the end. Can't move forward. + if (target == GetSize().EndExclusive()) + { + return target; + } + if (accessibilityMode) { - const auto lastCharPos{ GetLastNonSpaceCharacter() }; - return _GetWordEndForAccessibility(target, wordDelimiters, lastCharPos); + return _GetWordEndForAccessibility(target, wordDelimiters); } else { @@ -1134,20 +1139,13 @@ const COORD TextBuffer::GetWordEnd(const COORD target, const std::wstring_view w // Arguments: // - target - a COORD on the word you are currently on // - wordDelimiters - what characters are we considering for the separation of words -// - lastCharPos - the position of the last nonspace character in the text buffer (to improve performance) // Return Value: // - The COORD for the first character of the next readable "word". If no next word, return one past the end of the buffer -const COORD TextBuffer::_GetWordEndForAccessibility(const COORD target, const std::wstring_view wordDelimiters, const COORD lastCharPos) const +const COORD TextBuffer::_GetWordEndForAccessibility(const COORD target, const std::wstring_view wordDelimiters) const { const auto bufferSize = GetSize(); COORD result = target; - // Check if we're already on/past the last RegularChar - if (bufferSize.CompareInBounds(result, lastCharPos, true) >= 0) - { - return bufferSize.EndExclusive(); - } - // ignore right boundary. Continue through readable text found while (_GetDelimiterClassAt(result, wordDelimiters) == DelimiterClass::RegularChar) { @@ -1157,12 +1155,6 @@ const COORD TextBuffer::_GetWordEndForAccessibility(const COORD target, const st } } - // we are already on/past the last RegularChar - if (bufferSize.CompareInBounds(result, lastCharPos, true) >= 0) - { - return bufferSize.EndExclusive(); - } - // make sure we expand to the beginning of the NEXT word while (_GetDelimiterClassAt(result, wordDelimiters) != DelimiterClass::RegularChar) { @@ -1264,17 +1256,44 @@ void TextBuffer::_PruneHyperlinks() // - pos - The COORD for the first character on the "word" (inclusive) bool TextBuffer::MoveToNextWord(COORD& pos, const std::wstring_view wordDelimiters, COORD lastCharPos) const { - // move to the beginning of the next word - // NOTE: _GetWordEnd...() returns the exclusive position of the "end of the word" - // This is also the inclusive start of the next word. - auto copy{ _GetWordEndForAccessibility(pos, wordDelimiters, lastCharPos) }; + auto copy = pos; + const auto bufferSize = GetSize(); + + // Already at the end. Can't move forward. + if (pos == bufferSize.EndExclusive()) + { + return false; + } - if (copy == GetSize().EndExclusive()) + // started on a word, continue until the end of the word + while (_GetDelimiterClassAt(copy, wordDelimiters) == DelimiterClass::RegularChar) + { + if (!bufferSize.IncrementInBounds(copy)) + { + // last char in buffer is a RegularChar + // thus there is no next word + return false; + } + } + + // we are already on/past the last RegularChar + if (bufferSize.CompareInBounds(copy, lastCharPos) >= 0) { - // there is no next word return false; } + // on whitespace, continue until the beginning of the next word + while (_GetDelimiterClassAt(copy, wordDelimiters) != DelimiterClass::RegularChar) + { + if (!bufferSize.IncrementInBounds(copy)) + { + // last char in buffer is a DelimiterChar or ControlChar + // there is no next word + return false; + } + } + + // successful move, copy result out pos = copy; return true; } diff --git a/src/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp index d81c0327d0b..3d2617d38f1 100644 --- a/src/buffer/out/textBuffer.hpp +++ b/src/buffer/out/textBuffer.hpp @@ -224,7 +224,7 @@ class TextBuffer final const DelimiterClass _GetDelimiterClassAt(const COORD pos, const std::wstring_view wordDelimiters) const; const COORD _GetWordStartForAccessibility(const COORD target, const std::wstring_view wordDelimiters) const; const COORD _GetWordStartForSelection(const COORD target, const std::wstring_view wordDelimiters) const; - const COORD _GetWordEndForAccessibility(const COORD target, const std::wstring_view wordDelimiters, COORD lastCharPos) const; + const COORD _GetWordEndForAccessibility(const COORD target, const std::wstring_view wordDelimiters) const; const COORD _GetWordEndForSelection(const COORD target, const std::wstring_view wordDelimiters) const; void _PruneHyperlinks(); From 9b1ee57ba5281286b56be00c9343b7fde1a2bd39 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 29 Sep 2020 10:02:55 -0700 Subject: [PATCH 3/4] add a missing const --- src/buffer/out/textBuffer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index b49cf8031ce..3cc4e65b0b2 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1254,7 +1254,7 @@ void TextBuffer::_PruneHyperlinks() // 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 character on the "word" (inclusive) -bool TextBuffer::MoveToNextWord(COORD& pos, const std::wstring_view wordDelimiters, COORD lastCharPos) const +bool TextBuffer::MoveToNextWord(COORD& pos, const std::wstring_view wordDelimiters, const COORD lastCharPos) const { auto copy = pos; const auto bufferSize = GetSize(); From 9d07b2c55a60032361c2576880b596203406c7d8 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 29 Sep 2020 10:19:21 -0700 Subject: [PATCH 4/4] un-const --- src/buffer/out/textBuffer.cpp | 2 +- src/buffer/out/textBuffer.hpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 3cc4e65b0b2..b49cf8031ce 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1254,7 +1254,7 @@ void TextBuffer::_PruneHyperlinks() // 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 character on the "word" (inclusive) -bool TextBuffer::MoveToNextWord(COORD& pos, const std::wstring_view wordDelimiters, const COORD lastCharPos) const +bool TextBuffer::MoveToNextWord(COORD& pos, const std::wstring_view wordDelimiters, COORD lastCharPos) const { auto copy = pos; const auto bufferSize = GetSize(); diff --git a/src/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp index 3d2617d38f1..56a3c59f3b0 100644 --- a/src/buffer/out/textBuffer.hpp +++ b/src/buffer/out/textBuffer.hpp @@ -131,7 +131,7 @@ class TextBuffer final const COORD GetWordStart(const COORD target, const std::wstring_view wordDelimiters, bool accessibilityMode = false) const; const COORD GetWordEnd(const COORD target, const std::wstring_view wordDelimiters, bool accessibilityMode = false) const; - bool MoveToNextWord(COORD& pos, const std::wstring_view wordDelimiters, const COORD lastCharPos) const; + bool MoveToNextWord(COORD& pos, const std::wstring_view wordDelimiters, COORD lastCharPos) const; bool MoveToPreviousWord(COORD& pos, const std::wstring_view wordDelimiters) const; const til::point GetGlyphStart(const til::point pos) const;