-
Notifications
You must be signed in to change notification settings - Fork 316
Prototype screen reader support #4854
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
base: master
Are you sure you want to change the base?
Conversation
That should default to enabled when one is detected on startup, but also allows the support to be forcibly enabled.
Borrows the "better" algorithm from Electron, with attribution.
Spawns a quick `defaults` process since in .NET using the macOS events is difficult, but this is quick and easy.
That algorithm doesn't work for a non-windowed app like PowerShell.
Because they'll be rendered and it's useless noise.
50d8882
to
e6d62da
Compare
Including revert line, forward delete, and case-changing. Undo/redo tested across characters and strings, insertions and deletions.
e6d62da
to
8b8a080
Compare
This one's the hairest, but seems to be working like Zsh
8b8a080
to
9ba544b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces prototype screen reader support by implementing a SafeRender
abstraction that uses ANSI control codes when screen reader mode is enabled. The changes modify rendering behavior throughout the application to provide better accessibility for screen reader users.
Key changes:
- Introduces
SafeRender
method that outputs ANSI escape sequences instead of full re-rendering when screen reader support is enabled - Adds screen reader detection for Windows (including Narrator) and macOS VoiceOver
- Replaces many
Render()
calls withSafeRender()
calls across editing operations, undo/redo, history, and completion functionality
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
File | Description |
---|---|
PSReadLine/Accessibility.cs | Enhanced screen reader detection for Windows and macOS platforms |
PSReadLine/Cmdlets.cs | Added ScreenReader option with automatic detection as default |
PSReadLine/Options.cs | Added logic to disable predictions when screen reader is enabled |
PSReadLine/Render.cs | Implemented SafeRender method using ANSI control codes |
PSReadLine/UndoRedo.cs | Replaced Render calls with SafeRender for undo/redo operations |
PSReadLine/PublicAPI.cs | Updated Insert and Replace methods to use SafeRender |
PSReadLine/BasicEditing.cs | Modified editing operations to use SafeRender with appropriate ANSI codes |
PSReadLine/History.cs | Updated history navigation and search to use SafeRender |
PSReadLine/KillYank.cs | Modified kill operations to use SafeRender |
PSReadLine/Completion.cs | Added TODO for menu completion screen reader testing |
PSReadLine/KeyBindings.cs | Added TODO for WhatIsKey screen reader evaluation |
PSReadLine/ReadLine.cs | Added TODOs for screen reader evaluation in various render calls |
PSReadLine/PlatformWindows.cs | Added IsMutexPresent method for detecting Windows Narrator |
private static bool IsAnyWindowsScreenReaderEnabled() { | ||
// The official way to check for a screen reader on Windows is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Opening brace should be on the next line to follow C# formatting conventions.
private static bool IsAnyWindowsScreenReaderEnabled() { | |
// The official way to check for a screen reader on Windows is | |
private static bool IsAnyWindowsScreenReaderEnabled() | |
{ | |
// The official way to check for a screen reader on Windows is |
Copilot uses AI. Check for mistakes.
@@ -1140,6 +1143,7 @@ private void UpdateHistoryDuringInteractiveSearch(string toMatch, int direction, | |||
? HistoryMoveCursor.ToEnd | |||
: HistoryMoveCursor.DontMove; | |||
UpdateFromHistory(moveCursor); | |||
SafeRender($"\x1b[s;[1E;[K{_statusLinePrompt}{toMatch}_\x1b[u"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Complex ANSI escape sequence should be extracted to a named constant or helper method for better readability and maintainability.
SafeRender($"\x1b[s;[1E;[K{_statusLinePrompt}{toMatch}_\x1b[u"); | |
SafeRender(FormatISearchStatusLine(_statusLinePrompt, toMatch)); |
Copilot uses AI. Check for mistakes.
@@ -1154,7 +1158,8 @@ private void UpdateHistoryDuringInteractiveSearch(string toMatch, int direction, | |||
_emphasisStart = -1; | |||
_emphasisLength = 0; | |||
_statusLinePrompt = direction > 0 ? _failedForwardISearchPrompt : _failedBackwardISearchPrompt; | |||
Render(); | |||
// Deletes the original status line and then renders the new one | |||
SafeRender($"\x1b[s;[1E;[K{_statusLinePrompt}{toMatch}_\x1b[u"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ANSI escape sequence is duplicated from line 1146. Extract to a helper method to avoid code duplication.
Copilot uses AI. Check for mistakes.
private static bool IsVoiceOverEnabled() | ||
{ | ||
try | ||
{ | ||
// Use the 'defaults' command to check if VoiceOver is enabled | ||
// This checks the com.apple.universalaccess preference for voiceOverOnOffKey | ||
ProcessStartInfo startInfo = new() | ||
{ | ||
FileName = "defaults", | ||
Arguments = "read com.apple.universalaccess voiceOverOnOffKey", | ||
UseShellExecute = false, | ||
RedirectStandardOutput = true, | ||
RedirectStandardError = true, | ||
CreateNoWindow = true | ||
}; | ||
|
||
using Process process = Process.Start(startInfo); | ||
process.WaitForExit(250); | ||
if (process.ExitCode == 0) | ||
{ | ||
string output = process.StandardOutput.ReadToEnd().Trim(); | ||
// VoiceOver is enabled if the value is 1 | ||
return output == "1"; | ||
} | ||
} | ||
catch | ||
{ | ||
PlatformWindows.SystemParametersInfo(PlatformWindows.SPI_GETSCREENREADER, 0, ref returnValue, 0); | ||
// If we can't determine the status, assume VoiceOver is not enabled | ||
} | ||
|
||
return returnValue; | ||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic number 250 should be extracted to a named constant to clarify it represents timeout milliseconds.
Copilot uses AI. Check for mistakes.
try | ||
{ | ||
using var mutex = new System.Threading.Mutex(false, name); | ||
return Marshal.GetLastWin32Error() == 183; // ERROR_ALREADY_EXISTS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic number 183 should be extracted to a named constant for ERROR_ALREADY_EXISTS.
return Marshal.GetLastWin32Error() == 183; // ERROR_ALREADY_EXISTS | |
return Marshal.GetLastWin32Error() == ERROR_ALREADY_EXISTS; |
Copilot uses AI. Check for mistakes.
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | ||
return IsAnyWindowsScreenReaderEnabled(); // Check for macOS VoiceOver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment is incorrect - this line checks for Windows screen readers, not macOS VoiceOver.
return IsAnyWindowsScreenReaderEnabled(); // Check for macOS VoiceOver | |
return IsAnyWindowsScreenReaderEnabled(); // Check for Windows screen readers |
Copilot uses AI. Check for mistakes.
// Render as usual if we're not supporting a screen reader | ||
if (!_singleton.Options.ScreenReader) | ||
{ | ||
Render(); | ||
return; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SafeRender method calls Render() when screen reader is disabled, which could be optimized by calling the caller's original Render() directly instead of adding this indirection layer.
// Render as usual if we're not supporting a screen reader | |
if (!_singleton.Options.ScreenReader) | |
{ | |
Render(); | |
return; | |
} |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review in progress. Need to stop here today and want to share my comments so far.
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | ||
return IsAnyWindowsScreenReaderEnabled(); // Check for macOS VoiceOver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing style in this repo always uses curly braces for if blocks, even for the single-statement if blocks. Can you please update the code to adhere to the existing style? Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't though, I checked 🫠
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a formatting tool if there's a consistent style we're wishing to apply to this repo. Prettier is a great tool.
// Chromium, where the relevant screen reader libraries (modules) | ||
// are checked for does not work in the context of PowerShell | ||
// because it relies on those applications injecting themselves into | ||
// the app. Which they do not because it's not a windowed app, so |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which they do not because it's not a windowed app
Do you mean "PowerShell" is not a windowed app here? It'd be great if you can reword the comments here a little to make it clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct.
// NarratorRunning mutex (which is mostly not broken with | ||
// PSReadLine, as it were). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which is mostly not broken with PSReadLine, as it were
I'm not super sure what you mean by this :) Can you please reword it a bit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Windows Narrator is mostly not broken with PSReadLine. Not in the way that NVDA and VoiceOver are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to be programmed to ignore duplicate output, the whole re-rendering issue doesn't present itself under Narrator. Which made figuring out "what's wrong" (given no reproduction steps) a tad difficult.
|
||
using Process process = Process.Start(startInfo); | ||
process.WaitForExit(250); | ||
if (process.ExitCode == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick, to avoid an InvalidOperationException
in case the process doesn't finish within 250ms.
if (process.ExitCode == 0) | |
if (process.HasExited && process.ExitCode == 0) | |
@@ -138,7 +141,8 @@ private static void ForwardDeleteImpl(int endPosition, Action<ConsoleKeyInfo?, o | |||
!InViEditMode())); | |||
|
|||
buffer.Remove(current, length); | |||
_singleton.Render(); | |||
// Moves cursor to current (backwards probably) and then deletes length | |||
_singleton.SafeRender($"\x1b[{length}P", current); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ESC [ <n> P
only shifts characters from the current line, not the characters of the next line. So, this may not work for editing multi-line text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I should note in the PR description, this has not been tested with multi-line input at all.
A work-in-progress that utilizes a
SafeRender
abstraction that relies on ANSI control codes when a screen reader is enabled. Passes tests locally, handles all basic editing movements (including revert line, forward delete, and case-changing), up/down arrow history, incremental history search (needs more testing), completions (almost coincedentally), and undo/redo (tested across characters and strings, insertions and deletions. Predictions are disabled, and Vi mode has not fixed.Specifically tested with NVDA on Windows and VoiceOver on macOS within VS Code's integrated terminal, with shell integration loaded, and Code's screen reader optimizations enabled. Is probably not going to work in the old Windows Console.