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.

Sign up for GitHub

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

Jump to bottom

Fix failing UIA tests from #10886 #10924

Closed
carlos-zamora opened this issue Aug 11, 2021 · 3 comments · Fixed by #10991
Closed

Fix failing UIA tests from #10886 #10924

carlos-zamora opened this issue Aug 11, 2021 · 3 comments · Fixed by #10991
Assignees
@carlos-zamora
Labels
Area-Accessibility Issues related to accessibility Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal v1.11

Comments

@carlos-zamora
Copy link
Member

#10886 follow-up

There's some failing tests in the CSV that we're skipping right now. They shouldn't be failing, but the tests were verified to be correct when comparing our behavior to MS Word's.

We should update our UIA implementation to make these tests not fail.

IIRC I believe the failing tests are mainly:

  • "move by line" backwards on degenerate ranges are off by one
  • "move by document" do not report that they moved by the correct amount
  • a non-degenerate range that moves 0 should still expand by whatever unit you're moving by (even if you tried to move by 5/1/0 but ended up moving 0)
@carlos-zamora carlos-zamora added Area-Accessibility Issues related to accessibility Product-Terminal The new Windows Terminal. Issue-Task It's a feature request, but it doesn't really need a major design. labels Aug 11, 2021
@carlos-zamora carlos-zamora added this to the Terminal v1.11 milestone Aug 11, 2021
@carlos-zamora carlos-zamora self-assigned this Aug 11, 2021
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Aug 11, 2021
@carlos-zamora
Copy link
Member Author

Added to the v1.11 milestone, but feel free to move it elsewhere.

Also, this is in the shared UIA implementation, so the bug affects Windows Terminal and Conhost

