Free display on secure desktop switch by LeonarddeR · Pull Request #18810 · nvaccess/nvda

LeonarddeR · 2025-08-26T05:25:30Z

Link to issue number:

Alternative to #18789
Suggested in #18789 (comment)
Fixes #2315

Summary of the issue:

Currently, braille doesn't work properly on secure screens because the braille display is held by the logged in copy.

Description of user facing changes:

When NVDA is installed, it disconnects braille when entering secure desktop and reconnects when exiting it.

Description of developer facing changes:

None

Description of development approach:

Added a secure desktop switch extension point to the braille handler.
Set a private class property on the braille display driver to ensure the display isn't cleared, in order to make it clear that the secure desktop was entered. This is communicated with an ui.message in IAccessibleHandler.
When leaving secure desktop and auto detect was enabled, try to reconnect to the last detected display before doing a full scan.

Testing strategy:

Tested via USB with a APH Mantis Q40
Tested via Bluetooth with a APH Mantis Q40 as the only Bluetooth device available
Tested both automatic detect and manual display selection cases

Known issues with pull request:

After exiting the secure desktop, it takes some time before the display is back in action. I must say though that with Bluetooth this isn't considerably slower
When using a custom braille display driver bundled in an add-on, the add-on needs to be available on the system copy of NVDA. This has always been the case though, but is fixable with the approach in WIP: Use remote on Secure Desktop to send speech and braille to the user copy #18789.
When using manual braille display selection, reconnecting to the display after closing the secure desktop happens on the main thread. This can lead to a temporary freeze of NVDA, depending on how long it takes for the driver to connect. Note however that the same freeze would occur when starting NVDA.

Open question

Do we want to make this configurable?

I considered this, but then figured that the UX for this would at least be a bit vague. People never had Braille available on secure screens, and then adding a config option like "disable braille when entering secure desktop" would do exactly the opposite from the user's perspective. Not everyone is aware of the separate copy on secure desktop. Would especially be curious to @Qchristensen's opinion on this matter.

Code Review Checklist:

Documentation:
- Change log entry
- User Documentation
- Developer / Technical Documentation
- Context sensitive help for GUI changes
Testing:
- Unit tests
- System (end to end) tests
- Manual testing
UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers
- Localization in other languages / culture than English
API is compatible with existing add-ons.
Security precautions taken.

seanbudd · 2025-08-26T05:56:21Z

I like this new approach a lot more.

After exiting the secure desktop, it takes some time before the display is back in action. I must say though that with Bluetooth this isn't considerably slower. I have something in mind to improve this.

I think that adding this feature is worth the trade off. What do you have in mind to improve this?

When using a custom braille display driver bundled in an add-on, the add-on needs to be available on the system copy of NVDA

I think that makes sense for how NVDA should work.

LeonarddeR · 2025-08-26T06:11:59Z

After exiting the secure desktop, it takes some time before the display is back in action. I must say though that with Bluetooth this isn't considerably slower. I have something in mind to improve this.

I think that adding this feature is worth the trade off. What do you have in mind to improve this?

When a display is detected, we can store its device match on the braille handler as _lastDetectedDisplay. Then, after closing the secure desktop, we initialise detection giving preference to the last detected display.
We can even consider communicating this match to the secure copy, but then we need a more generic way to communicate this kind of things. We could consider making the IPC communication in the NVDA remote client more global.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

LeonarddeR · 2025-08-30T09:28:46Z

Regarding the ux, we can also bind the auto disconnect to the auto detect function only. I think that actually makes much sense. It also avoids NVDA main thread freezes when reinitializing a display on the main thread that was manually detected.

SaschaCowley · 2025-09-05T05:06:52Z

@LeonarddeR in general this looks pretty good to me. I am a little concerned about not freeing the display if auto-detection is disabled:

What if the braille display doesn't support auto-detection, but the user has set up their user and secure desktop copies of NVDA to use their display?
What if they are using a manually selected display, and switch users to another account which also has NVDA (or a different screen reader) and whose user wants to use the same braille display?

LeonarddeR · 2025-09-05T05:24:35Z

@SaschaCowley wrote:

* What if the braille display doesn't support auto-detection, but the user has set up their user and secure desktop copies of NVDA to use their display?

Behavior won't change this pr, i.e. the display is held by the user session.

* What if they are using a manually selected display, and switch users to another account which also has NVDA (or a different screen reader) and whose user wants to use the same braille display?

They can't as long as the user copy holds the display.

I'm happy to revert that particular change. Do we want to make the new feature configurable instead? I'm a bit worried that especially users of manual display selection will have a more laggy experience when entering and closing secure desktop, especially when Bluetooth is used. That said, I don't think we have Bluetooth displays in core that don't support automatic detection.
Making it configurable makes sense to me, but I find it really hard to come up with something in the GUI that explains the feature well and doesn't utterly confuse people.

This reverts commit e5086cd.

SaschaCowley · 2025-09-05T05:33:31Z

