-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Improve UIA movement testing methodology #10886
Conversation
Note to reviewersThis is the first time I'm really writing a PowerShell script 🎉. So be brutal! I wanna make sure I learn PowerShell good! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay so
- this is insane
- it adds 90 test cases, so that makes me 😄
- I don't really know powershell all that well, but the code seemed easy enough to follow, and was documented well enough that I wouldn't totally hate myself if I needed to add another case
- this doesn't run automatically as part of the build, this has to be run manually. That definitely cuts down on some of the complexity, which is good. IMO generating the tests automagically as part of the build just ain't worth it.
- I'm not really going to inspect the veracity of the tests themselves. I'm gonna trust that you've done your diligence here. Manually inspecting 90 test cases probably isn't worth the time. Skimming them, they seem like they make sense.
- There's a lot of skipped tests, so helpfully we can fix those soon. Presumably, those are the ones that are failing currently. How did we come up with the test cases that are failing currently but shouldn't be? Just logically working out what they should be doing? Was there some sort of reference app that you used to construct these cases? (this is merely a curiosity)
ffdb72d
to
b05dfa0
Compare
This comment has been minimized.
This comment has been minimized.
255 now that I went through all of the positions 😉. AND I still need to add some for word navigation.
Yeah, and we're not really changing these tests all the time. I did just add a "nice to have" where it all goes to a .g.cpp file though. So this process is now so much easier!
Updated the README to talk a bit more about this, actually. There's kinda two types of tests that we're skipping for now:
|
c5250b6
to
5e3d85c
Compare
This comment has been minimized.
This comment has been minimized.
size_t _index; | ||
}; | ||
|
||
struct ArrayIndexTaefAdapterSource : public Microsoft::WRL::RuntimeClass<Microsoft::WRL::RuntimeClassFlags<Microsoft::WRL::ClassicCom | Microsoft::WRL::InhibitFtmBase>, IDataSource> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be reasonable to use <winrt/base.h>
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe? I stole this from https://github.com/microsoft/terminal/blob/main/src/buffer/out/ut_textbuffer/ReflowTests.cpp.
I'll leave this as-is for now since we'll want to consolidate these at some point.
Hello @carlos-zamora! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
## 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
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`.
## 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
## 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)
…(Windows 11 Sun Valley 2) (#10964) Supersedes #9771 and #10716. Closes #1682. Closes #8653. Closes #9867. Closes #11172. Closes #11554. Summary of the issue: Microsoft has significantly improved performance and reliability of UIA console: * microsoft/terminal#4018 is an almost complete rewrite of the UIA code which makes the console's UIA implementation more closely align with the API specification. * microsoft/terminal#10886, microsoft/terminal#10925, and microsoft/terminal#11253 form a robust testing methodology for the UIA implementation, including bug fixes in response to newly added tests based on Word's UIA implementation. * microsoft/terminal#11122 removes the thousands of empty lines at the end of the console buffer, significantly improving performance and stability. Since all console text ranges are now within the text buffer's bounds, it is no longer possible for console to crash due to the manipulation by UIA clients of an out-of-bounds text range. * Countless other accessibility-related PRs too numerous to list here. We should enable UIA support on new Windows Console builds by default for performance improvement and controllable password suppression. Description of how this pull request fixes the issue: This PR: * Exposes all three options for the UIA console feature flag in the UI (replaces the UIA check box with a combo box). * Adds a runtime check to test if `apiLevel >= FORMATTED`, and use UIA in this case when the user preference is auto. This is the case on Windows 11 Sun Valley 2 (SV2) available now in beta and set for release in the second half of 2022.
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 formatGeneratedTests.ps1
: a PowerShell script that importsUiaTests.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:
MoveEndpoint
in addition toMove
.