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

Improves input help for keyboard #15907

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

Conversation

Emil-18
Copy link
Contributor

@Emil-18 Emil-18 commented Dec 12, 2023

Link to issue number:

fixes #15891, #6621

Summary of the issue:

When the key combination isn't a script, input help does not report the resulting character when holding down modifier keys and press a character key, e.g instead of reporting bang (!)in the english keyboard layout, it reports shift+1. There is also no way to know quickly if a key combination is a NVDA command or not, e.g the user hav to potentialy listen to modifier name+modifier name+ modifier name+main key name, only to find out that the key combination isn't a NVDA command

Description of user facing changes

When input help is on, if the key combination is a NVDA command, NVDA reports it as it always have. If not, but the key combination would have resulted in a typed character if input help was off, this character is reported. Else the modifier keys and the main key is reported as before, just with the + sign replaced with space, so the user can more quickly know that the key combination isn't a NVDA command

Description of development approach

When the gesture don't have a script, NVDA will get the character corresponding to the pressed keys. It does this by creating a keyState list, and set the values corresponding to the modifiers that are down according to NVDA to -128, wich will tell ToUnicodeEx that the key is down. It then sens this list to ToUnicodeEx, and also gives it the keyboard layout for the focused window. If ToUnicodeEx returns a negative value, wich means that a dead key character was pressed, it returns, as I personally think that had it reported the key, it may misleed users into thinking they would have written that character when pressing the key. It then checks if alt is pressed, as when alt is pressed, ToUnicodeEx sometimes gives the same character as if alt was not pressed. If it is pressed, alt is then removed from the key states and ToUnicodeEx is called again. If the two calls gives the same characters, they are counted as invalid. They are also counted as invalid if they are not printable, are space, or if the windows key is pressed. If ToUnicodeEx gave characters and all the characters are valid, these are reported. If not, + are replaced with space in textList and reported as normal.

Testing strategy:

I have tested with both the english and norwegian keyboard layouts, on a laptop keyboard without numbpad. Everything works as expected, except for the issue described below

Known issues with pull request:

If typing a dead key character in input help, and input help is exited without typing another character, the next typed character will be typed with the ded 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.

@AppVeyorBot
Copy link

See test results for failed build of commit a037005b74

@CyrilleB79
Copy link
Collaborator

Personally, I was not a big fan of this change, but it seems that it was expected by many people; so that's probably the way to go.

Testing this PR, this seems to work quite well, except the small issue with the pending dead key when exiting input help mode as already described in known issues; if no solution is found, this issue is probably acceptable though.

You have done the choice not to report dead keys when pressed in input help mode, and to report the modified letter on next press instead. Personally, I would have preferred the other solution, i.e. report dead key immediately and do not modify the next character. But that's just a matter of preference and I have no strong argument in favor of one solution. Both solutions are acceptable. Actually, Jaws behaves like this PR; on the opposite, Narrator reports the dead key immediately.

@amirmahdifard
Copy link

@Emil-18 actually, i was having an annoying problem with the input help that i was planning to report it but sadly i didn't have enough time to do it. when input help is onn (personally i use it for teaching to my students and any key presses shouldn't have any effect but only speech about what is that key), wich is ok in all other cases and i don't have any problem, for example, when input help is turned on, any toggle keys i press such as capslock, scrole lock, nvda reports them and they don't have any effect, but i have problem with only the numlock key. when input help is on, pressing the numlock button, toggles it between numlock on and numlock off as normal, which shouldn't be the case. can you possibly resolve it? thanks.

@LeonarddeR
Copy link
Collaborator

@CyrilleB79 wrote:

Personally, I was not a big fan of this change

I tend to agree.

You have done the choice not to report dead keys when pressed in input help mode, and to report the modified letter on next press instead. Personally, I would have preferred the other solution, i.e. report dead key immediately and do not modify the next character.

That would also be my preference.

I think I can live with this pr when it would report both the key sequence and the result. For example, report shift+1, bang.

I'd also rather see the magic happen in a helper function, rather than in _handleInputHelp itself. May be it even belongs in keyboardHandler. That already has logic to detect typed cahracters, may be that can be reused (not duplicated)?

@AppVeyorBot
Copy link

See test results for failed build of commit e5f2853305

@Emil-18
Copy link
Contributor Author

Emil-18 commented Dec 12, 2023

If I don't allow dead keys through like I do now, and the user presses a dead key character, and then turns on input help, it will continue to report characters with the dead key until input help is turned off.

@Emil-18
Copy link
Contributor Author

Emil-18 commented Dec 12, 2023

@amirmahdifard It is done on purpose. See #4226. I don't think it is possible to fix it

@AppVeyorBot
Copy link

See test results for failed build of commit 4e6b95b85c

@AppVeyorBot
Copy link

See test results for failed build of commit ce5850eb18

if isinstance(gesture, KeyboardInputGesture) and not script or not script.__doc__:
threadID = api.getFocusObject().windowThreadID
keyboardLayout = ctypes.windll.user32.GetKeyboardLayout(threadID)
buffer = ctypes.create_unicode_buffer(5)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why 5, could you use a constant here, or at least a comment.

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.

What if you put 0x4 instead? Does it fix the issue of dead keys having consequences after you exit help mode (at least for Windows 10 1607 or higher)? For more details, see other usages of ToUnicodeEx in NVDA's code, as well as MSDN's ToUnicodeEx documentation.

Suggested change
0x0,
0x4,

states[i] = -128 # We tell ToUnicodeEx that the modifier is down, even tho it isn't according to Windows
else:
states[i] = ctypes.windll.user32.GetKeyState(i)
res = ctypes.windll.user32.ToUnicodeEx(
Copy link
Collaborator

Choose a reason for hiding this comment

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

It may be interesting to write a wrapper of ToUnicodeEx in winAPI\winUser\functions.py for a cleaner abstraction of windows API and a handy tool for more general usage; this function could return directly the string (buffer's value). Do not put it in winUser.py, which should not accept new functions anymore.

Note: I am personally interested in using ToUnicodeEx to know which character is typed when pressing shift+1for another future unrelated contribution.

@Emil-18
Copy link
Contributor Author

Emil-18 commented Dec 17, 2023

@CyrilleB79 If I use 0x4, to my knolage, it is impossible to get information about characters that you have to type a dead key character first, and then the second character to produce. For example, if I type a dead key character, and then e, it would always have reported e regardless if e was another character when the dead key character was typed first. The opisit is also true if I type a dead key character and then turn on input help. The reason why I used 5 is because it is just used in the internalKeyDownEvent function in keyboardHandler in the same way. And should I continue to work on this as #15891 is blocked?

@codeofdusk
Copy link
Contributor

Personally, I was not a big fan of this change

Agree!

@lukaszgo1
Copy link
Contributor

Personally, I was not a big fan of this change

I tend to agree.

Could people who are against this provide some reasoning? For me this makes input help mode closest to the sighted person experience, where they can just glance at a given key to know what additional character can be typed with it.

I think I can live with this pr when it would report both the key sequence and the result. For example, report shift+1, bang.

If this is implemented I'd suggest to revert the order - user knows what the pressed keys do separately, so the new info i.e. the actual typed character should be presented first.

@cary-rowen
Copy link
Contributor

@Emil-18
could you resolve the conflict?

@AppVeyorBot
Copy link

See test results for failed build of commit 9195eb9c10

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.

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