Subject list scrolls undesirably while reading messages

VERIFIED FIXED in M18

Status

P3
major
VERIFIED FIXED
18 years ago
7 years ago

People

(Reporter: jar, Assigned: hyatt)

Tracking

Trunk
x86
Windows NT
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [rtm++])

Attachments

(4 attachments)

(Reporter)

Description

18 years ago
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.
(Reporter)

Comment 1

18 years ago
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.

Comment 2

18 years ago
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]

Comment 3

18 years ago
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

Updated

18 years ago
QA Contact: esther → laurel

Comment 4

18 years ago
Clearing need info, copying trudelle. Peter, can you evaluate for RTM?
Whiteboard: [need info]

Comment 5

18 years ago
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
(Reporter)

Comment 6

18 years ago
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 :-/.
(Assignee)

Comment 7

18 years ago
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
(Assignee)

Comment 8

18 years ago
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

Comment 9

18 years ago
hyatt says I'm off the hook.
Assignee: waterson → hyatt
(Assignee)

Comment 10

18 years ago
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
(Assignee)

Comment 11

18 years ago
Created attachment 17855 [details] [diff] [review]
Part 1 of 3
(Assignee)

Comment 12

18 years ago
Created attachment 17856 [details] [diff] [review]
Part 2 of 3
(Assignee)

Comment 13

18 years ago
Created attachment 17857 [details] [diff] [review]
Part 3 of 3
(Assignee)

Updated

18 years ago
Blocks: 57280
(Reporter)

Comment 14

18 years ago
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???
(Assignee)

Comment 15

18 years ago
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.

Comment 16

18 years ago
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?
(Reporter)

Comment 17

18 years ago
(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)

Comment 18

18 years ago
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.

Comment 19

18 years ago
were these made against the branch or the trunk? they're not patching cleanly
against the branch.

Comment 20

18 years ago
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.

Comment 21

18 years ago
adding self to cc list. If we could get clean patches for the branch, I'd be
happy to try them out.
(Assignee)

Comment 22

18 years ago
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.

Updated

18 years ago
Blocks: 56979

Updated

18 years ago
No longer depends on: 57483

Updated

18 years ago
Blocks: 57483

Comment 23

18 years ago
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.

Comment 24

18 years ago
Was this checked into the trunk?

Comment 25

18 years ago
hmm, maybe not. I thought they were, for some reason.
(Assignee)

Comment 26

18 years ago
No, this isn't checked in yet.  I need an r= and an a= please.

Comment 27

18 years ago
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
(Assignee)

Comment 28

18 years ago
Created attachment 18117 [details] [diff] [review]
Comprehensive single patch for the trunk.

Comment 29

18 years ago
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.
(Assignee)

Comment 30

18 years ago
Yes, open/close will be faster with this patch, because the parent is no longer 
reframed (only the kids).

Comment 31

18 years ago
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 :-)

Comment 32

18 years ago
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). 

Comment 33

18 years ago
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 :(

Comment 34

18 years ago
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.
(Assignee)

Comment 35

18 years ago
bienvenu, did you see this in unthreaded mode or threaded mode? 

Comment 36

18 years ago
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.
(Assignee)

Comment 37

18 years ago
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.

Comment 38

18 years ago
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.

Comment 39

18 years ago
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...

Comment 40

18 years ago
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.
(Assignee)

Comment 41

18 years ago
Got some nice clear steps to reproduce?
(Assignee)

Comment 42

18 years ago
D'oh.  You just answered me.
(Assignee)

Comment 43

18 years ago
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?

Comment 44

18 years ago
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.

Comment 45

18 years ago
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? 

Comment 46

18 years ago
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!

Comment 47

18 years ago
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

Comment 49

18 years ago
But that problem is not caused by this patch, is it?

Comment 50

18 years ago
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)

Comment 51

18 years ago
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.

Comment 52

18 years ago
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.
(Assignee)

Comment 53

18 years ago
a=ben@netscape.com
(Assignee)

Comment 54

18 years ago
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.
(Assignee)

Updated

18 years ago
Whiteboard: [rtm+] → [rtm+] FIXED ON TRUNK

Comment 56

18 years ago
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.

Comment 57

18 years ago
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.
(Reporter)

Comment 58

18 years ago
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
(Assignee)

Comment 59

18 years ago
fixed
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
Whiteboard: [rtm++] FIXED ON TRUNK → [rtm++]

Comment 60

18 years ago
jar - do you see this bug any longer with the limbo builds?

Comment 61

18 years ago
Looks OK to me using nov02 (limbo2) mn6 commercial branch build with win98,
linux rh6.0 and mac OS 9.0

Keywords: vtrunk
(Reporter)

Comment 62

18 years ago
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

Comment 63

18 years ago
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.