I'd remove the configurability for now. If we hear about cases where this negatively impacts users, we can add configuration back in later.

Copilot

Pull Request Overview

This PR enables braille displays to work on secure desktops by automatically disconnecting the display when entering a secure desktop and reconnecting when exiting. This allows the secure desktop copy of NVDA to access the braille display while preventing device conflicts.

Added secure desktop state change extension point to the braille handler
Implemented automatic disconnection/reconnection of braille displays on secure desktop transitions
Added preferred device support to braille detection for faster reconnection

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
user_docs/en/changes.md	Added changelog entry documenting the new braille secure desktop feature
source/gui/settingsDialogs.py	Added bdDetect to advanced settings debug logging options
source/config/configSpec.py	Added bdDetect configuration option for debug logging
source/braille.py	Core implementation of secure desktop switching with display disconnection/reconnection
source/bdDetect.py	Enhanced device detection with preferred device support and improved type annotations

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

LeonarddeR · 2025-09-05T05:40:11Z

@SaschaCowley In that case, I just removed the restriction on auto detect.

SaschaCowley

Thanks, @LeonarddeR, this is a very long awaited change

Free display on secure desktop switch

2ec0680

LeonarddeR mentioned this pull request Aug 26, 2025

WIP: Use remote on Secure Desktop to send speech and braille to the user copy #18789

Closed

9 tasks

LeonarddeR and others added 5 commits August 26, 2025 18:35

Update typing for bdDetect

d08742c

Try to store last detection

bde499d

Pre-commit auto-fix

d333cee

Undo broken type alias

a07633c

Improve logging

98517ad

LeonarddeR marked this pull request as ready for review August 27, 2025 06:23

Copilot AI review requested due to automatic review settings August 27, 2025 06:23

LeonarddeR requested a review from a team as a code owner August 27, 2025 06:23

LeonarddeR requested a review from SaschaCowley August 27, 2025 06:23

LeonarddeR marked this pull request as draft August 27, 2025 06:23

This comment was marked as outdated.

Sign in to view

LeonarddeR and others added 4 commits August 27, 2025 08:25

Apply suggestions from code review

13d5f95

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Use config driver

3d70bfb

No point in pausing bd detect when there is no logon screen registration

bbc9075

last changes before ready for review

b95205d

LeonarddeR marked this pull request as ready for review August 27, 2025 14:05

LeonarddeR requested a review from Qchristensen August 27, 2025 14:13

SaschaCowley added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Aug 28, 2025

Merge remote-tracking branch 'origin/master' into pauseBdDetect

378753c

Bind to autodetect

e5086cd

LeonarddeR added 2 commits September 5, 2025 07:25

Revert "Bind to autodetect"

fc6feaa

This reverts commit e5086cd.

Merge remote-tracking branch 'origin/master' into pauseBdDetect

4d8dc27

LeonarddeR requested a review from Copilot September 5, 2025 05:38

Copilot AI reviewed Sep 5, 2025

View reviewed changes

SaschaCowley approved these changes Sep 5, 2025

View reviewed changes

Merge branch 'master' into pauseBdDetect

9fc66eb

SaschaCowley enabled auto-merge (squash) September 5, 2025 05:58

SaschaCowley disabled auto-merge September 5, 2025 05:59

SaschaCowley merged commit e6557a7 into nvaccess:master Sep 5, 2025
40 checks passed

github-actions Bot added this to the 2026.1 milestone Sep 5, 2025

seanbudd mentioned this pull request Sep 11, 2025

HIMS Braille display keys stop responding when exiting from a protected screen #18894

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Free display on secure desktop switch#18810

Free display on secure desktop switch#18810
SaschaCowley merged 15 commits into
nvaccess:masterfrom
LeonarddeR:pauseBdDetect

LeonarddeR commented Aug 26, 2025 •

edited

Loading

Uh oh!

seanbudd commented Aug 26, 2025

Uh oh!

LeonarddeR commented Aug 26, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

LeonarddeR commented Aug 30, 2025

Uh oh!

SaschaCowley commented Sep 5, 2025

Uh oh!

LeonarddeR commented Sep 5, 2025

Uh oh!

SaschaCowley commented Sep 5, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

LeonarddeR commented Sep 5, 2025

Uh oh!

SaschaCowley left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

LeonarddeR commented Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of user facing changes:

Description of developer facing changes:

Description of development approach:

Testing strategy:

Known issues with pull request:

Open question

Code Review Checklist:

Uh oh!

seanbudd commented Aug 26, 2025

Uh oh!

LeonarddeR commented Aug 26, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

LeonarddeR commented Aug 30, 2025

Uh oh!

SaschaCowley commented Sep 5, 2025

Uh oh!

LeonarddeR commented Sep 5, 2025

Uh oh!

SaschaCowley commented Sep 5, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

LeonarddeR commented Sep 5, 2025

Uh oh!

SaschaCowley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

LeonarddeR commented Aug 26, 2025 •

edited

Loading