[Tree][dogfood] deleting last visible message scrolls tree to the top.

VERIFIED FIXED in M12

Status

SeaMonkey
MailNews: Message Display
P2
normal
VERIFIED FIXED
19 years ago
14 years ago

People

(Reporter: Chris McAfee, Assigned: Alec Flett)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [PDT+] 12/10 (waiting for review))

Attachments

(3 attachments)

(Reporter)

Description

19 years ago
linux., win32.

I have, say, 6 messages showing in 3-pane window.
I delete them all, then inbox scrolls to some ?
random portion of my inbox, and I have to scroll
 all the way up &back down again for things
to show up in the right order again.

A 5-second reaction to this problem is "oh, I have
no more new mail" which is bad.

Updated

19 years ago
OS: other → All
QA Contact: lchiang → esther

Comment 1

19 years ago
Scott or Alec, who wants this one?

Comment 2

19 years ago
I thought at first this might be how I handle selecting the next message after
deleting, but that seems to be working, it's selecting the one before the last
one deleted.

I have seen a similar bug while clicking and I'm wondering if it's a tree widget
problem.  Sometimes when I click, the tree scrolls to a completely different
place and I have to scroll around until I figure out where the selection really
is.

Updated

19 years ago
Assignee: phil → alecf
(Assignee)

Updated

19 years ago
Summary: 3-pane: delete all visible messages, scrolling screws up → [Tree] delete all visible messages, scrolling screws up
(Assignee)

Comment 3

19 years ago
This may be fixed by some of the stuff hyatt & I did yesterday, to be checked in
today.
(Assignee)

Updated

19 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 4

19 years ago
ok, it's not fixed, but I think I know what's going on:
After all visible content has been deleted, the tree is listed as having no
frames...so when reflow happens, it goes to rebuild all the frames. Somehow,
GetFirstFrame() must be finding the first content element instead of the last
one... it seems like part of what needs to happen is that when you delete n
rows, you need to update the scrollbar to reset maxpos=maxpos-n, and set curpos
to curpos - n.

Updated

19 years ago
Blocks: 18951
(Assignee)

Updated

19 years ago
Priority: P3 → P2
Target Milestone: M12

Updated

19 years ago
Blocks: 20203
(Assignee)

Comment 5

19 years ago
*** Bug 18533 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

19 years ago
Summary: [Tree] delete all visible messages, scrolling screws up → [DOGFOOD][Tree] scrolling screws up when selection changes
(Assignee)

Comment 6

19 years ago
nominate for dogfood. This is really a bug for the larger problem of frames
being recreated when the selection in the tree changes. This problem also causes
things like, select the first message, scroll down, select the last message, and
the tree refreshes back to the top before you finish selecting the last message.

Updated

19 years ago
Whiteboard: [PDT+]

Comment 7

19 years ago
Putting on PDT+ radar.

Updated

19 years ago
Blocks: 20870
(Assignee)

Updated

19 years ago
Whiteboard: [PDT+] → [PDT+] 12/10
(Assignee)

Comment 8

19 years ago
ok, I think I've tracked this down to something with
nsCSSFrameConstructor::AttributeChanged, but it's hard to debug because
mouseovers are causing AttributeChanged to fire.
(Assignee)

Comment 9

19 years ago
as usual, hyatt rocks... Turns out OnContentInserted and OnContentRemoved both
had problems:
OnContentInserted was always setting the top frame to null, and it was blowing
away all frames after the current node being removed - which meant that if the
content was off-screen, above the tree, it would blow away the whole tree. When
the top frame was null, this meant the whole tree would be rebuilt from the top
content row

in OnContentRemoved, we were again always setting the topframe to null, when in
fact we wanted to keep it.. unless of course that was the frame that we're
deleting, in which case we just move down one frame.

checking in a fix as soon as the tree goes green.
(Assignee)

Updated

19 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 19 years ago
Resolution: --- → FIXED
(Assignee)

Comment 10

19 years ago
ffffiiixxxeeeeddd!
(Reporter)

Comment 11

19 years ago
wooohoooooo!

Updated

19 years ago
Status: RESOLVED → REOPENED

Updated

19 years ago
Resolution: FIXED → ---

Comment 12

19 years ago
Using builds 1999120808 on win98, mac and linux, this is not fixed as stated in
scenario.  I talked with alecf and showed him how it was working on my system.
He agrees it was not fixed, he fixed a similiar bug.  He will find it and mark
it fixed.  Reopening.
(Assignee)

Updated

19 years ago
Status: REOPENED → ASSIGNED
Summary: [DOGFOOD][Tree] scrolling screws up when selection changes → [Tree] scrolling screws up when selection changes
Whiteboard: [PDT+] 12/10
(Assignee)

Comment 13

