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

Narrator line navigation gets stuck on some lines #2160

Closed
DHowett-MSFT opened this issue Jul 30, 2019 · 5 comments · Fixed by #4018
Closed

Narrator line navigation gets stuck on some lines #2160

DHowett-MSFT opened this issue Jul 30, 2019 · 5 comments · Fixed by #4018
Assignees
@carlos-zamora
Labels
Area-Accessibility Issues related to accessibility Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal v0.9

Comments

@DHowett-MSFT
Copy link
Contributor

DHowett-MSFT commented Jul 30, 2019 •
edited
Loading

Open PowerShell, type dir, press <Enter>
Caps + Page up a few times to switch to Line view
Caps + Left arrow to move up the history buffer
Caps + right arrow to move back down
When Narrator tries to read pass the heading area (see below, navigation got stuck. (Somehow Narrator rectangle was cover two lines all at once)
Mode LastWriteTime Length Name

I was able to get past that using word or character navigation.

migrated from 23051804
@DHowett-MSFT DHowett-MSFT added Area-Accessibility Issues related to accessibility Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conhost For issues in the Console codebase labels Jul 30, 2019
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jul 30, 2019
@DHowett-MSFT DHowett-MSFT added this to the 20H1 milestone Jul 30, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jul 30, 2019
@DHowett-MSFT
Copy link
Contributor Author

This is a bug in the Text pattern implementation of the application:

  1. Create a few distinct lines in the command prompt/PS.
  2. Get the document range and move it by line a few times (to such a line that the previous and current are very distinct).
  3. Collapse the line at the Start endpoint.
  4. Expand the endpoint to Word.

Expected: First word (however that unit is defined) in the navigated-to line (navigated in 2).
Actual: The line preceding the line navigated in (2).

@miniksa miniksa removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Aug 1, 2019
@miniksa
Copy link
Member

miniksa commented Aug 1, 2019

Carlos, I think we agreed that it could be acceptable to use the user-defined word delimiters list to segment any individual expansion from character to word to satisfy this ask. Can you start a mail thread and confirm that's acceptable then implement it eventually?

@carlos-zamora
Copy link
Member

Pushing back to 21H1 Milestone. We're not entirely ready to merge Accessibility changes to 20H1 and there's a workaraound for this issue. Better to push it back and fix altogether than potentially break more things.

@carlos-zamora carlos-zamora modified the milestones: 20H1, 21H1 Sep 17, 2019
@carlos-zamora carlos-zamora mentioned this issue Oct 11, 2019
Closed
@cinnamon-msft cinnamon-msft modified the milestones: 21H1, Terminal-1912 Nov 18, 2019
@carlos-zamora carlos-zamora changed the title Narrator line navigation gets stuck on some lines in powershell Narrator line navigation gets stuck on some lines Nov 21, 2019
@carlos-zamora
Copy link
Member

carlos-zamora commented Nov 21, 2019 •
edited
Loading

For the record, there's two bugs here and I'm pretty sure they are related.

  1. Line navigation downwards
    1. Narrator rereads the same line
  2. Line navigation upwards
    1. Narrator skips the line above

There's a few surprising issues here...

  • When using Inspect.exe or Accessibility Insights to explore the text buffer, this bug does not occur. These tools accurately move to the preceding or next line without any issues. The same goes for moving just the endpoints, as opposed to the entire Text Range.
  • I've tested this with Narrator and set two print breakpoints to know when a Move function call is performed and when an ExpandToEnclosingUnit (Enclose) one is performed (as well as the parameter used). And the results were interesting and confusing...

Result when on the WRL Upgrade branch

ENCLOSE: TextUnit_Document (6)
ENCLOSE: TextUnit_Document (6)
MOVE: TextUnit_Line (3)
ENCLOSE: TextUnit_Line (3)
ENCLOSE: TextUnit_Word (2)
MOVE: TextUnit_Word (2)
MOVE: TextUnit_Word (2)
MOVE: TextUnit_Word (2)
MOVE: TextUnit_Word (2)
MOVE: TextUnit_Word (2)
MOVE: TextUnit_Word (2)
ENCLOSE: TextUnit_Format (1)
ENCLOSE: TextUnit_Document (6)
ENCLOSE: TextUnit_Document (6)
ENCLOSE: TextUnit_Document (6)
ENCLOSE: TextUnit_Document (6)
ENCLOSE: TextUnit_Document (6)

Result when on the Word Navigation branch

ENCLOSE: TextUnit_Document (6)
ENCLOSE: TextUnit_Document (6)
MOVE: TextUnit_Line (3)
ENCLOSE: TextUnit_Line (3)
ENCLOSE: TextUnit_Word (2)
MOVE: TextUnit_Word (2)
MOVE: TextUnit_Word (2)
MOVE: TextUnit_Word (2)
MOVE: TextUnit_Word (2)
MOVE: TextUnit_Word (2)
MOVE: TextUnit_Word (2)
ENCLOSE: TextUnit_Format (1)
ENCLOSE: TextUnit_Format (1)         <---
ENCLOSE: TextUnit_Format (1)         <---
ENCLOSE: TextUnit_Document (6)
ENCLOSE: TextUnit_Document (6)
ENCLOSE: TextUnit_Document (6)
ENCLOSE: TextUnit_Document (6)
ENCLOSE: TextUnit_Document (6)
ENCLOSE: TextUnit_Format (1)         <---
ENCLOSE: TextUnit_Format (1)         <---

Here, the lines with a <--- are the new lines compared to the one before.

I noticed that Narrator updates the bounding rects to match the word it is reading at the time, AS IT READS IT.

But I don't understand why Format is being called. I'm guessing it just wants to do a better job with groupings.

Either way, I'm betting that this is somewhat related to the issue. Investigating further.

@carlos-zamora carlos-zamora mentioned this issue Jan 16, 2020
Merged
3 tasks
@ghost ghost added the In-PR This issue has a related PR label Jan 16, 2020
@ghost ghost closed this as completed in #4018 Jan 31, 2020
ghost pushed a commit that referenced this issue Jan 31, 2020
@carlos-zamora
Refactor UiaTextRange For Improved Navigation and Reliability (#4018)
29df540 
## Summary of the Pull Request
This pull request is intended to achieve the following goals...
1) reduce duplicate code
2) remove static functions
3) improve readability
4) improve reliability
5) improve code-coverage for testing
6) establish functioning text buffer navigation in Narrator and NVDA

