Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly handle and test a11y movement at end of buffer #7792

Merged
merged 4 commits into from
Oct 5, 2020

Conversation

carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented Oct 1, 2020

Summary of the Pull Request

The MovementAtExclusiveEnd test was improperly authored for the
following reasons:

  • it should have used TEST_METHOD_PROPERTY to cover all of the
    TextUnits
  • TextUnit::Document (arguably one of the most important) was ommitted
    accidentally (!= TextUnit_Document was used instead of <=)
  • The created range was not EndExclusive, but rather, the last cell in
    the buffer (EndInclusive)

The first half of this PR fixes the test.

The second half of this PR expands the test and fixes any related issues
to make the test pass (i.e. #7771):

  • TEST_METHOD_PROPERTY was added for it to be degenerate (start/end at
    EndExclusive) or not (last cell of buffer)
  • utr->_start is now also validated after moving backwards

NOTE: utr->_start was not validated when moving forwards because
moving forwards should always fail when at/past the last chell in the
buffer.

Closes #7771

@ghost ghost added Area-Accessibility Issues related to accessibility Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-0 Bugs that we consider release-blocking/recall-class (P0) Product-Conhost For issues in the Console codebase Severity-Crash Crashes are real bad news. labels Oct 1, 2020
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for doing this. I like the look of this, and just had some process questions!

@@ -1359,9 +1359,15 @@ const til::point TextBuffer::GetGlyphEnd(const til::point pos) const
bool TextBuffer::MoveToNextGlyph(til::point& pos, bool allowBottomExclusive) const
{
COORD resultPos = pos;
const auto bufferSize = GetSize();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the fix for the crash?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. In general, I thought it was only possible for _end to be at EndExclusive(), so allowBottomExclusive is used to track if we are working with _end. That definitely wasn't the case because you can end up with _start at EndExclusive() by doing the repro steps.

So this check for EndExclusive() is basically thrown around everywhere to ensure we don't crash on an IncrementInBounds or DecrementInBounds check (which checks if the COORD is in the buffer, and EndExclusive isn't)

@@ -953,7 +953,7 @@ void UiaTextRangeBase::_moveEndpointByUnitCharacter(_In_ const int moveCount,
}
break;
case MovementDirection::Backward:
success = buffer.MoveToPreviousGlyph(target, allowBottomExclusive);
success = buffer.MoveToPreviousGlyph(target);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is because it is safe to move backwards off the exclusive end, and you're checking before loading the glyph data that it is a valid position?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. It's safe to move backwards off the exclusive end. So let's not bother passing in allowBottomExclusive and always allow it.

else // textUnit <= TextUnit::TextUnit_Document:
{
VERIFY_ARE_EQUAL(origin, utr->_start);
VERIFY_ARE_EQUAL(degenerate ? origin : endExclusive, utr->_end);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i know it might be a chore, but having @codeofdusk review this test case for sanity would be helpful

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(a chore for him, not a chore for you. since this is a bunch of dense WEX C++)

@zadjii-msft
Copy link
Member

In general, this seems fine to me, but I can't really say I understand the problem space well enough to give an official ✔️. Maybe if you came through and answered some of Dustin's comments, then I could make better sense of what's going on? Or if @codeofdusk thinks the tests are good, then I'd probably defer to his expertise in the area

@carlos-zamora
Copy link
Member Author

@codeofdusk found that there was a crash when expanding at the end. So I added testing for that and fixed any failing tests.

@DHowett
Copy link
Member

DHowett commented Oct 1, 2020

is there any way at all whatsoever that we can automatically do the things a human might do

@codeofdusk
Copy link
Contributor

is there any way at all whatsoever that we can automatically do the things a human might do

Maybe, by adapting NVDA's system test framework? Cc @feerrenrut, @michaelDCurran.

Copy link
Contributor

@codeofdusk codeofdusk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I don't feel comfortable commenting on the robustness of the unit tests, this PR definitely fixes my reported UIA bugs!

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good enough for me!

Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine to me. I appreciate all the extra tests thrown in to be more comprehensive than before.

@DHowett
Copy link
Member

DHowett commented Oct 14, 2020

This fix was just released as part of Windows in Insider Build 20236!

DHowett pushed a commit that referenced this pull request Oct 19, 2020
The `MovementAtExclusiveEnd` test was improperly authored for the
following reasons:
- it should have used `TEST_METHOD_PROPERTY` to cover all of the
  TextUnits
- TextUnit::Document (arguably one of the most important) was ommitted
  accidentally (`!= TextUnit_Document` was used instead of `<=`)
- The created range was not `EndExclusive`, but rather, the last cell in
  the buffer (`EndInclusive`)

The first half of this PR fixes the test.

The second half of this PR expands the test and fixes any related issues
to make the test pass (i.e. #7771):
- `TEST_METHOD_PROPERTY` was added for it to be degenerate (start/end at
  `EndExclusive`) or not (last cell of buffer)
- `utr->_start` is now also validated after moving backwards

NOTE: `utr->_start` was not validated when moving forwards because
moving forwards should always fail when at/past the last chell in the
buffer.

Closes #7771

(cherry picked from commit e401edf)
@ghost
Copy link

ghost commented Nov 11, 2020

🎉Windows Terminal v1.4.3141.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Nov 11, 2020
@ghost
Copy link

ghost commented Nov 11, 2020

🎉Windows Terminal Preview v1.5.3142.0 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Accessibility Issues related to accessibility Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-0 Bugs that we consider release-blocking/recall-class (P0) Product-Conhost For issues in the Console codebase Severity-Crash Crashes are real bad news.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UIA: crash at very end of buffer
5 participants