19 years ago
Argh, so it looks like here's what happened with this bug:
- I thought this problem was more general than it was, so I updated the summary
from "delete all visible messages, scrolling screws up" to "scrolling screws up
when selection changes"... I fixed the general case of the problem, but the
specific case of scrolling to the top when you delete all visible messages still
exists.

Because the description of this bug is for the specific case, the bug has
morphed. I'd really like to morph it back, but it's marked PDT+.

I nominated the general case of the bug for dogfood though, so here's what I'm
going to do: Leave this bug reopened, but take off the [DOGFOOD] and [PDT+]ness
of the bug... then I'll log another bug for the more general problem, mark it
[DOGFOOD] and [PDT+] and them mark it fixed.
(Assignee)

Comment 14

19 years ago
ok, the new bug is #21194
(Assignee)

Updated

19 years ago
Summary: [Tree] scrolling screws up when selection changes → [Tree] deleting last visible message scrolls tree to the top.
(Assignee)

Comment 15

19 years ago
By the way, this particular bug is really tricky. Here's the issue: when we
delete the last frame in the toplevel frame, we're calling
mTopFrame->GetNextSibling(&mTopFrame) which sets mTopFrame to null... we're also
invalidating the cellmap, which means that there's no way for the tree to "catch
up" and figure out where it should be. I'm not even sure where it should be
though.. I guess we should just bring the last row into view... in 4.x we're
smart enough to bring in as many rows as we can to fill up the view.

My only thought is to either figure out what the content row before mTopFrame
was, and somehow set up the cellmap correctly.

Comment 16

19 years ago
I know how to fix this.

In OnContentRemoved, if mTopFrame becomes null, then you need to do the
following.

(1) Using the old mTopFrame (cache it before you update it), get its content
node.
(2) Get the next sibling of the top frame.  If you have one, it means that
there's a frame that needs to be "pulled up".
(3) Give the tree row group frame a little content chain and point it at this
new content node.
(4) Let the tree be marked as dirty (just as it is already).

On the reflow, you'll pick up the content node and do the right thing.

Updated

19 years ago
Summary: [Tree] deleting last visible message scrolls tree to the top. → [Tree][dogfood] deleting last visible message scrolls tree to the top.

Updated

19 years ago
Whiteboard: [PDT+]

Comment 17

19 years ago
we'd like to keep this on PDT+ and mark your new one PDT+ too. Putting back on
PDT+ radar.
(Assignee)

Comment 18

19 years ago
I'm attaching a proposed patch.
(Assignee)

Comment 19

19 years ago
Created attachment 3340 [details] [diff] [review]
proposed fix
(Assignee)

Comment 20

19 years ago
ok, that patch doesn't do anything useful best I can tell. Worth a try.
(Assignee)

Comment 21

19 years ago
ah, and I just found out why.
The reason is that FindPreviousRowContent fails because the content node has
already been removed from the content model... argh!

The parent is still set, so I might try fiddling with that.

Comment 22

19 years ago
There are some other issues with this function.  We need to talk about this in
person I think.
(Assignee)

Comment 23

19 years ago
Sure.
Just an update - I tried passing the node as the _downward_ hint, which should
immediately go to the parent and then pick the last child... this SEEMS like the
right thing to do (i.e. the content chain should be set up right, and mTopRow
should be null) but it still has the same problem.
I'll drop by tomorrow.
(Assignee)

Updated

19 years ago
Blocks: 21343
(Assignee)

Comment 24

19 years ago
ok, I talked to hyatt and I'm attaching a new patch
hyatt, mind reviewing?
take a look at bug 21343 in case you have any ideas, I don't think that bug is
very critical though.
(Assignee)

Comment 25

19 years ago
Created attachment 3369 [details] [diff] [review]
new fix - finds the previous row correctly.
(Assignee)

Updated

19 years ago
Whiteboard: [PDT+] → [PDT+] 12/10 (waiting for review)
(Assignee)

Comment 26

19 years ago
duh, I'm an idiot.
Better fix attached that doesn't have this wonderful side effect
(Assignee)

Comment 27

19 years ago
Created attachment 3394 [details] [diff] [review]
final fix (no goofy side effects)

Comment 28

19 years ago
Chofmann gives approval IF you have run the smoke tests, and reviewed etc.
(Assignee)

Comment 29

19 years ago
heh, thanks for the reminder. I've tested mail, I'll bookmarks/etc
(Assignee)

Comment 30

19 years ago
(in addition to regular smoke tests,etc)
(Assignee)

Updated

19 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 19 years ago19 years ago
Resolution: --- → FIXED
(Assignee)

Comment 31

19 years ago
done! Fixed!

Updated

19 years ago
Status: RESOLVED → VERIFIED

Comment 32

19 years ago
Using builds 1999121209m12 on win98, and build 1999121308m12 mac and linux, this
is fixed as stated in scenario.  Verified

Updated

18 years ago
No longer blocks: 18951

Updated

18 years ago
No longer blocks: 20203

Updated

18 years ago
No longer blocks: 20870
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.