Skip to content

Commit

Permalink
Fix a11y crash in alt buffer apps (#13250)
Browse files Browse the repository at this point in the history
This fixes the crashes caused by using a screen reader when in an app that uses the alt buffer via two changes:
1. Fix `Terminal::ViewEndIndex()`
   - `UiaTextRangeBase` receives a coordinate that is outside of the bounds of the text buffer via the following chain of functions... `_getDocumentEnd()` --> `GetLastNonSpaceCharacter()` --> `_getOptimizedBufferSize()` --> `GetTextBufferEndPoisition()` --> `ViewEndIndex()`
   - Since support for the alt buffer was added recently, `ViewEndIndex()` was recently changed, so that explains why this issue came up recently. We were accidentally setting the view end index to `height` instead of `height-1`. Thanks @j4james for finding this!
   - The UIA code would get the "exclusive end" of the alt buffer. Since it was using `ViewEndIndex()` to calculate that, it was one more than it should be. The UIA code has explicit allowance for "one past the end of the viewport" in its `IsInBounds()` check. Since the `ViewEndIndex()` is way beyond that, it's not allowed, hitting the fail fast.
2. Replace `FAIL_FAST_IF` with `assert`
   - These fail fast calls have caused so many issues with our UIA code. Those checks still provide value, but they shouldn't take the whole app down. This change replaces the `Viewport` and `UiaTextRangeBase` fail fasts with asserts to still perform those checks, but not take down the entire app in release builds.

Closes #13183

While using Narrator...
- opened nano in bash
- generated text and scrolled in nano
- generated text and scrolled in PowerShell

(cherry picked from commit 94e1697)
Service-Card-Id: 82972865
Service-Version: 1.14
  • Loading branch information
carlos-zamora authored and DHowett committed Jun 30, 2022
1 parent ddf46f2 commit efa02f3
Show file tree
Hide file tree
Showing 3 changed files with 9 additions and 8 deletions.
2 changes: 1 addition & 1 deletion src/cascadia/TerminalCore/Terminal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -990,7 +990,7 @@ int Terminal::ViewStartIndex() const noexcept

int Terminal::ViewEndIndex() const noexcept
{
return _inAltBuffer() ? _altBufferSize.height : _mutableViewport.BottomInclusive();
return _inAltBuffer() ? _altBufferSize.height - 1 : _mutableViewport.BottomInclusive();
}

// _VisibleStartIndex is the first visible line of the buffer
Expand Down
8 changes: 4 additions & 4 deletions src/types/UiaTextRangeBase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1289,9 +1289,9 @@ try
}
}

FAIL_FAST_IF(!(newViewport.Top >= topRow));
FAIL_FAST_IF(!(newViewport.Bottom <= bottomRow));
FAIL_FAST_IF(!(_getViewportHeight(oldViewport) == _getViewportHeight(newViewport)));
assert(newViewport.Top >= topRow);
assert(newViewport.Bottom <= bottomRow);
assert(_getViewportHeight(oldViewport) == _getViewportHeight(newViewport));

Unlock.reset();

Expand Down Expand Up @@ -1338,7 +1338,7 @@ const COORD UiaTextRangeBase::_getScreenFontSize() const noexcept
// - The viewport height
const unsigned int UiaTextRangeBase::_getViewportHeight(const SMALL_RECT viewport) const noexcept
{
FAIL_FAST_IF(!(viewport.Bottom >= viewport.Top));
assert(viewport.Bottom >= viewport.Top);
// + 1 because COORD is inclusive on both sides so subtracting top
// and bottom gets rid of 1 more then it should.
return viewport.Bottom - viewport.Top + 1;
Expand Down
7 changes: 4 additions & 3 deletions src/types/viewport.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -362,11 +362,12 @@ bool Viewport::DecrementInBoundsCircular(COORD& 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)
#pragma warning(suppress : 4100)
int Viewport::CompareInBounds(const COORD& first, const COORD& second, bool allowEndExclusive) const noexcept
{
// Assert that our coordinates are within the expected boundaries
FAIL_FAST_IF(!IsInBounds(first, allowEndExclusive));
FAIL_FAST_IF(!IsInBounds(second, allowEndExclusive));
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 @@ -429,7 +430,7 @@ bool Viewport::WalkInBounds(COORD& pos, const WalkDir dir, bool allowEndExclusiv
bool Viewport::WalkInBoundsCircular(COORD& pos, const WalkDir dir, bool allowEndExclusive) const noexcept
{
// Assert that the position given fits inside this viewport.
FAIL_FAST_IF(!IsInBounds(pos, allowEndExclusive));
assert(IsInBounds(pos, allowEndExclusive));

if (dir.x == XWalk::LeftToRight)
{
Expand Down

0 comments on commit efa02f3

Please sign in to comment.