Skip to content

Commit

Permalink
Revert "Remove most uses of CompareInBounds (#13244)" (#13907)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@carlos-zamora
carlos-zamora committed Sep 2, 2022
1 parent fe0d570 commit bfd5248
Show file tree
Hide file tree
Showing 6 changed files with 60 additions and 55 deletions.
30 changes: 10 additions & 20 deletions src/buffer/out/textBuffer.cpp
View file
Expand Up @@ -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());
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);

Expand Down
6 changes: 3 additions & 3 deletions src/cascadia/TerminalCore/TerminalSelection.cpp
View file
Expand Up @@ -251,7 +251,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
Expand Down Expand Up @@ -698,7 +698,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;
Expand All @@ -721,7 +721,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);
Expand Down
4 changes: 2 additions & 2 deletions src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp
View file
Expand Up @@ -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)
Expand Down
49 changes: 26 additions & 23 deletions src/types/UiaTextRangeBase.cpp
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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));

Expand All @@ -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);
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down
4 changes: 2 additions & 2 deletions src/types/inc/viewport.hpp
View file
Expand Up @@ -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;
Expand All @@ -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
{
Expand Down
22 changes: 17 additions & 5 deletions src/types/viewport.cpp
View file
Expand Up @@ -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();
}
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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)
{
Expand Down

0 comments on commit bfd5248

Please sign in to comment.