Revert "Remove most uses of CompareInBounds (#13244)" (#13907)

This reverts commit f785168 (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 bfd5248)
Service-Card-Id: 85405833
Service-Version: 1.15

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<til::po
     const auto limit{ limitOptional.value_or(bufferSize.EndExclusive()) };
 
     // Clamp pos to limit
-    if (resultPos > 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<til::point> 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<til::point>
     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<til::inclusive_rect> 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);
 

src/cascadia/TerminalCore/TerminalSelection.cpp

@@ -228,7 +228,7 @@ void Terminal::SetSelectionEnd(const til::point viewportPos, std::optional<Selec
 // - the new start/end for a selection
 std::pair<til::point, til::point> 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);

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)

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<UiaTextRangeBase&>(**ppRetVal);
@@ -872,15 +872,15 @@ 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;
         }
 
         // 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;
         }

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
         {

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,18 +355,22 @@ 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.
 // -  Positive if First is to the right of the Second.
 // -  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)
     {