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

UIA: GetVisibleRanges() returning text beyond the visible range #7803

Closed
codeofdusk opened this issue Oct 2, 2020 · 6 comments
Closed

UIA: GetVisibleRanges() returning text beyond the visible range #7803

codeofdusk opened this issue Oct 2, 2020 · 6 comments
Assignees
Labels
Area-Accessibility Issues related to accessibility Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Product-Conhost For issues in the Console codebase

Comments

@codeofdusk
Copy link
Contributor

Environment

Windows build number: [run `[Environment]::OSVersion` for powershell, or `ver` for cmd] OpenConsole (relaese build of #7792 )
Windows Terminal version (if applicable):

Any other software? NVDA

Steps to reproduce

  1. Call GetVisibleRanges.
  2. Make a degenerate range at the end of this range.
  3. Expand the degenerate range to line.

Expected behaviour

The last visible line is contained in the range.

Actual behaviour

An empty line is contained in the range.

@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 Oct 2, 2020
@codeofdusk
Copy link
Contributor Author

Cc @carlos-zamora.

@codeofdusk codeofdusk reopened this Oct 2, 2020
@carlos-zamora
Copy link
Member

Root Cause

UiaTextRanges (UTR) are { inclusive start, exclusive end }. To encompass the first line of the buffer, an UTR would be { (0,0), (0,1) }.

GetVisibleRanges returns the UTR encompassing the viewport.

Creating a degenerate range at the end of the visible viewport means that we have a degenerate range underneath the viewport. This is because the exclusive end is {0, viewport.Height()+1}.

Expanding by line makes you encompass the line that _start is currently on. Since _start = _end, and _end is on the line under the viewport, you encompass the line under the viewport.

The fix is simple: if we have a degenerate range, and we try to expand by line, move _start up instead of _end down.

@DHowett This can be added into #7792, if so desired. It would make a minor modification to the "expand by line" change I made, but I would want to add a test that specifically covers these repro steps. It's also small enough that we can make this a separate PR. I'm fine either way. Risk is minor since we would only do this when it's a degenerate range.

@carlos-zamora carlos-zamora 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 and removed Needs-Tag-Fix Doesn't match tag requirements labels Oct 2, 2020
@carlos-zamora carlos-zamora self-assigned this Oct 2, 2020
@codeofdusk
Copy link
Contributor Author

I think this will also fix #5481 (which is also separately fixed by nvaccess/nvda#11639).

@DHowett
Copy link
Member

DHowett commented Oct 2, 2020

Separate PR plz

@carlos-zamora
Copy link
Member

So, I attempted to fix this on dev/cazamor/a11y/expand-line-under-viewport. That definitely fixes the bug, but I don't think it's the correct behavior overall. I've summarized in the collapsible section below some concerns with the implementation.

Concerns with my branch My branch makes it such that a degenerate range at the beginning of a line expands backwards, instead of forwards. Being in the state described in this bug report (degenerate range at the end of the visible ranges) satisfies this case. When you expand by character/line, you now get the last character/line of the visible range. So the bug is fixed.

However, this would introduce another bug/discrepancy. Consider a line being encompassed in a range. Then you move "end" to "start". When you expand by line, I expect the current line to be covered. Instead, it'll cover the previous line. So the question is, when would we distinguish between wanting to expand forwards vs backwards?

It sounds like UIA may be implemented properly for this scenario, and NVDA would have to do the extra work here by moving to the previous character, then expanding by line.

In MS Word, if you have a few paragraphs of text (=lorem(5,5))...

  • Expand by paragraph
  • move start to end
  • NOTE: you are now at the beginning of the next paragraph
  • expand by line
  • NOTE: you now have a range of the first line of the next paragraph

ConHost doesn't have paragraph support, but we can kind of equate getting the visible ranges to getting a paragraph in MS Word (because it's a range that expands multiple lines). If that's the case, the current behavior is correct.

@codeofdusk
Copy link
Contributor Author

These seem to be NVDA issues. See nvaccess/nvda#11722.

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. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Product-Conhost For issues in the Console codebase
Projects
None yet
Development

No branches or pull requests

3 participants