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

Windows Terminal: Leverage new UI Automation notifications support #13781

Closed
codeofdusk opened this issue Jun 9, 2022 · 4 comments · Fixed by #14047
Closed

Windows Terminal: Leverage new UI Automation notifications support #13781

codeofdusk opened this issue Jun 9, 2022 · 4 comments · Fixed by #14047
Labels
app/windows-terminal New terminal app, potentially supersedes app/windows-console (repo: microsoft/terminal) p4 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority triaged Has been triaged, issue is waiting for implementation.
Milestone

Comments

@codeofdusk
Copy link
Contributor

Is your feature request related to a problem? Please describe.

With microsoft/terminal#12358, wt now sends UIA notifications containing new text written to the terminal. To prevent conflict with our existing LiveText implementation, NVDA PR #13261 ignores these notifications. However, replacing LiveText with UIA notifications eliminates NVDA's need to diff the terminal (and possibly the need to listen to textChange events), increasing performance and stability. See related issue #11002 (which is significantly improved when switching to UIA notifications).

Describe the solution you'd like

Consider rethinking how NVDA interacts with Windows Terminal:

  • We no longer need to diff, so don't inherit from LiveText and instead create a new, separate class for these terminals supporting UIA notifications.
  • If we don't react to textChange any more (i.e. stop following the EnhancedTermTypedCharSupport approach), consider a new way to detect when typed characters aren't being echoed to the screen (i.e. don't speak typed passwords).

Describe alternatives you've considered

Make no modifications.

@codeofdusk
Copy link
Contributor Author

CC @carlos-zamora.

@seanbudd seanbudd added p4 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority app/windows-terminal New terminal app, potentially supersedes app/windows-console (repo: microsoft/terminal) triaged Has been triaged, issue is waiting for implementation. labels Jun 9, 2022
@LeonarddeR
Copy link
Collaborator

I guess this is currently blocked by the fact that we can't yet distinguish between a terminal with and without notifications at the time of initializing the overlay classes.

@codeofdusk
Copy link
Contributor Author

codeofdusk commented Aug 22, 2022

I guess this is currently blocked by the fact that we can't yet distinguish between a terminal with and without notifications at the time of initializing the overlay classes.

Per official word from the Windows Terminal team (CCing @DHowett), NVDA should always assume that Windows Terminal supports notifications, since all versions that don't are now unsupported. I think we can now consider this unblocked.

I'm officially out of time to work on this issue (I start work on Microsoft Accessibility Insights later today), but I'll leave a few notes here if someone picks it up. Happy to review PRs and provide guidance where allowed by Microsoft policies.

  • Since all active versions of Windows Terminal (wt.exe) now support notifications, NVDAObjects.UIA.winConsoleUIA.WinTerminalUIA should not inherit from NVDAObjects.behaviors.EnhancedTermTypedCharSupport, because we explicitly don't want LiveText behaviour any more (we don't need to diff, unlike every other terminal in NVDA).
    • During development, maybe create a new WinTerminalUIAWithNotifications overlay class that doesn't inherit from EnhancedTermTypedCharSupport and choose which class to use in NVDAObjects.UIA.findOverlayClasses via a feature flag.
    • NVDAObjects.UIA.UIA seems like a suitable new base class here.
    • NVDAObjects.UIA.winConsoleUIA.WinConsoleUIA (the conhost.exe/Windows Console overlay class) should continue to have LiveText, since there are no plans to add notifications support at this time. (wt will be the default terminal on Windows in a future update, so I don't know how much more work conhost will get).
  • Since WinTerminalUIA no longer inherits from Terminal, you'll need to manually set the object's role (refer to NVDAObjects.behaviors.Terminal).
  • The WinTerminalUIA.event_UIA_notification override that blocks notification events will need to be removed (or not added to the new class, depending on the development approach).
  • Since we don't need to diff, there's no need for NVDA to register for or receive event_textChange in terminals. Processing the number of text change events sent by wt and conhost is a huge bottleneck (probably the biggest contributer to performance issues in terminal today), so not registering for or receiving text changes should seriously improve performance/lockups when a lot of text is written to the terminal. I added global support for this in UIAHandler by mapping UIAHandler.UIA.UIA_Text_TextChangedEventId to textChanged in UIAHandler.UIAEventIdsToNVDAEventNames on Windows 10 (motivated by UIA console), maybe it can be made conditional/disabled somehow?
  • You'll need to figure out how to determine if typed characters are passwords/actually are printed to the screen without relying on text changes and a queue (i.e. my EnhancedTermTypedCharSupport approach).
    • If config.conf["terminals"]["speakPasswords"] is True, all typed characters should always be spoken (i.e. don't do anything special).
    • If config.conf["terminals"]["speakPasswords"] is False, typed characters that don't appear on-screen should be filtered out (i.e. never reported by NVDA/event_typedCharacter should be thrown out).

@LeonarddeR
Copy link
Collaborator

According to microsoft/terminal#13601 (comment), all WT versions that are currently supported have UIA notifications implemented. Therefore it is safe to drop the older approach for Windows Terminal, at least for NVDA 2022.4 and newer.

seanbudd pushed a commit that referenced this issue Oct 25, 2022
…ng for new text (#14047)

Closes #13781

Summary of the issue:
Windows Terminal supports UIA notification events to notify of new text. This is way quicker than the current diffing we use to calculate new text to speak in a terminal.

Description of user facing changes
Note: This is hidden behind a feature flag.
When enabled, this should improve responsivity whereas having minimal user impact. The only thing I noticed is that when typing echo is on, the last typed character/word is spoken after pressing enter, whereas in the old situation, the last typed char/word was always silenced. I'd say this is an improvement.

Description of development approach
Handle the Windows Terminal object as regular Editable Text, i.e. remove the terminal specific stuff from it.
@nvaccessAuto nvaccessAuto added this to the 2023.1 milestone Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app/windows-terminal New terminal app, potentially supersedes app/windows-console (repo: microsoft/terminal) p4 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority triaged Has been triaged, issue is waiting for implementation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants