Closed Bug 1468940 Opened Last year Closed Last year

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

Categories

(Thunderbird :: Search, defect)

All
Windows
defect
Not set

Tracking

(thunderbird61 wontfix, thunderbird62 fixed)

RESOLVED FIXED
Thunderbird 62.0
Tracking Status
thunderbird61 --- wontfix
thunderbird62 --- fixed

People

(Reporter: bugzilla2007, Assigned: Paenglab)

References

(Blocks 2 open bugs)

Details

(Keywords: regression)

Attachments

(2 files)

+++ 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.
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?
Flags: needinfo?(vseerror)
Flags: needinfo?(richard.marti)
Flags: needinfo?(acelists)
As mentioned in bug 1432537 I currently cannot reproduce on windows
Flags: needinfo?(vseerror)
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
(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)?
Flags: needinfo?(jorgk)
(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.
Flags: needinfo?(jorgk) → needinfo?(alice0775)
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
Flags: needinfo?(alice0775)
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.
With this, the DEL key is working again.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Flags: needinfo?(richard.marti)
Flags: needinfo?(acelists)
Attachment #8985793 - Flags: review?(jorgk)
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.
Attachment #8985793 - Flags: review?(jorgk) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/02374870bf1a
Add missing command in SearchDialog.xul to make the DEL key work agaian. r=jorgk
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 62.0
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.
(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?
(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.
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.
Attachment #8985981 - Flags: ui-review?(richard.marti)
Attachment #8985981 - Flags: review?(jorgk)
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.
Attachment #8985981 - Flags: feedback?(bugzilla2007)
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+. :-)
Attachment #8985981 - Flags: ui-review?(richard.marti) → ui-review+
The point of the UI review was to check that there is no change in behaviour :)
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.
Attachment #8985981 - Flags: feedback?(bugzilla2007) → feedback+
Comment on attachment 8985981 [details] [diff] [review]
1468940.patch - file_message_button

Thanks, Thomas!
Attachment #8985981 - Flags: review?(jorgk) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/7c8d92b04134
populate command file_message_button with its implementation. r=jorgk
You need to log in before you can comment on or make changes to this bug.