Skip to content

Remote Access: prevent multiple disconnection confirmation dialogs#19442

Merged
seanbudd merged 4 commits into
nvaccess:betafrom
cary-rowen:fix/remote-client-multiple-disconnect-dialogs
Jan 21, 2026
Merged

Remote Access: prevent multiple disconnection confirmation dialogs#19442
seanbudd merged 4 commits into
nvaccess:betafrom
cary-rowen:fix/remote-client-multiple-disconnect-dialogs

Conversation

@cary-rowen

@cary-rowen cary-rowen commented Jan 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Link to issue number:

None.

Summary of the issue:

In NVDA Remote Access, if a user is in "Follower" mode and the "Confirm disconnection when controlled" option is enabled, triggering the "Disconnect" action multiple times in rapid succession (e.g. by clicking the menu item repeatedly) results in multiple confirmation dialogs stacking on top of each other.

Description of user facing changes:

When attempting to disconnect from a Remote Access session, if a confirmation dialog is already open, NVDA will now focus the existing dialog instead of opening a new one.

Description of developer facing changes:

None.

Description of development approach:

The RemoteClient now maintains a reference to the active disconnection confirmation dialog in self._disconnectConfirmationDialog.
In doDisconnect, if this reference is set, the existing dialog is brought to the foreground. Otherwise, a new dialog is created, stored in the reference, and cleared in a finally block after it closes. This prevents re-entrancy while targeting only the specific dialog instance.

Testing strategy:

  • Enabled "Confirm disconnection when controlled" in Remote Access settings.
  • Triggered the "Disconnect" menu item repeatedly.
  • Verified that only one dialog appeared and subsequent triggers focused the existing one.

Known issues with pull request:

None.

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.

@cary-rowen cary-rowen requested a review from a team as a code owner January 13, 2026 07:40
@cary-rowen cary-rowen force-pushed the fix/remote-client-multiple-disconnect-dialogs branch from e54d975 to bccb2ea Compare January 13, 2026 07:48
@cary-rowen

cary-rowen commented Jan 13, 2026

Copy link
Copy Markdown
Contributor Author

I checked the failed jobs and they don't seem to be related to this change; it's a minor improvement.

@SaschaCowley

SaschaCowley commented Jan 14, 2026

Copy link
Copy Markdown
Member

I haven't looked at the code here, but based on the description, I think it might be better to keep a (weak) reference to the confirmation dialog, and bring it, specifically, to the foreground programmatically.
Consider what happens if the user performs the toggle connection/disconnect gesture, the confirmation dialog appears, and then another modal dialog is shown: with the current solution, the user must dismiss the new modal dialog before interacting with the disconnect dialog, even if they execute the toggle connection/disconnect command again.
Otherwise this is a good catch, thank you

@cary-rowen

cary-rowen commented Jan 14, 2026

Copy link
Copy Markdown
Contributor Author

Hello @SaschaCowley
I agree with you. I've updated it; although there's a bit more code, I think it's worth it.

@seanbudd

seanbudd commented Jan 16, 2026

Copy link
Copy Markdown
Member

@SaschaCowley thoughts on targeting this to beta for 2026.1? it's a small bug fix

@SaschaCowley

SaschaCowley commented Jan 16, 2026

Copy link
Copy Markdown
Member

I think this should be fine to go into 2026.1

@seanbudd seanbudd added this to the 2026.1 milestone Jan 16, 2026
@seanbudd

seanbudd commented Jan 16, 2026

Copy link
Copy Markdown
Member

@cary-rowen - could you re-target to beta?

@cary-rowen cary-rowen changed the base branch from master to beta January 16, 2026 05:35
@cary-rowen cary-rowen requested a review from a team as a code owner January 16, 2026 05:35
@cary-rowen cary-rowen force-pushed the fix/remote-client-multiple-disconnect-dialogs branch from 8980487 to 25f920e Compare January 16, 2026 08:15
@cary-rowen cary-rowen force-pushed the fix/remote-client-multiple-disconnect-dialogs branch from 25f920e to 636c8f6 Compare January 16, 2026 08:23
@cary-rowen

cary-rowen commented Jan 16, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @seanbudd @SaschaCowley
It looks like everything is ready. Thank you for making this minor fix available in 2026.1.

seanbudd
seanbudd reviewed Jan 16, 2026
View reviewed changes
Comment thread source/_remoteClient/client.py Outdated
Comment thread source/_remoteClient/client.py
@tareh7z tareh7z mentioned this pull request Jan 16, 2026
Closed
5 tasks
@SaschaCowley SaschaCowley added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Jan 20, 2026
seanbudd
seanbudd approved these changes Jan 21, 2026
View reviewed changes

@seanbudd seanbudd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @cary-rowen

@seanbudd seanbudd merged commit 2b3e9c2 into nvaccess:beta Jan 21, 2026
37 of 39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seanbudd seanbudd seanbudd approved these changes

@SaschaCowley SaschaCowley Awaiting requested review from SaschaCowley SaschaCowley is a code owner automatically assigned from nvaccess/developers

@Qchristensen Qchristensen Awaiting requested review from Qchristensen Qchristensen was automatically assigned from nvaccess/userdocs

Assignees

No one assigned

Labels

conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.

Projects

None yet

Milestone

2026.1

Development

Successfully merging this pull request may close these issues.

3 participants

@cary-rowen @SaschaCowley @seanbudd