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

UI Automation in Windows Console: improve reliability of visible range checks #9957

Merged
merged 14 commits into from
Aug 1, 2019

Conversation

codeofdusk
Copy link
Contributor

@codeofdusk codeofdusk commented Jul 22, 2019

Link to issue number:

Builds on #9614. Supersedes #9735 and #9899. Closes #9891.

Summary of the issue:

Currently:

  • When the console window is maximized (or the review cursor is otherwise placed outside the visible text), text review is not functional.
  • The review top line and review bottom line scripts do not function as intended.

Description of how this pull request fixes the issue:

This PR backports some functionality from #9735. In particular:

  • The isOffscreen property has been implemented as UIAUtils.isTextRangeOffscreen.
  • When checking if the text range is out of bounds, we now also check that oldRange is in bounds before stopping movement.
  • Re-implemented POSITION_FIRST and POSITION_LAST in terms of visible ranges.

Testing performed:

Repeated testing from #9899 and verified that the review top/bottom scripts are functional.

Known issues with pull request:

  • If review is out of bounds, the review top/bottom line scripts may not function as intended (i.e. they bring the cursor to the first/last visible range, not the first/last line of the buffer). This is likely needed to insure stability in the console.

Change log entry:

== Changes ==

@codeofdusk
Copy link
Contributor Author

UIAHandler.TextPatternRangeEndpoint_Start, lastVisiRange,
UIAHandler.TextPatternRangeEndpoint_End) >= 0
else:
# Visible ranges not available, so fail gracefully.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not raise an error here and catch that when calling?

source/NVDAObjects/UIA/winConsoleUIA.py Show resolved Hide resolved
@@ -172,6 +172,23 @@ def getChildrenWithCacheFromUIATextRange(textRange,cacheRequest):
c=CacheableUIAElementArray(c)
return c

def isTextRangeOffscreen(range, visiRanges):
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change the range argument so it does not shadow the builtin function

Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

Overall the code looks ok, though from the PR description it's not really clear what use-cases this enables. After some playing with it I think I know what is going on, but it would be nice for others if there were steps (specifying key presses) to demonstrate.

While here I also think it is worth mentioning that you can also use incrementally select text with shift+up/down/left/right arrows, I was able to read many screens by:

  1. shift+home, to select back to the first column
  2. shift+up arrow, to select previous lines. Of course the lines are in reverse order.
  3. shift+down arrow to then unselect the text and hear it in the right order.

As we discussed selecting and jumping by word (control+shift+left/right arrow) is broken when NVDA is running.

Can you update the user guide to mention control+up/down arrow and control+home/end to scroll the console window, since it's not obvious how you would know about it.

Also as mentioned, please change the param name from range to textRange

@codeofdusk
Copy link
Contributor Author

codeofdusk commented Jul 27, 2019

Can you update the user guide to mention control+up/down arrow and control+home/end to scroll the console window, since it's not obvious how you would know about it.

Where in the user guide should this be mentioned? Also, since this is a Windows Console feature (not an NVDA one) should this still be in the NVDA user guide? It seems that we tend not to put application commands in the NVDA docs (i.e. if the command still works without NVDA on, we usually don't mention it).

@feerrenrut
Copy link
Contributor

since this is a Windows Console feature (not an NVDA one) should this still be in the NVDA user guide?

It's true that we tend not to do this. However, it's quite a useful thing to know how to do when using NVDA, and not very discoverable. If there was not a built in feature, we would be trying to implement something similar I expect.

Overall I think a short guide to using the Windows Console is quite appropriate.

Where in the user guide should this be mentioned?

You can add a subsection to Application specific features:

  • Make sure that text that mentions "console" with context of windows console, links to this new subsection.
  • From the new subsection link back to 5.5. Reviewing Text

@codeofdusk
Copy link
Contributor Author

since this is a Windows Console feature (not an NVDA one) should this still be in the NVDA user guide?

It's true that we tend not to do this. However, it's quite a useful thing to know how to do when using NVDA, and not very discoverable. If there was not a built in feature, we would be trying to implement something similar I expect.

Overall I think a short guide to using the Windows Console is quite appropriate.

Where in the user guide should this be mentioned?

You can add a subsection to Application specific features:

  • Make sure that text that mentions "console" with context of windows console, links to this new subsection.
  • From the new subsection link back to 5.5. Reviewing Text

Just updated. It's not exactly clear what control+home actually does though, so I didn't mention it yet.

@feerrenrut
Copy link
Contributor

It's not exactly clear what control+home actually does though, so I didn't mention it yet.

control+home scrolls the cmd window back to the first line in the (history/buffer).

@feerrenrut feerrenrut merged commit 8a8c1c2 into nvaccess:master Aug 1, 2019
@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Aug 1, 2019
feerrenrut added a commit that referenced this pull request Aug 1, 2019
@XLTechie
Copy link
Collaborator

XLTechie commented Aug 1, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI Automation in Windows Console: text review behaves inconsistently when the window is maximized
5 participants