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

lionel-rowe · 2023-03-27T09:20:06Z

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

CLAassistant · 2023-03-27T09:20:12Z

All committers have signed the CLA.

bartlomieju · 2023-04-02T16:59:13Z

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

runtime/Cargo.toml

runtime/js/41_prompt.js

kt3k · 2023-04-14T08:06:40Z

NO_COLOR env var doesn't look respected?

runtime/js/41_prompt.js

kt3k · 2023-04-14T08:09:41Z

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

lionel-rowe · 2023-04-21T04:17:46Z

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 · 2023-04-21T11:43:12Z

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 · 2023-04-21T12:11:27Z

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 · 2023-08-08T01:14:30Z

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 · 2023-08-25T22:38:50Z

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 · 2023-08-26T04:33:45Z

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:

Returning undefined here is aligned to what browsers do when dialog boxes are disabled. ref. feat: add alert, confirm, and prompt #7507 (review)

NO_COLOR env var doesn't look respected?

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?

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)

bartlomieju · 2023-08-28T06:54:48Z

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 · 2023-09-02T14:54:33Z

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.

runtime/Cargo.toml

runtime/ops/tty.rs

runtime/js/41_prompt.js

bartlomieju · 2023-09-03T19:39:24Z

runtime/js/41_prompt.js

+    stdin.setRaw(true);
+    stdin.readSync(new Uint8Array(1024));


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?

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 · 2023-09-12T10:56:55Z

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

lionel-rowe · 2023-09-12T12:06:57Z

@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.

Prerequisite for #18453. This update also makes it possible to address #8049 by using https://docs.rs/rustyline/latest/rustyline/struct.Editor.html#method.create_external_printer

dsherret

LGTM

…nt with browsers (#18453)" This reverts commit 346d812.

@lionel-rowe

…nt with browsers" (#21739) Reverts #18453 Fixes #21602 #21631 #21641 Reasons for revert: - alert() and confirm() swallowed ^C with raw mode. - prompt() did not re-raise the interrupt signal from rustyline. - Default 'Y' on confirm() is a bad default and breaking change. cc @lionel-rowe

@lionel-rowe

…nt with browsers" (#21739) Reverts #18453 Fixes #21602 #21631 #21641 Reasons for revert: - alert() and confirm() swallowed ^C with raw mode. - prompt() did not re-raise the interrupt signal from rustyline. - Default 'Y' on confirm() is a bad default and breaking change. cc @lionel-rowe

lionel-rowe force-pushed the alert-prompt-confirm-keyboard-interaction branch 2 times, most recently from ae4ed8a to b266536 Compare April 4, 2023 02:23

lionel-rowe and others added 4 commits April 4, 2023 03:23

Rebase and fix tests

d279a07

Merge branch 'main' into alert-prompt-confirm-keyboard-interaction

8b237e9

Merge branch 'main' into alert-prompt-confirm-keyboard-interaction

d31fe0e

breaking - throw instead of returning early if !isatty

780797d

bartlomieju reviewed Apr 12, 2023

View reviewed changes

runtime/Cargo.toml Outdated Show resolved Hide resolved

kt3k reviewed Apr 14, 2023

View reviewed changes

runtime/js/41_prompt.js Outdated Show resolved Hide resolved

kt3k reviewed Apr 14, 2023

View reviewed changes

runtime/js/41_prompt.js Outdated Show resolved Hide resolved

lionel-rowe closed this Apr 21, 2023

lionel-rowe reopened this Apr 21, 2023

bartlomieju mentioned this pull request May 7, 2023

Limitation: Prompts limit input to 255 characters #19010

Closed

bartlomieju added this to the 1.37 milestone Aug 25, 2023

lionel-rowe added 2 commits September 2, 2023 14:52

Merge branch 'main' into alert-prompt-confirm-keyboard-interaction

8861563

Implement changes from code review

d679281

lionel-rowe force-pushed the alert-prompt-confirm-keyboard-interaction branch from e954446 to d679281 Compare September 2, 2023 14:50

bartlomieju reviewed Sep 3, 2023

View reviewed changes

Merge branch 'main' into alert-prompt-confirm-keyboard-interaction

dc369c5

Merge branch 'main' into alert-prompt-confirm-keyboard-interaction

67a5e25

lionel-rowe added 2 commits September 12, 2023 19:05

Merge branch 'main' into alert-prompt-confirm-keyboard-interaction

d3b7253

Exclude ctrl+c/ctrl+d tests from macos

94e192f

lionel-rowe and others added 3 commits September 12, 2023 20:07

Merge branch 'main' into alert-prompt-confirm-keyboard-interaction

27d449a

Fix test on Windows

9c2c740

Merge branch 'main' into alert-prompt-confirm-keyboard-interaction

f8f13de

dsherret modified the milestones: 1.37, 1.38 Sep 19, 2023

dsherret modified the milestones: 1.38, 1.39 Nov 1, 2023

bartlomieju added 2 commits December 6, 2023 15:51

Merge branch 'main' into alert-prompt-confirm-keyboard-interaction

1332048

update rustyline

6778a12

bartlomieju mentioned this pull request Dec 6, 2023

chore: update rustyline to 13.0.0 #21481

Merged

bartlomieju added 2 commits December 6, 2023 22:05

Merge branch 'main' into alert-prompt-confirm-keyboard-interaction

ebb3f63

Merge branch 'main' into alert-prompt-confirm-keyboard-interaction

2634dc6

bartlomieju requested review from dsherret and bartlomieju December 8, 2023 03:51

Merge branch 'main' into alert-prompt-confirm-keyboard-interaction

445c895

dsherret approved these changes Dec 13, 2023

View reviewed changes

bartlomieju merged commit 346d812 into denoland:main Dec 13, 2023
14 checks passed

lionel-rowe mentioned this pull request Dec 14, 2023

Consistent text styles between prompt vs alert/confirm #21565

Open

toebeann mentioned this pull request Dec 16, 2023

confirm, alert and prompt no longer respect quit signals since updating to Deno v1.39.0 #21602

Closed

mmastrac mentioned this pull request Dec 18, 2023

Latest changes to prompt() break Ctrl + C #21631

Closed

littledivy added a commit that referenced this pull request Dec 30, 2023

Revert "fix(runtime): Make native modal keyboard interaction consiste…

6a9a68a

…nt with browsers (#18453)" This reverts commit 346d812.

littledivy mentioned this pull request Dec 30, 2023

Revert "fix(runtime): Make native modal keyboard interaction consistent with browsers" #21739

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lionel-rowe commented Mar 27, 2023 •

edited by bartlomieju

Loading

CLAassistant commented Mar 27, 2023 •

edited

Loading

bartlomieju commented Apr 2, 2023

kt3k commented Apr 14, 2023

kt3k commented Apr 14, 2023

lionel-rowe commented Apr 21, 2023 •

edited

Loading

kt3k commented Apr 21, 2023

lionel-rowe commented Apr 21, 2023

0f-0b commented Aug 8, 2023 •

edited

Loading

bartlomieju commented Aug 25, 2023

lionel-rowe commented Aug 26, 2023

bartlomieju commented Aug 28, 2023

lionel-rowe commented Sep 2, 2023

bartlomieju left a comment

bartlomieju Sep 3, 2023

lionel-rowe Sep 4, 2023

bartlomieju commented Sep 12, 2023

lionel-rowe commented Sep 12, 2023

dsherret left a comment

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

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

Conversation

lionel-rowe commented Mar 27, 2023 • edited by bartlomieju Loading

CLAassistant commented Mar 27, 2023 • edited Loading

bartlomieju commented Apr 2, 2023

kt3k commented Apr 14, 2023

kt3k commented Apr 14, 2023

lionel-rowe commented Apr 21, 2023 • edited Loading

kt3k commented Apr 21, 2023

lionel-rowe commented Apr 21, 2023

0f-0b commented Aug 8, 2023 • edited Loading

bartlomieju commented Aug 25, 2023

lionel-rowe commented Aug 26, 2023

bartlomieju commented Aug 28, 2023

lionel-rowe commented Sep 2, 2023

bartlomieju left a comment

Choose a reason for hiding this comment

bartlomieju Sep 3, 2023

Choose a reason for hiding this comment

lionel-rowe Sep 4, 2023

Choose a reason for hiding this comment

bartlomieju commented Sep 12, 2023

lionel-rowe commented Sep 12, 2023

dsherret left a comment

Choose a reason for hiding this comment

lionel-rowe commented Mar 27, 2023 •

edited by bartlomieju

Loading

CLAassistant commented Mar 27, 2023 •

edited

Loading

lionel-rowe commented Apr 21, 2023 •

edited

Loading

0f-0b commented Aug 8, 2023 •

edited

Loading