200138 - Delete Mail Marked as Junk changes/forces selection

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect)

Description

[Pratik]

Thsi happens for IMAP mail and when "Mark message as deleted" option is set

1. Say you have 1 mail that Mozilla identified as Junk
2. Current focus is on some other message in teh same folder
3. Click on Tools -> Delete Mail marked as Junk

Actual Result
Mozilla deletes mail and moves selection to the junk mail

Expected Results
Mozilla should just delete the mail (i.e. mark it as deleted) and not change focus.

Comment 1

[Ray Trent]

I think I have a related (identical?) issue:

If I select the Junk folder so I can review my junk in case of false-positives,
usually I just want to look at the headers, note that they are all junk, and hit 
"Tools->Delete Mail Marked As Junk in Folder". 

However, when I do this, the sequence that occurs is this: all the junk is
selected in the folder, which displays the first junk message, then all the junk
is deleted, but the first junk message still displays. 

There are 2 problems with this sequence of events: 1) the junk shouldn't be
displayed when selecting all in preparation for deletion, and 2) even if #1
happens, mail should not continue to be displayed after it is deleted, even if
it's the only message left in the folder (this is perhaps a separate issue, and
I plan to file a bug on that once I can verify it). 

I'm also using an IMAP folder for my junk folder, but I don't have "Mark Message
as deleted" selected (I use "move to trash") (though for the life of me I can't
find that setting in the preferences right now :-0).

Comment 2

[Andrew Jones]

"...all the junk is selected in the folder, which displays the first junk
message, then all the junk is deleted, but the first junk message still displays."

This exact same thing happens to me with my POP account.

In my opinion, the junk mail being selected could be a pretty serious security
risk if the message contains malicious code that works in Mozilla.

Comment 3

[Andrew Jones]

I forgot to mention, I'm using nightly build ID 2003062508 on Windows Me.

Comment 4

[Mike Cowperthwaite]

*** Bug 211361 has been marked as a duplicate of this bug. ***

Comment 5

[Mike Cowperthwaite]

*** Bug 203855 has been marked as a duplicate of this bug. ***

Comment 6

[Mike Cowperthwaite]

The symptom of displaying a junk mail when all the mail in the folder is deleted 
is bug 205529, which maybe should be a dupe of this.

One of the dupes notes that if no message is opened (has the focus), then after 
deleting junk, there still should be no message opened.

I note that currently (1.4 Final), if only some of the mail is junk, one of the 
junk messages is momentarily opened, but then focus shifts to some other message 
after all have been deleted, opening that one.

Comment 7

[NilsE]

*** Bug 212014 has been marked as a duplicate of this bug. ***

Comment 8

[Ryan Grange]

Besides being a security risk, the act of opening up a spam email is that it
confirms to the spammer that you're a valid address and encourages even more
spam.  This can only be a good thing if you like giving your spam filters extra
work to do.

Comment 9

[Joe Infla]

*** Bug 218351 has been marked as a duplicate of this bug. ***

Comment 10

[Joe Infla]

*** Bug 205529 has been marked as a duplicate of this bug. ***

Comment 11

[Alfonso Martinez]

*** Bug 220342 has been marked as a duplicate of this bug. ***

Comment 12

[Frédéric Buclin]

Just received a junk mail in japanese which has been automatically redirected to
the Junk folder.
When going to that folder and applying "Delete mail marked as junk in folder",
this mail received the focus and has been loaded (while it was being deleted).

Mozilla froze for a few tens of seconds and my HD worked hard!!! I do not want
to know what was inside this mail but I really want Mozilla *not* to load such
messages!!!

JavaScript is not allowed to run in mails & newsgroups but this is definitely
not enough.

Comment 13

[Joe Infla]

*** Bug 220381 has been marked as a duplicate of this bug. ***

Comment 14

[jeisen]

I can confirm this bug if it's the same issue I have. I'm using POP email,
however. If I'm reading my inbox and find a piece of junk mail that hasn't been
caught, I press J to mark it as junk and remove it. This used to work just like
pressing delete, where it would just move onto the next message. Ever since the
last build I tried (last week) and then this one, pressing J will lose focus on
the inbox. Very irritating.

I'm using Windows 98 and Thunderbird nightly build 20041027.

Comment 15

[Mike Cowperthwaite]

People encountering the actual symptom of this bug may be interested in
bug 262636.

(In reply to comment #14)
> I can confirm this bug if it's the same issue I have.

Actually, it's unrelated.

> If I'm reading my inbox and find a piece of junk mail that hasn't been
> caught, I press J to mark it as junk and remove it. This used to work just
> like pressing delete, where it would just move onto the next message. Ever
> since the last build I tried (last week) and then this one, pressing J will
> lose focus on the inbox. Very irritating.

This sounds like bug 266066.

Comment 16

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1a2) Gecko/20060517 SeaMonkey/1.1a] (nightly) (W98SE)
[Mozilla Thunderbird, version 2 alpha 1 (20060517)] (nightly) (W98SE)

Bug still there, in both SM and TB.

Comment 17

[Mike Cowperthwaite]

This isn't really a focus-shift, it's a selection.

Comment 18

[Mike Cowperthwaite]

*** Bug 250437 has been marked as a duplicate of this bug. ***

Comment 19

[Mike Cowperthwaite]

*** Bug 345870 has been marked as a duplicate of this bug. ***

Comment 20

[Mike Cowperthwaite]

I think there used to be a visible symptom where one of the junked messages getting deleted would display momentarily, but I don't see that issue any longer.  The real problem, as reported originally, is that the selection changes from whatever it was to a single message -- I'm pretty sure the selected message is one that appeared in the list next to one of the junk messages, but I'm not sure of the details.

That newly selected message may be as-yet-unmarked junk, which is annoying; but it might be a desired message that was still Unread, and now gets marked Read before the user is ready for it.  And the original selection may have been the message the user was actually interested in.  Or, the original selection might have included one or more 

What is desired here is for the selection to revert, as closely as possible, to the original selection -- *and*, the 'current' indicator (that is, the dashed box that displays around a single message) should also revert, as closely as possible, to the original 'current'.

'Current':
If the original 'current' indicator was on one of the messages just deleted, the new 'current' should be placed on a surviving message either just below or just above the contiguous block of junk messages that contained the 'current' originally.  Otherwise, it should remain on the same message as before.

Selection:
1) If there was no selection before, there still should be no selection.
2) If the previous selection was entirely composed of messages that were just deleted, then no message should be selected after the deletion.
3) Otherwise, the selection should be on those messages that were selected before, less any that might have just been deleted.

There is an edge case in #3 which might cause minor annoyance: if the selection was composed of all the messages that were deleted plus one, after the deletion that plus-one will be selected, and so loaded into the message pane (which previously would have been empty), and marked read if it weren't already.  This is a general symptom of the single-message-selected => message loaded behavior; another example is using Ctrl+A to Select All in a folder that contains only one message.


The command is executed by deleteJunkInFolder() [mailCommands.js] which destroys the existing selection in order to perform the deletion.  Then it calls SetNextMessageAfterDelete() [msgMail3PaneWindow.js] which sets up a global or two; then performs the actual deletion; then clears the selection; then exits.  I have no idea where the new selection is actually put in place.
This will be a tricky one to fix.

Comment 21

[Kent James (:rkent)]

I'll look into this. It would be trivial to simply leave no selection after the delete (meeting the "forcing" part of the description). I also want to try deleting using folder commands instead of view commands.

Comment 22

[Kent James (:rkent)]

Attachment: Use folder commands to keep selection

For non-virtual folders, by switching to folder-based commands I was able to keep the selected messages correctly. For single-folder virtual folders, I still had to use the view commands, but I cleared the selection rather than let it point to some random message as before.

I also removed some trailing white space from junkCommands.js, and removed some debug warnings that were occurring when a thread based on a junk message could not be found after a delete.

Comment 23

[[e-mail]]

Comment on attachment 328835 [details] [diff] [review]
Use folder commands to keep selection

>+      gDBView.msgFolder.deleteMessages(junkMsgHdrs, msgWindow, false, false, null, true);
Hmm, what happens if the current message is junk?

>+  gNextMessageViewIndexAfterDelete = nsMsgViewIndex_None;;
Nit: double ;

Comment 24

[Kent James (:rkent)]

