Skip to content

Detach disabling "talk" speech mode warning dialog#15960

Merged
seanbudd merged 7 commits into
nvaccess:betafrom
hwf1324:Related-15894
Dec 27, 2023
Merged

Detach disabling "talk" speech mode warning dialog#15960
seanbudd merged 7 commits into
nvaccess:betafrom
hwf1324:Related-15894

Conversation

@hwf1324

@hwf1324 hwf1324 commented Dec 24, 2023

Copy link
Copy Markdown
Contributor

Link to issue number:

Related #15894

Summary of the issue:

Disable the "talk" speech mode warning dialog box can be completely separated for judgment, as the status of this option does not need to be guaranteed when determining the settings

Description of user facing changes

When "talk" is deselected, a warning dialog box pops up. If "no" is chosen, then "talk" is set back to the selected state

Description of development approach

Move the "talk" speech mode warning dialog to the method bound to the wx.EVT_CHECKLISTBOX event.

Testing strategy:

  1. Open the speech settings panel
  2. Deselect the "talk" speech mode in loop speech mode

Known issues with pull request:

Code Review Checklist:

  • Documentation:
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • API is compatible with existing add-ons.
  • Security precautions taken.

@hwf1324 hwf1324 requested a review from a team as a code owner December 24, 2023 12:00
@hwf1324 hwf1324 requested review from seanbudd and removed request for a team December 24, 2023 12:00
@hwf1324 hwf1324 changed the title Detach disabling "talk" voice mode warning dialog Detach disabling "talk" speech mode warning dialog Dec 24, 2023
@hwf1324

hwf1324 commented Dec 24, 2023

Copy link
Copy Markdown
Contributor Author

This PR does not require a change log

@lukaszgo1

Copy link
Copy Markdown
Contributor

Given discussion in #15894 especially #15894 (comment) this was quite unpopular with users. If you believe that these arguments are not valid please explain why. Otherwise I suggest to close this.

@hwf1324

hwf1324 commented Dec 24, 2023

Copy link
Copy Markdown
Contributor Author

Given discussion in #15894 especially #15894 (comment) this was quite unpopular with users. If you believe that these arguments are not valid please explain why. Otherwise I suggest to close this.

No, this will not trigger warning dialog boxes frequently.
This will only trigger the warning dialog when the "talk" speech mode is canceled.
The judgment for selecting two options is still made before saving.
I believe users will not frequently cancel/select an option

@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit 426cb62a01

@lukaszgo1

Copy link
Copy Markdown
Contributor

I believe we should be aiming for consistency here, i.e. all warnings appear at the same time that is when closing the dialog. Lets wait for more feedback on this PR.

@CyrilleB79

Copy link
Copy Markdown
Contributor

On the contrary, I believe that we should issue warnings / error as soon as possible.

It was not possible for the "select at least two items" use case, since a standard way to select two items may be transitioning through a state where only one item is selected.

On the contrary, as soon as a user deselects "Talk":

  • either it was unintended, in which case the warning is more than useful
  • or it's intended, but we have already agreed that the use case is so rare that the warning is acceptable in this case

In conclusion, I am in favor of the changes in this PR.

@seanbudd seanbudd added this to the 2024.1 milestone Dec 26, 2023
@seanbudd seanbudd added the blocked/needs-product-decision A product decision needs to be made. Decisions about NVDA UX or supported use-cases. label Dec 27, 2023
Comment thread source/gui/settingsDialogs.py Outdated
@seanbudd

Copy link
Copy Markdown
Member

if we don't want users to deselect talk I think we should remove it from the user accessible options entirely and forcibly include it internally. I think we initially included it in the settings as an earlier goal was to ensure either "on-demand" or "talk" was included, not just "talk".

@lukaszgo1

Copy link
Copy Markdown
Contributor

if we don't want users to deselect talk I think we should remove it from the user accessible options entirely and forcibly include it internally. I think we initially included it in the settings as an earlier goal was to ensure either "on-demand" or "talk" was included, not just "talk".

From the discussion on #15806 and #15873 the goal was to allow users to deselect 'talk', since this may be potentially useful for magnification users, but make sure this is not done by mistake. Frankly given how much time has been spent discussing if it should be de-selectable or not I'd suggest not to spent more time on this i.e. merge this PR, as it improves the behavior. IMO the UX can be reconsidered if one of the following happens:

  1. We receive complains from users who disabled 'talk' mode by mistake despite the warning or
  2. We receive a lot of reports about warning being annoying from people who intentionally want to disable 'talk' mode.

@seanbudd

Copy link
Copy Markdown
Member

Thanks for the reminder and explainer @lukaszgo1

@seanbudd seanbudd merged commit e4a2474 into nvaccess:beta Dec 27, 2023
@hwf1324 hwf1324 deleted the Related-15894 branch December 29, 2023 14:36
seanbudd pushed a commit that referenced this pull request Jan 7, 2024
Fixes #16005.

Summary of the issue:
When enabling/disabling checkboxes in the speech modes list, under voice settings, the new status was not reported by speech or Braille.

Description of user facing changes
Now user hears "enabled" or "disabled", and gets his Braille display refreshed.

Description of development approach
PR #15960 has introduced a new handler of EVT_CHECKLISTBOX event for speechModesList.

But this latter is an nvdaControls.CustomCheckListBox, that already defines an handler for that event, to fix specifically this accessibility issue of wx.

So I simply put evt.Skip() at the beginning of new handler, to propagate event to custom control handler.
Adriani90 pushed a commit to Adriani90/nvda that referenced this pull request Mar 13, 2024
Related nvaccess#15894

Summary of the issue:
Disable the "talk" speech mode warning dialog box can be completely separated for judgment, as the status of this option does not need to be guaranteed when determining the settings

Description of user facing changes
When "talk" is deselected, a warning dialog box pops up. If "no" is chosen, then "talk" is set back to the selected state

Description of development approach
Move the "talk" speech mode warning dialog to the method bound to the wx.EVT_CHECKLISTBOX event.
Adriani90 pushed a commit to Adriani90/nvda that referenced this pull request Mar 13, 2024
Fixes nvaccess#16005.

Summary of the issue:
When enabling/disabling checkboxes in the speech modes list, under voice settings, the new status was not reported by speech or Braille.

Description of user facing changes
Now user hears "enabled" or "disabled", and gets his Braille display refreshed.

Description of development approach
PR nvaccess#15960 has introduced a new handler of EVT_CHECKLISTBOX event for speechModesList.

But this latter is an nvdaControls.CustomCheckListBox, that already defines an handler for that event, to fix specifically this accessibility issue of wx.

So I simply put evt.Skip() at the beginning of new handler, to propagate event to custom control handler.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blocked/needs-product-decision A product decision needs to be made. Decisions about NVDA UX or supported use-cases.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants