Improve shortcut key reporting by CyrilleB79 · Pull Request #14900 · nvaccess/nvda

CyrilleB79 · 2023-05-03T16:20:22Z

Link to issue number:

Summary of the issue:

When reported by speech, some shortcut key or combination cannot be understood correctly in the following cases:

A punctuation may not be reported due to symbol level being too low, e.g. "alt+." reported as "alt plus" instead of "alt plus dot" in Outlook.
A character reported as a word instead of being reported as a character. E.g. in French, the letter "à" (a grave) is a word; in MS Word, the shortcut "alt+à" is reported as "alt plus a" instead of "alt plus a grave". Same for "Y" which is a word in French and the name of the letter is "i grec" whereas the word just reads "i" ("ee").

Description of user facing changes

Shortcut key reporting will be improved in some cases. This applies to on-demand reporting (shift+numpad2) as well as when "Report Object Shortcut Keys" option is checked in Object presentation settings.

The cases where it should be improved are:

punctuation key, e.g. "." (dot)
letter keys when the name of the letter is not spoken the same way as the equivalent one-letter word, e.g. "à" or "y" in French.

Braille reporting should remain unchanged.

Description of development approach

Generate a speech sequence to report shortcut keys from the shortcut key string provided by the API. This shortcut key string may contain one or two shortcuts. When there are two shortcuts, they are separated by two spaces.
Each shortcut is split into its component keys.
If a key is a punctuation, symbol reporting processing is used so that the punctuation can be reported no matter the punctuation level.
If a key is a single character, CharacterModeCommand is passed to the synth so that the name of the letter is reported and not a possible one-letter word.

Note: all the processing to split shortcut keys is in speech/speech.py as private functions. Reviewer: should they remain here or would they fit better elsewhere?

Testing strategy:

Manual tests for the following shortcuts:

Sequential shortcut mixing comma and space separators: Word (French): "Alt, F, L, Y 2"
Two shortcuts for the same control, one sequential and another simultaneous: Word (French): "Alt, É, É Ctrl+F"
Two shortcuts for the same control, one sequential and another simultaneous using the "+" (plus) key: Word (French): "Alt, L, 6 Ctrl+Maj++"
Letter with diacritic alone: NVDA menu -> Help -> About (French): "à" from "À propos..."
Letter with diacritic combined with modifier: Outlook - Message writing: "Alt+à"
Shortcut with punctuation alone: "?" used as help menu in 6pad++.
Shortcut with punctuation combined with modifier: Outlook's message (writing): "Alt+."

Tested the shortcut "Touche de logo Windows + Plus (+)" which is from Windows Magnifier. This shortcut has its shortcut keys separated with " + " (with space around the "plus" symbol) and uses the "+" symbol in a key label. It was tested as follows in the console:

>>> from speech.shortcutKeys import getKeyboardShortcutsSpeech
>>> getKeyboardShortcutsSpeech('Touche de logo Windows + Plus (+)')
['Touche de logo Windows + Plus (+)']

Direct test on the magnifier cannot be done running from source since the Magnifier UI is not accessible when running from source (maybe UI Access is required for it?)

