128446 - (double?)clicking on 'name' tab reverses account sort order

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Liam O'Connell]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.8) Gecko/20020204
BuildID:    2002020415

(double-?)Clicking on the 'name' tab in the mail window sometimes rearranges the
order in which mail/news accounts are shown. clicking again doesn't put them
back into normal order.

Reproducible: Sometimes
Steps to Reproduce:
1. open mail
2. click on 'name' tab above account list
3. try to click again to undo the effect and fail

Actual Results:  news account are now at the top, imap account is at the bottom.
doing this again doesn't reverse it.

Expected Results:  clicking again puts everything back into previous order, or
it doesn't get done in the first place. to undo you have to close mail and open
it again

this only happens occasionally (normally by accident when I mean to click on
'get messages'). it's hard to reproduce deliberately but furiously clicking on
the tab sometimes does the trick. (sometimes this will also undo the effect)

Comment 1

[laurel]

I can't reproduce this with 02-28 or 03-01 commercial trunk on linux rh6.2.
I know you said this happens infrequently.

Comment 2

[laurel]

I just saw this.

Comment 3

[laurel]

Win98, mac OS 10.1, too.

Comment 4

[laurel]

OK, this not only reverses the account sort order, but will also reverse the
folder order within the default mail account (didn't notice this in the other
mail accounts).  So your inbox can wind up at the bottom of the folder list for
the account even when the account appears at the top of the folder pane.  It is
not the easiest thing to get back out of this state, either.

Comment 5

[James Rome]

On the 5/1 win2k build I clicked on "Name" by mistake and the
accounts are ordered alphabetically now. But I liked my mail accounts first and
there seems to be no way to restore the original order!

How do I recover the order?

What should happen is that one should be able to drag the accounts into the
correct order.

Comment 6

[David Campbell]

I've found that if you click on the "N" in "Name", that clicking actually does
something, but if you click elsewhere, it does nothing.

Comment 7

[Liam O'Connell]

Re comment #6: Nice, this seems to make it 100% reproducible. Silly I didn't
spot this. Clicking on 'N' doesn't seem to reliably undo the state change
though, and I'm sure when 'randomly' clicking I've seen it spontaneously revert
to the original sort order.

Comment 8

[laurel]

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

Comment 9

[laurel]

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

Comment 10

[Myk Melez [:myk] [@mykmelez]]

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

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: fixes problem

The problem is that nsTreeBodyFrame::GetCellAt does "*aRow =
(y/mRowHeight)+mTopRowIndex;" to determine what row the user clicked on, but
"y/mRowHeight" rounds towards zero, causing header row clicks in which y is
negative and mRowHeight is some sufficiently large number to round to zero,
making it look like the user clicked on the first row of the tree instead of
the header row.

Normally FolderPaneOnClick cancels the click event anyway to prevent it from
bubbling up to the sort listener, but if the user clicks on the far left
portion of the "name" column header, then nsTreeBodyFrame::GetCellAt, which
thinks the user clicked in the first row of the tree, reports that he clicked
on the twistie in that row, which causes FolderPaneOnClick to bypass the code
that cancels the click event (and thus allows the sort listener to fire and
resort the tree).

This patch fixes nsTreeBodyFrame::GetCellAt to return row=-1 when the user
clicks on the header row and modifies FolderPaneOnClick to cancel the click
event when that happens.  It also fixes nsTreeBodyFrame::GetRowAt to return
row=-1 in the same situation.

As far as I can tell, that's the return value those functions intended to give
in this situation, so this change shouldn't cause any problems, but it might be
useful to audit the various bits of code in the tree that use these functions
to make sure they aren't making assumptions about their return values.

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 112653 [details] [diff] [review]
patch v1: fixes problem

Jan, can you review the tree-view-specific parts of this code?	Afterwards I'll
ask someone from the mailnews team to review the mailnews-specific parts.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Note bug 190641 for removing the sort direction indicator on the column header.

Comment 14

[Jan Varga [:janv]]

Comment on attachment 112653 [details] [diff] [review]
patch v1: fixes problem

r=varga although you may want to check for negative y right after calling
AdjustEventCoordsToBoxCoordSpace

Comment 15

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 112653 [details] [diff] [review]
patch v1: fixes problem

Cavin, could you review the mailnews-specific portion of this bug fix?

Comment 16

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

Comment on attachment 112653 [details] [diff] [review]
patch v1: fixes problem

sorry for the delay.

thanks for fixing this myk.

r/sr=sspitzer on the mail parts.

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: addresses Jan's suggestions

This version checks for negative y right after calling
AdjustEventCoordsToBoxCoordSpace and removes the redundant conditions "*_retval
< mTopRowIndex" and "*aRow < mTopRowIndex", which will never be true for y >=
0.

Comment 18

[Jan Varga [:janv]]

Comment on attachment 112813 [details] [diff] [review]
patch v2: addresses Jan's suggestions

looks good
r=varga

Comment 19

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 112813 [details] [diff] [review]
patch v2: addresses Jan's suggestions

Scott, can you super-review the layout part of this patch?  Seth already
super-reviewed the mail part.

Comment 20

[Scott Collins]

Comment on attachment 112813 [details] [diff] [review]
patch v2: addresses Jan's suggestions

sr=scc

Comment 21

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 112813 [details] [diff] [review]
patch v2: addresses Jan's suggestions

This patch makes two changes:

First, it fixes a XUL tree bug that caused events in tree headers to be
reported as occurring in the first row of the tree.

Second, it fixes the event listener for folder pane clicks to handle tree
header clicks now that they are being reported correctly.

I tested this patch on Linux, and it should have no cross platform
implications.  I also looked at all other code in the tree that calls the same
XUL tree functions and verified that no other code relies on the previously
erroneous behavior.

Comment 22

[Asa Dotzler [:asa]]

Comment on attachment 112813 [details] [diff] [review]
patch v2: addresses Jan's suggestions

a=asa (on behalf of drivers) for checkin to 1.3beta.

Comment 23

[Myk Melez [:myk] [@mykmelez]]

Reassigning to author of patch.

Comment 24

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

going to test and land this patch now...

Comment 25

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

fixed checked in for myk.

thanks, myk

Comment 26

[R.K.Aa.]

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