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

Readline keypress event is triggered only after pressing Esc key 3 times #7379

Closed
sindresorhus opened this issue Jun 23, 2016 · 12 comments
Closed
Labels
confirmed-bug Issues with confirmed bugs. readline Issues and PRs related to the built-in readline module.

Comments

@sindresorhus
Copy link

  • Version: 6.2.2 and 4.4.5
  • Platform: macOS 10.11.5
  • Subsystem: readline

Test case

const readline = require('readline');
readline.emitKeypressEvents(process.stdin);
process.stdin.setRawMode(true);
process.stdin.on('keypress', console.log);

Expected

After pressing Esc once, I would get this output:

undefined { sequence: '\u001b',
  name: 'escape',
  ctrl: false,
  meta: true,
  shift: false }

Actual

After pressing Esc three times, I get this output:

undefined { sequence: '\u001b\u001b\u001b',
  name: 'escape',
  ctrl: false,
  meta: true,
  shift: false }

It works fine on 0.10.44 and 0.12.13. After pressing Esc once:

{ name: 'escape',
  ctrl: false,
  meta: false,
  shift: false,
  sequence: '\u001b' }

Also notice that meta is false here.

sindresorhus added a commit to sindresorhus/emoj that referenced this issue Jun 23, 2016
Didn't need the readline interface. It's now a lot faster too. Added some keyboard shortcuts.

Escape bug is still there, but now reported: nodejs/node#7379
@MylesBorins MylesBorins added confirmed-bug Issues with confirmed bugs. readline Issues and PRs related to the built-in readline module. labels Jun 23, 2016
@MylesBorins MylesBorins self-assigned this Jun 23, 2016
@MylesBorins
Copy link
Contributor

I've dug into this a bit and can confirm the bug exists in v4.x and v5.x

It would appear to have been introduced in v2.0.2 I'll dig into that release and see where it came from

@MylesBorins
Copy link
Contributor

So it would appear that escape started acting weird after aed6bce906 landed.

Doesn't appear to be easily revertable... I'm digging into what is going on with escape

@MylesBorins
Copy link
Contributor

MylesBorins commented Jun 23, 2016

Found the offending section

https://github.com/nodejs/node/blob/master/lib/internal/readline.js#L144-L151

The original PR introduced this logic which yields to more results if the first character is an escape... which is problematic if your entire payload is an escape. It allows for double escaped characters, which is why things work on the third press

Not sure the best way to approach this, but I'll revisit in the morning

@princejwesley
Copy link
Contributor

@thealphanerd It was mentioned in the PR itself.

@MylesBorins
Copy link
Contributor

@princejwesley sigh... I wish I had read the pr instead of just the commit and then digging into the code.

@cjihrig you have the blame on most of internal/readline.js would you specify this as a wont-fix rather than as a bug?

@princejwesley
Copy link
Contributor

@thealphanerd I'll fix it!

@cjihrig
Copy link
Contributor

cjihrig commented Jun 23, 2016

Sorry I'm late to this party. It looks like a proposed fix is available. But for the record, I don't think this is a won't fix. I don't think we should have accepted the original PR that knowingly broke the Escape key.

@silverwind
Copy link
Contributor

silverwind commented Jun 23, 2016

@cjihrig I accepted that drawback as the primary use of esc in a terminal is to start an escape sequence, but I now see the issue with applications relying on that key.

@princejwesley I guess a timeout-based approach to triggering the key is the way to go. I just don't know if 700ms is the correct value, seems a tad high.

@princejwesley
Copy link
Contributor

@silverwind
Default timeout values for zsh is 400ms and for vim & ncurses, its 1sec. @sindresorhus suggested to kepp the middle value between 400 - 1000. Shall I update with the middle value of '400 - 700' (550ms) or 400ms? One second is too high.

@silverwind
Copy link
Contributor

How about 500ms, like in GNU readline (http://man7.org/linux/man-pages/man3/readline.3.html)?

@princejwesley
Copy link
Contributor

@silverwind Done

princejwesley added a commit to princejwesley/node that referenced this issue Aug 13, 2016
evanlucas pushed a commit that referenced this issue Aug 20, 2016
Fixes: #7379
PR-URL: #7382
Reviewed-By: jasnell - James M Snell <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MylesBorins pushed a commit that referenced this issue Sep 30, 2016
Fixes: #7379
PR-URL: #7382
Reviewed-By: jasnell - James M Snell <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
@MylesBorins
Copy link
Contributor

This has been backported to v4.x in 68a2979

rvagg pushed a commit that referenced this issue Oct 18, 2016
Fixes: #7379
PR-URL: #7382
Reviewed-By: jasnell - James M Snell <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MylesBorins pushed a commit that referenced this issue Oct 26, 2016
Fixes: #7379
PR-URL: #7382
Reviewed-By: jasnell - James M Snell <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. readline Issues and PRs related to the built-in readline module.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants