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

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

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

CyrilleB79
Copy link
Collaborator

@CyrilleB79 CyrilleB79 commented Oct 26, 2021

Link to issue number:

Fix-up of #12800.
Described in #12800 (comment), paragraph "Test almost successful", "Test 1".

Summary of the issue:

If the multi-category settings dialog is already opened and you try to open it a second time, it will be refocused. But the new expected category panel is not shown; the old panel remains shown instead.

Description of how this pull request fixes the issue:

Change the panel when refocusing a MultiCategorySettingsDialog according to the panel passed as parameter.

Testing strategy:

Test 1:

Press successively NVDA+control+G, NVDA+control+O, NVDA+control+M, NVDA+control+K.
Checked that the expected panel is shown each time.

Test 2:

  • Press NVDA+control+D
  • Change a setting (e.g. report font size)
  • Press NVDA+control+K
  • Change a setting (e.g. skim reading)
  • Press enter to validate the settings dialog
  • Reopen each setting panel to check that the options have been saved correctly in the config.

Test 3:

Press NVDA+Control+K, Alt+Tab to Windows Explorer (or whatever), open NVDA menu -> Preferences -> Settings.
Settings dialog is refocused on keyboard settings panel.

Test 4:

  1. Press NVDA+control+V
  2. tab to "Change" button and press it
  3. Alt+Tab to Windows Explorer (or whatever)
  4. Press NVDA+control+G
  5. Alt+Tab to settings dialog
  6. Press Escape

Result:

All tests are successful except test 4.

Known issues with pull request:

None

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

@CyrilleB79
Copy link
Collaborator Author

This PR is a follow-up of #12800.
I am blocked however with one of the manual tests (test 4) whose result is not satisfying. A satsifying result could be:

  • Focus the Choose synthesizer dialog
    or
  • The settings dialog (or the choose synth dialog) is not refocused. But when exiting the choose synth dialog, the voice panel should still be selected, not the general settings panel.
    @seanbudd or anyone else interested in it, could you help me get one of this result? I do not know how to proceed. Thanks.

@seanbudd
Copy link
Member

With #12944 closed, are the testing steps in 4 still an issue?

Can you merge in master and reconfirm testing?

@CyrilleB79
Copy link
Collaborator Author

CyrilleB79 commented Feb 17, 2022

I have updated merging the latest master branch.
I have re-tested and confirm that test 4 is till an issue.

Regarding test 4, I can add that step 3. is actually not mandatory and can be ignored.

For now, I still do not know how to solve step 4. If you have any advice, let me know.

@seanbudd let me know if I should keep this issue as a draft or not.

@seanbudd
Copy link
Member

I think in absence of a solution for test 4, this PR is still an improvement of behaviour.
I imagine fixing this will involve handling a wxEvent on a certain control.

@CyrilleB79
Copy link
Collaborator Author

No @seanbudd, I do not consider this PR as is as an improvement due to the following result which is not acceptable IMO:

  • At step 6. the focus is reported on the "Change" button (from Speech settings). But in realty, the General settings panel is shown.

This is a new and concerning issue introduced by this PR, since NVDA+Tab incorrectly reports that the change button has the focus, whereas this button is not displayed anymore.

Would you have any hint to implement one of the suggestions made in #12995 (comment)?

@seanbudd
Copy link
Member

I spent some time investigating this a couple of weeks ago and hit dead ends unfortunately.
I've scheduled to pick this up later.

@amirmahdifard
Copy link

@CyrilleB79 i was looking at nvda's pull request history, and looked at this, even tho i've looked at this several times before. This is really interesting, any update about this? or you stil don't have any ideas of fixing the mentioned issues and make this ready? I'd like to try this as whel, can you please trigger the ap veyor build by merging the master? or it is currently impossible? thanks.

@amirmahdifard
Copy link

