130982 - delete/move context menu command attempts to restore focus to invalid row

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Sean Su]

steps to reproduce:
1) left-click on a new folder in the folder pane (this ensures that
   there isn't a message loaded in the message pane).  Make sure that
   this folder has more messages than can be shown in the thread pane.
2) scroll down to the last message in the thread pane.
3) right-click on the last message (or any message here) and select
   the delete message menu item.

Notice that it scrolls back to the top attempting to show the row above the
first possible row.

Comment 1

[Sean Su]

Attachment: patch v1.0

Comment 2

[Aleksey Nogin]

I see this on RedHat Linux 7.2, BuildID 2002031319, so marking OS:ALL.

Comment 3

[Olga]

Great catch Sean!

Comment 4

[[e-mail]]

Comment on attachment 74137 [details] [diff] [review]
patch v1.0

This is a bit of a guess but wouldn't it be easier to move the Ensure call
under the if (... >= 0) condition?

Comment 5

[Sean Su]

no, that wouldn't be correct.  There are actually two problems here:
 1) Delete/Move To commands ensure that the reselected row is visible.
 2) Calls to ensureRowIsVisible() is sometimes being passed in a value
    < 0.

To fix 1), EnsureRowInThreadOutlinerIsVisible() needs to be removed from
HandleDeleteOrMoveMsgCompleted().

To fix 2), EnsureRowInThreadOutlinerIsVisible() needs to not call
ensureRowIsVisible() for indexes < 0.

Comment 6

[racham]

Comment on attachment 74137 [details] [diff] [review]
patch v1.0

r=bhuvan

Comment 7

[(not reading, please use [e-mail] instead)]

part of this change is to the heavily hacked on HandleDeleteOrMoveMsgCompleted()

fix one bug, you regress an old one, or cause a new one.

Why was that call there in the first place?  can you use cvsblame to help figure
that out, and then if it was added for a bug, see if you change doesn't regress it.

Comment 8

[Sean Su]

Attachment: patch v1.1

This new patch will only patch problem 2).  Problem 1) has been filed as bug
131307.

Comment 9

[Sean Su]

Attachment: patch v1.2

added comments to the previous patch.

Comment 10

[Sean Su]

Comment on attachment 74434 [details] [diff] [review]
patch v1.2

moving r= from the earlier patch for problem 2).
got sr=sspitzer for problem 2).

Comment 11

[Asa Dotzler [:asa]]

Comment on attachment 74434 [details] [diff] [review]
patch v1.2

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 12

[Sean Su]

patch checked in to trunk only.

Comment 13

[(not reading, please use [e-mail] instead)]

yes, sr=sspitzer on that patch.

thanks for splitting it up.

Comment 14

[Olga]

Verified on trunk build at 04/22/02 on Linux, Win2K, Mac 10.1.3. 
Focus remains on the selected folder. No message loaded in the message pane.