1742707 - Default button on dialog widget triggers even when modifier keys are used, but should react to plain `Enter` keypress only

Status: NEW

(Toolkit :: UI Widgets, defect, P3)

Description

[Thomas D. (:thomas8)]

+++ This bug was initially created as a clone of Thunderbird Bug #620853: Holding Ctrl+Enter a little too long can cause unintentional confirmation of "Send Message?" warning popup - only plain Enter (without modifier key) should confirm the prompt +++

---

STR

• On any dialog provided by the toolkit widget and having a default button, press Ctrl/Alt/Shift/Cmd + Enter

Actual

• default button gets triggered although Enter keypress was modified

Expected

• default button should only get triggered for plain Enter keypress, i.e. only when no modifier keys are pressed.

For these simple out-of-the-box dialogs, it's neither feasible nor possible to change button action based on modifier key, so imo checking exactly for plain Enter should be sufficient, expected and correct.

---

Thunderbird use case

Ctrl+Enter is Thunderbird shortcut for Send message now and comes with a confirmation prompt to prevent accidentally sending a message. However, due to this bug, the prompt's default button will also trigger on Ctrl+Enter, which is self-defeating and makes the prompt a no-op.

STR

1. Compose a message
2. Hold Ctrl+Enter for a second or two

Actual Results:

• Send message? confirmation prompt is immediately confirmed by the same keyboard shortcut which brought it up, Ctrl+Enter. This is unexpected and self-defeats the purpose of the prompt. Also sends multiple copies of the message as long as you keep holding Ctrl+Enter.

Expected Results:

• Dialog prompt should ignore Ctrl+Enter and remain open until confirmed with plain Enter

Comment 1

[Thomas D. (:thomas8)]

Attachment: dialog.diff

Here's a patch which ensures that only unmodified Enter keypress triggers the default button on a toolkit dialog.

Masayuki, what do you think?

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 9252205 [details] [diff] [review]
dialog.diff

I'm not sure. In theory of DOM events, another keypress event listener should prevent its default when it needs to handle Enter key with specific modifier keys. The line which you changed does not check event.defaultPrevented, but _hitEnter does it.
https://searchfox.org/mozilla-central/rev/65d4d3399afa79c8de5a0cc11752d2ba7c31edc1/toolkit/content/widgets/dialog.js#498-500

So, doesn't Thunderbird call peventDefault() properly?

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 9252205 [details] [diff] [review]
dialog.diff

Oops, I tried to request a NeedInfo...

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Clear a needinfo that is pending on an inactive user.

Inactive users most likely will not respond; if the missing information is essential and cannot be collected another way, the bug maybe should be closed as INCOMPLETE.

For more information, please visit auto_nag documentation.

Comment 6

[Suhaib Mujahid [:suhaib]]

Sorry, there was a problem with the detection of inactive users. I'm reverting the change.

Comment 7

[Thomas D. (:thomas8)]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #2)

Comment on attachment 9252205 [details] [diff] [review]
dialog.diff

Thanks for looking into this, Masayuki, and sorry for the lag. I understand your point, and I have two replies below.
Based on that, I still think the correct way of fixing this bug would be my patch attachment 9252205 [details] [diff] [review].

I'm not sure. In theory of DOM events, another keypress event listener should prevent its default when it needs to handle Enter key with specific modifier keys.

(1) While this may be possible, the problem of this bug would remain on the dialog itself: Why should the default button of an out-of-the-box dialog respond to anything other than plain/unmodified Enter? Imho, as a matter of ux-error-prevention, modified Enter (e.g. Ctrl+Enter) on the dialog should never activate the default button, regardless if some other unrelated command happens to prevent the modified key or not. The only reason for the dialog's default button to respond to modified Enter would be if that could modify the default action somehow - however, this looks like an extremely unlikely scenario wrt UX, and it's actually technically not supported by this dialog. Reversely, what's the benefit for the dialog's default button to (unexpectedly) respond to modified Enter if it can't change the default action based on the modifier? And why would a user press Ctrl+Enter deliberately on this type of simple dialog? I assume that this dialog can never have multiline text inputs which may require Ctrl+Enter for default action, right?

The line which you changed does not check event.defaultPrevented, but _hitEnter does it.
https://searchfox.org/mozilla-central/rev/65d4d3399afa79c8de5a0cc11752d2ba7c31edc1/toolkit/content/widgets/dialog.js#498-500

So, doesn't Thunderbird call preventDefault() properly?

(2) Well actually, currently we cannot, and iiuc this issue would fall back to Toolkit/XUL widgets again: Unfortunately, Thunderbird is still using <xul:key> to listen for this keyboard shortcut, so the entire event listening design where we could do preventDefault() is all in the XUL widget. I don't think that further down the actual command handling in TB's command controller, we still have the event available to effect preventDefault().

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Thomas D. (:thomas8) from comment #7)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #2)

Comment on attachment 9252205 [details] [diff] [review]
dialog.diff

Thanks for looking into this, Masayuki, and sorry for the lag. I understand your point, and I have two replies below.
Based on that, I still think the correct way of fixing this bug would be my patch attachment 9252205 [details] [diff] [review].

I checked the default button behavior in native apps' dialog on Win11 and Ubuntu 22.04. Then, Ctrl + Enter submits the dialog on Win11, but not on Ubuntu 22.04. For conform to native apps' behavior, we should keep current behavior at least on Windows. Therefore, changing the default button behavior itself is not proper solution (and I wonder, if a dialog which has a default button is opened by unmodified Enter key press, it will keep having same problem even we ban all modified Enter key presses).

I'm not sure. In theory of DOM events, another keypress event listener should prevent its default when it needs to handle Enter key with specific modifier keys.

(1) While this may be possible, the problem of this bug would remain on the dialog itself: Why should the default button of an out-of-the-box dialog respond to anything other than plain/unmodified Enter?

I don't understand this sentence. keypress event target is focused element, in the case, the dialog box's element or its descendant. Therefore, if an element outside of the dialog, keypress event should not be received by the dialog so that it won't be able to handle it. In the example user style in comment 0 tells me that it's the case of confirm dialog's "OK" button...

I thought that Thunderbird created their own dialog and then, the overlay code should prevent modified Enter key presses. But yeah, this is about the common dialog...

Imho, as a matter of ux-error-prevention, modified Enter (e.g. Ctrl+Enter) on the dialog should never activate the default button, regardless if some other unrelated command happens to prevent the modified key or not. The only reason for the dialog's default button to respond to modified Enter would be if that could modify the default action somehow - however, this looks like an extremely unlikely scenario wrt UX, and it's actually technically not supported by this dialog. Reversely, what's the benefit for the dialog's default button to (unexpectedly) respond to modified Enter if it can't change the default action based on the modifier? And why would a user press Ctrl+Enter deliberately on this type of simple dialog? I assume that this dialog can never have multiline text inputs which may require Ctrl+Enter for default action, right?

Right, but the dialog widget is used for creating any complicated dialog boxes.

The line which you changed does not check event.defaultPrevented, but _hitEnter does it.
https://searchfox.org/mozilla-central/rev/65d4d3399afa79c8de5a0cc11752d2ba7c31edc1/toolkit/content/widgets/dialog.js#498-500

So, doesn't Thunderbird call preventDefault() properly?

(2) Well actually, currently we cannot, and iiuc this issue would fall back to Toolkit/XUL widgets again: Unfortunately, Thunderbird is still using <xul:key> to listen for this keyboard shortcut, so the entire event listening design where we could do preventDefault() is all in the XUL widget. I don't think that further down the actual command handling in TB's command controller, we still have the event available to effect preventDefault().

I wonder, if it's the problem that accidental Ctrl + Enter is handled by dialog, aren't the events are caused by auto-repeat feature of OS? If so, can it fix this bug with ignoring the events whose KeyboardEvent.repeat is true?

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.