1688483 - Fix and clean up duplicate keypress event handling in "Edit Contact" popup panel: Prevent navigation keypresses from leaking and affecting messages in main window. Ctrl+C/Ctrl+A keyboard shortcuts fail in readonly "Email" field.

Status: ASSIGNED

(Thunderbird :: Message Reader UI, defect)

Description

[Thomas D. (:thomas8)]

+++ This bug was initially created as a clone of Bug #752188 +++

TL;DR: Small user errors for keypresses in the "Edit Contact" panel can leak into the main window and unexpectedly trigger random actions on the selected message, even message deletion. This will happen fast and unexpectedly in the background and might go completely unnoticed, making recovery hard/unlikely. Trivial functionality like Ctrl+C fails on Email field content (bug 752188).

STR

1 In message reader, click on recipient, then "Edit contact..."
2 Press Tab to select select email address in Email field in "Edit contact" popup
3a Press Ctrl+C, try to paste somewhere
3b Press Ctrl+A when email address not selected
4 Press Tab to focus AB menulist (do not open the dropdown)
5 With focus on closed AB menulist, press "C" to select "Collected Addresses"
6 With focus on closed AB menulist, press "X" (a character which does not match the start of the name of any of your address books - but maybe you once had it and forgot that you renamed or deleted that).
7 [Caveat: ensure that you have a disposable test message selected!]. With focus on closed AB menulist, press DEL key (maybe falsely hoping to delete an old AB or the currently displayed card, or because you think focus is in some editable field - users think all sorts of things, and errors are common...). Alternatively, press Ctrl+A.
8 place a console.log ("keypress handler") in onKeyPress() handler (editContactPanel.js#63) and press Tab or perform any keyboard actions in the panel (Enter on focused buttons, etc.).

Actual Result

3a - Ctrl+C on selected text of email address does nothing
3b - Ctrl+A to select text of email address does nothing
5 - Like it or not, pressing initial character of known entries in a closed menulist generally works to navigate and select another entry without opening the list.
6 - Unfortunately, if for whatever reason you hit a character key not matching one of your ABs, and that key happens to be A, J, M, or S - you'll end up triggering random actions on your selected message (Archive, Junk, Mark read, Star) - clearly violating the principle of "Focus gets keyboard input" (and not some other, completely unrelated element).
7 - Pressing DEL or Backspace with focus on closed menulist or on any of the Panel buttons will randomly delete the selected message which does NOT have focus. Such user errors do happen, especially when falsely assuming that focus is where you're looking at, or where you have positioned your mouse pointer without clicking (have seen that with less experienced users quite often). Ctrl+A will unexpectedly select all messages.
8 - Keypress handler gets called twice for virtually every keypress, often with no difference in handling at all. Code comments are scarce and misleading, and it's pretty hard to tell what's happening. Hence the uncovered cases...

Expected Result

3a - Ctrl+C on selected text should copy - failing on such trivial functionality is irritating
3b - Ctrl+A should select the text
6 - Accidentally pressing the wrong character expecting to navigate in closed menulist (which is a feature) should not randomly start acting on selected message in main window (existing code explicitly tries but fails to prevent that). TB should respect "Focus gets keyboard input" UX principle.
7 - Accidentally pressing DEL or Backspace with focus on closed menulist or on any of the Panel buttons should not randomly delete the selected message which does NOT have focus. Such user errors do happen (see above for details)! Without this patch, fast keyboarders can even end up deleting all of their messages when they press Ctrl+A, then DEL quickly.
8 - For a given event, double-firing the same event listener from different spots, partly with different parameters, should be avoided because it's confusing and not exactly safe/predictable.

Comment 1

[Thomas D. (:thomas8)]

Attachment: 1688483_fixEditContactPanelKeypressHandling.diff

Hi Geoff, as we've recently fixed duplicate observers together, here's a similar thing on duplicate DOM event handler... More intricate than initially expected, but much better/safer than the existing hack. Could you have a look at the patch?

Comment 2

[Thomas D. (:thomas8)]

Comment on attachment 9198886 [details] [diff] [review]
1688483_fixEditContactPanelKeypressHandling.diff

Review of attachment 9198886 [details] [diff] [review]:
-----------------------------------------------------------------

I've replaced deprecated `!aEvent.charCode` with `aEvent.key.length !== 1`. Here's a comment on that wrt non-Latin localizations.

::: mail/base/content/editContactPanel.js
@@ +103,5 @@
> +        // For any other targets not matching the above conditions (readonly
> +        // textual inputs, closed AB menulist, buttons), allow all non-printable
> +        // keys to fire (e.g. navigation keys), except keys for deletion,
> +        // to prevent deleting the message.
> +        (aEvent.key.length !== 1 &&

Note: I am not sure how this behaves with IME generated multibyte UTF8 keys. However, I assume that even on Chinese localization, single-letter shortcuts (A -> Archive) and modified shortcuts (Ctrl+A -> select all messages) still work with our Latin keys, and those are the relevant keys which we want to exclude here to handle them with preventDefault below. So I think this should work correctly.

Comment 3

[Geoff Lankow (:darktrojan)]

Instead of using event.preventDefault could we make this whole thing a lot simpler by using event.stopPropagation? That should stop events leaving the panel without affecting the controls inside the panel, IIRC. Might be worth trying.

Comment 4

[Geoff Lankow (:darktrojan)]

Comment on attachment 9198886 [details] [diff] [review]
1688483_fixEditContactPanelKeypressHandling.diff

Waiting for an answer to comment 3.