343973 - Contacts sidebar: Implement [DEL] keyboard shortcut to delete selected and focused contact(s)

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, enhancement)

Description

[Geert Poels]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4

When enabling the 'contacts sidebar' within a message compose window, the entries from the address book are shown to the left.
A couple of actions can be performed on each contact : properties, delete, add to cc:, etc. ... .
These actions can only be executed from the right-click popup menu, not through a key-stroke.  The address book on the contrary does feature keybindings for the actions (del for delete, space for properties, ...).
The contacts sidebar should also feature these keybindings.

To be fully correct, selecting an entry in contacts sidebar should add these five actions to the edit menu. (as is also the case in the address book)

Reproducible: Always

Comment 1

[Wayne Mery (:wsmwk)]

Reporter, Can you confirm whether this problem is gone, or still exists on a current version of thunderbird?  

We are working to help old bugs move along, so your comment will be helpful.

Comment 2

[Wayne Mery (:wsmwk)]

bcc, ccc, add are available via alt-B, D, A respectively, so morphing this to what remains, delete and edit properties.  And what's left i'm not sure his highly desirable - but then I'm not a good person to judge, not being a sidebar user myself.

Mark, do you know if netscape ever had this?

Comment 3

[Geert Poels]

Keybindings still don't work on my versions.

The actions are already there, it should be a 'quicky' to add the delete key to the delete-action I guess ?
On a sidenote : I'm not able to reach the contacts sidepanel using keystrokes.
I'd expect control-tab to also jump to the contacts sidebar when opened.

Comment 4

[Wayne Mery (:wsmwk)]

(In reply to comment #3)
> Keybindings still don't work on my versions.

I forgot to mention ... thunderbird 3 beta
  http://www.mozillamessaging.com/en-US/thunderbird/early_releases/


> The actions are already there, it should be a 'quicky' to add the delete key to
> the delete-action I guess ?
> On a sidenote : I'm not able to reach the contacts sidepanel using keystrokes.
> I'd expect control-tab to also jump to the contacts sidebar when opened.

thanks the interest.  please file a new bug - we work primarily on one issue per bug.

Comment 5

[Bryan Clark (DevTools PM) [:clarkbw]]

Did previous TB releases have a keyboard shortcut for delete and edit of contacts in the sidebar?

I'm sorry, but I would not allow such a thing inside the sidebar.  There is not good undo system for _quickly_ deleting a contact, especially in the sidebar space.  If a user were to accidentally shift focus there from their message and press delete several times (expecting to delete text) they could lose lots of contacts.  We can't allow this and should remove it if it has gone in.  I'd recommend WONTFIX for this.

The keyboard access is still available by using the keyboard to right click menu key and then the accelerator for the delete menu item.

Comment 6

[Thomas D. (:thomas8)]

This bug needs to be disentangled. Bryan's comment 5 is sceptical only about accidental irreversible DELetion of contact(s), but we could certainly agree on a shortcut for "Edit contact" dialogue (which is "Properties" in the context menu).

-> Followup Bug 650745 - Contacts sidebar: Implement [Enter] and/or Alt+Enter keyboard shortcut to edit the properties of selected contact

Comment 7

[Thomas D. (:thomas8)]

(In reply to comment #5)
> I'm sorry, but I would not allow such a thing inside the sidebar.  There is
> not good undo system for _quickly_ deleting a contact, especially in the
> sidebar space.  If a user were to accidentally shift focus there from their 
> message and press delete several times (expecting to delete text) they could
> lose lots of contacts.  We can't allow this and should remove it if it has
> gone in.  I'd recommend WONTFIX for this.

IMHO the better approach would be to fix
Bug 94407 - [Feature] Undo/Redo option for deleted address book cards
and, furthermore, to add a confirmation dialogue for any deletion of address book cards, as has been requested by many users in bug 94407 and elsewhere, e.g. on http://getsatisfaction.com/mozilla_messaging/topics/contact_list_v_human_error.

With that, or even either of the two security options, the problems correctly mentioned by Bryan would be avoided, and we could even have DEL shortcut on selected contact(s) from contact sidebar without danger of accidental dataloss.
So for many users this might be a convenient and safe way of deleting their contacts right from the contacts sidebar.

For those few users who might *accidentally* hit DEL on selected contacts (yes, computers work based on focus!!!), if Bryan still thinks we need to avoid any inconvenience for them, let's pick another, safer shortcut here: How about ALT+DEL or CTRL+DEL?

> The keyboard access is still available by using the keyboard to right click
> menu key and then the accelerator for the delete menu item.

Comment 8

[Thomas D. (:thomas8)]

Now that bug 526541 (confirmation on address book delete) has been fixed, and assuming that it has also been fixed for the contacts sidebar (can't test right now), the only reason mentioned against fixing this has been nullified, because accidental DEL on a contact will no longer result in dataloss.

It's high time that Thunderbird returns to standard-compliant behaviour with respect to keyboard access. For those who use it, the contacts side bar is a compact version of the address book, and we need to provide the respective interactions for such users to have efficient and full keyboard access.
Surely, this is a valid request. -> New

Comment 9

[Thomas D. (:thomas8)]

(In reply to comment #8)
> Now that bug 526541 (confirmation on address book delete) has been fixed,
> and assuming that it has also been fixed for the contacts sidebar

Incorrect assumption... before doing this bug, we need to fix Bug 665267 first:
Bug 665267 Contacts Sidebar: Please add a "confirm on delete" option for address book entries

Comment 10

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #9)
> before doing this bug, we need to fix Bug 665267 first:
> Bug 665267 Contacts Sidebar: Please add a "confirm on delete" option for
> address book entries

Now that Bug 665267 has been fixed, there's no reason why DEL on deliberatly selected AND focused contact(s) in sidebar should not DELete them. Let's go for it then...

Implement [DEL] keyboard shortcut for deleting selected and focused contact(s) from sidebar (after confirmation)

STR

1) In composition's contacts side bar (F9), select one or more contacts, so that focus is also in the list of contacts.
2) On deliberately selected AND focused contact(s), press DEL, expecting to delete the selected contact(s).

Actual Result:

- Nothing

Expected Result:

- DELete the selected and focused contact(s)

Reasons:

1) ux-consistency

1a) with address book:
- DEL on selected and focused contact(s) in AB will DELete them
- Contacts side bar is nothing but a small version of AB, so the behaviour should be the same

1b) with many other lists in TB:
- In the message lists, attachment lists, search results, Lightning's calendar and task views, even in folder list, we allow efficient DELetion of selected and focused items (even folderS!) with DEL key (even without confirmation in some cases)
- Bugs have been filed for unexpected exceptions (e.g. Bug 557990)
- We have recently restored DEL shortcut for similar scenarios (DEL on selected and focused attachment(s) will delete the attachment).

1c) with OS / external applications:
We should be externally consistent with all OS and most application software, where DEL on selected and focused list entries will always trigger DELetion of those entries.

1d) with keyboard interaction standards within this UI element
- It's high time that Thunderbird returns to standard-compliant behaviour with respect to keyboard access.
- It comes as a surprise that while we allow other standard keyboard shortcuts in this list (like Ctrl+A, Ctrl+Space etc.), we don't offer DEL

2) ux-efficiency

- Obviously, DELetion of selected contacts with DEL is maximally efficient,
- and much more efficient than getting the context menu and finding "Delete" command within the menu, especially for keyboard users

3) Very low risk of accidental DELetion
- User needs to actively/deliberately set the focus into the list of contacts and select contact(s) before DEL will work as a keyboard shortcut. That's unliky to happen by accident.
- If we were to prevent such accidents completely, we'd have to abolish half of our application behaviour (cf. 1a and 1b above), where we always rely on focus in such scenarios, like any other application in this whole wide world...
- More importantly, we safeguard against accidental DELetion with the confirmation dialogue, see 4) below.

4) No risk of dataloss (confirmation dialogue)
Now that Bug 526541 has been fixed for the sidebar, too (Bug 665267), even accidental pressing of DEL on selected and focused contacts will no longer cause dataloss, because of the confirmation dialogue. Combined with need for deliberate focus as explained in 3) above, it's really hard to do the wrong thing here unless you actually want it.

Finally, this should be really easy to implement.

Comment 11

[Thomas D. (:thomas8)]

Attachment: Request ui-review for DEL to delete selected AND focused contacts in sidebar

Blake, now that we have a confirmation dialogue for deletion of contacts in contacts sidebar, there is no reason why DEL on selected and focused contacts shouldn't work as a keyboard shortcut. On the contrary, there are a lot of reasons why DEL SHOULD WORK. Find all the details in bug 343973, comment 10.

This should be straightforward for ui-review, and it's easy to implement.
Let's make TB a little more standard-compliant again.

Comment 12

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 586097 [details]
Request ui-review for DEL to delete selected AND focused contacts in sidebar

>1) In composition's contacts side bar (F9), select one or more contacts, so that focus is also in the list of contacts.
>2) On deliberately selected AND focused contact(s), press DEL, expecting to delete the selected contact(s).
>
>- DELete the selected and focused contact(s)

