Cannot delete contacts from "Advanced address book search" results (implement DEL keyboard shortcut and [Delete] button)

RESOLVED FIXED in Thunderbird 26.0

Status

Thunderbird
Address Book
--
enhancement
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: Alan, Assigned: aceman)

Tracking

(Blocks: 2 bugs, {addon-compat, ux-consistency, ux-efficiency})

11 Branch
Thunderbird 26.0
addon-compat, ux-consistency, ux-efficiency
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [STR comment 2][tb31features])

Attachments

(1 attachment, 3 obsolete attachments)

7.81 KB, patch
aceman
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
User Agent: Mozilla/5.0 (Windows NT 6.0; rv:12.0) Gecko/20100101 Firefox/12.0
Build ID: 20120420145725

Steps to reproduce:

Address > Edit > Search addresses

You cannot delete contacts from this list when searching.

The result of this is that you manually have to sift through the entire address book to find the contact (outside the search window) and delete it.

Comment 1

6 years ago
(In reply to Alan from comment #0)
Works for me:
User Agent: Mozilla/5.0 (X11; Linux i686; rv:12.0) Gecko/20120420 Thunderbird/12.0
Application Build ID: 20120420153403

Once you perform search, right click the result record then hit Delete key.
(In reply to Hashem Masoud from comment #1)
> (In reply to Alan from comment #0)
> Works for me:
> User Agent: Mozilla/5.0 (X11; Linux i686; rv:12.0) Gecko/20120420
> Thunderbird/12.0
> Application Build ID: 20120420153403
> 
> Once you perform search, right click the result record then hit Delete key.

Hashem, before you state wfm, pls ensure to follow the STR of reporter exactly as described. Many reporters don't use precise terminology, so we have to be creative and try several possible scenarios to understand problem of reporter. Applied to this case, if reporter says "cannot delete", maybe he wants to say "pressed DEL key to no effect" (which would be a legitimate RFE). Furthermore, if STR mention "Edit > Search Addresses", we have to follow that path exactly:

STR

1 open TB address book
2 from main menu: Edit > Search Addresses
-> Advanced AB Search popup dialogue
3 enter search terms which will find some contacts (Display Name contains "searchword"), click Search button
4 Select one or more found contacts from the results list
5a hit DEL
5b right-click on selected contact(s)

Actual Result

5a nothing (DEL does not delete selected and focused contacts)
5b nothing (no context menu on search results)
-> No way at all of deleting contacts from here
-> have to exit Advanced AB search and use AB quick search to find contacts again, then delete them from there (using DEL or context menu, delete)

Expected Result

5a allow deletion of selected and focused contacts using DEL (with confirmation)
5b provide contextual actions for found contacts, including "Delete"
(In reply to Hashem Masoud from comment #1)
> Once you perform search, right click the result record then hit Delete key.

Btw, even in AB quick search, "right-click result record, then hit DEL key" does nothing. It's either
- "hit DEL on selected and focused result records" or
- "right-click, then choose 'Delete' from context menu"
The summary of this bug also correctly states "Advanced Search".
Then, before confirming, let's check for duplicates in "Thunderbird" and "MailNews Core" products (ideally, in AB components, but I'll skip that for laziness):

https://bugzilla.mozilla.org/buglist.cgi?quicksearch=%3Athun%2Cmail%20address%2Cab%20advanced%20search%20del

which returns e.g.

Bug 126331 - Advanced Search, needs additional buttons (Delete, Copy, Move)

For a larger scope, maybe check for bugs with bad summaries that do not mention "advanced":

https://bugzilla.mozilla.org/buglist.cgi?quicksearch=%3Athun%2Cmail%20address%2Cab%20search%20del

which returns e.g.

similar Bug 465535 - Cannot delete a contact from a mailing list within the quick search results

similar Bug 343973 - Contacts sidebar: Implement [DEL] keyboard shortcut to delete selected and focused contact(s)
Finally, considerations to arrive at a status decision for this bug:

- we could mark this as a duplicate of bug 126331, but that bug has a different scope, and we'll make it harder to find this because bug 126331 is in MailNews Core
- however, this bug and and bug 126331 are certainly related and have intersections, and whoever bothers to fix bug 126331 is likely to fix this bug, too
- this bug has a narrower scope, so at least for the minimal fix (allow DEL keyboard shortcut), it will be easier to fix than bug 126331 with UI design changes, which is another reason to keep this open

So let's confirm this bug as a wanted/necessary part of bug 126331:
-> NEW
-> this bug blocks bug 126331
Blocks: 126331
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: cannot delete contacts from address book in advanced search → Cannot delete contacts from "Advanced address book search" results (minimal fix: implement DEL shortcut)
From a usability pov, this is really a bug:

- violating ux-consistency (it is inconsistent within TB and compared with other applications that DEL on selected and focused item does not delete that item)
- violating ux-efficiency (as comment 0 points out, the workaround is really clumsy)

-> Keywords: ux-consistency, ux-efficiency

However technically, it's "Enhancement", since we never offered to delete contacts from here, and there is nothing of that sort in this particular part of the UI, so there's nothing technically broken.

-> Severity: Enhancement

And then, it's not limited to Windows:

-> Platform/OS: All/All

(I've added a bit more of explanation here so that others can learn.)
Severity: normal → enhancement
Keywords: ux-consistency, ux-efficiency
OS: Windows Vista → All
Hardware: x86 → All
And anything related to keyboard shortcuts (here: DEL) needs to be added to the tb-keyboard-tracker.
Blocks: 713979
And for others trying to understand this bug, let's point them to better STR of comment 2.

-> Whiteboard: [STR comment 2]
Whiteboard: [STR comment 2]
Similar bugs that are not duplicates or dependent/blocking can be linked in "See also" field for easy reference.
See Also: → bug 465535, bug 343973

Comment 11

6 years ago
(In reply to Thomas D. from comment #10)
Your comments are certainly textbook example of how to handle bugs!

Updated

5 years ago
Component: General → Address Book
Depends on: 343973
Whiteboard: [STR comment 2] → [STR comment 2][has ui-review+]
(Assignee)

Comment 12

5 years ago
Strange that this does not work. The AbResultsPane.js seems to have the necessary code to handle delete and edit card. Not sure why it is not wired up in the Advanced search.

Thomas, where have you got ui-review+ from?
Assignee: nobody → acelists
(In reply to :aceman from comment #12)
> Strange that this does not work. The AbResultsPane.js seems to have the
> necessary code to handle delete and edit card. Not sure why it is not wired
> up in the Advanced search.
> 
> Thomas, where have you got ui-review+ from?

By analogy from Bug 343973, comment 12:

(In reply to Blake Winton (:bwinton) from Bug 343973, 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?
> 
> >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.

Exactly the same situation here, for exactly the same reasons and we are bound to comply with ux-principles.
(Assignee)

Comment 14

5 years ago
Created attachment 767368 [details] [diff] [review]
patch

Would this be enough?
Attachment #767368 - Flags: ui-review?(bwinton)
Attachment #767368 - Flags: review?(mconley)
Attachment #767368 - Flags: feedback?(bugzilla2007)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
Comment on attachment 767368 [details] [diff] [review]
patch

Wow, that's a great improvement over the status quo, and elegant in code. :)
So we're also fixing (for TB, at least) the larger part of age-old bug 126331, dating back to 2002. To be feature-complete, the missing part would be hooking up the existing context menu with found contacts...

Here's a nice usecase from that bug that says it all - and, well, another instance of "good ideas don't die"...

(In reply to Jesse Glick from bug 126331 comment #14)
> Especially Delete would be helpful. You cannot even press the Delete key on a
> card shown in Advanced Search! But if you do a simple search in the main
> window,
> you can delete. So say your addresses are partitioned into those containing
> "mycompany.com" and others, and you want to go through each to delete unused
> collected addresses. First enter "mycompany.com" in the "Name or Email
> contains:" field. Then go through the internal addresses using arrow keys and
> Delete unused ones - fine. But now there is no easy way to delete external
> addresses. You can only find them using the Advanced window, and when you do,
> you cannot then delete them easily.

Iow, Advanced Search is the *only* way to perform some types of searches, e.g. search contacts that do NOT match a certain criterion like "mycompany.com", and Aceman's patch will finally allow users to perform on selected search results an action as trivial as deleting...

So here's a great chance to work towards ux-consistency and ux-efficiency throughout all of Contacts side bar, main AB view, and AB Advanced search...

Note how the current behaviour of both main AB view and AB Advanced search also supports Enter and ALT+Enter to get at the card properties, as requested for contacts side bar in bug 650745.
Attachment #767368 - Flags: feedback?(bugzilla2007) → feedback+
Whiteboard: [STR comment 2][has ui-review+] → [STR comment 2]
Comment on attachment 767368 [details] [diff] [review]
patch

I might have expected the Delete key to also work, but I think that's a different bug.  In either case, this works as expected, so ui-r=me.
Attachment #767368 - Flags: ui-review?(bwinton) → ui-review+
Comment on attachment 767368 [details] [diff] [review]
patch

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

::: mail/base/content/ABSearchDialog.js
@@ +88,4 @@
>  function initializeSearchWindowWidgets()
>  {
>    gSearchStopButton = document.getElementById("search-button");
> +  gPropertiesButton = document.getElementById("cmd_properties");

Technically, this isn't a button. It's a command. Let's clear this up by calling these gPropertiesCmd, gComposeCmd, gDeleteCmd.

@@ +94,2 @@
>    gStatusText = document.getElementById('statusText');
> +  disableButtons();

And call this disableCommands, or disableCmds.
Attachment #767368 - Flags: review?(mconley) → feedback+
(Assignee)

Comment 18

5 years ago
(In reply to Blake Winton (:bwinton) from comment #16)
> I might have expected the Delete key to also work, but I think that's a
> different bug.  In either case, this works as expected, so ui-r=me.
Delete works for me. The patch adds that and uses KeyEvent.DOM_VK_DELETE. What OS did you try and is the keycode different there?
(Assignee)

Comment 19

5 years ago
Created attachment 776518 [details] [diff] [review]
patch v2

Fixed the nits.
Attachment #767368 - Attachment is obsolete: true
Attachment #776518 - Flags: review?(mconley)
I was on Mac OS X, and I have no idea whether it uses a different keycode or not…  (It might use DOM_VK_BACKSPACE, if such a thing exists…)
(In reply to Blake Winton (:bwinton) from comment #20)
> I was on Mac OS X, and I have no idea whether it uses a different keycode or
> not…  (It might use DOM_VK_BACKSPACE, if such a thing exists…)

That's exactly right - a quick test on my Macbook shows that hitting the "Delete" key registers a keypress with the DOM_VK_BACKSPACE keycode.
Comment on attachment 776518 [details] [diff] [review]
patch v2

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

Thanks aceman!

::: mail/base/content/ABSearchDialog.js
@@ +329,5 @@
> +  case KeyEvent.DOM_VK_ENTER:
> +  case KeyEvent.DOM_VK_RETURN:
> +    onProperties();
> +    break;
> +  case KeyEvent.DOM_VK_DELETE:

Let's add DOM_VK_BACKSPACE here too.
Attachment #776518 - Flags: review?(mconley) → review+
Comment on attachment 776518 [details] [diff] [review]
patch v2

Patch has ui-r=bwinton from comment 16, only technical nitfixes thereafter. Great! Ready to land...
Attachment #776518 - Flags: ui-review+
Keywords: checkin-needed
(Assignee)

Comment 24

5 years ago
Created attachment 788720 [details] [diff] [review]
patch v3

OK, adding the backspace. Thanks.
Now this is ready for check in.
Attachment #776518 - Attachment is obsolete: true
Attachment #788720 - Flags: ui-review+
Attachment #788720 - Flags: review+
https://hg.mozilla.org/comm-central/rev/9b1bc5f28553
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 26.0
(Assignee)

Comment 26

5 years ago
Created attachment 789068 [details] [diff] [review]
patch v4

Sorry, the key seems to be called DOM_VK_BACKSPACE (http://mxr.mozilla.org/comm-central/source/mozilla/dom/interfaces/events/nsIDOMKeyEvent.idl#13).
Please back out the patch and check in this one.
Attachment #788720 - Attachment is obsolete: true
Attachment #789068 - Flags: review+
Attachment #789068 - Flags: feedback?(mbanner)
(Assignee)

Comment 27

5 years ago
I mean DOM_VK_BACK_SPACE .
Keywords: checkin-needed
Comment on attachment 789068 [details] [diff] [review]
patch v4

Ryan did the backout, I don't think my feedback is necessary here.
Attachment #789068 - Flags: feedback?(mbanner)
Keywords: addon-compat
Summary: Cannot delete contacts from "Advanced address book search" results (minimal fix: implement DEL shortcut) → Cannot delete contacts from "Advanced address book search" results (implement DEL keyboard shortcut and [Delete] button)
[tb31features]

While hidden in the darker corners of TB, the ancient AB, this is a small but powerful feature addition which finally allows deleting addresses from advanced search. This fills a significant usability gap because depending on data structure and volume of say business ABs, and due to the inherent limits of AB Quick Search (e.g. no negative searches), Advanced AB search might be the /only/ efficient way of finding certain contacts, but before this bug you'd have to search from main AB all over again to delete contacts which were already right in front of you in the advanced search results. For such scenarios, this actually improves things a lot.

For release notes, here's the choice:
- we might not want to point to AB stuff at all (because its so bad in general)
- or we might want to point out even smaller improvements achieved because something is better than nothing and this feature actually helps and also shows some care for AB.

I would tend in favour of mentioning small improvements, so we have more stuff on the list of improvements, and people can benefit from them and be more aware that TB is getting better.

(In reply to Thomas D. from comment #15)
> Comment on attachment 767368 [details] [diff] [review]
> patch
> 
> Wow, that's a great improvement over the status quo, and elegant in code. :)
> So we're also fixing (for TB, at least) the larger part of age-old bug
> 126331, dating back to 2002. To be feature-complete, the missing part would
> be hooking up the existing context menu with found contacts...
> 
> Here's a nice usecase from that bug that says it all - and, well, another
> instance of "good ideas don't die"...
> 
> (In reply to Jesse Glick from bug 126331 comment #14)
> > Especially Delete would be helpful. You cannot even press the Delete key on a [...]
> > addresses. You can only find them using the Advanced window, and when you do,
> > you cannot then delete them easily.
> 
> Iow, Advanced Search is the *only* way to perform some types of searches,
> e.g. search contacts that do NOT match a certain criterion like
> "mycompany.com", and Aceman's patch will finally allow users to perform on
> selected search results an action as trivial as deleting...
Whiteboard: [STR comment 2] → [STR comment 2][tb31features]
You need to log in before you can comment on or make changes to this bug.