Skip to content

Fix bug where NVDA would not speak until restarted after add-on installation.#15448

Merged
seanbudd merged 6 commits into
betafrom
try-fix-14525
Sep 20, 2023
Merged

Fix bug where NVDA would not speak until restarted after add-on installation.#15448
seanbudd merged 6 commits into
betafrom
try-fix-14525

Conversation

@seanbudd

@seanbudd seanbudd commented Sep 14, 2023

Copy link
Copy Markdown
Member

Link to issue number:

Fixes #14525

Summary of the issue:

NVDA blocks exit on modal dialogs which require an answer.
If a message box is created after NVDA has triggered a shutdown, NVDA blocks exit on the message box, even after synthesizers have been terminated.

Description of user facing changes

Fix bug where NVDA would not speak until restarted after add-on installation.
Similar bugs with the same root cause may also be fixed.

Description of development approach

Open, pending modal dialogs should block the exit of NVDA.
To prevent a deadlock NVDA should prevent opening new modal dialogs if the core shutdown has been triggered.

Testing strategy:

Test STR in #14525

Known issues with pull request:

None

Change log entries:

refer to diff

Code Review Checklist:

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

@seanbudd seanbudd added this to the 2023.3 milestone Sep 14, 2023
@seanbudd seanbudd requested a review from a team as a code owner September 14, 2023 06:01
@seanbudd seanbudd requested review from michaelDCurran and removed request for a team September 14, 2023 06:01
@seanbudd

Copy link
Copy Markdown
Member Author

@CyrilleB79 would you be able to confirm this build fixes #14525

@CyrilleB79

Copy link
Copy Markdown
Contributor

@seanbudd, tests on the commit 8a16014 show that STR of #14525 is fixed.

There is however a slightly different use case that remains problematic.
STR:
0. Use a brand new NVDA config

  1. Take any add-on, e.g. the simple add-on Say Product Name and Version
  2. In an NVDA without any add-on installed, install your add-on and restart
  3. Once NVDA is restarted, open the add-on manager dialog, but do not validate the warning dialog (keep it open)
  4. Alt+Tab to Windows Explorer, keeping Add-on manager window opened in the background
  5. Install again your add-on from Windows Explorer
  6. When prompted to restart, press Yes

Actual behavior:
NVDA does not restart and an error is logged.
No problem with NVDA speech this time.

Expected behavior:
NVDA should restart, or a message should indicate why it cannot execute restart.

Note however that this issue is much more less concerning because we can imagine that people usually do not keep this dialog open. If you cannot find any fix for this, I'd suggest to merge the current PR anyway because it's already an improvement.

michaelDCurran
michaelDCurran previously approved these changes Sep 14, 2023
@seanbudd seanbudd marked this pull request as draft September 19, 2023 00:01
@seanbudd seanbudd marked this pull request as ready for review September 20, 2023 00:30
@seanbudd

Copy link
Copy Markdown
Member Author

@CyrilleB79 this should be fixed now

michaelDCurran
michaelDCurran previously approved these changes Sep 20, 2023
@seanbudd seanbudd merged commit cd4fcab into beta Sep 20, 2023
@seanbudd seanbudd deleted the try-fix-14525 branch September 20, 2023 01:58
@CyrilleB79

Copy link
Copy Markdown
Contributor

@CyrilleB79 this should be fixed now

Confirmed in beta branch: I get now the "Unavailable action" message after clicking Yes in the restart message.
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants