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.

Sign up for GitHub

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

Jump to bottom

readline: fix key name for function keys combined with modifiers #35268

Merged
merged 1 commit into from
Sep 21, 2020
Merged

readline: fix key name for function keys combined with modifiers #35268

merged 1 commit into from
Sep 21, 2020

Conversation

elie-g
Copy link
Contributor

@elie-g elie-g commented Sep 19, 2020 •
edited
Loading

Added proper key names for function key (e.g. F4) combined with modifiers (e.g. Ctrl).

I also added support for:

  • Alt + Enter
  • Alt + KeypadEnter
  • Alt + Backspace

Fixes #35251

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the readline Issues and PRs related to the built-in readline module. label Sep 19, 2020
@elie-g elie-g mentioned this pull request Sep 19, 2020
Open
@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 20, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 20, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/33173/

@elie-g @Trott
readline: fix key name for function keys with modifiers
17ebd46 
Fixes: nodejs#35251

PR-URL: nodejs#35268
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott Trott force-pushed the fix-emitted-key-name branch from 4002565 to 17ebd46 Compare September 21, 2020 14:44
@Trott Trott merged commit 17ebd46 into nodejs:master Sep 21, 2020
@Trott
Copy link
Member

Trott commented Sep 21, 2020

Landed in 17ebd46

@Trott
Copy link
Member

Trott commented Sep 21, 2020

Thanks for the contribution! 🎉

@elie-g elie-g deleted the fix-emitted-key-name branch September 21, 2020 14:48
@elie-g
Copy link
Contributor Author

elie-g commented Sep 21, 2020 •
edited
Loading

@Trott @addaleax @joyeecheung
Is it normal that key.name can have both undefined (e.g. when you press the period . key) and the string "undefined" as value (see here) ?

PS: I didn't want to open an new issue just for this, I'm not even sure if it's really an issue.

@addaleax
Copy link
Member

@DrunkenPoney That doesn’t sound intentional to me, no. Feel free to fix this if you like 👍

ruyadorno pushed a commit that referenced this pull request Sep 21, 2020
@elie-g @ruyadorno
readline: fix key name for function keys with modifiers
4f4faa8 
Fixes: #35251

PR-URL: #35268
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Sep 21, 2020
Merged
4 tasks
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
@elie-g @joesepi
readline: fix key name for function keys with modifiers
2203dc2 
Fixes: nodejs#35251

PR-URL: nodejs#35268
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung approved these changes

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott approved these changes

Assignees
No one assigned
Labels
readline Issues and PRs related to the built-in readline module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Readline: Function keys (e.g. F1) return "undefined" key name
5 participants
@elie-g @nodejs-github-bot @Trott @addaleax @joyeecheung