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: Downgrade E_FAIL when comparing selection changes to debugWarning #11039

Merged

Conversation

codeofdusk
Copy link
Contributor

@codeofdusk codeofdusk commented Apr 20, 2020

Link to issue number:

microsoft/terminal#5399.
Blocking #10964.

Summary of the issue:

Conhost has two buffers: a normal, used for most commands; and an alternate, used, for example, for screen-based editors on Linux machines. Calling compareEndPoints on text infos returns E_FAIL, since these ranges aren't comparable (they're in different buffers). Therefore, when attempting to detect selection changes when switching from normal to alternate buffer (or vice versa), an error is logged.

To see the error, run the latest inbox conhost or OpenConsole. You can try using the build listed in #10618 (comment) (run OpenConsole.exe), and run nano (either in wsl or over ssh).

Description of how this pull request fixes the issue:

Downgrades the expected exception in detectPossibleSelectionChange from error to debugWarning.

Testing performed:

  1. Opened the test build linked in the PR.
  2. Connected to a Linux system over ssh.
  3. Ran nano. Observed that no error sound was played, the exception was logged at debugWarning, and the console functioned normally.

Known issues with pull request:

Change log entry:

None.

@LeonarddeR
Copy link
Collaborator

It looks like microsoft/terminal#5399 is merged. Is this pr still valid?

@codeofdusk
Copy link
Contributor Author

Yes, it depends on changes from that PR.

@feerrenrut
Copy link
Contributor

What is NVDA trying to do when this exception is raised? There is very little context here about what effect this error has on NVDA. Can you answer why NVDA is attempting to compare two endpoints in different buffers/coordinate systems?

@codeofdusk
Copy link
Contributor Author

See microsoft/terminal#5309.

@codeofdusk codeofdusk marked this pull request as draft May 1, 2020 23:09
@codeofdusk

This comment has been minimized.

@codeofdusk codeofdusk marked this pull request as ready for review October 1, 2020 23:52
@codeofdusk
Copy link
Contributor Author

I discussed this offline with @carlos-zamora. microsoft/terminal#5406 won't make it into 21H1, but we think this is a reasonable approach.

@codeofdusk
Copy link
Contributor Author

I've updated the description of this PR to improve clarity. Cc @feerrenrut, @michaelDCurran. Also note that this PR is blocking #10964.

@feerrenrut
Copy link
Contributor

I couldn't reproduce the initial problem from the testing steps listed. Without that I can't verify this fixes the issue.

With

  • NVDA 2020.3rc1
  • Windows Terminal Version: 1.3.2651.0

I tried with a fresh config also (and enabled "use UI Automation to access the Windows Console when available")

Then:

  • With WSL Ubuntu directly (just open nano)
  • ssh from Ubuntu to another Ubuntu machine and run nano
  • ssh from cmd to Ubuntu and run nano.

Looking at microsoft/terminal#5309 it seems that this may only reproducible on Win10-21H1? Can you let me know if this is the case. This kind of information is quite important for us to know how to prioritize a PR.

@codeofdusk
Copy link
Contributor Author

codeofdusk commented Oct 9, 2020

This doesn't happen on Windows Terminal, as terminal doesn't have an alt buffer (is that correct, @carlos-zamora and @DHowett)? It only applies to UIA in inbox and OpenConsole.

@DHowett
Copy link

DHowett commented Oct 9, 2020

That's true today, but only due to time and prioritization :)

@codeofdusk
Copy link
Contributor Author

Right, so this PR is helpful on conhost now and Windows Terminal eventually.

@carlos-zamora
Copy link

Yeah, our discussion on microsoft/terminal#5309 had this issue not repro in Windows Terminal for some reason. But your reasoning is correct. In a situation like this, we're comparing two text ranges that belong to two entirely different coordinate spaces.

@codeofdusk
Copy link
Contributor Author

@feerrenrut are you able to see the error generated (before this PR at level error, after it at level debug warning) using the build in #10618 (comment) (run OpenConsole.exe)?

@feerrenrut
Copy link
Contributor

Please update the description with accurate testing steps.

@codeofdusk
Copy link
Contributor Author

Done.

@feerrenrut
Copy link
Contributor

Sorry @codeofdusk these steps still leave a large amount of assumed knowledge. Please give step by step instructions. I'd prefer not to waste time testing the wrong thing again.

Please answer at least the following:

  • What windows build do you assume?
  • Can this be tested on v2004?
  • What does this mean It only applies to UIA in inbox and OpenConsole.? I believe MS folk talk about an "in-box solution" to mean something included with windows. But this doesn't seem to fit here. OpenConsole?
  • What exact steps must I complete? (EG win+r, type cmd, connect via ssh (which ssh implementation? putty? openssh?) to a linux machine (eg ubuntu), run nano

@codeofdusk
Copy link
Contributor Author

codeofdusk commented Oct 12, 2020

  • What does this mean It only applies to UIA in inbox and OpenConsole.? I believe MS folk talk about an "in-box solution" to mean something included with windows. But this doesn't seem to fit here. OpenConsole?

You are correct re inbox. OpenConsole is the open-source version of Windows Console (AKA conhost) available on Github (available in the same repo as the new Windows Terminal, which is a different application altogether).

  • Can this be tested on v2004?

Yes, if you use OpenConsole this can be tested (I think) as far back as Windows 10 1607. I've run OpenConsole on builds as early as 1903 with no problem.

  • What windows build do you assume?

Either a 21H1 build (in which case, you can run the inbox console) or OpenConsole on Windows 10 1903 or later.

  • What exact steps must I complete? (EG win+r, type cmd, connect via ssh (which ssh implementation? putty? openssh?) to a linux machine (eg ubuntu), run nano
  • Run conhost:
    • If Windows version is less than 21H1, run OpenConsole.exe (built from the repo or the build I linked).
    • Else run cmd
  • ssh (type in ssh user@host in the console) to a Linux system.
  • In the Linux shell, run nano.

@feerrenrut
Copy link
Contributor

Right. So this fix doesn't really apply unless you are running insider builds of Windows, or have built OpenConsole manually.

Given the low risk for this change and the difficulty of testing it, I'm going ahead with it as it is.

@feerrenrut feerrenrut merged commit e274931 into nvaccess:master Oct 12, 2020
@nvaccessAuto nvaccessAuto added this to the 2020.4 milestone Oct 12, 2020
@codeofdusk codeofdusk deleted the cmduia-catch-endpoint-exception branch October 12, 2020 14:41
codeofdusk added a commit to codeofdusk/nvda that referenced this pull request Jun 26, 2021
codeofdusk added a commit to codeofdusk/nvda that referenced this pull request Jun 27, 2021
codeofdusk added a commit to codeofdusk/nvda that referenced this pull request Jun 28, 2021
codeofdusk added a commit to codeofdusk/nvda that referenced this pull request Jul 2, 2021
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.

6 participants