This also required a change to the wrapper class `XamlUiaTextRange` that has been causing issues with Narrator and NVDA.

See below for additional context.

## References
#3976 - I believe this might have been a result of improperly handling degenerate ranges. Fixed here.
#3895 - reduced the duplicate code. No need to separate into different files
#2160 - same as #3976 above
#1993 - I think just about everything is no longer static

## PR Checklist
* [x] Closes #3895, Closes #1993, Closes #3976, Closes #2160 
* [x] CLA signed
* [x] Tests added/passed

## Detailed Description of the Pull Request / Additional comments

### UiaTextRange
- converted endpoints into the COORD system in the TextBuffer coordinate space
- `start` is inclusive, `end` is exclusive. A degenerate range is when start == end.
- all functions are no longer static
- `MoveByUnit()` functions now rely on `MoveEndpointByUnit()` functions
- removed unnecessary typedefs like `Endpoint`, `ScreenInfoRow`, etc..
- relied more heavily on existing functionality from `TextBuffer` and `Viewport`

### XamlUiaTextRange
- `GetAttributeValue()` must return a special HRESULT that signifies that the requested attribute is not supported. This was the cause of a number of inconsistencies between Narrator and NVDA.
- `FindText()` should return `nullptr` if nothing was found. #4373 properly fixes this functionality now that Search is a shared module

### TextBuffer
- Word navigation functionality is entirely in `TextBuffer` for proper abstraction
- a total of 6 functions are now dedicated to word navigation to get a good understanding of the differences between a "word" in Accessibility and a "word" in selection

As an example, consider a buffer with this text in it:
"  word   other  "
In selection, a "word" is defined as the range between two delimiters, so the words in the example include ["  ", "word", "   ", "other", "  "].
In accessibility , a "word" includes the delimiters after a range of readable characters, so the words in the example include ["word   ", "other  "].

Additionally, accessibility word navigation must be able to detect if it is on the first or last word. This resulted in a slight variant of word navigation functions that return a boolean instead of a COORD.

Ideally, these functions can be consolidated, but that is too risky for a PR of this size as it can have an effect on selection.

### Viewport
- the concept of `EndExclusive` is added. This is used by UiaTextRange's `end` anchor as it is exclusive. To signify that the last character in the buffer is included in this buffer, `end` must be one past the end of the buffer. This is `EndExclusive`
- Since many functions check if the given `COORD` is in bounds, a flag must be set to allow `EndExclusive` as a valid `COORD` that is in bounds.

### Testing
- word navigation testing relies more heavily on TextBuffer tests
- additional testing was created for non-movement focused functions of UiaTextRange
- The results have been compared to Microsoft Word and some have been verified by UiAutomation/Narrator contacts as expected results.

## Validation Steps Performed
Tests pass
Narrator works
NVDA works
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Jan 31, 2020
@carlos-zamora carlos-zamora mentioned this issue Feb 11, 2020
Closed
22 tasks
@ghost
Copy link

ghost commented Feb 13, 2020

🎉This issue was addressed in #4018, which has now been successfully released as Windows Terminal Preview v0.9.433.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-Bug It either shouldn't be doing this or needs an investigation. Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Terminal v0.9
Development

Successfully merging a pull request may close this issue.

Refactor UiaTextRange For Improved Navigation and Reliability microsoft/terminal
4 participants
@carlos-zamora @DHowett-MSFT @miniksa @cinnamon-msft