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

Status: RESOLVED FIXED

(Thunderbird :: Address Book, enhancement)

Description

[Alan]

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

[Hashem Masoud]

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

Comment 2

[Thomas D. (:thomas8)]

(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"

Comment 3

[Thomas D. (:thomas8)]

(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"

Comment 4

[Thomas D. (:thomas8)]

The summary of this bug also correctly states "Advanced Search".

Comment 5

[Thomas D. (:thomas8)]

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)

Comment 6

[Thomas D. (:thomas8)]

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

Comment 7

[Thomas D. (:thomas8)]

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

Comment 8

[Thomas D. (:thomas8)]

And anything related to keyboard shortcuts (here: DEL) needs to be added to the tb-keyboard-tracker.

Comment 9

[Thomas D. (:thomas8)]

And for others trying to understand this bug, let's point them to better STR of comment 2.

-> Whiteboard: [STR comment 2]

Comment 10

[Thomas D. (:thomas8)]

Similar bugs that are not duplicates or dependent/blocking can be linked in "See also" field for easy reference.

Comment 11

[Hashem Masoud]

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

Comment 12

[:aceman]

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?

Comment 13

[Thomas D. (:thomas8)]

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

Comment 14

[:aceman]

Attachment: patch

Would this be enough?

Comment 15

[Thomas D. (:thomas8)]

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.

Comment 16

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

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.

Comment 17

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

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.

Comment 18

[:aceman]

(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?

Comment 19

[:aceman]

Attachment: patch v2

Fixed the nits.

Comment 20

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

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…)

Comment 21

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

(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 22

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

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.

Comment 23

[Thomas D. (:thomas8)]

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

Comment 24

[:aceman]

Attachment: patch v3

OK, adding the backspace. Thanks.
Now this is ready for check in.

Comment 25

[Mark Banner (:standard8)]

https://hg.mozilla.org/comm-central/rev/9b1bc5f28553

Comment 26

[:aceman]

Attachment: 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.

Comment 27

[:aceman]

I mean DOM_VK_BACK_SPACE .

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Backed out:
https://hg.mozilla.org/comm-central/rev/8c7098503583

Relanded:
https://hg.mozilla.org/comm-central/rev/406a65fd1a66

Comment 29

[Mark Banner (:standard8)]

Comment on attachment 789068 [details] [diff] [review]
patch v4

Ryan did the backout, I don't think my feedback is necessary here.

Comment 30

[Thomas D. (:thomas8)]

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