fix(runtime): Make native modal keyboard interaction consistent with browsers

[lionel-rowe]

Fixes #18223.
Fixes #21477

Side-by-side comparison (current version on left, updated version on right): https://www.youtube.com/watch?v=J6P5Mi5j6G4

[CLAassistant]

All committers have signed the CLA.

[bartlomieju]

Thanks for the PR @lionel-rowe! Could you please rebase against main.

[kt3k]

NO_COLOR env var doesn't look respected?

[kt3k]

I wonder if we really need selection by arrow keys. That's not aligned to what we do with the permission prompt.

[lionel-rowe]

I wonder if we really need selection by arrow keys. That's not aligned to what we do with the permission prompt.

This is also with the goal of consistency with the browser where possible — the browser shows the options and allows selecting between them, albeit with Tab rather than arrow keys (I think using Tab would be too far outside what's expected of console apps though). It's also with the secondary goal of being beginner-friendly, as the native modals are frequently used in beginner tutorials as a simple way of getting user input.

If consensus is still that this isn't necessary, what would be the ideal interaction and message text? E.g. message text = [Yn], Y/y/Enter writes "y" and continues, N/n/Esc writes "n" and cancels, all other button presses are swallowed (nothing written to stdout)?

[kt3k]

I'm in favor of changing the default selection and adding ESC handling, as suggested in #18233, but I don't see enough motivation to other subtle interaction changes.

the browser shows the options and allows selecting between them, albeit with Tab rather than arrow keys

I can't confirm this browser behavior with chrome, firefox, safari on my mac. Tab key (or any keys) doesn't seem to allow navigating between Ok/Cancel options. Or am I missing something?

[lionel-rowe]

I can't confirm this browser behavior with chrome, firefox, safari on my mac. Tab key (or any keys) doesn't seem to allow navigating between Ok/Cancel options. Or am I missing something?

That's weird, maybe Mac's different from Windows in this respect?

Actually I just checked, and all 3 pairs of Tab/Shift+Tab, Left/Right, and Up/Down work in both Chrome and Firefox on Windows. Chrome captures the focus within the modal, whereas Firefox allows tabbing away to other browser UI elements (but not page UI elements).

It'd be surprising if Mac didn't allow any keyboard-based way of switching focus between the buttons, as that'd be a pretty major accessibility problem.

[0f-0b]

Firefox and Safari on Mac allow moving focus between the buttons only when keyboard navigation is turned on in System Settings. I'd expect the same behavior from Chrome.

[bartlomieju]

Thank you for the PR @lionel-rowe and sorry for a slow turnaround. I'm interested in landing it for v1.37 (beginning of September). Could you please rebase it?

[lionel-rowe]

Thank you for the PR @lionel-rowe and sorry for a slow turnaround. I'm interested in landing it for v1.37 (beginning of September). Could you please rebase it?

@bartlomieju No worries, I also dropped the ball on this! Happy to rebase, but I need a steer on what else needs changing. Specifically, per various comments from @kt3k:

1. Returning undefined here is aligned to what browsers do when dialog boxes are disabled. ref. feat: add alert, confirm, and prompt #7507 (review)
2. NO_COLOR env var doesn't look respected?
3. I wonder if we really need selection by arrow keys. That's not aligned to what we do with the permission prompt.
   ...
   I can't confirm this browser behavior with chrome, firefox, safari on my mac. Tab key (or any keys) doesn't seem to allow navigating between Ok/Cancel options. Or am I missing something?

Does this list of corresponding changes look good, and is there anything else to add?

1. In non-interactive environments, return undefined (alert), null (prompt), and false (confirm), without throwing.
2. Respect NO_COLOR env var.
3. Remove arrow selection functionality. New interaction for confirm as follows:
   • Message text shows [Yn]
   • Pressing Y/y/Enter writes "y" to stdout and returns true
   • Pressing N/n/Esc writes "n" to stdout and returns false
   • All other button presses (arrow keys, tab, any other letter) are swallowed (nothing happens and nothing is written to stdout)

[bartlomieju]

In non-interactive environments, return undefined (alert), null (prompt), and false (confirm), without throwing.
Respect NO_COLOR env var.
Remove arrow selection functionality. New interaction for confirm as follows:
Message text shows [Yn]
Pressing Y/y/Enter writes "y" to stdout and returns true
Pressing N/n/Esc writes "n" to stdout and returns false
All other button presses (arrow keys, tab, any other letter) are swallowed (nothing happens and nothing is written to stdout)

Yes, these all make sense to me 👍

[lionel-rowe]

Yes, these all make sense to me 👍

All of those are implemented now.

[bartlomieju]

Thanks for the updates @lionel-rowe, I have a few more questions and suggestions.

[bartlomieju]

If we're supposed to read only a single key here, why do we need a buffer with size 1024? Could you also explain why you're setting the terminal to raw mode here?

[lionel-rowe]

If we're supposed to read only a single key here, why do we need a buffer with size 1024?

A single logical keypress (most notably, combos such as shift+insert/ctrl+v) can send an arbitrarily large amount of input to stdin. 1024 is just an arbitrary size that should typically be large enough to avoid problems but small enough to avoid undue memory usage.

For example, if we used a smaller buffer size like 10:

const { stdin } = Deno

const b = new Uint8Array(10)
stdin.setRaw(true)
stdin.readSync(b)

That can be broken (Deno exits with Error: Io(Kind(InvalidData))) by copying 字字字字 to the clipboard and pasting it with the next keypress, because the 3 UTF-8 bytes of the last 字 lie across the boundary.

Could you also explain why you're setting the terminal to raw mode here?

Raw mode is necessary to capture ESC keypresses and to progress immediately after a key is pressed.

[bartlomieju]

@lionel-rowe seems like there's an actual test failure in one of the updated tests. Could you try to fix it?

[lionel-rowe]

@lionel-rowe seems like there's an actual test failure in one of the updated tests. Could you try to fix it?

I can't reproduce the failure locally (Ubuntu 22.04.2 on WSL) so guessing it may only be reproducable on macos as that's where the CI is failing? I've wrapped what seem to be the offending tests (related to sending interrupts Ctrl+C \x03 and Ctrl+D \x04) in a #[cfg(not(target_os = "macos"))] block.

[dsherret]

LGTM