AFTER the confirmation message appears.

Did you also want "Enter" to show the properties dialog?

>Reasons:
>1) ux-consistency (with address book, most other lists in TB, lists controls in OS / external applications...)
>2) ux-efficiency (this is maximally efficient, and more efficient than context menu)
>3) Very low risk of accidental DELetion (user needs to deliberately place focus AND selection for DEL to work)
>4) No risk of dataloss even for accidental DEL keypress (confirmation dialogue)

Sure, ui-r=me.

(But having said that, I don't think it's enough of a priority to put any of the core developers on, so if you want to see it committed, you'll probably need to write the patch yourself.)

Thanks,
Blake.

Comment 13

[Thomas D. (:thomas8)]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #12)
> Comment on attachment 586097 [details]
> Request ui-review for DEL to delete selected AND focused contacts in sidebar
> 
> >1) In composition's contacts side bar (F9), select one or more contacts, so that focus is also in the list of contacts.
> >2) On deliberately selected AND focused contact(s), press DEL, expecting to delete the selected contact(s).
> >- DELete the selected and focused contact(s)
> AFTER the confirmation message appears.
> 
> Did you also want "Enter" to show the properties dialog?

I once *did*, as requested in bug 650745. However, you didn't like it and I admit it's really hard to get the behaviour right. I think, for a start, it would be much easier, reasonably intuitive and absolutely no-risk to have *Alt+Enter* for the contact's properties dialogue.

*Plain* ENTER on selected & focussed contact(s) in sidebar is VERY ambiguous, with lots of pros and cons about using it for the properties dialogue vs. using it for adding a recipient (where, however, we are facing the problem of picking the right flavor between To/CC/BCC, and furthermore, Bug 271917). Note that we are currently violating ux-consistency and standards compliance as double-click ("Add to To") behaves different from select+Enter (nothing). I have been cracking my head, but I can't find a satisfying one-for-all solution which is ux-consistent, transparent/intuitive/ux-minimalistic/ux-discoverable, and standards compliant. I suppose we should leave that problem to bug 650745 and discuss the details there, and keep this bug for DEL only.

Comment 14

[Thomas D. (:thomas8)]

Aceman, thanks for being so active to drive things forward, that's great! :)

You showed interest in bug 236340, so this is in the same corner, and probably simple, with the extra advantage of an unambiguous ui-review+ from Blake :)

So let's fix it quickly before :bwinton changes his mind... ;)

Comment 15

[:aceman]

Attachment: patch

This seems to do it. This also fixes an exception when clicking inside the contacts search results. After fixing that delete key somehow works automatically (picks cmd_delete from mailnews/addrbook/content/abResultsPane.js) so this patch makes also the menuitem use the same command instead of calling AbDelete directly. I'd like mconley to check this too whether the sharing is OK.
See also patch in bug 650745 for how this can be used more.

Comment 16

[:aceman]

If needed I can try to show the key in the menuitem similarly to bug 650745.

Comment 17

[Thomas D. (:thomas8)]

For selected contacts in sidebar, all of the contextual actions should also be available from main menu, as is currently the case for Edit > Select All which selects all contacts in sidebar.

For a start, at least Delete and Properties should be made available.

Edit > Delete (needs to be enabled for deleting selected and focussed contacts from sidebar, or perhaps it already is?)

Not sure about captions, probably one of these:

Edit > Properties... OR
Edit > Contact Properties... OR
Edit > Contact... OR
Edit > Edit Contact...

In address book, when you select a card, there's
Edit > Properties...
So consistency would be nice.

I'm also wondering if those "Properties" (context) menu captions are somewhat outdated and should be replaced by less technical and more action-oriented menu caption "Edit [Contact]..." because that's also the title of the properties dialogue: "Edit Contact [...]" (can't verify exact title right now).

But we can also do such little things in a followup bug.

Comment 18

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #16)
> If needed I can try to show the key in the menuitem similarly to bug 650745.

Pls do. I think showing keys in context menus is very desirable, because that's where they are most likely to be found by users in the very context where they really help. I'm sure to have seen a bug for that, but I can't find it.

Comment 19

[Thomas D. (:thomas8)]

For bonus points, also add [DEL] shortcut to delete selected AND focused contacts
- in AB Advanced Search Results (bug 749564)
- in AB Mailing List View (bug 465535)

For both of these, we can assume ui-review+ by analogy from bwinton's ui-r+ in this bug 343973 comment 12.

Setting dependency for ease of tracking.

Comment 20

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #19)
> For bonus points, also add [DEL] shortcut to delete selected AND focused
> contacts
> - in AB Advanced Search Results (bug 749564)
> - in AB Mailing List View (bug 465535)

Bug 465535 might be different, but bug 749564 is same as this one.

Comment 21

[Magnus Melin [:mkmelin]]

Comment on attachment 766305 [details] [diff] [review]
patch

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

Looks good to me! r=mkmelin
(If you want you can take the opportunity to move the chrome://messenger/ import down like in bug 885268, which i'll obsolete since it fixes the same exception.)

Comment 23

[:aceman]

Attachment: patch v2

Thanks.

Comment 24

[:aceman]

Attachment: patch v3

This also shows the key in the menuitem.

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 769509 [details] [diff] [review]
patch v3

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

This looks OK to me, but you're going to want some mailnews/ folks to look at it too.

Comment 26

[Thomas D. (:thomas8)]

Comment on attachment 769509 [details] [diff] [review]
patch v3

Neil, would you have a minute to review this straight-forward mini-patch that lets you delete selected and focused contacts from composition's contact side bar using DEL key?

Already has ui-review+ and review+ for TB, just needs a greenlight for mailnews.
Thanks.

Comment 27

[[e-mail]]

Comment on attachment 769509 [details] [diff] [review]
patch v3

Sorry, but I don't understand why you need to change abResultsPane.js now that you're including globalOverlay.js in abContactsPanel.xul (SeaMonkey's addressbook-panel.xul includes globalOverlay.js and the delete key works fine).

Comment 28

[:aceman]

Neil, see comment 15. It is true that those changes are probably not necessary for the current problem in TB. But they are necessary for any other caller that will not include globalOverlay.js. Such a case is allowed per the comments in the header. So I make it really work without throwing exceptions.
Do you want me to split it into a separate bug?

Comment 29

[[e-mail]]

(In reply to aceman from comment #28)
> Neil, see comment 15. It is true that those changes are probably not
> necessary for the current problem in TB. But they are necessary for any
> other caller that will not include globalOverlay.js. Such a case is allowed
> per the comments in the header. So I make it really work without throwing
> exceptions.
Not according to my reading of the comment - if (as here) you want to use ResultsPaneController then you must include globalOverlay.js for it to work.

Comment 30

[:aceman]

Do you see where TB was using ResultsPaneController ? Previously I think it was not using its (at least not intentionally) functionality as it was calling the AB code (e.g. AdDelete) directly. So it appears to me TB didn't want the ResultsPaneController but was still getting the exceptions.

Comment 31

[[e-mail]]

(In reply to aceman from comment #30)
> Do you see where TB was using ResultsPaneController ? Previously I think it
> was not using its (at least not intentionally) functionality as it was
> calling the AB code (e.g. AdDelete) directly. So it appears to me TB didn't
> want the ResultsPaneController but was still getting the exceptions.

It looks as if bug 443035 changed that behaviour - you used to have to opt in to the controller but now SetAbView automatically does that for you. So at best I'd say that the comment is out of date and goSetMenuValue should just appear on the list of requirements.

Comment 32

[:aceman]

So that means my patch would allow a caller to agains at least ignore ResultsPaneController forced upon him. But I can drop that from the patch if you wish and let the remaining callers without globalOverlay.js (if any) take care.

Comment 33

[[e-mail]]

(In reply to aceman from comment #32)
> So that means my patch would allow a caller to agains at least ignore
> ResultsPaneController forced upon him. But I can drop that from the patch if
> you wish and let the remaining callers without globalOverlay.js (if any)
> take care.

Yes, I would prefer if you drop those changes, thanks.

Comment 34

[:aceman]

Attachment: patch v4

OK, dropped the change to mailnews.

Comment 35

[Mark Banner (:standard8)]

https://hg.mozilla.org/comm-central/rev/eb2a926e6fb5

Comment 36

[Thomas D. (:thomas8)]

Thanks aceman for the patch and everyone for finally fixing this after 7 years of discussion, groundwork in other bugs, and waiting for the right people to reach here... Just imagine, basic keyboard interaction now available for this particular command! :)

Users of contact pane will be indebted to you forever as we've made another step in the ongoing journey towards ux-efficiency and ux-consistency in this important area of the UI. :)

Comment 37

[:aceman]

Thanks :)
Please mark the bug Verified when you try a nightly build with the patch included.

Comment 38

[Thomas D. (:thomas8)]

New keyboard shortcuts should always be mentioned in release notes so that users can benefit from them, and I think that's what we've done in the past.