Closed Bug 57139 Opened 24 years ago Closed 24 years ago

Subject list scrolls undesirably while reading messages

Categories

(SeaMonkey :: MailNews: Message Display, defect, P3)

Product:
SeaMonkey
Component:
MailNews: Message Display
Platform:
x86
Windows NT
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: jar, Assigned: hyatt)

References

Details

(Whiteboard: [rtm++])

Attachments

(4 files)

Part 1 of 3
10.69 KB, patch
Details | Diff | Splinter Review
Part 2 of 3
4.66 KB, patch
Details | Diff | Splinter Review
Part 3 of 3
1.39 KB, patch
Details | Diff | Splinter Review
Comprehensive single patch for the trunk.
28.89 KB, patch
Details | Diff | Splinter Review
a) Go into non-threaded view for inbox b) Select reverse chronological order (re: most recent messages at top of subject list) c) Scroll down one or two pages of subjects from the top of your inbox d) Select a message (as though reading messages sequentially) e) Have a message arrive in your inbox Expected result: No change in scrolling position of messsage subject list Undesired result: Message list scrolls to top of list so that you can see the new subject header. This is nice 'cause you get to see the new mail... but evil because you loose your position in the subject list and must scroll your way back down to continue your reading :-( I thought this was filed... but now I'm told it is was not. Side possibility: It *might* be that the thumb in the elevator bar for the subject pane remains at the "lower level," even though the actual visible list of subjects "scrolls" to the top of the list.
Since mail folks were pretty alarmed about this bug (relative to other usabiliity bugs they have addressed), I'm nominating this for rtm. Note that unless this had a Really Small fix, I don't believe there is much justification for repairing this at this late date.
Adding Jar's RTM keyword. Jar, are you also experiencing form submit problems ?-) Scott, do we agree the behavior is incorrect? What does 4.x do? What's your estimate of the change required?
Assignee: selmer → putterman
Keywords: rtm
Whiteboard: [need info]
reassigning to hyatt. I don't know the answer to this. I'm pretty sure that we're just telling RDF we have a new item and when it gets put in the top, it's scrolling there. The opposite doesn't happen when you are adding to the bottom.
Assignee: putterman → hyatt
QA Contact: esther → laurel
Clearing need info, copying trudelle. Peter, can you evaluate for RTM?
Whiteboard: [need info]
This doesn't sound like it fits the profile for RTM, since the inconvenience is so slight, and presumably affects very few users. However, on Win98 this scrolls seemingly without updating the srcollbar, so they get out of whack, and subsequent attempts to scroll up just smear the new message. Possibly worth some investigation for RTM, so setting rtm need info. Still a P3 though.
Whiteboard: [rtm need info]
Target Milestone: --- → M18
Just for the record... the inconvenience is very great, assuming you have a large inbox, and have automated checking for email (every n minutes). Simply put, you can be in the middle of your inbox reading a message, and the entire message list will "scroll away." You have *no* way to get back where you were reading messages prior to the auto-scroll. In nav 4.x, you could just re-sort the list, and the "current" message would appear in the visible area. This is not true in Netscape 6. There is, IMO, no way to just continue reading your email. The closest you get is to turn off automatic checking of email... but I consider that a very nice feature, and a shame to loose because of this bug :-/.
This is a bug in the RDF sort service. The sort service is actually removing and re-inserting the outermost tree children when new messages come in. The sort service should only ever do this when the sort being imposed on the tree changes. It cannot do this just because new messages have come in. Maybe this was a perceived performance optimization to avoid multiple notifications, but it should be undone if so. Even if I enhanced the treechildren so that it could save and restore its scroll position, new messages are being inserted, so I wouldn't be able to restore correctly anyway if yanked. We should patch the sort service to behave more nicely. I'm smart about not wasting time if the insertion happens offscreen, so I don't need the sort service to be yanking my outermost row group (treechildren) out of the tree and putting it back in unless a resort has actually occurred. Reassigning to waterson.
Assignee: hyatt → waterson
Note that if this is fixed, then bug #57483 will become more important, since instead of scrolling up to the top, you'll end up staying where you are, but the scrollbar will end up out of sync with the tree. I'm going to connect these two bugs with a dependency.
Depends on: 57483
hyatt says I'm off the hook.
Assignee: waterson → hyatt
I have a one-line patch that fixes this problem... however, note that 57483 then becomes the issue. Given that with the jumping you're also out of sync, fixing this bug alone does put you in a better situation, i.e., you're still out of sync, but at least you didn't jump.
Status: NEW → ASSIGNED
Attached patch Part 1 of 3 — — Splinter Review
Attached patch Part 2 of 3 — — Splinter Review
Attached patch Part 3 of 3 — — Splinter Review
Blocks: 57280
Jeepers... what happened to the one-line-fix that would get me most of the way back to Kansas?? These three patches don't look especially safe at this point in the RTM cycle :-( :-(. Any chance you'll fess up with the one liner???
The one-liner didn't work. :( It was much more involved than I thought. Basically the tree widget gets totally screwed whenever content is removed, inserted, or appended above you offscreen. It also gets screwed on async appends (which is what FTP was doing). This patch fixes all of those problems, but yeah, it's bigger.
The code we currently have in these areas is *known* to be very *unsafe*. Rather than just the usual r=/a=, I suggest we get a team of software proctologists to examine every token of these patches, land them on the trunk, and have QA run every test known to man. We *need* these problems fixed. cc bryner, alecf for starters. Who else?
(this is Jar with his PDT hat off) hmmm... just for the record... other folks were not especially sympathetic to this bug of mine. It sounds like you now understand the issue well enough that you *might* be able to illuminate even more evil scenarios, and gather some support... Please do so if you can (i.e., describe evil side effects in all their glory). As Peter noted (and I paraphrase here) "the patch is big." I'm sure the PDT would need a very crisp understanding of the problem, and a concise explanation of why the fix works, and won't break any other working parts of the universe, before even contemplating such a landing. Bottom line: In addition to the patch, a double plus would require a strong benefit statement, and a compelling exposition that risk was closer to zarro than Peter's (and my) observation would suggest. Thanks, Jim (who personally wishes this bug gets fixed... but won't hold breath or N6)
Jim, this patch does have more broad benefits than fixing the issue you reported here. It looks like hyatt's patches address all sorts of tree manipulation problems. This goes beyond this bug as it would also affect the FTP and file system views in Mozilla. Currently, those views are broken in a way which amounts to data loss (bug 57280, which depends on this). Having a tree view which actually works (the current tree widget simply does not work in many mainstream cases) will significantly increase the appearance of Mozilla as a quality product. Shipping NS6 with the current tree widget will give the appearance of a very shoddy product, as the users will not be able to browse FTP sites or their own local disks, and as you note there are also UI nuisances. Hope that helps.
were these made against the branch or the trunk? they're not patching cleanly against the branch.
I had problems on both trees, partly due to embedded ctrl-Ms but one of the files seems to have changed enough that I don't think I can do a confident job hand editing like I had to with the rest. David, could you attach nsCSSFrameConstructor.cpp verbatim or mail to me? Branch would be preferrable, but I will take whatever you have.
adding self to cc list. If we could get clean patches for the branch, I'd be happy to try them out.
Patching nsCSSFrameConstructor.cpp on the branch to account for these changes is nontrivial. I'm not going to waste time doing it until I know these bugs will actually be approved on the branch. You should just test on the trunk. if they approve the bugs, then the tree widget on the trunk will become exactly equivalent to the one on the branch when they are landed.
Blocks: 56979
No longer depends on: 57483
Blocks: 57483
I hate to say this, but I still see problems in the thread pane on the trunk. They are mostly paint type problems (e.g., the wrong message looks selected, or selection looks multiple when it's only single) and the problem where the thread pane stops painting, I think caused by expanding a large thread at the bottom of the thread pane or at the top.
Was this checked into the trunk?
hmm, maybe not. I thought they were, for some reason.
No, this isn't checked in yet. I need an r= and an a= please.
I see the stop-painting problem and the message-looks-selected problem on the branch all the time... most of the time on linux, but occasionally on windows too
This patch seems like a huge improvement for the mail/news thread pane. Things like multiple deletes are a lot smoother, expanding a large thread is much faster (is it possible that these changes would speed that up?), I haven't seen the selection get weird yet, or had things jump around like crazy. And, when I create a new local folder, it shows up w/o having to collapse and expand the server. So far, so good.
Yes, open/close will be faster with this patch, because the parent is no longer reframed (only the kids).
I see something like a 3x speedup on expanding a thread of 30 messages - it's more or less instantaneous, whereas before, there was a big pause. waterson or alec, can we get an r=? I'd review it, but it wouldn't be meaningful since I don't know the code :-)
hyatt and I were looking at one failure mode last night, where biff arrival of messages into a threaded view of the threadPane could confuse the tree. However, I do agree that most operations are more stable, and even when I could get it "confused", it was relatively easier to recover (and I haven't made it crash at all).
Ok, I've tried to look this over and I just don't know enough about the tree anymore...(it's changed alot since I poked at it) I wish I could confidently say r=alecf, but I just can't :(
yes, I've seen that too - what happens is after deleting the newly arrived mail, the selection gets a bit confused (the deleted line doesn't go away, and the selection looks multiple). It might have to do with the newly arrived mail getting added to the end of the thread pane, but I'm not positive. That happened without the patch too.
bienvenu, did you see this in unthreaded mode or threaded mode?
always threaded, *but* the newly arrived message was not part of another thread, i.e., it was its own thread, as was the message that was selected after the newly arrived message was deleted. This makes me suspect that it might happen in non-threaded mode too, but most of my folders are threaded.
Do you sort by date, and if so, do your new messages come in at the top or bottom? I want to start running with your setup so I can debug these glitches.
yes, sorted by date, and new msgs come in at the bottom (or sometimes right before the bottom). I'll try to see if I notice a pattern as to when it happens.
Dave, I've noticed that this bug happens coincidentally with the message headers not showing up bug, and that when I read a message, the unread count in the folder pane is not updated either - it's as if all layout changes are blocked until I move the splitter. Don't know if this helps...
to be clear, here's what happens: 1. I open a folder with unread messages in it. 2. Scroll to the bottom 3. Select the last message (which is unread) The message header area does not appear. The message does not noticeably get marked read, nor does the folder unread count in the folder pane go down. I can mark other messages read or unread by clicking on the green diamond, but the state change is not reflected in the UI until I move the folder/thread pane splitter. Very odd.
Got some nice clear steps to reproduce?
D'oh. You just answered me.
jrgm, could this be one of those "magic height" kinda things, i.e., if you set the splitter to certain heights, the tree gets messed up?
it seems to happen when we had to download new headers for an imap folder. I'm not sure why that should matter since we download the headers before we tell rdf about any of the headers. I'll look into it a little more.
Possibly. I just tried bienvenu's steps, and didn't get this behaviour (opt. win2k with your patch). I'll try varying the size of the threadPane. One question though: when you say "The message header area" are you referring to the "Subject:, From:, Date:" toolbar?
Yes, that's what I'm referring to. But I now think those not appearing is orthogonal to the other bugs - the headers can disappear when the other layout/repaint bugs don't happen. I haven't seen it the other way around (the headers appear but the other bugs happen) but that could be because the headers almost always disappear when I open a folder. Hope that makes sense!
I saw what you described (where the message headers didn't come in, and the Read count was not changed in the UI, plus when I scrolled I began to get blank lines), but haven't quite been able to do it again. I think that this must be on a newly visited folder (no message selected, and the last message at the bottom must be partially covered by the splitter.
looks good to me, although it would be nice to get this problem bienvenu is seeing fixed. r=bryner
But that problem is not caused by this patch, is it?
No, that bug existed before this patch. I think bryner is just saying that it would be good to nail all the problems we know about at once. I've been running with this patch for a few days and it seems great. Aim is working fine as well (I heard there were some tree problems with AIM as well)
Yeah, that's what I figured, but I want to ensure that everything is explicit and clear for PDT triage. We may only have one more chance at triage, so I want to get ++ on Monday, not 'need info'. I don't think that adding to this patch to address more cases would be a good idea at this point, because it would mean less testing time, but if we get a second chance, fixing that may be worthwhile.
Here's another clue - when I'm in this mode, if I cause a repaint to happen (by bringing another window forward and then bringing the mail window back to the top), the folder pane repaints correctly, and the thread pane mostly repaints correctly, so it looks like reflow is happening, but lines in the tree aren't getting invalidated. I say "mostly repaints correctly" because I did get one of the lines painted blank.
a=ben@netscape.com
I inflicted the a= process on ben. Thanks for taking the time to listen to the brain dump, ben!
Whiteboard: [rtm need info] → [rtm+]
that a=ben@netscape.com was from me, but posted from hyatt's machine.
Whiteboard: [rtm+] → [rtm+] FIXED ON TRUNK
PDT: This fixes about 6 or 7 filed rtm bugs on the threadpane, and it makes everything feel MUCH better when dealing with trees. Although the patch is large, the effects are a definate winner for RTM.
This bug is in candidate limbo. We will reconsider this fix once we have a candidate in hand, but we can't take this fix before then. Please check into the trunk ASAP.
Marking RTM double plus. Per discussion at PDT, we would like this checked into the BRANCH this evening (Tuesday eve, 10/30). We have a tag of the build done earlier in the evening, and would like to have tomorrow morning's build include this checkin. As discussed, this is something of a "carpool" for a larger than average limbo bug. Thanks, Jim (for PDT, with fingers crossed... shooting the limbo moon) Roskind
Whiteboard: [rtm+] FIXED ON TRUNK → [rtm++] FIXED ON TRUNK
fixed
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Whiteboard: [rtm++] FIXED ON TRUNK → [rtm++]
jar - do you see this bug any longer with the limbo builds?
Looks OK to me using nov02 (limbo2) mn6 commercial branch build with win98, linux rh6.0 and mac OS 9.0
Keywords: vtrunk
This bug is fixed as far as I can see. I no longer see the strange scrolling behaviour. There is one tiny artifact left, but it might be similar to what happens in 4.x. If I'm reading email, and the scroll bar in the subject pane is at the very top (i.e., I can see the subject line for the most recent message), but I'm reading (for example) the fourth message down (and hence it is highlighted), and a new message arrives.... then the message pane scrolls when a new message arrives. IMO, the arrival of a new message should cause the creation of a new subject line *above* the top of the header window, and out of view. (Recall that my messages are in reverse chronoligical order, so new messages go to the TOP of the list). Note that if I can't see the first message, then arrival of new messages *don't* cause scrolling of the subject pane. Oh well... subtle point... but the basic bug I hit is Very Fixed now. Thanks, Jim
The original problem is OK using commercial trunk builds, dec6, linux rh6.0, Win98 and Mac OS 9.0. I'll log the remaining issue jar mentions (I see the problem) as a separate bug.
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: