Set focus on existing dialog rather than raise an error if the dialog when launching a dialog which is already open by seanbudd · Pull Request #12800 · nvaccess/nvda

seanbudd · 2021-09-02T07:24:51Z

Link to issue number:

Summary of the issue:

NVDA raises an error when launching a dialog, when the dialog is already open. This is unfriendly to the user, and should instead focus the open dialog.

Example:

Press NVDA+n to open the NVDA menu
Launch the settings dialog by pressing p then s
Change a setting, so that you can confirm a new instance isn't created
Minimise the settings dialog window
Press NVDA+n to open the NVDA menu
Launch the settings dialog by pressing p then s
An error message is raised

Description of how this pull request fixes the issue:

Set focus on existing dialog rather than raise an error if the dialog when launching a dialog which is already open.

Testing strategy:

Manual testing:

Press NVDA+n to open the NVDA menu
Launch the settings dialog by pressing p then s
Change a setting, so that you can confirm a new instance isn't created
Minimise the settings dialog window
Press NVDA+n to open the NVDA menu
Launch the settings dialog by pressing p then s
Confirm the same settings dialog from before is now focused, a new instance was not created, and the error message dialog isn't opened.

System testing

System testing could be added here, but for such a specific feature it seems unworthy, the reviewer may disagree.

Known issues with pull request:

Change log entries:

Changes