Tested also with various voices (Eloquence, OneCore, SAPI5 (Windows), eSpeak.

System tests have been updated to pass since the number of spaces in shortcut keys has changed. It is due to the use of CharacterModeCommand in the speech sequence which adds some extra spaces when this sequence is converted to string.

Known issues with pull request:

Seen with OneCore that "é" in French is not reported as "e accent aigu" ("e acute") although the CharacterModeCommand is used.
This PR does not fix this which is more a OneCore issue.
Shortcut keys as reported in the speech viewer may have some extra spaces with respect to what was previously reported. It is due to the usage of CharacterModeCommand in the speech sequence which converts to extra space when this sequence is converted to string for the speech viewer.

Change log entries:

Bug fixes
Reporting of object shortcut keys has been improved. (#10807)

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
Security precautions taken.

AppVeyorBot · 2023-05-03T17:07:46Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/44840bamrd4n0pkf/artifacts/output/nvda_snapshot_pr14900-28188,7b192d83.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.8,
INSTALL_END 0.8,
BUILD_START 0.0,
BUILD_END 23.6,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 21.5,
FINISH_END 0.1

See test results for failed build of commit 7b192d8345

cary-rowen · 2023-05-03T18:17:25Z

Hi @CyrilleB79
thank you for your efforts.

Can the fix of this PR cover the situation I described below:
If I check "Report object shortcut keys", NVDA will automatically guess the shortcuts for some menu items in the Chinese interface.
For example, on the "Recycle Bin" icon on the desktop, there is an item called "Empty Recycle Bin" in the right-click menu.

If this item does not have an artificial shortcut key specified using &, then NVDA will automatically guess that its shortcut key is the first letter "E" of the first word.
However, for the Chinese interface, NVDA will regard the first Chinese character "清" of the five "清空回收站" as a shortcut.
This is weird and doesn't help.

Thanks

CyrilleB79 · 2023-05-03T21:23:42Z

Hi @CyrilleB79 thank you for your efforts.

Can the fix of this PR cover the situation I described below: If I check "Report object shortcut keys", NVDA will automatically guess the shortcuts for some menu items in the Chinese interface. For example, on the "Recycle Bin" icon on the desktop, there is an item called "Empty Recycle Bin" in the right-click menu.

If this item does not have an artificial shortcut key specified using &, then NVDA will automatically guess that its shortcut key is the first letter "E" of the first word. However, for the Chinese interface, NVDA will regard the first Chinese character "清" of the five "清空回收站" as a shortcut. This is weird and doesn't help.

In languages with Latin alphabet, or maybe also other alphabets, this default shortcuts may be useful. But I understand that in the case of languages using ideograms, this feature do not bring anything useful.

Unfortunately, this PR does not bring anything new regarding the issue you describe. That's not NVDA guessing the shortcut with the first letter/character of the text; I think that it's Windows who provides this information via IAccessible interface. If you test with Narrator, you will have the same issue. You should thus report it to Microsoft.

cary-rowen · 2023-05-04T01:12:49Z

@CyrilleB79
Thanks for your answer, it's clear now.

AppVeyorBot · 2023-05-05T15:21:46Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/cqg8bnu120wo6gm9/artifacts/output/nvda_snapshot_pr14900-28195,f7535cca.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.8,
INSTALL_END 0.8,
BUILD_START 0.0,
BUILD_END 23.2,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 21.6,
FINISH_END 4.9

See test results for failed build of commit f7535ccad9

michaelDCurran

In general this approach looks okay to me.
However, I found _splitShortcut a little confusing. If '+' (with or without whitespace is the only separator, You could pretty much get rid of the function, just split on '+', process each key with getKeySpeech (stripping any whitespace) and join it again with ' + '.
However, as for _splitSequentialShortcut: I found this very hard to follow. At very least could you provide a dostring explaining exactly what it does? Then perhaps I may have morefeedback on how that one could be improved.

CyrilleB79 · 2023-05-09T12:45:14Z

@michaelDCurran, @seanbudd, I have added more documentation and slightly refactored the code for clarity. Also added a fix for corner case when the key "+" (plus) is used.

I plan to add unit tests to capture all cases listed in the "Tests" section of the initial description. Let me know once you agree on all the functions signatures so that I can add it.

@michaelDCurran youwrite:

However, I found _splitShortcut a little confusing. If '+' (with or without whitespace is the only separator, You could pretty much get rid of the function, just split on '+', process each key with getKeySpeech (stripping any whitespace) and join it again with ' + '.

No, this does not work when the name of a key contains the "+" character. E.g. "Touche de logo Windows + Plus (+)" (Zoom in button in Windows Magnifier, French version). This shortcut uses " + " as a separator ("plus" with spaces around) and uses "plus" between parenthesis in the key name.

However, as for _splitSequentialShortcut: I found this very hard to follow. At very least could you provide a dostring explaining exactly what it does? Then perhaps I may have morefeedback on how that one could be improved.

I have added the docstring. I do not add any further comment here so that I can check if the code is clear enough or not. According to your feedback, I may add extra comments in the code if needed or change the algorithm (if you or I find another one). In this specific case, adding unit tests will probably not add more clarification since I have put an example in the docstring.

… to review.

seanbudd · 2023-05-31T05:01:15Z

+	return seqOut
+
+
+def _getKeyboardShortcutSpeech(keyboardShortcut: str) -> SpeechSequence:


it would be great if simple unit tests were added to these helper functions, just to document what they do.

seanbudd

Thanks @CyrilleB79, this LGTM

seanbudd · 2023-06-01T03:38:36Z

Note: all the processing to split shortcut keys is in speech/speech.py as private functions. Reviewer: should they remain here or would they fit better elsewhere?

Hi sorry, just saw this note. I agree it's probably worth moving all the added functions to their own submodule, e.g. speech.shortcutKeys

CyrilleB79 · 2023-06-01T15:22:12Z

Note: all the processing to split shortcut keys is in speech/speech.py as private functions. Reviewer: should they remain here or would they fit better elsewhere?

Hi sorry, just saw this note. I agree it's probably worth moving all the added functions to their own submodule, e.g. speech.shortcutKeys

Public functions speakKeyboardShortcuts and getKeyboardShortcutsSpeech are made with the same naming convention than other ones found in speech.speech.py, e.g. speakObject and getObjectSpeech. Would it make sense to keep them in speech.speech.py or should I also move them in the new file speech.shortcutKeys.py?
Should I move only private functions (prefixed with "_") instead?

Moreover, in _getKeySpeech, I call speech.speech.getCurrentLanguage which is defined in speech.speech.py. Moving this function as is in a separate file may cause cyclic import issues.

An alternative may be to pass the language as parameter, but in this case, I need to pass it as parameter in _getKeySpeech and _getKeyboardShortcutSpeech. This seems a bit ugly.

What do you think?

If you expect also the public functions to be moved in speech.shortcutKeys.py, more functions would need to be passed the language as parameter, which seems worse.

seanbudd · 2023-06-02T00:04:24Z

Public functions speakKeyboardShortcuts and getKeyboardShortcutsSpeech are made with the same naming convention than other ones found in speech.speech.py, e.g. speakObject and getObjectSpeech. Would it make sense to keep them in speech.speech.py or should I also move them in the new file speech.shortcutKeys.py?
Should I move only private functions (prefixed with "_") instead?

Thanks, I think moving all new functions would be ideal. The goal is to move functions out of speech.speech rather than add to it. e.g. a new submodule of speech.NVDAObjects would be ideal.

Moreover, in _getKeySpeech, I call speech.speech.getCurrentLanguage which is defined in speech.speech.py. Moving this function as is in a separate file may cause cyclic import issues.

You should be able to lazy import this in _getKeySpeech

CyrilleB79 · 2023-06-02T07:41:36Z

Thanks, I think moving all new functions would be ideal. The goal is to move functions out of speech.speech rather than add to it. e.g. a new submodule of speech.NVDAObjects would be ideal.

Done in 5ff17f7. While at it, I have also added one more unit test on getKeyboardShortcutsSpeech to capture the case when the shortcut key string contains two shortcuts.

Moreover, in _getKeySpeech, I call speech.speech.getCurrentLanguage which is defined in speech.speech.py. Moving this function as is in a separate file may cause cyclic import issues.

You should be able to lazy import this in _getKeySpeech

Yes. I have done this needing two lazy imports. IMO, using lazy imports show a suboptimal code organization. But since splitting speech.py would be out-of-scope of this PR (and I do not intend to do it), let's keep them.

seanbudd

Thanks @CyrilleB79

This reverts commit e6612e9.

This reverts commit e6612e9. Fixes #15233

Fixes #15233 Summary of the issue: Unfortunately pr #14900 causes Code Factory Eloquence to spell out menu items with shortcut keys, such as in the NVDA menu. Description of user facing changes Menu items are no longer spelled out when focusing menu items when using the Code Factory Eloquence driver. Description of development approach Although the bug is technically in the Code Factory driver I think - not handlin character mode command correctly when embedded in other speech, PR 14900 is the first known case that exercises the bug directly. Because 2023.2 is not an add-on breaking release, we are reverting pr #14900. A replacement pr for 2023.3 can be opened that avoids affecting Code Factory Eloquence if at all possible, or a new copy of the reverted pr can be opened for NVDA 2024.1.

michaelDCurran · 2023-08-01T03:19:26Z

This PR has been reverted in #15237

Unfortunately this PR causes Code Factory Eloquence to spell out menu items with shortcut keys, such as in the NVDA menu.
Although the bug is technically in the Code Factory driver I think - not handlin character mode command correctly when embedded in other speech,
this PR is the first known case that exercises the bug directly.
Because 2023.2 is not an add-on breaking release, we are reverting pr #14900.
A replacement pr for 2023.3 can be opened that avoids affecting Code Factory Eloquence if at all possible, or a new copy of the reverted pr can be opened for NVDA 2024.1.

CyrilleB79 · 2023-08-01T09:55:14Z

In anticipation of the reintroduction of this PR in NVDA 2024.1, the following code snippet executed in the console should allow Code Factory developers to test if their driver correctly supports CharacterModeCommand.

import core
from speech import speak
from speech.commands import CharacterModeCommand

# The following lines executed in the console may not pronounce the sentence correctly due to
# a possible confusion between "a" spoken as a character and "a" spoken as a word.
seq1 = ['The letter ', 'a', 'spoken as a character should not be pronounced as the word a spoken as a whole word.']
core.callLater(1000, lambda: speech.speak(seq1)) and None

# The following lines executed in the console should pronounce the sentence correctly due to
# explicit indication of when "a" should be poken as a character (letter).
seq2 = ['The letter ', CharacterModeCommand(True), 'a', CharacterModeCommand(False), 'spoken as a character should not be pronounced as the word a spoken as a whole word.']
core.callLater(1000, lambda: speech.speak(seq2)) and None

@michaelDCurran have you had a contact with CodeFactory regarding this issue? Do they confirm that the bug is in their current driver?

CyrilleB79 · 2023-09-05T08:35:15Z

@seanbudd, @michaelDCurran
Now that we have entered the 2024.1 dev cycle, may I reintroduce the changes of this PR? Should I do it now or should I wait later (and when) in the dev cycle?
If I introduce it now, it will break the UX for alpha users using Code Factory synthesizer; but it will provide Code Factory devs early a version on which they can work to fix the issue.
Thanks.

seanbudd · 2023-09-05T08:36:16Z

Yep, opening a PR for this would be welcome.

…ccess#15237)" This reverts commit e15fe8a.

Fixes #10807 Reintroduce the changes of PR #14900 that were reverted in #15237. Summary of the issue: See #14900 for the initial issue. #14900 had to be reverted since it caused any speech sequence containing a character mode command to spell the whole speech sequence with Code Factory driver. Although the issue is in the driver and not NVDA, it has been decided to wait for an API-breaking release to issue this fix so that Code Factory synth users be not impacted by this bug. Description of user facing changes Same as #14900, that is: Shortcut key reporting will be improved in some cases. This applies to on-demand reporting (shift+numpad2) as well as when "Report Object Shortcut Keys" option is checked in Object presentation settings. The cases where it should be improved are: punctuation key, e.g. "." (dot) letter keys when the name of the letter is not spoken the same way as the equivalent one-letter word, e.g. "à" or "y" in French. Braille reporting should remain unchanged. Description of development approach Revert #15237 to reintroduce the changes in #14900 and remove change log modifications.

…5572) Closes #15566 Fix-up of #14900 / #15373. Summary of the issue: In #14900 / #15373., spelling mode was used (CharacterModeCommand) no matter the state of the "Use spelling functionality" option. The use spelling functionality exists because some old SAPI synth do not support spelling mode correctly. For them, this option needs to be honored. Description of user facing changes Use spelling functionality option will now be honored when reporting shortcuts. Description of development approach See logic in the code. If use spelling functionality option is disabled, the key is just reported as is.

Improve shortcut key reporting

aa32f5c

CyrilleB79 added 2 commits May 5, 2023 10:55

Merge branch 'master' into speakShortcut

17b9bcc

Code improvement

abea825

Fix system tests

701f4ac

CyrilleB79 marked this pull request as ready for review May 7, 2023 14:55

CyrilleB79 requested a review from a team as a code owner May 7, 2023 14:55

CyrilleB79 requested a review from seanbudd May 7, 2023 14:55

michaelDCurran requested changes May 8, 2023

View reviewed changes

seanbudd reviewed May 9, 2023

View reviewed changes

Comment thread source/speech/speech.py Outdated

CyrilleB79 requested review from michaelDCurran and seanbudd May 9, 2023 12:46

Fixed corner case and refactor, add comments and type hints according…

5daa259

… to review.

CyrilleB79 force-pushed the speakShortcut branch from 449e876 to 5daa259 Compare May 9, 2023 14:33

seanbudd reviewed May 31, 2023

View reviewed changes

CyrilleB79 added 2 commits May 31, 2023 10:03

Merge branch 'master' into speakShortcut

4076afb

Unit tests

807750e

seanbudd approved these changes Jun 1, 2023

View reviewed changes

update changes

734a229

Separate shortcut key speaking in a new file

5ff17f7

CyrilleB79 requested a review from seanbudd June 2, 2023 07:41

seanbudd approved these changes Jun 5, 2023

View reviewed changes

seanbudd merged commit e6612e9 into nvaccess:master Jun 5, 2023

nvaccessAuto added this to the 2023.2 milestone Jun 5, 2023

CyrilleB79 deleted the speakShortcut branch June 5, 2023 07:03

michaelDCurran added a commit that referenced this pull request Jul 28, 2023

Revert "Improve shortcut key reporting (#14900)"

8bf07a3

This reverts commit e6612e9.

michaelDCurran mentioned this pull request Aug 1, 2023

Revert "Improve shortcut key reporting (#14900)" #15237

Merged

6 tasks

michaelDCurran added a commit that referenced this pull request Aug 1, 2023

Revert "Improve shortcut key reporting (#14900)"

46f62df

This reverts commit e6612e9. Fixes #15233

CyrilleB79 mentioned this pull request Aug 21, 2023

Make NVDA announce shortcut letters by letter name, not as though in a sentence #15301

Open

CyrilleB79 added a commit to CyrilleB79/nvda that referenced this pull request Sep 5, 2023

Revert "Revert "Improve shortcut key reporting (nvaccess#14900)" (nva…

7f8a256

…ccess#15237)" This reverts commit e15fe8a.

CyrilleB79 mentioned this pull request Sep 5, 2023

Reintroduce Improve shortcut key reporting (#14900) #15373

Merged

6 tasks

CyrilleB79 added a commit to CyrilleB79/nvda that referenced this pull request Sep 5, 2023

Revert "Revert "Improve shortcut key reporting (nvaccess#14900)" (nva…

4228022

…ccess#15237)" This reverts commit e15fe8a.

Mohamed00 mentioned this pull request Oct 3, 2023

Updated shortcut key reporting doesn't respect use spelling functionality option #15566

Closed

CyrilleB79 mentioned this pull request Oct 3, 2023

Honor useSpellingFunctionality parameter when reporting shortcuts #15572

Merged

5 tasks

seanbudd mentioned this pull request Nov 10, 2023

NVDA 2023.3 is not reading certain lists appropriately across Microsoft office. #15743

Closed

CyrilleB79 mentioned this pull request Dec 19, 2023

Vocalizer spells wortds in the NVDA menu instead of saying them again. #15940

Closed

		return seqOut


		def _getKeyboardShortcutSpeech(keyboardShortcut: str) -> SpeechSequence:

Uh oh!

Conversation

CyrilleB79 commented May 3, 2023 • 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 user facing changes

Description of development approach

Testing strategy:

Known issues with pull request:

Change log entries:

Code Review Checklist:

Uh oh!

AppVeyorBot commented May 3, 2023

Uh oh!

cary-rowen commented May 3, 2023

Uh oh!

CyrilleB79 commented May 3, 2023

Uh oh!

cary-rowen commented May 4, 2023

Uh oh!

AppVeyorBot commented May 5, 2023

Uh oh!

michaelDCurran left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CyrilleB79 commented May 9, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seanbudd May 31, 2023

Choose a reason for hiding this comment

Uh oh!

seanbudd left a comment

Choose a reason for hiding this comment

Uh oh!

seanbudd commented Jun 1, 2023

Uh oh!

CyrilleB79 commented Jun 1, 2023

Uh oh!

seanbudd commented Jun 2, 2023

Uh oh!

CyrilleB79 commented Jun 2, 2023

Uh oh!

seanbudd left a comment

Choose a reason for hiding this comment

Uh oh!

michaelDCurran commented Aug 1, 2023

Uh oh!

CyrilleB79 commented Aug 1, 2023

Uh oh!

CyrilleB79 commented Sep 5, 2023

Uh oh!

seanbudd commented Sep 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

CyrilleB79 commented May 3, 2023 •

edited

Loading