From ecfe4b5d6a5bc46fadefd28410aa28051f28dec0 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Fri, 2 Sep 2022 14:25:09 -0700 Subject: [PATCH] Revert "Remove most uses of `CompareInBounds` (#13244)" (#13907) This reverts commit f785168aacf79e2923f0d140e17fdd4c9364e51d (PR #13244) The error logged to NVDA was caused by the following line of code in `_getTextValue()`: `THROW_HR_IF(E_FAIL, !bufferSize.IsInBounds(_start) || !bufferSize.IsInBounds(_end));` NVDA would expand a text range to encompass the document in the alt buffer. This means that the "end" would be set to the dangling "endExclusive" point (x = left, y = one past the end of the buffer). This is a valid range! However, upon extracting the text, we would hit the code above. The exclusive end doesn't actually point to anything in the buffer, so we would falsly throw `E_FAIL`. Though this could be fixed by adding a special check for the `endExclusive` in the line above, I suspect there are other places throughout the UIA code that hit this problem too. The safest course of action is to revert this commit entirely since it was a code health commit (it doesn't actually close an issue). Closes #13866 (cherry picked from commit bfd5248a2e947c28d6ffa375f5764ebbcf117b7e) Service-Card-Id: 85405833 Service-Version: 1.15 --- src/buffer/out/textBuffer.cpp | 30 ++++-------- .../TerminalCore/TerminalSelection.cpp | 6 +-- .../UiaTextRangeTests.cpp | 4 +- src/types/UiaTextRangeBase.cpp | 49 ++++++++++--------- src/types/inc/viewport.hpp | 4 +- src/types/viewport.cpp | 22 +++++++-- 6 files changed, 60 insertions(+), 55 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index ac699ec4351..859bc5b231f 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1138,7 +1138,7 @@ til::point TextBuffer::GetWordStart(const til::point target, const std::wstring_ // that it actually points to a space in the buffer copy = bufferSize.BottomRightInclusive(); } - else if (target >= limit) + else if (bufferSize.CompareInBounds(target, limit, true) >= 0) { // if at/past the limit --> clamp to limit copy = limitOptional.value_or(bufferSize.BottomRightInclusive()); @@ -1254,7 +1254,7 @@ til::point TextBuffer::GetWordEnd(const til::point target, const std::wstring_vi // Already at/past the limit. Can't move forward. const auto bufferSize{ GetSize() }; const auto limit{ limitOptional.value_or(bufferSize.EndExclusive()) }; - if (target >= limit) + if (bufferSize.CompareInBounds(target, limit, true) >= 0) { return target; } @@ -1282,7 +1282,7 @@ til::point TextBuffer::_GetWordEndForAccessibility(const til::point target, cons const auto bufferSize{ GetSize() }; auto result{ target }; - if (target >= limit) + if (bufferSize.CompareInBounds(target, limit, true) >= 0) { // if we're already on/past the last RegularChar, // clamp result to that position @@ -1419,7 +1419,7 @@ bool TextBuffer::MoveToNextWord(til::point& pos, const std::wstring_view wordDel const auto limit{ limitOptional.value_or(bufferSize.EndExclusive()) }; const auto copy{ _GetWordEndForAccessibility(pos, wordDelimiters, limit) }; - if (copy >= limit) + if (bufferSize.CompareInBounds(copy, limit, true) >= 0) { return false; } @@ -1466,7 +1466,7 @@ til::point TextBuffer::GetGlyphStart(const til::point pos, std::optional limit) + if (bufferSize.CompareInBounds(resultPos, limit, true) > 0) { resultPos = limit; } @@ -1494,7 +1494,7 @@ til::point TextBuffer::GetGlyphEnd(const til::point pos, bool accessibilityMode, const auto limit{ limitOptional.value_or(bufferSize.EndExclusive()) }; // Clamp pos to limit - if (resultPos > limit) + if (bufferSize.CompareInBounds(resultPos, limit, true) > 0) { resultPos = limit; } @@ -1524,19 +1524,9 @@ til::point TextBuffer::GetGlyphEnd(const til::point pos, bool accessibilityMode, bool TextBuffer::MoveToNextGlyph(til::point& pos, bool allowExclusiveEnd, std::optional limitOptional) const { const auto bufferSize = GetSize(); - bool pastEndInclusive; - til::point limit; - { - // if the limit is past the end of the buffer, - // 1) clamp limit to end of buffer - // 2) set pastEndInclusive - const auto endInclusive{ bufferSize.BottomRightInclusive() }; - const auto val = limitOptional.value_or(endInclusive); - pastEndInclusive = val > endInclusive; - limit = pastEndInclusive ? endInclusive : val; - } + const auto limit{ limitOptional.value_or(bufferSize.EndExclusive()) }; - const auto distanceToLimit{ bufferSize.CompareInBounds(pos, limit) + (pastEndInclusive ? 1 : 0) }; + const auto distanceToLimit{ bufferSize.CompareInBounds(pos, limit, true) }; if (distanceToLimit >= 0) { // Corner Case: we're on/past the limit @@ -1579,7 +1569,7 @@ bool TextBuffer::MoveToPreviousGlyph(til::point& pos, std::optional const auto bufferSize = GetSize(); const auto limit{ limitOptional.value_or(bufferSize.EndExclusive()) }; - if (pos > limit) + if (bufferSize.CompareInBounds(pos, limit, true) > 0) { // we're past the end // clamp us to the limit @@ -1621,7 +1611,7 @@ const std::vector TextBuffer::GetTextRects(til::point start // (0,0) is the top-left of the screen // the physically "higher" coordinate is closer to the top-left // the physically "lower" coordinate is closer to the bottom-right - const auto [higherCoord, lowerCoord] = start <= end ? + const auto [higherCoord, lowerCoord] = bufferSize.CompareInBounds(start, end) <= 0 ? std::make_tuple(start, end) : std::make_tuple(end, start); diff --git a/src/cascadia/TerminalCore/TerminalSelection.cpp b/src/cascadia/TerminalCore/TerminalSelection.cpp index 58a1ba72913..b9732be10aa 100644 --- a/src/cascadia/TerminalCore/TerminalSelection.cpp +++ b/src/cascadia/TerminalCore/TerminalSelection.cpp @@ -228,7 +228,7 @@ void Terminal::SetSelectionEnd(const til::point viewportPos, std::optional Terminal::_PivotSelection(const til::point targetPos, bool& targetStart) const { - if (targetStart = targetPos <= _selection->pivot) + if (targetStart = _activeBuffer().GetSize().CompareInBounds(targetPos, _selection->pivot) <= 0) { // target is before pivot // treat target as start @@ -511,7 +511,7 @@ void Terminal::_MoveByWord(SelectionDirection direction, til::point& pos) case SelectionDirection::Left: { const auto wordStartPos{ _activeBuffer().GetWordStart(pos, _wordDelimiters) }; - if (_selection->pivot < pos) + if (_activeBuffer().GetSize().CompareInBounds(_selection->pivot, pos) < 0) { // If we're moving towards the pivot, move one more cell pos = wordStartPos; @@ -534,7 +534,7 @@ void Terminal::_MoveByWord(SelectionDirection direction, til::point& pos) case SelectionDirection::Right: { const auto wordEndPos{ _activeBuffer().GetWordEnd(pos, _wordDelimiters) }; - if (pos < _selection->pivot) + if (_activeBuffer().GetSize().CompareInBounds(pos, _selection->pivot) < 0) { // If we're moving towards the pivot, move one more cell pos = _activeBuffer().GetWordEnd(pos, _wordDelimiters); diff --git a/src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp b/src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp index d73379d548e..90e0302861a 100644 --- a/src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp +++ b/src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp @@ -484,9 +484,9 @@ class UiaTextRangeTests Log::Comment(L"_start and end should be 2 units apart. Sign depends on order of comparison."); THROW_IF_FAILED(utr1->CompareEndpoints(TextPatternRangeEndpoint_End, utr2.Get(), TextPatternRangeEndpoint_End, &comparison)); - VERIFY_IS_TRUE(comparison == -1); + VERIFY_IS_TRUE(comparison == -2); THROW_IF_FAILED(utr2->CompareEndpoints(TextPatternRangeEndpoint_End, utr1.Get(), TextPatternRangeEndpoint_End, &comparison)); - VERIFY_IS_TRUE(comparison == 1); + VERIFY_IS_TRUE(comparison == 2); } TEST_METHOD(ExpandToEnclosingUnit) diff --git a/src/types/UiaTextRangeBase.cpp b/src/types/UiaTextRangeBase.cpp index 877bed745a1..4efd48977aa 100644 --- a/src/types/UiaTextRangeBase.cpp +++ b/src/types/UiaTextRangeBase.cpp @@ -70,7 +70,7 @@ try RETURN_IF_FAILED(RuntimeClassInitialize(pData, pProvider, wordDelimiters)); // start is before/at end, so this is valid - if (start <= end) + if (_pData->GetTextBuffer().GetSize().CompareInBounds(start, end, true) <= 0) { _start = start; _end = end; @@ -166,7 +166,7 @@ bool UiaTextRangeBase::SetEndpoint(TextPatternRangeEndpoint endpoint, const til: case TextPatternRangeEndpoint_End: _end = val; // if end is before start... - if (_end < _start) + if (bufferSize.CompareInBounds(_end, _start, true) < 0) { // make this range degenerate at end _start = _end; @@ -175,7 +175,7 @@ bool UiaTextRangeBase::SetEndpoint(TextPatternRangeEndpoint endpoint, const til: case TextPatternRangeEndpoint_Start: _start = val; // if start is after end... - if (_start > _end) + if (bufferSize.CompareInBounds(_start, _end, true) > 0) { // make this range degenerate at start _end = _start; @@ -246,13 +246,13 @@ try const auto mine = GetEndpoint(endpoint); // TODO GH#5406: create a different UIA parent object for each TextBuffer - // We should return E_FAIL if we detect that the endpoints are from two different TextBuffer objects. - // For now, we're fine to not do that because we're not using any code that can FAIL_FAST anymore. + // This is a temporary solution to comparing two UTRs from different TextBuffers + // Ensure both endpoints fit in the current buffer. + const auto bufferSize = _pData->GetTextBuffer().GetSize(); + RETURN_HR_IF(E_FAIL, !bufferSize.IsInBounds(mine, true) || !bufferSize.IsInBounds(other, true)); // compare them - *pRetVal = mine < other ? -1 : - mine > other ? 1 : - 0; + *pRetVal = bufferSize.CompareInBounds(mine, other, true); UiaTracing::TextRange::CompareEndpoints(*this, endpoint, *range, targetEndpoint, *pRetVal); return S_OK; @@ -293,7 +293,7 @@ void UiaTextRangeBase::_expandToEnclosingUnit(TextUnit unit) // If we're past document end, // set us to ONE BEFORE the document end. // This allows us to expand properly. - if (_start >= documentEnd) + if (bufferSize.CompareInBounds(_start, documentEnd, true) >= 0) { _start = documentEnd; bufferSize.DecrementInBounds(_start, true); @@ -653,8 +653,8 @@ try bufferSize.IncrementInBounds(end, true); // make sure what was found is within the bounds of the current range - if ((searchDirection == Search::Direction::Forward && end < _end) || - (searchDirection == Search::Direction::Backward && start > _start)) + if ((searchDirection == Search::Direction::Forward && bufferSize.CompareInBounds(end, _end, true) < 0) || + (searchDirection == Search::Direction::Backward && bufferSize.CompareInBounds(start, _start) > 0)) { RETURN_IF_FAILED(Clone(ppRetVal)); auto& range = static_cast(**ppRetVal); @@ -872,7 +872,7 @@ IFACEMETHODIMP UiaTextRangeBase::GetBoundingRectangles(_Outptr_result_maybenull_ // startAnchor: the earliest til::point we will get a bounding rect for auto startAnchor = GetEndpoint(TextPatternRangeEndpoint_Start); - if (startAnchor < viewportOrigin) + if (bufferSize.CompareInBounds(startAnchor, viewportOrigin, true) < 0) { // earliest we can be is the origin startAnchor = viewportOrigin; @@ -880,7 +880,7 @@ IFACEMETHODIMP UiaTextRangeBase::GetBoundingRectangles(_Outptr_result_maybenull_ // endAnchor: the latest til::point we will get a bounding rect for auto endAnchor = GetEndpoint(TextPatternRangeEndpoint_End); - if (endAnchor > viewportEnd) + if (bufferSize.CompareInBounds(endAnchor, viewportEnd, true) > 0) { // latest we can be is the viewport end endAnchor = viewportEnd; @@ -889,7 +889,7 @@ IFACEMETHODIMP UiaTextRangeBase::GetBoundingRectangles(_Outptr_result_maybenull_ // _end is exclusive, let's be inclusive so we don't have to think about it anymore for bounding rects bufferSize.DecrementInBounds(endAnchor, true); - if (IsDegenerate() || _start > viewportEnd || _end < viewportOrigin) + if (IsDegenerate() || bufferSize.CompareInBounds(_start, viewportEnd, true) > 0 || bufferSize.CompareInBounds(_end, viewportOrigin, true) < 0) { // An empty array is returned for a degenerate (empty) text range. // reference: https://docs.microsoft.com/en-us/windows/win32/api/uiautomationclient/nf-uiautomationclient-iuiautomationtextrange-getboundingrectangles @@ -997,7 +997,7 @@ std::wstring UiaTextRangeBase::_getTextValue(til::CoordType maxLength) const // TODO GH#5406: create a different UIA parent object for each TextBuffer // nvaccess/nvda#11428: Ensure our endpoints are in bounds - THROW_HR_IF(E_FAIL, !bufferSize.IsInBounds(_start) || !bufferSize.IsInBounds(_end)); + THROW_HR_IF(E_FAIL, !bufferSize.IsInBounds(_start, true) || !bufferSize.IsInBounds(_end, true)); // convert _end to be inclusive auto inclusiveEnd = _end; @@ -1052,11 +1052,11 @@ try constexpr auto endpoint = TextPatternRangeEndpoint::TextPatternRangeEndpoint_Start; const auto bufferSize{ _pData->GetTextBuffer().GetSize() }; const auto documentEnd = _getDocumentEnd(); - if (_start > documentEnd) + if (bufferSize.CompareInBounds(_start, documentEnd, true) > 0) { _start = documentEnd; } - if (_end > documentEnd) + if (bufferSize.CompareInBounds(_end, documentEnd, true) > 0) { _end = documentEnd; } @@ -1126,11 +1126,11 @@ IFACEMETHODIMP UiaTextRangeBase::MoveEndpointByUnit(_In_ TextPatternRangeEndpoin } CATCH_LOG(); - if (_start > documentEnd) + if (bufferSize.CompareInBounds(_start, documentEnd, true) > 0) { _start = documentEnd; } - if (_end > documentEnd) + if (bufferSize.CompareInBounds(_end, documentEnd, true) > 0) { _end = documentEnd; } @@ -1180,7 +1180,7 @@ try const auto bufferSize = _pData->GetTextBuffer().GetSize(); const auto mine = GetEndpoint(endpoint); const auto other = range->GetEndpoint(targetEndpoint); - RETURN_HR_IF(E_FAIL, !bufferSize.IsInBounds(mine) || !bufferSize.IsInBounds(other)); + RETURN_HR_IF(E_FAIL, !bufferSize.IsInBounds(mine, true) || !bufferSize.IsInBounds(other, true)); SetEndpoint(endpoint, range->GetEndpoint(targetEndpoint)); @@ -1206,7 +1206,10 @@ try else { const auto bufferSize = _pData->GetTextBuffer().GetSize(); - RETURN_HR_IF(E_FAIL, !bufferSize.IsInBounds(_start) || !bufferSize.IsInBounds(_end)); + if (!bufferSize.IsInBounds(_start, true) || !bufferSize.IsInBounds(_end, true)) + { + return E_FAIL; + } auto inclusiveEnd = _end; bufferSize.DecrementInBounds(inclusiveEnd); _pData->SelectNewRegion(_start, inclusiveEnd); @@ -1520,7 +1523,7 @@ void UiaTextRangeBase::_moveEndpointByUnitWord(_In_ const int moveCount, { case MovementDirection::Forward: { - if (nextPos >= documentEnd) + if (bufferSize.CompareInBounds(nextPos, documentEnd, true) >= 0) { success = false; } @@ -1741,7 +1744,7 @@ void UiaTextRangeBase::_moveEndpointByUnitDocument(_In_ const int moveCount, } CATCH_LOG(); - if (preventBoundary || target >= documentEnd) + if (preventBoundary || bufferSize.CompareInBounds(target, documentEnd, true) >= 0) { return; } diff --git a/src/types/inc/viewport.hpp b/src/types/inc/viewport.hpp index 0772d8fe8cb..d762f64348f 100644 --- a/src/types/inc/viewport.hpp +++ b/src/types/inc/viewport.hpp @@ -55,7 +55,7 @@ namespace Microsoft::Console::Types til::size Dimensions() const noexcept; bool IsInBounds(const Viewport& other) const noexcept; - bool IsInBounds(const til::point pos) const noexcept; + bool IsInBounds(const til::point pos, bool allowEndExclusive = false) const noexcept; void Clamp(til::point& pos) const; Viewport Clamp(const Viewport& other) const noexcept; @@ -65,7 +65,7 @@ namespace Microsoft::Console::Types bool IncrementInBoundsCircular(til::point& pos) const noexcept; bool DecrementInBounds(til::point& pos, bool allowEndExclusive = false) const noexcept; bool DecrementInBoundsCircular(til::point& pos) const noexcept; - int CompareInBounds(const til::point first, const til::point second) const noexcept; + int CompareInBounds(const til::point first, const til::point second, bool allowEndExclusive = false) const noexcept; enum class XWalk { diff --git a/src/types/viewport.cpp b/src/types/viewport.cpp index f0eb50cf33e..15dc97a0a31 100644 --- a/src/types/viewport.cpp +++ b/src/types/viewport.cpp @@ -203,10 +203,18 @@ bool Viewport::IsInBounds(const Viewport& other) const noexcept // - Determines if the given coordinate position lies within this viewport. // Arguments: // - pos - Coordinate position +// - allowEndExclusive - if true, allow the EndExclusive til::point as a valid position. +// Used in accessibility to signify that the exclusive end +// includes the last til::point in a given viewport. // Return Value: // - True if it lies inside the viewport. False otherwise. -bool Viewport::IsInBounds(const til::point pos) const noexcept +bool Viewport::IsInBounds(const til::point pos, bool allowEndExclusive) const noexcept { + if (allowEndExclusive && pos == EndExclusive()) + { + return true; + } + return pos.X >= Left() && pos.X < RightExclusive() && pos.Y >= Top() && pos.Y < BottomExclusive(); } @@ -347,6 +355,9 @@ bool Viewport::DecrementInBoundsCircular(til::point& pos) const noexcept // Arguments: // - first- The first coordinate position // - second - The second coordinate position +// - allowEndExclusive - if true, allow the EndExclusive til::point as a valid position. +// Used in accessibility to signify that the exclusive end +// includes the last til::point in a given viewport. // Return Value: // - Negative if First is to the left of the Second. // - 0 if First and Second are the same coordinate. @@ -354,11 +365,12 @@ bool Viewport::DecrementInBoundsCircular(til::point& pos) const noexcept // - This is so you can do s_CompareCoords(first, second) <= 0 for "first is left or the same as second". // (the < looks like a left arrow :D) // - The magnitude of the result is the distance between the two coordinates when typing characters into the buffer (left to right, top to bottom) -int Viewport::CompareInBounds(const til::point first, const til::point second) const noexcept +#pragma warning(suppress : 4100) +int Viewport::CompareInBounds(const til::point first, const til::point second, bool allowEndExclusive) const noexcept { // Assert that our coordinates are within the expected boundaries - assert(IsInBounds(first)); - assert(IsInBounds(second)); + assert(IsInBounds(first, allowEndExclusive)); + assert(IsInBounds(second, allowEndExclusive)); // First set the distance vertically // If first is on row 4 and second is on row 6, first will be -2 rows behind second * an 80 character row would be -160. @@ -421,7 +433,7 @@ bool Viewport::WalkInBounds(til::point& pos, const WalkDir dir, bool allowEndExc bool Viewport::WalkInBoundsCircular(til::point& pos, const WalkDir dir, bool allowEndExclusive) const noexcept { // Assert that the position given fits inside this viewport. - assert((allowEndExclusive && pos == EndExclusive()) || IsInBounds(pos)); + assert(IsInBounds(pos, allowEndExclusive)); if (dir.x == XWalk::LeftToRight) {