KEMBAR78
Remove `IsScreenReaderActive()` check by andyleejordan · Pull Request #26118 · PowerShell/PowerShell · GitHub
Skip to content

Remove IsScreenReaderActive() check #26118

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Sep 30, 2025
Merged

Remove IsScreenReaderActive() check #26118

merged 2 commits into from
Sep 30, 2025
+12 −117

Conversation

@andyleejordan
Copy link
Member

@andyleejordan andyleejordan commented Sep 29, 2025 •
edited
Loading

PR Summary

With the upcoming inclusion of PSReadLine 2.4, screen readers are now properly supported so we can stop avoiding the loading of PSReadLine. Essentially reverses #10385.

PR Context

Given the new screen reader support from PowerShell/PSReadLine#4854 included in PSReadLine 2.4, and that we are planning to include that beta in the next PowerShell 7.6 preview (with a 2.4 GA to be included in 7.6 RC), we can remove this workaround that avoided loading PSReadLine when screen readers were detected.

The same SPI check is performed in PSReadLine to enable screen reader mode, so the default user experience should switch from no PSReadLine (and a fairly broken legacy REPL implementation) to PSReadLine 2.4 in screen reader mode with known, improved support. There is a small known edge case where a user-installed older version of PSReadLine may be unexpectedly loaded where it was previously not being loaded, but the workaround is to update or remove the user-install. The other known potential issue is that we are changing the default screen reader experience. We believe the move is an improvement, and it is currently in real-world use in VS Code. But if there is a bug that requires the previous behavior, the user can Remove-Module PSReadLine to workaround it, and if we find we need to revert this change, it is very small.

PR Checklist

@andyleejordan
Copy link
Member Author

I'm unsure what labels are appropriate for this PR.

@andyleejordan andyleejordan force-pushed the remove-screenreader-check branch from 0065a7c to a816ace Compare September 29, 2025 21:30
@andyleejordan andyleejordan marked this pull request as ready for review September 29, 2025 22:47
@andyleejordan
Copy link
Member Author

andyleejordan commented Sep 29, 2025 •
edited by unfurl-links bot
Loading

Marking as ready to review because the three failures are unrelated, confirmed by seeing the same three tests fail in https://github.com/PowerShell/PowerShell/actions/runs/18079717180/job/51471840175 for #26109

GitHub
PowerShell for every system! Contribute to PowerShell/PowerShell development by creating an account on GitHub.

@daxian-dbw daxian-dbw added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Sep 29, 2025
iSazonov
iSazonov reviewed Sep 30, 2025
View reviewed changes
Copy link
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

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

If there is a risk of regression why not implement this as an experimental feature so that users could turn off this until fix will be prepared?

src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHost.cs
consoleRunspace = null;
psReadlineFailed = true;
}
consoleRunspace = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder why don't we dispose the runspace?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a question I don't have an answer to.

@andyleejordan
Copy link
Member Author

If there is a risk of regression why not implement this as an experimental feature so that users could turn off this until fix will be prepared?

We don't believe the risk is high enough to justify going through extra process beyond getting this into the preview and release candidates.

@xtqqczze xtqqczze mentioned this pull request Sep 30, 2025
Closed
daxian-dbw
daxian-dbw approved these changes Sep 30, 2025
View reviewed changes
Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

LGTM

@daxian-dbw
Copy link
Member

/azp run PowerShell-CI-linux-packaging, PowerShell-Windows-Packaging-CI

@azure-pipelines
Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@andyleejordan @daxian-dbw
Remove IsScreenReaderActive() check
a64fd7c 
With the upcoming inclusion of PSReadLine 2.4,
screen readers are now properly supported.
@andyleejordan @daxian-dbw
Remove SystemParametersInfo P/Invoke too
842f8b3
@daxian-dbw daxian-dbw force-pushed the remove-screenreader-check branch from ea3dad9 to 842f8b3 Compare September 30, 2025 18:47
@daxian-dbw
Copy link
Member

/azp run PowerShell-CI-linux-packaging, PowerShell-Windows-Packaging-CI

@azure-pipelines
Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@daxian-dbw daxian-dbw merged commit 3aa49cc into PowerShell:master Sep 30, 2025
36 checks passed
@microsoft-github-policy-service
Copy link
Contributor

microsoft-github-policy-service bot commented Sep 30, 2025 •
edited by unfurl-links bot
Loading

📣 Hey @@andyleejordan, how did we do? We would love to hear your feedback with the link below! 🗣️

🔗 https://aka.ms/PSRepoFeedback

@xtqqczze xtqqczze mentioned this pull request Sep 30, 2025
Open
@andyleejordan andyleejordan deleted the remove-screenreader-check branch September 30, 2025 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov left review comments

@daxian-dbw daxian-dbw daxian-dbw approved these changes

Assignees

No one assigned

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@andyleejordan @daxian-dbw @iSazonov