@carlos-zamora carlos-zamora removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Aug 11, 2021
@carlos-zamora carlos-zamora mentioned this issue Aug 11, 2021
Merged
ghost pushed a commit that referenced this issue Aug 19, 2021
@carlos-zamora
Improve UIA movement testing methodology (#10886)
1678b58 
Introduces a new methodology to maintain tests for UI Automation. This includes...
- `UiaTests.csv`: an excel spreadsheet designed to store UIA movement tests in a compact format
- `GeneratedTests.ps1`: a PowerShell script that imports `UiaTests.csv` and outputs a C++ TEST_METHOD for `UiaTextRangeTests.

This new system can be used to easily add more UIA movement tests.

Read https://github.com/microsoft/terminal/blob/dev/cazamor/a11y-7000/testing/tools/TestTableWriter/README.md for more details.

Follow-up work items:
- #10924 **Failing Tests**: this found some failing tests. We should make them not fail.
- #10925 **Missing Tests: Word navigation**: Word navigation is missing.
- #10926 **MoveEndpoint Tests**: an additional column can be added to the CSV "EndpointTarget", which can be "start", "end", or "both". This will allow us to test `MoveEndpoint` in addition to `Move`.
@carlos-zamora carlos-zamora mentioned this issue Aug 19, 2021
Merged
2 tasks
@ghost ghost added the In-PR This issue has a related PR label Aug 19, 2021
@ghost ghost closed this as completed in #10991 Aug 24, 2021
@ghost ghost removed the In-PR This issue has a related PR label Aug 24, 2021
ghost pushed a commit that referenced this issue Aug 24, 2021
@carlos-zamora
Fix failing UIA movement tests (#10991)
c53fe1c 
## Summary of the Pull Request
Follow-up for #10886. The new UIA movement tests found some failing cases. This PR fixes UiaTextRangeBase to have movement match that of MS Word. In total, this fixes 64 tests.

## PR Checklist
* [X] Closes #10924
* [X] Tests added/passed

## Detailed Description of the Pull Request / Additional comments
Root causes include...
1. if we were a non-degenerate range and we failed to move, we should still expand to enclose the unit
2. non-degenerate ranges are treated as if they already encompassed their given unit.
   - this one is a bit difficult to explain. Consider these examples:
      1. document movement
         - state: you have a 1-cell wide range on the buffer, and you try to move by document
         - result: move by 0 (there is no next/prev document), but the range now encompasses the entire document
      2. line movement
         - state: you have a 1-cell wide range on a line, and you try to move back by a line
         - result: you go to the previous line (not the beginning of this line)
   - conversely, a degenerate range successfully moves to the beginning/end of the current unit (i.e. document/line)
   - this (bizarre) behavior was confirmed using MS Word

As a bonus, occasionally, Narrator would get stuck when navigating by line. This issue now seems to be fixed.

## Updates to existing tests
- `CanMoveByCharacter`
   - `can't move backward from (0, 0)` --> misauthored, result should be one character wide.
   - `can't move past the last column in the last row` --> misauthored and already covered in generated tests
- `CanMoveByLine`
   - `can't move backward from top row` --> misauthored, end should be on next line. Already covered by generated tests
   - `can't move forward from bottom row` --> misauthored, end should be on next line
   - `can't move backward when part of the top row is in the range` --> misauthored, should expand
   - `can't move forward when part of the bottom row is in the range` --> misauthored, degenerate range moves to end of buffer
- `MovementAtExclusiveEnd`
   - populate the text buffer _before_ we do a move by word operation
   - update to match the now fixed behavior
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Aug 24, 2021
DHowett pushed a commit that referenced this issue Aug 25, 2021
@carlos-zamora @DHowett
Improve UIA movement testing methodology (#10886)
5abb574 
Introduces a new methodology to maintain tests for UI Automation. This includes...
- `UiaTests.csv`: an excel spreadsheet designed to store UIA movement tests in a compact format
- `GeneratedTests.ps1`: a PowerShell script that imports `UiaTests.csv` and outputs a C++ TEST_METHOD for `UiaTextRangeTests.

This new system can be used to easily add more UIA movement tests.

Read https://github.com/microsoft/terminal/blob/dev/cazamor/a11y-7000/testing/tools/TestTableWriter/README.md for more details.

Follow-up work items:
- #10924 **Failing Tests**: this found some failing tests. We should make them not fail.
- #10925 **Missing Tests: Word navigation**: Word navigation is missing.
- #10926 **MoveEndpoint Tests**: an additional column can be added to the CSV "EndpointTarget", which can be "start", "end", or "both". This will allow us to test `MoveEndpoint` in addition to `Move`.
DHowett pushed a commit that referenced this issue Aug 25, 2021
@carlos-zamora @DHowett
Fix failing UIA movement tests (#10991)
9ac7ce2 
## Summary of the Pull Request
Follow-up for #10886. The new UIA movement tests found some failing cases. This PR fixes UiaTextRangeBase to have movement match that of MS Word. In total, this fixes 64 tests.

## PR Checklist
* [X] Closes #10924
* [X] Tests added/passed

## Detailed Description of the Pull Request / Additional comments
Root causes include...
1. if we were a non-degenerate range and we failed to move, we should still expand to enclose the unit
2. non-degenerate ranges are treated as if they already encompassed their given unit.
   - this one is a bit difficult to explain. Consider these examples:
      1. document movement
         - state: you have a 1-cell wide range on the buffer, and you try to move by document
         - result: move by 0 (there is no next/prev document), but the range now encompasses the entire document
      2. line movement
         - state: you have a 1-cell wide range on a line, and you try to move back by a line
         - result: you go to the previous line (not the beginning of this line)
   - conversely, a degenerate range successfully moves to the beginning/end of the current unit (i.e. document/line)
   - this (bizarre) behavior was confirmed using MS Word

As a bonus, occasionally, Narrator would get stuck when navigating by line. This issue now seems to be fixed.

## Updates to existing tests
- `CanMoveByCharacter`
   - `can't move backward from (0, 0)` --> misauthored, result should be one character wide.
   - `can't move past the last column in the last row` --> misauthored and already covered in generated tests
- `CanMoveByLine`
   - `can't move backward from top row` --> misauthored, end should be on next line. Already covered by generated tests
   - `can't move forward from bottom row` --> misauthored, end should be on next line
   - `can't move backward when part of the top row is in the range` --> misauthored, should expand
   - `can't move forward when part of the bottom row is in the range` --> misauthored, degenerate range moves to end of buffer
- `MovementAtExclusiveEnd`
   - populate the text buffer _before_ we do a move by word operation
   - update to match the now fixed behavior
@ghost
Copy link

ghost commented Aug 31, 2021

🎉This issue was addressed in #10991, which has now been successfully released as Windows Terminal Preview v1.10.2383.0.:tada:

Handy links:

@ghost
Copy link

ghost commented Aug 31, 2021

🎉This issue was addressed in #10991, which has now been successfully released as Windows Terminal Preview v1.11.2421.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@carlos-zamora carlos-zamora

Labels
Area-Accessibility Issues related to accessibility Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Terminal v1.11
Development

Successfully merging a pull request may close this issue.

Fix failing UIA movement tests microsoft/terminal
1 participant
@carlos-zamora