- When opening a settings dialog which is already open, NVDA sets focus on the existing dialog rather than raise an error. (#5383)

Code Review Checklist:

Pull Request description is up to date.
Unit tests.
System (end to end) tests.
Manual testing.
User Documentation.
Change log entry.
Context sensitive help for GUI changes.
UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers

cary-rowen · 2021-09-02T08:27:20Z

I tested this PR and the current experience is good.

Thanks.

CyrilleB79 · 2021-09-02T13:21:34Z

Hello
I have tested this PR. Even if the overall experience has improved, there are still some issues remaining. Here is the detailed results of my tests for various dialogs:

Test successful

Punctuation and symbols
Input gesture
Setting dialogs of add-ons. Example tested: Access8Math general settings.

Test almost successful

Test 1

STR:

NVDA+ctrl+G
focus desktop
NVDA+ctrl+K
Expected: keyboard settings focused
Actual: general settings focused

Test 2

STR:

Open default dictionary
focus desktop
Open voice dictionary
Expected: Voice dictionary focused
Actual: default dictionary focused

Test with error

STR:

Open braille settings
press "Change button to choose braille display
focus desktop
press NVDA+ctrl+V to open voice settings
Error:

IO - external:inputCore.executeGesture (15:08:06.965) - winInputHook (10856):
Input: kb(desktop):NVDA+control+v
DEBUG - external:gui.settingsDialogs.__new__ (15:08:07.004) - MainThread (11064):
Creating new settings dialog (multiInstanceAllowed:False). State of _instances {<gui.settingsDialogs.NVDASettingsDialog object at 0x1255FD50>: <DialogState.DESTROYED: 1>, <gui.settingsDialogs.BrailleDisplaySelectionDialog object at 0x1260B760>: <DialogState.CREATED: 0>}
ERROR - external:gui.settingsDialogs.__new__ (15:08:07.004) - MainThread (11064):
Opening new settings dialog while instance still exists: <gui.settingsDialogs.NVDASettingsDialog object at 0x1255FD50>

Some dialogs are not re-focused when opened a second time:

config profile dialog
Add-on manager

NVDA+N not working while these dialogs are opened

Welcom dialog
About dialog

Note:

Most of the mentioned issues (if not all) were already present in NVDA. Thus this PR should not be reverted. But while fixing "Already opened dialog" problem, I think that they should be considered.

seanbudd · 2021-09-02T23:42:59Z

Hi @CyrilleB79, thank you for testing this.
Would you be able to create issues for these if they do not already exist?
I am going to look into the "Test with Error" soon, as it also creates an additional preferences dialog, which is dangerous behaviour (and also happens with 2021.1).

All tests except the "Test almost successful" tests are existing behaviour (eg in 2021.1). These cases would throw the error message normally, and need to be improved as well as this will lead to confusion. Perhaps this PR should be reverted because of this.

cary-rowen · 2021-09-03T00:38:40Z

@CyrilleB79
Hello, I did not receive an error when I executed the following STR:
Test with error
STR:
• Open braille settings
• press "Change button to choose braille display
• focus desktop
• press NVDA+ctrl+V to open voice settings

feerrenrut · 2021-09-09T03:57:20Z

@seanbudd is there a regression here? If so this may need to be reverted (before 2021.3) until these issues are resolved.

seanbudd · 2021-09-09T04:28:47Z

@seanbudd is there a regression here? If so this may need to be reverted (before 2021.3) until these issues are resolved.

@feerrenrut
See my earlier comment:

All tests except the "Test almost successful" tests are existing behaviour (eg in 2021.1). These cases would throw the error message normally, and need to be improved as well as this will lead to confusion. Perhaps this PR should be reverted because of this.

There is no regression - but there is a change of behaviour. Instead of throwing an error message when a settings dialog is already opened, the dialog is focused, but not necessarily the target pane (eg Keyboard settings). Same for dictionary dialogs.

The other bugs @CyrilleB79 raised can be reproduced with 2021.1

feerrenrut · 2021-09-09T04:34:56Z

Thanks for the summary @seanbudd

…naries are of different types. (#12967) Fix-up of #12800. Described in #12800 (comment), paragraph "Test almost successful", "Test 2". Summary of the issue: Since the merge of #12800, trying to re-open an already opened dialog allows to focus it rather than issuing an error as done previously. However, in the case of dictionaries, if you try to open the voice dictionary while the default is already open will lead to focus the default dictionary rather than opening the voice dictionary dialog; Description of how this pull request fixes the issue: Since the check of an existing opened dialog is made on the class of the dialog, I have sub-classed DictionaryDialog for the three types of dictionaries to make them distinct one from another. Co-authored-by: Sean Budd <sean@nvaccess.org>

Link to issue number: Follow-up of #12967 Summary of the issue: Since the merge of #12800, trying to re-open an already opened dialog allows to focus it rather than issuing an error message box. The dialog is refocused according to its class, i.e. trying to open a dialog of the same class rather refocus the first already opened dialog of the same class. But with #12800, it was not possible to open 2 dictionaries of different type (e.g. speech and default). This was fixed with #12967 by subclassing DictionaryDialog. However, addons which directly use DictionaryDialog will still have the issue of opening 2 dictionaries at once i.e. #5383. Description of how this pull request fixes the issue: force DictionaryDialog to be subclassed by making the initializer and abstract method

Set focus on existing dialog rather than raise an error

31f4cae

seanbudd requested a review from a team as a code owner September 2, 2021 07:24

seanbudd requested a review from michaelDCurran September 2, 2021 07:24

seanbudd mentioned this pull request Sep 2, 2021

focus existing NVDA Settings dialog if the same dialog is attempted to be opened again instead of presenting Error dialog #5383

Closed

michaelDCurran approved these changes Sep 2, 2021

View reviewed changes

update changeS

ca66076

seanbudd merged commit d624b5a into master Sep 2, 2021

seanbudd deleted the fix-5383 branch September 2, 2021 07:50

nvaccessAuto added this to the 2021.3 milestone Sep 2, 2021

CyrilleB79 mentioned this pull request Sep 8, 2021

Double setting dialog opened #12818

Closed

seanbudd mentioned this pull request Oct 4, 2021

NVDA crashes if exiting while welcome dialog open #12907

Closed

CyrilleB79 mentioned this pull request Oct 20, 2021

Multiple dictionary dialogs can now be opened simultaneously if the dictionaries are of different types #12967

Merged

5 tasks

CyrilleB79 mentioned this pull request Oct 26, 2021

Refocus multi-category settings dialog with the provided category. #12995

Closed

5 tasks

CyrilleB79 mentioned this pull request Jan 21, 2022

Force DictionaryDialog to be subclassed to use it #13268

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Set focus on existing dialog rather than raise an error if the dialog when launching a dialog which is already open#12800

Set focus on existing dialog rather than raise an error if the dialog when launching a dialog which is already open#12800
seanbudd merged 2 commits into
masterfrom
fix-5383

seanbudd commented Sep 2, 2021 •

edited

Loading

Uh oh!

cary-rowen commented Sep 2, 2021

Uh oh!

CyrilleB79 commented Sep 2, 2021

Uh oh!

seanbudd commented Sep 2, 2021

Uh oh!

cary-rowen commented Sep 3, 2021

Uh oh!

feerrenrut commented Sep 9, 2021

Uh oh!

seanbudd commented Sep 9, 2021

Uh oh!

feerrenrut commented Sep 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

seanbudd commented Sep 2, 2021 • 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 how this pull request fixes the issue:

Testing strategy:

System testing

Known issues with pull request:

Change log entries:

Code Review Checklist:

Uh oh!

cary-rowen commented Sep 2, 2021

Uh oh!

CyrilleB79 commented Sep 2, 2021

Test successful

Test almost successful

Test 1

Test 2

Test with error

Some dialogs are not re-focused when opened a second time:

NVDA+N not working while these dialogs are opened

Note:

Uh oh!

seanbudd commented Sep 2, 2021

Uh oh!

cary-rowen commented Sep 3, 2021

Uh oh!

feerrenrut commented Sep 9, 2021

Uh oh!

seanbudd commented Sep 9, 2021

Uh oh!

feerrenrut commented Sep 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

seanbudd commented Sep 2, 2021 •

edited

Loading