(In reply to comment #23)
>
> Hmm, what happens if the current message is junk?
>
No message will be selected. The c++ changes are there to remove some unneeded warnings that were coming up. Higher level code has comments that say nothing will be selected if the old message and thread keys are invalid, and that indeed seems to be the case, but the lower-level code still had warnings.

> >+  gNextMessageViewIndexAfterDelete = nsMsgViewIndex_None;;
> Nit: double ;
> 
Oops.

Comment 25

[[e-mail]]

(In reply to comment #24)
> (In reply to comment #23)
> > Hmm, what happens if the current message is junk?
> No message will be selected.
OK, but will the UI update appropriately? I guess I could always try it...

Comment 26

[Kent James (:rkent)]

(In reply to comment #25)
>
> OK, but will the UI update appropriately? I guess I could always try it...
> 
From my tests, the UI updates appropriately.

Comment 27

[[e-mail]]

(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #23)
> > > Hmm, what happens if the current message is junk?
> > No message will be selected.
> OK, but will the UI update appropriately? I guess I could always try it...
Hmm, when I tried it in SeaMonkey the messages were removed from the view, but since the loaded message was one of the junk messages I was expecting the message pane to be cleared and all the relevant toolbarbuttons to be disabled.

Comment 28

[[e-mail]]

Comment on attachment 328835 [details] [diff] [review]
Use folder commands to keep selection

OK, so it helps if I take out the debugging code I had in for a different patch ;-) Don't forget the semicolon.

Comment 29

[[e-mail]]

Comment on attachment 328835 [details] [diff] [review]
Use folder commands to keep selection

Trust me to think of some issues now, rather than earlier.
Anyway, I doubt it's a problem, but doing things this way means that you delete all junk messages in the folder including those that wouldn't have been visible before, whether because they're read in an unread view or there's a quicksearch or something limiting the view.

>+    junkMsgHdrs = Components.classes["@mozilla.org/array;1"]
>+                            .createInstance(Components.interfaces.nsIMutableArray);
Actually, isn't this missing a var?

>+  // We'll leave no selection after the delete
>+  gNextMessageViewIndexAfterDelete = nsMsgViewIndex_None;;
>   gDBView.doCommand(nsMsgViewCommandType.deleteMsg);
>   treeSelection.clearSelection();
>+  ClearMessagePane();
Doesn't clearing the selection do this already?
(In fact, no index after delete might also clear the selection)

Comment 30

[[e-mail]]

Run Filters on Folder also affects unviewed items, so that's OK.

Comment 31

[Kent James (:rkent)]

Attachment: Fixed nits, patch to checkin

Fixed Neil's nits. Concerning:

>>   treeSelection.clearSelection();
>>+  ClearMessagePane();
> Doesn't clearing the selection do this already?
> (In fact, no index after delete might also clear the selection)

You would think that would be true, but it was not, so I needed to add that clear. I just tried it again, the clear does not occur when I remove that statement.

Carrying over reviews, ready to check in.

Comment 32

[Mark Banner (:standard8)]

Checking in mailnews/base/resources/content/junkCommands.js;
/cvsroot/mozilla/mailnews/base/resources/content/junkCommands.js,v  <--  junkCommands.js
new revision: 1.5; previous revision: 1.4
done
Checking in mailnews/base/src/nsMsgDBView.cpp;
/cvsroot/mozilla/mailnews/base/src/nsMsgDBView.cpp,v  <--  nsMsgDBView.cpp
new revision: 1.320; previous revision: 1.319
done
Checking in mailnews/db/msgdb/src/nsMsgDatabase.cpp;
/cvsroot/mozilla/mailnews/db/msgdb/src/nsMsgDatabase.cpp,v  <--  nsMsgDatabase.cpp
new revision: 1.406; previous revision: 1.405
done

Comment 33

[Mike Cowperthwaite]

Why was this not marked Fixed?

I can verify it's working as expected in 3a2-0720, Win2K.

Comment 34

[Kent James (:rkent)]

(In reply to comment #33)
> Why was this not marked Fixed?
> 
Standard8 leaves it to the assignee (me) to mark fixed. I like to verify that it works before marking it.

Comment 35

[Mike Cowperthwaite]

OK!

V

Comment 36

[[e-mail]]

Does this fix bug 228949 in most cases?

Comment 37

[Kent James (:rkent)]

(In reply to comment #36)
> Does this fix bug 228949 in most cases?
> 

No, I can still reproduce the symptoms of bug 228949 on my build where bug 200138 is fixed.