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

Improved Input Help Mode to Report Characters Typed by Key Combinations in Normal Mode #17629

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

Conversation

cary-rowen
Copy link
Contributor

Link to issue number:

Fixes #15891, #6621
Based on abandoned PR #15907
Most of this work has already been done by @Emil-18, I just picked it up.

As someone who frequently teaches beginners to familiarize themselves with NVDA keyboard shortcuts, I believe this PR addresses an important issue. It helps users quickly identify key combinations, which is often a first step for new learners.

Summary of the issue:

When the key combination isn't a script, input help doesn't report the resulting character when holding down modifier keys and pressing a character key. For example, instead of reporting a "bang" (!) in the English keyboard layout, it reports shift+1. There is also no easy way to determine if a key combination is an NVDA command or not. Users may have to listen to multiple modifier names (e.g., shift+shift+shift+1) only to find out that the combination isn’t an NVDA command.

Description of user facing changes

  • When a key combination would produce a character in normal input mode, the character is reported first, followed by the key combination.
  • If the key combination corresponds to an NVDA command, the behavior remains the same as before, i.e. the description of the command is reported.

Description of development approach

When the gesture doesn't correspond to a script, NVDA will retrieve the character corresponding to the pressed keys. This is achieved by creating a keyState list and setting the values for the pressed modifiers to -128 (as per NVDA’s implementation). This signals ToUnicodeEx that the key is pressed. The list is then sent to ToUnicodeEx, along with the keyboard layout for the focused window.
If ToUnicodeEx returns a negative value, indicating that a dead key was pressed, the process returns immediately. This prevents misleading behavior where users might assume a specific character would be typed, even though the key press only results in a dead key.
Next, NVDA checks if the alt key is pressed. In some cases, ToUnicodeEx returns the same character with or without the alt key. If alt is pressed, it is removed from the keyState list, and ToUnicodeEx is called again. If the two calls return the same character, the result is considered invalid. Characters are also marked as invalid if they are non-printable, a space, or if the Windows key is pressed. If ToUnicodeEx returns valid characters, these are reported.

Testing strategy:

I tested this using the Chinese keyboard layout, while the original author (@Emil-18) tested it in a Norwegian environment.

Known issues with pull request:

If a dead key character is typed in input help, and input help is exited without typing another character, the next typed character will include the dead key character.

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.

@coderabbitai summary

@cary-rowen cary-rowen requested a review from a team as a code owner January 16, 2025 14:39
@cary-rowen cary-rowen marked this pull request as draft January 16, 2025 14:41
Copy link
Collaborator

@LeonarddeR LeonarddeR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Contrary to #15907, I can certainly live with this as the original sequence is reported. I think code can be simpler, though.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My feedback from #15907 still stands. I'd rather not see this stuff being added to _handleInputHelp.
Idially,, we'd have something like a property on InputGesture called _nameForInputHelp. It should return self.displayName by default. Then, for KeyboardInPutGesture, the character name can be added upfront. It makes sense to have consistent behavior, so even when I'd be so weird to assign a script to the bang, I want the bang to be reported when pressing it, then shift+1, then the actual script.

states,
buffer,
ctypes.sizeof(buffer),
0x0,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be

Suggested change
0x0,
0x04

See a similar call in internal_keyDownEvent

@CyrilleB79
Copy link
Collaborator

I am testing on French (France) keyboard.

The design as is today is confusing because:

  • when a command is assigned to the gesture, we can hear each key pressed and then the effect, e.g. "NVDA+f12" and then "If pressed once, reports the current time. If pressed twice, reports the current date"
  • when a script is not assigned to the gesture, we can hear first the effect produced, i.e. the character being produced (e.g. "€"), and then each key pressed (e.g. "ctrl+alt+e").

This is not consistent and thus confusing in some cases, especially when no modifier is used:

  1. The most concerning case are number keys (top row of main keyboard), e.g. "& 1", "é 2", etc. Please not the specificity of this keyboard layout where the key is named "1", but you need to press shift+1 to type "1", whereas pressing only 1 types "&".
  2. But also the press after having pressed a dead key. E.g if I have pressed "^" dead key (at the right of "P"), if I then press "o" key, I can hear "ô o", that is with eSpeak "o circumflex o".

Also, I find that the experience with dead keys is a bit confusing. With the current version of this PR, if you first press a dead key (e.g. "^") and then a normal key that combines with it, e.g. "o", you get the dead key character (e.g. "^") reported after first press and the combined character (e.g. "ô") after the second press. I'd rather expect the non combined character (e.g. "o") to be reported after the second press. I do not expect input help to record a specific keyboard state between one press and the other.

Also, this is not new to this PR, but since more information is added, the case of all the numpad keys (digits or +-/) with numlock on is a bit hard to understand. Internally, numlock is handled as a modifier for +-/, but the from the user point of view, it should be the same, whether digits or operator keys are pressed.

Last but not least: I would have preferred the feature reporting the typed characters to be separate from input help. But I was in minority to think this way and even NV Access has triaged the issue to develop this feature in input help instead. So just mentioning here in case people would change their mind for any reason. But I will not insist anymore on this point.

@Emil-18
Copy link
Contributor

Emil-18 commented Jan 19, 2025

@cary-rowen Thank you for continuing this! I was unsure what to do with it because people seamed unhappy about the changes, and because the issue haddent been triaged. I am unable to test it, for what ever reason, the latest NVDA alpha crashes on my machine when I try to run it, and by extencion this pr.
The reason why I originally made it so it would only report the character when a key combination that contained modifiers was because the user would allready have pressed the modifiers, and thus known what their names are, making it unnessesery to report it an extra time.
What about this.

  • On one press, if the key combination produces a character, and is not bound to a script with a description, the character, not the key combination, is reported.
  • On 2 presses, the key names are always reported, regardless if a named script is bound to the gesture.

@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Jan 20, 2025
@@ -52,6 +52,9 @@ To use this feature, "allow NVDA to control the volume of other applications" mu
* Short versions of the most commonly used command line options have been added: `-d` for `--disable-addons` and `-n` for `--lang`.
Prefix matching on command line flags, e.g. using `--di` for `--disable-addons` is no longer supported. (#11644, @CyrilleB79)
* Microsoft Speech API version 5 and Microsoft Speech Platform voices now use WASAPI for audio output, which may improve the responsiveness of those voices. (#13284, @gexgd0419)
*Input help mode has been improved: (#placeholder, @Cary-rowen, @Emil-18)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
*Input help mode has been improved: (#placeholder, @Cary-rowen, @Emil-18)
*Input help mode has been improved: (#17629, @Cary-rowen, @Emil-18)

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.

Make it possible to quickly identify if a key combination is an NVDA command or not in input help
6 participants