1468940 - In advanced "Search messages" (Ctrl+Shift+F), DEL key fails to delete selected result message(s)

Status: RESOLVED FIXED

(Thunderbird :: Search, defect)

Description

[Thomas D. (:thomas8)]

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

Bug 557990 and friends like bug 1432537 are all a bit messy and/or uncertain about reproducing, so I decided to give this a fresh start because from what I am seeing on current Daily, this has now regressed even more than before, and is 100% reproducible.

Seen on Daily 62.0a1 (2018-06-14) (64-bit)

STR

0. Create some discardable test messages in your account's drafts folder (Pop or Imap), and save them as a draft (Ctrl+S):
msg1: subject: foo asdf
msg2: subject: bar asdf

1. Advanced "Search messages" (Ctrl+Shift+F)
(FTR, Pop or Imap drafts folder, search subfolders checked, match all of the following, Subject contains is preset; but I don't think any of that matters)

2. type a searchword which matches messages (e.g. "asdf") into input field after [Subject] [contains]: [asdf|     ]

3. Press Enter to start search (or Alt+S, or click Search)

4. Select any message (even multiple messages) from results list, e.g. msg1 (subject: foo asdf).

5. Press [DEL] key on your keyboard (sic!)
5b. Try anything to change focus and (re)select any message(s), and press [DEL] key

6. For comparison, click [Delete] button in the search dialog

Actual result

5. [DEL] key does nothing (this bug)
5b. Changing focus/selection in any way (keyboard, mouse) does no longer change the result as it used to in bug 557990, comment 17 - still nothing.

6. Using [Delete] button, selected message(s) immediately deleted (as expected).

Expected result

5. Delete selected message(s)
6. Dito.

Comment 1

[Thomas D. (:thomas8)]

Let's please try to nail this down, because I find it quite depressing to start failing on such basic requirements of ux-efficiency again.

When trying to reproduce, please mention your OS and TB version.

Wayne, can you reproduce? Are you on Windows?
Aceman, can you reproduce on Linux?
Richard, can you reproduce on MAC?

Comment 2

[Wayne Mery (:wsmwk)]

As mentioned in bug 1432537 I currently cannot reproduce on windows

Comment 3

[Thomas D. (:thomas8)]

For all cases except those in bug 557990, comment 17, this has regressed compared to 52.8.0 (32-bit) where such cases still work, and only the first virgin click fails under certain circumstances.

So when testing for regression, you could use these slightly more prescriptive steps:

1. Advanced "Search messages" (Ctrl+Shift+F)
2. Click and type searchword
3.a Press Enter to start search (or Alt+S, or click Search)
3.b Press 2x TAB to focus results list first (important! this works around pre-existing bug 557990, so as to see the latest regression)
4. Press SPACE to select first result item if not selected yet (or 2x Ctrl+Space if already selected)
5. Press DEL key on keyboard

Comment 4

[Thomas D. (:thomas8)]

(In reply to Wayne Mery (:wsmwk) from comment #2)
> As mentioned in bug 1432537 I currently cannot reproduce on windows

Which version of Windows and Thunderbird exactly?
I'd be surprised if this does not reproduce for you if we're using the same versions...

Jörg, can you reproduce on Win 10 with Daily 62.0a1 (2018-06-14) (64-bit)?

Comment 5

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. (currently busy elsewhere) from comment #4)
> Jörg, can you reproduce on Win 10 with Daily 62.0a1 (2018-06-14) (64-bit)?
I had a few messages in the trash, did a search there. Hitting "delete" on the keyboard after selecting in the search result deletes the messages from the result and the original (trash) folder. I don't see a problem. That's on TB 60 beta 7.

Tried the same on a locally compiled Daily and there I see it. Hitting "delete" in the search results does nothing :-(

Alice, can you please find the regression here:
STR:
For any folder, do a search (like: right-click, search messages).
Search for anything you like.
In the result list, select one or more messages.
Hit "delete" on the keyboard.

Comment 6

[Alice0775 White]

At least on Tb52.8, You can not delete the searched messages that were selected for the first time with the [Del] key.
You need re-select the searched message after select the other message.
Above problem should be the other bug(may be Bug 557990).

Regression window :
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=9c6c4038b3b5ab074c1d9c2bf91bae0a062adaab&tochange=89435f04afee556e91d8f17634cd377dff5ac8d2
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b4aeb99d1cb601e5a5288ca05630913fa8528a1c&tochange=6862624e24d005fb4f8fb07c6800d2acef1d287e

Comment 7

[Jorg K (CEST = GMT+2)]

Thanks Alice!!!

Clearly a regression from bug 1446609. I was thinking that this came from on of the overlay removal bugs since they shuffled key assignments around, like here:
https://hg.mozilla.org/comm-central/rev/6bea4bcfa60f#l4.36

Richard, can you please take a look.

Comment 8

[Richard Marti (:Paenglab)]

Attachment: SearchDelete.patch

With this, the DEL key is working again.

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 8985793 [details] [diff] [review]
SearchDelete.patch

Thanks, that works for me. I also reran
  mozmake SOLO_TEST=search-window/test-search-window.js mozmill-one
since that was the test which was giving us trouble in bug 1446609.

TB 61 will be broken as well, but I don't think we'll ever ship a TB 61 beta.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/02374870bf1a
Add missing command in SearchDialog.xul to make the DEL key work agaian. r=jorgk

Comment 11

[:aceman]

Comment on attachment 8985793 [details] [diff] [review]
SearchDelete.patch

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

::: mail/base/content/SearchDialog.xul
@@ +51,5 @@
>        <command id="cmd_open" oncommand="goDoCommand('cmd_open')" disabled="true"/>
>        <command id="button_delete" oncommand="goDoCommand('button_delete')" disabled="true"/>
>        <command id="open_in_folder_button" oncommand="goDoCommand('open_in_folder_button')" disabled="true"/>
>        <command id="saveas_vf_button" oncommand="goDoCommand('saveas_vf_button')" disabled="false"/>
>        <command id="file_message_button"/>

If somebody could please investigate if this file_message_button command is needed for anything.

Comment 12

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #11)
> >        <command id="file_message_button"/>
> 
> If somebody could please investigate if this file_message_button command is
> needed for anything.

The messages are still moved without this line. Should this line be removed or let in as reference that this command exists?

Comment 13

[Thomas D. (:thomas8)]

(In reply to Richard Marti (:Paenglab) from comment #12)
> (In reply to :aceman from comment #11)
> > >        <command id="file_message_button"/>
> > 
> > If somebody could please investigate if this file_message_button command is
> > needed for anything.
> 
> The messages are still moved without this line. Should this line be removed
> or let in as reference that this command exists?

Before suggesting to remove a command, brute search for the command id is certainly advised...
It can't be removed, because you'd lose the enabled status of the button depending on selection (here poorly realized via observes attribute of the button which observes this command).

https://dxr.mozilla.org/comm-central/rev/51678c5b0b3edd827c57a70f1c5baf02d44acead/mail/base/content/SearchDialog.xul#192-198

>        <button id="fileMessageButton" type="menu" label="&moveButton.label;"
>                accesskey="&moveButton.accesskey;"
>                observes="file_message_button"
>                oncommand="MoveMessageInSearch(event.target)">
>          <menupopup type="folder" showFileHereLabel="true" mode="filing"/>
>        </button>

Gee. Ugly. Non-generic command handling code with specific parameters in the layout layer.
The target function should at least be one level higher in <command>'s oncommand attribute (I could fix that).
The command is not handled in nsSearchResultsController's doCommand() function because of bug 959494 / bug 461578, i.e. that so far event has not been made available there; see bug 959494, comment 5.

Comment 14

[:aceman]

Attachment: 1468940.patch - file_message_button

Not remove it, I just meant to move the implementation into it so it does not look empty and unused.

I have tried it and it seems not to regress bug 1319299.

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 8985981 [details] [diff] [review]
1468940.patch - file_message_button

I'm really not a great XUL reviewer, maybe Thomas can take a look here.

Comment 16

[Richard Marti (:Paenglab)]

Comment on attachment 8985981 [details] [diff] [review]
1468940.patch - file_message_button

Not really worth for ui-r as it's no change in behavior or ui. But you asked and here is the ui-r+. :-)

Comment 17

[:aceman]

The point of the UI review was to check that there is no change in behaviour :)

Comment 18

[Thomas D. (:thomas8)]

Comment on attachment 8985981 [details] [diff] [review]
1468940.patch - file_message_button

This code looks correct and we're using the same structure for shifted commands like cmd_reply (also using event), so that should work. I don't have time to update my test environment and test.

Comment 19

[Jorg K (CEST = GMT+2)]

Comment on attachment 8985981 [details] [diff] [review]
1468940.patch - file_message_button

Thanks, Thomas!

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7c8d92b04134
populate command file_message_button with its implementation. r=jorgk