Skip to content

Prevent multiple settings dialogs opened#13117

Merged
seanbudd merged 8 commits into
masterfrom
test-12818
Feb 10, 2022
Merged

Prevent multiple settings dialogs opened#13117
seanbudd merged 8 commits into
masterfrom
test-12818

Conversation

@seanbudd

@seanbudd seanbudd commented Dec 2, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Link to issue number:

Fixes #12818, #12994

Summary of the issue:

Two settings dialogs can be opened when using the steps in #12818.

Wx destroy events (and other events) are sent to parent dialogs for handling. Read More.
When navigating to braille or speech settings, a ExpandoTextCtrl is initialized.
The ExpandoTextCtrl creates a destroy event as part of initialization, this caused the NVDASettings dialog to be incorrectly set to destroyed.

Description of how this pull request fixes the issue:

Filter the destroy event on NVDA Settings dialogs by checking if the dialog itself is being destroyed, as opposed to a child control.

Testing strategy:

system tests are used in the commit history to confirm the fix.
Manual testing of the steps in #12818 also confirms this fix.

Known issues with pull request:

In many cases we perform actions like this based on wx events which are coming from child controls.
Documentation of this is added in #13325

Change log entries:

Bug fixes

- Fixed an issue where multiple settings dialogs could be opened at the same time. (#12818)

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 requested a review from a team as a code owner December 2, 2021 01:12
@seanbudd seanbudd requested a review from feerrenrut December 2, 2021 01:12
@seanbudd seanbudd changed the title Add system test for #12818 Add system test for #12818 - multiple settings dialogs opened Dec 2, 2021
@CyrilleB79

CyrilleB79 commented Dec 2, 2021

Copy link
Copy Markdown
Contributor

@seanbudd you write:

Perhaps the example could become more minimal - eg without the refocus to desktop.

To clarify why I had added the "focus desktop" step:
If you remove the "refocus desktop" step, you will still have 2 settings dialogs opened. However:

  • if you refocus desktop (or any other application) before opening the voice settings dialog, you will be able to reach both settings dialogs (braille display settings and voice settings) with Alt+Tab
  • if you open voice settings while braille display settings is focused (i.e. wi==omitting the focus desktop step), you will not be able to reach the braille settings dialog with Alt+Tab.
    Alt+Tabbing to each settings dialog is the way I have found to evidence that 2 settings dialogs are opened at the same time.

@feerrenrut

feerrenrut commented Dec 3, 2021

Copy link
Copy Markdown
Contributor

This is quite specific behavior to test. I'm not sure there would be ongoing value from running this system test. This test will be helpful while fixing the issue, but after that I'm not so sure. In any case, if we do decide to integrate this, I think the test should be kept separate from the "get GUI screenshot" test. I see that the name of that robot file "settingsDialogs" is the more appropriate for this test, so I'd suggest moving the screenshot gathering to something more specific like "settingsDialogsVisualCheck"

@seanbudd

seanbudd commented Dec 3, 2021

Copy link
Copy Markdown
Member Author

I think once the issue is fixed it should be excluded from the build, and removed if we don't see value in keeping it.
Alternatively, this PR could be left as a draft until the issue is fixed and the following work done separately:

I think the test should be kept separate from the "get GUI screenshot" test. I see that the name of that robot file "settingsDialogs" is the more appropriate for this test, so I'd suggest moving the screenshot gathering to something more specific like "settingsDialogsVisualCheck"

I agree

@seanbudd seanbudd marked this pull request as draft December 3, 2021 03:14
@AppVeyorBot

AppVeyorBot commented Feb 3, 2022

Copy link
Copy Markdown

See test results for failed build of commit 794dd23e5b

@AppVeyorBot

AppVeyorBot commented Feb 3, 2022

Copy link
Copy Markdown

See test results for failed build of commit a8900369f9

@AppVeyorBot

AppVeyorBot commented Feb 8, 2022

Copy link
Copy Markdown

See test results for failed build of commit 388be3416f

@AppVeyorBot

AppVeyorBot commented Feb 8, 2022

Copy link
Copy Markdown

See test results for failed build of commit 93853ef124

@seanbudd seanbudd marked this pull request as ready for review February 8, 2022 23:46
@seanbudd seanbudd changed the title Add system test for #12818 - multiple settings dialogs opened Prevent multiple settings dialogs opened Feb 8, 2022
@seanbudd seanbudd merged commit d482845 into master Feb 10, 2022
@seanbudd seanbudd deleted the test-12818 branch February 10, 2022 23:49
@nvaccessAuto nvaccessAuto added this to the 2022.1 milestone Feb 10, 2022
@seanbudd seanbudd mentioned this pull request Feb 16, 2022
Closed
@seanbudd seanbudd linked an issue Feb 16, 2022 that may be closed by this pull request
Double setting window opened #12994
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaelDCurran michaelDCurran Awaiting requested review from michaelDCurran

1 more reviewer

@feerrenrut feerrenrut feerrenrut approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2022.1

Development

Successfully merging this pull request may close these issues.

Double setting window opened Double setting dialog opened

5 participants

@seanbudd @CyrilleB79 @feerrenrut @AppVeyorBot @nvaccessAuto