@CyrilleB79 regarding the problem of opening select sinthesizer dialog causes a bug for refocusing the provide catigory in the settings dialog, i have a good, standard and easy idea. My idea is:
first, please compare 2 things.
1: open the (Select Synthesizer dialog) using the (NVDA+CTRL+S) gesture, and while it is open, scrole your open windows with (alt+tab) keys.
2: open the (Select Synthesizer dialog) by going to NVDA settings, speech catigory, pressing the change button. And while it is open, scrole your open windows with (alt+tab) keys.
in case 1: when you are scrolling your windows, you can see the (Select Synthesizer dialog) as one of your windows, and you can swich to it as normal as it is a standalone dialog, wich is the expected behavior and the case 2 should become like case 1 to fix this problem.
in case 2: When you are scroling your windows, you cannot see the (Select Synthesizer dialog) in your windows list. You can only see the (NVDA Settings: Speech (normal configuration) dialog) in your windows list, But when you swich to it, you are forcefully bumped in to the (Select Synthesizer dialog), and you can't access the speech pannle in any case if you don't close the (Select Synthesizer dialog) first. This behavior is also same for the (Select Braille Display dialog).
The behavior in the case 2 is incorrect, because when opening the (Select Synthesizer dialog) or the (Select Braille Display dialog) by following the steps of the case 2, The (Select Synthesizer dialog) or the (Select Braille Display dialog) should be shown in your windows list when you are scrolling your windows with (alt+tab) keys. My point is that, the (nvda settings dialog) and the other dialogs such as select brale display or select Synthesizer dialogs should be separate instead of for example the (Select Synthesizer dialog) being depend on the (nvda settings dialog) exactly like the case 1, as all of those dialogs are standalone dialogs and we should have the ability to swich to each of them with (alt+tab) keys while doing something else in the other dialog for example, but the current behavior of these dialogs being depending on each other, disallowes us from doing that.
And if you stil don't understand what do i mean by dialogs should be separate and not depend on each other, do the following. Opening these dialogs with the steps below, will make the dialogs completely open separately and independent of each other, and you can see all of them in your windows list and swich to each of them with out seeing a dialog showes another dialog and such problems
1: open the (Select Synthesizer dialog) using the (NVDA+CTRL+S) gesture.
2: alt tab to file explorer for example.
3: either open nvda settings from the nvda menu, or open a specific catigory for example (NVDA+CTRL+G).
4: now, hold alt down, and scrole all your windows with tab key. You can see (Select Synthesizer dialog), (NVDA settings dialog), You can see all of them completely separate and independent of each other.
This is 1: completely reasonable, 2: gives another standard, reasonable and advance advantage to people to be able to swich to standalone dialogs while doing something else in another dialog, thus giving people the ability to do 2 difrent things in 2 difrent dialogs at the same time, or just have all of them listed in the windows list if wanted, 3: fixes this problem that you were facing in this pr in the grate and standard way.
thanks. I hope this would be helpful, and you like it. I'm waiting for your feedback, and i tested all possible things with the idea i just provided and it didn't had any problem.
Thanks.

@amirmahdifard
Copy link

guys, was my idea sound good and helpful for you in this repo?

@HanaDadfar
Copy link
Contributor

hi
reproducing the test steps of the problem given by
@CyrilleB79
by following the idea provided by
@amirmahdifard
The mentioned problem is no longer exist
Test 4:

  1. Press NVDA+control+V
  2. Tab to "Change" button and press it
  3. Alt+Tab to Windows Explorer (or whatever)
  4. Press NVDA+control+G
    result: the General Categorie opens with out any problem, given the results of the idea provided by
    @amirmahdifard
    that when you press Change button, the Select Synthesizer dialog will open independently of NVDA Settings dialog, and can be swiched to with alt+tab
    the problem of test 6 is also no longer exist, given that the Select Synthesizer dialog will no longer take over the nvda settings dialog when swiched to
    personally, I also like the idea given by
    @amirmahdifard
    in this case, because of firstly, given the points and reasons provided by
    @amirmahdifard
    for this idea, and i should add that, the current behavior of these dialogs are ambiguous and confusing a bit, because like, the Select Synthesizer dialog doesn't show up when navigating windows, and appears when nvda settings dialog takes focus, wich is not a true dialog it self.
    thanks

@CyrilleB79
Copy link
Collaborator Author

@amirmahdifard and @HanaDadfar, actually, if you wish to discuss the benefit of having speech synth dialog accessible either directly or through the Speech settings dialog, I'd recommend to do it in a separate issue.
Indeed, the issue with the current PR is not restricted to the speech synth or the braille device dialog. It is occurring with any dialog opened within the settings dialog, that is:

  • the restart dialog when changing language
  • the error / warning dialogs when unchecking checkable lists of NVDA keys or available speech modes
  • probably other error/warning dialog that will be introduced with the sound split feature

BTW @amirmahdifard, regarding re-triggering the appVeyor build, as already told in another issue, I cannot do it unless I also fix the conflicts. Moreover I have encouraged you to install and debug your own local build setup, with help from nvda-devel mailing list if needed, but have not seen any news from you on that list.
Please understand that people cannot re-trigger appVeyor build each time you are interested in a PR, the more if you do not answer to alternative advice provided to you.

@HanaDadfar
Copy link
Contributor

@CyrilleB79
i'm not sure about that, but the idea of the Select Synthesizer dialog and the Select Braille Display dialog not being children of the settings dialog sounded good to me, as we can interact with both of them open at the same time, and the mentioned above.
but then i don't know if all dialogs/error/warning dialogs can be not children of the settings dialog, but i don't see any disadvantage in it.
and i talked about that here because i was thinking this is the way to fix this problem, but you are true. It's not limited only to these 2 dialogs.

@seanbudd
Copy link
Member

Hi Cyrille, I noticed there's been some recent movement here, should this PR be kept open?

@CyrilleB79
Copy link
Collaborator Author

Quite low priority for me, but I'd like to try a solution based on something similar to the blocking decorator when a setting dialog is already opened.

@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Jul 2, 2024
@seanbudd
Copy link
Member

seanbudd commented Sep 2, 2024

@CyrilleB79 - do you plan to do any further work here? Otherwise this should be closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants