Skip to content

Commit

Permalink
[A11y] Treat last character as 'end of buffer' (#11122)
Browse files Browse the repository at this point in the history 
## Summary of the Pull Request
Updates our `UiaTextRange` to no longer treat the end of the buffer as the "document end". Instead, we consider the "document end" to be the line beneath the cursor or last legible character (whichever is further down). In the event where the last legible character is on the last line of the buffer, we use the "end exclusive" position (left-most point on a line one past the end of the buffer). 

When movement of any kind occurs, we clamp each endpoint to the document end. Since the document end is an actual spot in the buffer (most of the time), this should improve stability because we shouldn't be pointing out-of-bounds anymore.

The biggest benefit is that this significantly improves the performance of word navigation because screen readers no longer have to take into account the whitespace following the end of the prompt.

Word navigation tests were added to the `TestTableWriter` (see #10886). 24 of the 85 tests were failing, however, they don't seem to interact with the document end, so I've marked them as skip and will fix them in a follow-up. This PR is large enough as-is, so I'm hoping I can take time in the follow-up to clean some things on the side (aka `preventBoundary` and `allowBottomExclusive` being used interchangeably).

## References
#7000 - Epic
Closes #6986 
Closes #10925

## Validation Steps Performed
- [X] Tests pass
- [X] @codeofdusk has been personally testing this build (and others)
  • Loading branch information
@carlos-zamora
carlos-zamora authored Sep 16, 2021
1 parent 4793541 commit d08afc4
Show file tree
Hide file tree
Showing 10 changed files with 2,064 additions and 710 deletions.
3 changes: 3 additions & 0 deletions .github/actions/spelling/allow/allow.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ liga
lje
locl
lorem
Llast
Lmid
Lorigin
maxed
mkmk
mru
Expand Down
162 changes: 104 additions & 58 deletions src/buffer/out/textBuffer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1057,9 +1057,10 @@ const DelimiterClass TextBuffer::_GetDelimiterClassAt(const COORD pos, const std
// - accessibilityMode - when enabled, we continue expanding left until we are at the beginning of a readable word.
// Otherwise, expand left until a character of a new delimiter class is found
// (or a row boundary is encountered)
// - limitOptional - (optional) the last possible position in the buffer that can be explored. This can be used to improve performance.
// Return Value:
// - The COORD for the first character on the "word" (inclusive)
const COORD TextBuffer::GetWordStart(const COORD target, const std::wstring_view wordDelimiters, bool accessibilityMode) const
const COORD TextBuffer::GetWordStart(const COORD target, const std::wstring_view wordDelimiters, bool accessibilityMode, std::optional<til::point> limitOptional) const
{
// Consider a buffer with this text in it:
// " word other "
Expand All @@ -1072,20 +1073,25 @@ const COORD TextBuffer::GetWordStart(const COORD target, const std::wstring_view
// NOTE: the start anchor (this one) is inclusive, whereas the end anchor (GetWordEnd) is exclusive

#pragma warning(suppress : 26496)
// GH#7664: Treat EndExclusive as EndInclusive so
// that it actually points to a space in the buffer
auto copy{ target };
const auto bufferSize{ GetSize() };
const auto limit{ limitOptional.value_or(bufferSize.EndExclusive()) };
if (target == bufferSize.Origin())
{
// can't expand left
return target;
}
else if (target == bufferSize.EndExclusive())
{
// treat EndExclusive as EndInclusive
// GH#7664: Treat EndExclusive as EndInclusive so
// that it actually points to a space in the buffer
copy = { bufferSize.RightInclusive(), bufferSize.BottomInclusive() };
}
else if (bufferSize.CompareInBounds(target, limit, true) >= 0)
{
// if at/past the limit --> clamp to limit
copy = *limitOptional;
}

if (accessibilityMode)
{
Expand Down Expand Up @@ -1179,9 +1185,10 @@ const COORD TextBuffer::_GetWordStartForSelection(const COORD target, const std:
// - accessibilityMode - when enabled, we continue expanding right until we are at the beginning of the next READABLE word
// Otherwise, expand right until a character of a new delimiter class is found
// (or a row boundary is encountered)
// - limitOptional - (optional) the last possible position in the buffer that can be explored. This can be used to improve performance.
// Return Value:
// - The COORD for the last character on the "word" (inclusive)
const COORD TextBuffer::GetWordEnd(const COORD target, const std::wstring_view wordDelimiters, bool accessibilityMode) const
const COORD TextBuffer::GetWordEnd(const COORD target, const std::wstring_view wordDelimiters, bool accessibilityMode, std::optional<til::point> limitOptional) const
{
// Consider a buffer with this text in it:
// " word other "
Expand All @@ -1193,16 +1200,17 @@ 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())
// Already at/past the limit. Can't move forward.
const auto bufferSize{ GetSize() };
const auto limit{ limitOptional.value_or(bufferSize.EndExclusive()) };
if (bufferSize.CompareInBounds(target, limit, true) >= 0)
{
return target;
}

if (accessibilityMode)
{
const auto lastCharPos{ GetLastNonSpaceCharacter() };
return _GetWordEndForAccessibility(target, wordDelimiters, lastCharPos);
return _GetWordEndForAccessibility(target, wordDelimiters, limit);
}
else
{
Expand All @@ -1215,44 +1223,46 @@ 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)
// - limit - the last "valid" position 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 COORD limit) const
{
const auto bufferSize = GetSize();
COORD result = target;
const auto bufferSize{ GetSize() };
COORD result{ target };

// Check if we're already on/past the last RegularChar
if (bufferSize.CompareInBounds(result, lastCharPos, true) >= 0)
if (bufferSize.CompareInBounds(target, limit, true) >= 0)
{
return bufferSize.EndExclusive();
}
// if we're already on/past the last RegularChar,
// clamp result to that position
result = limit;

// ignore right boundary. Continue through readable text found
while (_GetDelimiterClassAt(result, wordDelimiters) == DelimiterClass::RegularChar)
// make the result exclusive
bufferSize.IncrementInBounds(result, true);
}
else
{
if (!bufferSize.IncrementInBounds(result, true))
auto iter{ GetCellDataAt(result, bufferSize) };
while (iter && iter.Pos() != limit && _GetDelimiterClassAt(iter.Pos(), wordDelimiters) == DelimiterClass::RegularChar)
{
break;
// Iterate through readable text
++iter;
}
}

// we are already on/past the last RegularChar
if (bufferSize.CompareInBounds(result, lastCharPos, true) >= 0)
{
return bufferSize.EndExclusive();
}
while (iter && iter.Pos() != limit && _GetDelimiterClassAt(iter.Pos(), wordDelimiters) != DelimiterClass::RegularChar)
{
// expand to the beginning of the NEXT word
++iter;
}

// make sure we expand to the beginning of the NEXT word
while (_GetDelimiterClassAt(result, wordDelimiters) != DelimiterClass::RegularChar)
{
if (!bufferSize.IncrementInBounds(result, true))
result = iter.Pos();

// Special case: we tried to move one past the end of the buffer,
// but iter prevented that (because that pos doesn't exist).
// Manually increment onto the EndExclusive point.
if (!iter)
{
// we are at the EndInclusive COORD
// this signifies that we must include the last char in the buffer
// but the position of the COORD points to nothing
break;
bufferSize.IncrementInBounds(result, true);
}
}

Expand Down Expand Up @@ -1345,18 +1355,20 @@ void TextBuffer::_PruneHyperlinks()
// Arguments:
// - pos - 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)
// - limitOptional - (optional) the last possible position in the buffer that can be explored. This can be used to improve performance.
// 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, std::optional<til::point> limitOptional) 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) };
const auto bufferSize{ GetSize() };
const auto limit{ limitOptional.value_or(bufferSize.EndExclusive()) };
const auto copy{ _GetWordEndForAccessibility(pos, wordDelimiters, limit) };

if (copy == GetSize().EndExclusive())
if (bufferSize.CompareInBounds(copy, limit, true) >= 0)
{
return false;
}
Expand Down Expand Up @@ -1393,19 +1405,23 @@ bool TextBuffer::MoveToPreviousWord(COORD& pos, std::wstring_view wordDelimiters
// - Update pos to be the beginning of the current glyph/character. This is used for accessibility
// Arguments:
// - pos - a COORD on the word you are currently on
// - limitOptional - (optional) the last possible position in the buffer that can be explored. This can be used to improve performance.
// Return Value:
// - pos - The COORD for the first cell of the current glyph (inclusive)
const til::point TextBuffer::GetGlyphStart(const til::point pos) const
const til::point TextBuffer::GetGlyphStart(const til::point pos, std::optional<til::point> limitOptional) const
{
COORD resultPos = pos;
const auto bufferSize = GetSize();
const auto limit{ limitOptional.value_or(bufferSize.EndExclusive()) };

if (resultPos == bufferSize.EndExclusive())
// Clamp pos to limit
if (bufferSize.CompareInBounds(resultPos, limit, true) > 0)
{
bufferSize.DecrementInBounds(resultPos, true);
resultPos = limit;
}

if (resultPos != bufferSize.EndExclusive() && GetCellDataAt(resultPos)->DbcsAttr().IsTrailing())
// limit is exclusive, so we need to move back to be within valid bounds
if (resultPos != limit && GetCellDataAt(resultPos)->DbcsAttr().IsTrailing())
{
bufferSize.DecrementInBounds(resultPos, true);
}
Expand All @@ -1419,12 +1435,19 @@ const til::point TextBuffer::GetGlyphStart(const til::point pos) const
// - pos - a COORD on the word you are currently on
// Return Value:
// - pos - The COORD for the last cell of the current glyph (exclusive)
const til::point TextBuffer::GetGlyphEnd(const til::point pos) const
const til::point TextBuffer::GetGlyphEnd(const til::point pos, std::optional<til::point> limitOptional) const
{
COORD resultPos = pos;

const auto bufferSize = GetSize();
if (resultPos != bufferSize.EndExclusive() && GetCellDataAt(resultPos)->DbcsAttr().IsLeading())
const auto limit{ limitOptional.value_or(bufferSize.EndExclusive()) };

// Clamp pos to limit
if (bufferSize.CompareInBounds(resultPos, limit, true) > 0)
{
resultPos = limit;
}

if (resultPos != limit && GetCellDataAt(resultPos)->DbcsAttr().IsLeading())
{
bufferSize.IncrementInBounds(resultPos, true);
}
Expand All @@ -1438,29 +1461,43 @@ const til::point TextBuffer::GetGlyphEnd(const til::point pos) const
// - Update pos to be the beginning of the next glyph/character. This is used for accessibility
// Arguments:
// - pos - a COORD on the word you are currently on
// - allowBottomExclusive - allow the nonexistent end-of-buffer cell to be encountered
// - allowExclusiveEnd - allow result to be the exclusive limit (one past limit)
// - limit - boundaries for the iterator to operate within
// 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 cell of the current glyph (inclusive)
bool TextBuffer::MoveToNextGlyph(til::point& pos, bool allowBottomExclusive) const
bool TextBuffer::MoveToNextGlyph(til::point& pos, bool allowExclusiveEnd, std::optional<til::point> limitOptional) const
{
COORD resultPos = pos;
const auto bufferSize = GetSize();
const auto limit{ limitOptional.value_or(bufferSize.EndExclusive()) };

if (resultPos == GetSize().EndExclusive())
const auto distanceToLimit{ bufferSize.CompareInBounds(pos, limit, true) };
if (distanceToLimit >= 0)
{
// Corner Case: we're on/past the limit
// Clamp us to the limit
pos = limit;
return false;
}
else if (!allowExclusiveEnd && distanceToLimit == -1)
{
// we're already at the end
// Corner Case: we're just before the limit
// and we are not allowed onto the exclusive end.
// Fail to move.
return false;
}

// try to move. If we can't, we're done.
const bool success = bufferSize.IncrementInBounds(resultPos, allowBottomExclusive);
if (resultPos != bufferSize.EndExclusive() && GetCellDataAt(resultPos)->DbcsAttr().IsTrailing())
// Try to move forward, but if we hit the buffer boundary, we fail to move.
auto iter{ GetCellDataAt(pos, bufferSize) };
const bool success{ ++iter };

// Move again if we're on a wide glyph
if (success && iter->DbcsAttr().IsTrailing())
{
bufferSize.IncrementInBounds(resultPos, allowBottomExclusive);
++iter;
}

pos = resultPos;
pos = iter.Pos();
return success;
}

Expand All @@ -1471,12 +1508,21 @@ bool TextBuffer::MoveToNextGlyph(til::point& pos, bool allowBottomExclusive) con
// 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 cell of the previous glyph (inclusive)
bool TextBuffer::MoveToPreviousGlyph(til::point& pos) const
bool TextBuffer::MoveToPreviousGlyph(til::point& pos, std::optional<til::point> limitOptional) const
{
COORD resultPos = pos;
const auto bufferSize = GetSize();
const auto limit{ limitOptional.value_or(bufferSize.EndExclusive()) };

if (bufferSize.CompareInBounds(pos, limit, true) > 0)
{
// we're past the end
// clamp us to the limit
pos = limit;
return true;
}

// try to move. If we can't, we're done.
const auto bufferSize = GetSize();
const bool success = bufferSize.DecrementInBounds(resultPos, true);
if (resultPos != bufferSize.EndExclusive() && GetCellDataAt(resultPos)->DbcsAttr().IsLeading())
{
Expand Down
16 changes: 8 additions & 8 deletions src/buffer/out/textBuffer.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,15 +141,15 @@ class TextBuffer final

Microsoft::Console::Render::IRenderTarget& GetRenderTarget() noexcept;

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;
const COORD GetWordStart(const COORD target, const std::wstring_view wordDelimiters, bool accessibilityMode = false, std::optional<til::point> limitOptional = std::nullopt) const;
const COORD GetWordEnd(const COORD target, const std::wstring_view wordDelimiters, bool accessibilityMode = false, std::optional<til::point> limitOptional = std::nullopt) const;
bool MoveToNextWord(COORD& pos, const std::wstring_view wordDelimiters, std::optional<til::point> limitOptional = std::nullopt) const;
bool MoveToPreviousWord(COORD& pos, const std::wstring_view wordDelimiters) const;

const til::point GetGlyphStart(const til::point pos) const;
const til::point GetGlyphEnd(const til::point pos) const;
bool MoveToNextGlyph(til::point& pos, bool allowBottomExclusive = false) const;
bool MoveToPreviousGlyph(til::point& pos) const;
const til::point GetGlyphStart(const til::point pos, std::optional<til::point> limitOptional = std::nullopt) const;
const til::point GetGlyphEnd(const til::point pos, std::optional<til::point> limitOptional = std::nullopt) const;
bool MoveToNextGlyph(til::point& pos, bool allowBottomExclusive = false, std::optional<til::point> limitOptional = std::nullopt) const;
bool MoveToPreviousGlyph(til::point& pos, std::optional<til::point> limitOptional = std::nullopt) const;

const std::vector<SMALL_RECT> GetTextRects(COORD start, COORD end, bool blockSelection, bool bufferCoordinates) const;

Expand Down Expand Up @@ -242,7 +242,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 COORD lastCharPos) const;
const COORD _GetWordEndForAccessibility(const COORD target, const std::wstring_view wordDelimiters, const COORD limit) const;
const COORD _GetWordEndForSelection(const COORD target, const std::wstring_view wordDelimiters) const;

void _PruneHyperlinks();
Expand Down
Loading

0 comments on commit d08afc4

Please sign in to comment.