Skip to content
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

Add check for open messageBoxes to prevent exit #12984

Merged
merged 10 commits into from
Oct 28, 2021

Conversation

seanbudd
Copy link
Member

@seanbudd seanbudd commented Oct 25, 2021

Link to issue number:

Relates to #12976

Summary of the issue:

NVDA crashes if you attempt to exit NVDA with a messageBox open (eg the About Dialog).
This is due to wx Idle Events firing on the dialogs while they are being forced to close in the exit process.

  1. Have NVDA running
  2. Press NVDA+n then h to open the help menu, down arrow to "About dialog" and enter to open that.
  3. Press NVDA+Q then ENTER to quit NVDA

Description of how this pull request fixes the issue:

Prevent exit while a MessageBox dialog is open - this is because the state of a MessageBox dialog is important and should be determined before exit.

Testing strategy:

  1. Start NVDA, close any open dialogs
  2. Open a message dialog, such as the About Dialog via NVDA menu > About
  3. Attempt to exit NVDA (exiting NVDA methods)
  4. Confirm the exit dialog does not open and the exit does not occur when the ExitDialog is disabled

Known issues with pull request:

The state of the About Dialog is not important, and a different dialog that can be expected to close on exit should be used instead.

The messageBox module could be redesigned, but this is the safest solution while preserving backwards compatibility.

Change log entries:

Changes

- NVDA will no longer attempt to exit when dialogs are awaiting a required action (eg Confirm/Cancel).

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

@seanbudd seanbudd added this to the 2021.3 milestone Oct 25, 2021
@seanbudd seanbudd requested a review from a team as a code owner October 25, 2021 00:15
@michaelDCurran
Copy link
Member

As inMessageBox is now used quite a bit more, including disallowing exit of NVDA, it might be an idea to put some further protections around this variable.
For instance:

  • rename inMessageBox variable to _inMessageBox
  • add a new isInMessageBox() function
  • in gui.messagebox function:
    • after setting _inMessageBox to true, start a try-block which covers the calls to prePopup and wx.MesageBox, and then the setting of _inMessageBox to False should be done in a finally-block.
      This would all make sure that _inMessageBox never gets accidentally left as True.

@seanbudd seanbudd marked this pull request as draft October 26, 2021 04:41
@seanbudd
Copy link
Member Author

rebasing from master to beta

@seanbudd seanbudd changed the base branch from master to beta October 26, 2021 04:46
@seanbudd seanbudd marked this pull request as ready for review October 26, 2021 04:46
Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

I think this approach is ok for a temporary fix in 2021.3, but I'd like a more robust design for 2022.1. If we agree on this please update the description to make this clear.

The design for 2022.1 should allow the exit strategy to be set when the message box is created (eg: autoclose, bring to foreground). There seem to be different requirements for different messages.

I'm not sure if the following is necessary. Perhaps leave this out, and it can be considered in the follow up PR for 2022.1:

- ``gui.messageBox`` should be replaced with ``gui.message.messageBox`` in 2022.1. (#12984)
- ``gui.isInMessageBox`` should be replaced with ``gui.message.isInMessageBox()`` in 2022.1. (#12984)

source/gui/message.py Outdated Show resolved Hide resolved
@seanbudd
Copy link
Member Author

I'm not sure if the following is necessary. Perhaps leave this out, and it can be considered in the follow up PR for 2022.1.

This follows our pattern for the other releases this year, of warning about deprecating changes "early".
But if we are likely to change the approach in 2022.1, perhaps a notice does more harm than good.

@feerrenrut
Copy link
Contributor

But if we are likely to change the approach in 2022.1, perhaps a notice does more harm than good.

Given the development of 2022.1 has already started, it's not really a warning. But more importantly I don't think we can be confident this is the approach we will follow, and I don't want to spend time blocking the beta to gain that confidence.

@michaelDCurran
Copy link
Member

michaelDCurran commented Oct 27, 2021 via email

Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

Also, please update the changes entries in the description. I don't think we should recommend something for 2022.1 until we know for sure what it will be. If we can't make a recommendation then I don't think there needs to be something in the changes file.

source/gui/__init__.py Outdated Show resolved Hide resolved
seanbudd and others added 2 commits October 28, 2021 09:40
Co-authored-by: Reef Turner <feerrenrut@users.noreply.github.com>
@AppVeyorBot
Copy link

See test results for failed build of commit cdeeddfba0

source/gui/__init__.py Outdated Show resolved Hide resolved
feerrenrut
feerrenrut previously approved these changes Oct 28, 2021
Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

Thanks @seanbudd

@feerrenrut feerrenrut merged commit 47a4605 into beta Oct 28, 2021
@feerrenrut feerrenrut deleted the preventMessageBoxExitCrash branch October 28, 2021 03:08
seanbudd pushed a commit that referenced this pull request Nov 1, 2021
seanbudd pushed a commit that referenced this pull request Nov 1, 2021
seanbudd pushed a commit that referenced this pull request Nov 1, 2021
seanbudd added a commit that referenced this pull request Nov 4, 2021
Fix up of #12984

Summary of the issue:
Backwards compatibility for the following code was broken in 2021.3 beta.

from gui import isInMessageBox
Note: this code creates a copy of the value of isInMessageBox, and as such, was only accurate if it was being imported when it was being checked.

Importing like this also prevents gui.isInMessageBox from being updated:

from gui import isInMessageBox
isInMessageBox = False  # this doesn't update gui.isInMessageBox

Description of how this pull request fixes the issue:
Creates an isInMessageBox that is updated by isInMessageBox.
Note that while from gui import isInMessageBox can run without error now, using or updating the variable will not work as expected, nor has it ever worked properly.
seanbudd added a commit that referenced this pull request Mar 3, 2022
…essageBoxActive (#13376)

Relates to #13007, as the new design should be backwards compatible, we can implement the API changes as-is for 2022.1.
Follow up to the API proposed in #12984

Summary of the issue:
The API for gui.message had not yet been determined for 2022.1, as per #13007.
As the future API is intended to be backwards compatible, this PR commits to the API proposed in #12984.

Description of how this pull request fixes the issue:
gui.IsInMessageBox was a boolean, this has been replaced by a function gui.message.isModalMessageBoxActive, which tracks if a messageBox is open in a safer manner.

Changes logic gui/message.py to be safer with locking and handling errors.
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.

None yet

4 participants