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

Fixes somes context help issues in advanced settings panel. #12034

Merged
merged 2 commits into from Feb 8, 2021

Conversation

CyrilleB79
Copy link
Collaborator

Link to issue number:

None

Summary of the issue:

In #12025 (superseding #11079) and in #11639, new options were added to the advanced settings panel and a dedicated paragraph was added to the user doc corresponding to each of them.
However, context help does not work for these options. I.e. when the focus is on one of this option, pressing F1 reports "No help available here".
This is due to 2 issues:

  • No help event was bound to these controls. Thus F1 does not opens the doc on the dedicated paragraph
  • No helpId was assigned to AdvancedPanelControls subpanel. Thus the default behaviour when the controls do not have associated help events (i.e. opening the doc on the advanced settings paragraph) was not executed either.

Description of how this pull request fixes the issue:

  1. Bound both controls to the corresponding help events
  2. Added helpId to AdvancedPanelControls subpanel.
  3. Rename the tag "ChromiumUIA" to "AdvancedSettingsChromiumUIA" to conform to the majority of other help tags formatting, I.e. combining panel name and option name. This is still possible because this option has not been released yet. On the contrary it is not recommended to change tag names once the doc was included in a stable release of NVDA.

Testing performed:

  • Tested point 2 during implementation when point 1 and 3 were not yet implemented to check that pressing F1 on these 2 options jumps to the main "Advanced settings" paragraph.
  • Once the three points were implemented, tested that F1 opens the user guide at the appropriate paragraph.

Known issues with pull request:

None

Change log entry:

None: this PR only fixes issues that have not yet been released.

@CyrilleB79
Copy link
Collaborator Author

Cc @feerrenrut, @LeonarddeR and @codeofdusk as authors of initial PR's and/or potential reviewers of future PRs.

In the future, please do not forget context help when new controls are added in the GUI.

@feerrenrut
Copy link
Contributor

Thanks @CyrilleB79, I have created a code review checklist to remind us of this and other similar forgotten tasks: #12037

@feerrenrut
Copy link
Contributor

Rename the tag "ChromiumUIA" to "AdvancedSettingsChromiumUIA" to conform to the majority of other help tags formatting, I.e. combining panel name and option name. This is still possible because this option has not been released yet. On the contrary it is not recommended to change tag names once the doc was included in a stable release of NVDA.

Hmm, while it's a loss of consistency I think it is better not to include the category in the tag. Since we can't / don't want to change the name of these tags, then moving settings to new sections will result in a very confusing tag. It would be more sensible if the tag does not include the section name. The main reason I can see for wanting to include the section name is to ensure that the tag it is unique. Instead of a tag naming scheme, I'd propose an automated test. There isn't much we can do about the ones already in place, however I would like to also note that some of the "advanced settings" are quite likely to eventually graduate to another settings panel.

@feerrenrut
Copy link
Contributor

Otherwise the changes look good.

@CyrilleB79
Copy link
Collaborator Author

Hmm, while it's a loss of consistency I think it is better not to include the category in the tag. Since we can't / don't want to change the name of these tags, then moving settings to new sections will result in a very confusing tag. It would be more sensible if the tag does not include the section name. The main reason I can see for wanting to include the section name is to ensure that the tag it is unique. Instead of a tag naming scheme, I'd propose an automated test. There isn't much we can do about the ones already in place, however I would like to also note that some of the "advanced settings" are quite likely to eventually graduate to another settings panel.

Yes, this makes sense. I have removed the category name from the tags of these 2 options that have not yet been included in any release.
Also re-tested the help link (F1) on the updated branch.

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 @CyrilleB79

@feerrenrut feerrenrut merged commit a73b481 into nvaccess:master Feb 8, 2021
@nvaccessAuto nvaccessAuto added this to the 2021.1 milestone Feb 8, 2021
@CyrilleB79 CyrilleB79 deleted the helpInAdvancedSettings branch July 14, 2022 13:18
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

3 participants