Closed Bug 89628 Opened 23 years ago Closed 23 years ago

Message pane remembers scroll position from message to message

Categories

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

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

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.3

People

(Reporter: scottputterman, Assigned: mscott)

References

Details

(Whiteboard: [PDT+], have reviews, ready for the branch)

Attachments

(2 files)

force mailnews to always bypass session history whenever we load a url
4.85 KB, patch
Details | Diff | Splinter Review
patch to docshell
2.08 KB, patch
Details | Diff | Splinter Review 
Using the 7/6 branch build. 

Open a message that requires a scrollbar. Scroll down.
Open another message that requires a scrollbar.  When you view this message,
instead of being at the top of the message it opens up in the same position that
the first message was left in.
Adding nsbranch because I think this will be a very bad user experience.
Keywords: nsBranch
Priority: -- → P1
Target Milestone: --- → mozilla0.9.3
I don't thinks this is too big a deal. The thread pane has always had the same
behavior for me.  clicking between a large and a small folder and the scroll
position is remembered.....
I agree the thread pane shouldn't do that either, but people don't switch
folders very often.  Messages get changed all of the time.  This is a regression
(at least it works on the 7/3 trunk build).  It's pretty annoying to have to
scroll up your message almost every time you switch messages (I say almost
because if the new message doesn't need a scrollbar then it isn't a problem).
ahh I didn't think it was a regression. Doesn't look like anything changed in
mail on the branch with that iframe. 

guess we'll have to start looking at layout to see what changed on the branch.
*** Bug 89607 has been marked as a duplicate of this bug. ***
FYI, this works fine on the 7/5 branch build.
adding PDT+ per Sol.

there are a few bugs out there about web pages not finishing loading.  Any
possibility that the scrollbar isn't getting whatever event it needs to get to
reset itself to the top?
Whiteboard: [PDT+]
I see this too.
Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9.2) Gecko/20010706 Netscape6/6.1
I can't reproduce this on a trunk build from today. Branch only I wonder?
Seems like it.  I also couldn't reproduce this on the trunk.
I have a debug build from Thursday afternoon containing 70% of the checkins that
went in on Thursday including the new modern landing. That build doesn't have
the problem. So it should be easy to see what's been checked in since and then
figure out the culprit.
I am unable to reproduce this using a mozilla branch build from today. Can
anyone else? 
weird...seems to be commercial only (or at least release only). I am able to
reproduce this using a commercial branch build.
The only difference on the console between a commercial (broken) and a mozilla
(works) build is the following warning which gets dumped to the console on a
commercial build:

WARNING: not supported for views, file d:\beta1\mozilla\view\src\nsScrollPortVie
w.cpp, line 98

I see this warning every time I click on a message which requires a scrollbar.

This must be a sign. I just don't know what it means.
looks like layout is calling:

nsScrollBoxFrame::RestoreState

every time we display a message on a commercial build. 

and we never call this routine on a mozilla build. That's the difference.
RestoreState adjusts the scrollbar position.
Turns out all these problems are caused by the start page. I'm guessing the
start page recently changed? 

Looks like we sending a redirect for the start page. We start to load:
http://home.netscape.com/bookmark/6_1/messengerstart.html
then that redirects us to
http://home.netscape.com/bookmark/6/messengerstart.html

this redirection causes us to create a session history object for the mail
docshell. The presence of that session history is used to determine whether
layout should remember the frame state. (which includes scrollbar position). *doh*

If I manually change my prefs to point my start page directly at
/6/messengerstart.html, everything works great. 

I can try to work around it but the risk by changing nsDocShell at this point of
the game is probably much higher than resolving the redirection issue with our
start page.
Status: NEW → ASSIGNED
cc'ing joki in case this is related to the other fix that prevented a crash when
this redirect happened.
they are probably not related.
actually it doesn't have to do with the start page (although the start page is
the trigger).  Radha's checkin to nsDocShell.cpp / nsDocShell.h to change
session history casued this regression. Her change isn't present on the trunk
which explains why the commercial trunk builds don't show this problem.

Her bug was #86330.


cc'ing radha.
by not clearing out mOSHE entry correctly like we used to b4 this change, we end
up  always calling PersistLayoutHistoryState for every message we view in the
message window. 

    if (mOSHE) {
        nsCOMPtr<nsIPresShell> shell;

        rv = GetPresShell(getter_AddRefs(shell));
        if (NS_SUCCEEDED(rv) && shell) {
            nsCOMPtr<nsILayoutHistoryState> layoutState;
            rv = shell->CaptureHistoryState(getter_AddRefs(layoutState),
                                            PR_TRUE);
            if (NS_SUCCEEDED(rv) && layoutState) {
                rv = OSHE->SetLayoutHistoryState(layoutState);
            }
        }

In the above code b4 Radha's change, we were clearing mOSHE to null so we'd
never  end up calling  CaptureHistoryState. We lost this after the docshell
session history changes. Unfortunately the changes aren't in the trunk so we
didn't get a chance to bake them on the trunk and notice this regression. I've
pinged radha in the other bug to figure out why this is a trunk only patch and
to see what we can do to fix this regression.

  
I've looked into the bonsai and it's clear that the patch from bug 86330 is BOTH
on the TRUNK and on the BRANCH. So there must be something else which triggers
this bug on branch and not on the trunk. I've retested the latest trunk build on
Linux and it correctly scrolls to the top of the message every time I skip to
next or previous or whatever else message.

My patch is in the trunk too. v1.332 to nsDocshell.cpp.It looks like message
window is affected by the change to nsDocShell::InternalLoad(). I don't
understand why mLSHE and mOSHE have valid values in mail message window. Does it
use Session History?
QA Contact: esther → nbaca
my bad my trunk build wasn't up to date which is why I didn't notice the change
in the trunk. The problem happens with the commercial start page which is why a
mozilla build doesn't show it. The problem is now on both the trunk and the
branch commercial builds. 

They initially have valid values when the start page is loaded but then mOSHE
was getting cleared out when you first clicked on a message because mLSHE would
be null  and this caused mOSHE to get cleared out by the following assignment:
mOSHE = mLSHE. 

That no longer happens now because of the if (mLSHE) clause that got added.
QA Contact: nbaca → esther
Why does mLSHE and mOSHE have valid values at startup? Is it used by mail
message window at all? My changes are required for Back/forward  navigations on
subframes with anchors. 
they have valid values because http urls are loaded inthe message pane at
startup (the start page) and apparently docshell creates a session history entry
for those urls. Everything worked in the past because we ended up clearing that
value making the assignment:

mOSHE = mLSHE which is no longer happening.
It looks like the problem is, in having a valid value for mOSHE when it should
be null. It was working so long, because bug 86330 existed.  Is it possible to
clear out mLSHE and mOSHE when the first  message after the  start page  is
viewed., or even for all messages.  If we can do that then mLSHE and mOSHE will
remain null for all successive messages and we wouldn't store the scrollbar
position in mOSHE.
docshell won't create a SH entry for the url if  the docshell did not have 
mSessionHistory created for it in the first place and/or you pass the
LOAD_FLAGS_BYPASS_HISTORY
"docshell won't create a SH entry for the url if  the docshell did not have 
mSessionHistory created"

Actually that's not true and maybe that's part of the problem. There are cases
where mSessionHistory is null but we still create SH Entries and set those
values on mOSHE and mLSHE. nsDocShell::AddToSessionHistory can return a new
entry that gets copied into mLSHE even if mSessionHistory is NULL and if the
docshell doesn't have a parent that has a session history object. 

The last few lines of that method are as follows:
    // Return the new SH entry...
    if (aNewEntry) {
        *aNewEntry = nsnull;
        if (NS_SUCCEEDED(rv)) {
            *aNewEntry = entry;
            NS_ADDREF(*aNewEntry);
        }
    }

This ends up giving mLSHE a value when this routine gets called even though we
don't have a mSessionHistory. Later on, mLSHE gets copied into mOSHE and then we
have problems.


Using loadflags, nsIDocShellLoadInfo::loadHistory, in the second argument
nsDocShellLoadInfo to LoadURI()  or nsIWebNavigation::LOAD_FLAGS_BYPASS_HISTORY
in the third argument to LoadURI() avoid the creation of SH entry for a page, by
avoiding the call to AddToSessionHistory() in OnNewURI().   However the third
argument should be used exclusively. In LoadURI(), it is overridden by whatever
is put in nsDocShellLoadInfo.
hmm I don't think we are communicating very well here =). Changing the load
flags when I load mailnews urls in a docshell isn't going to make this problem
go away. The message pane is just an iframe. On start up, that iframe has an
initial source for an HTML start page. There are no load flags that get set when
you set the src of an iframe. There's now way to control this. <iframe
src="http://...some start page" />.


I can certainly pass in bypass session history when I load mail urls in the
docshell. But that's not going to fix this problem. 

As I keep saying, AddToSessionHistory creates a history entry even if you don't
have a session history object in that docshell chain. As a result just setting
the src of the iframe to the start page causes mLSHE to get intialized. That in
turn causes mOSHE to get a value which never gets cleared because of the if
clause you added. Before your change mOSHE would get cleared to null and all was
good in the world.

Should I be re-assigning this to you? PDT is going to want to know where I am
with this bug today and I don't know what to tell them. This really isn't a mail
problem it's a session history issue I think. Is it really correct for
AddToSessionHistory to always return a session entry even if we or a parent
docshell doesn't have a session history object??
I see what you are saying about AddToSessionHistory(). But I'm not sure it is
safe to make that change now. AddToSessionHistory() presumes that the caller
knows what they are doing. 
I'm trying to get a workaround for this. 
thanks for helping look into the fix Radha!
Here's a fix Radha came up with that fixes the problem. I apologize when I said
earlier in the bug report that this change wouldn't fix it. I had only changed
one spot in imap when I tested that theory and it turns out there were two spots
=). 

This fixes the problem for me. However we'll need to do some heavy trunk testing
tomorrow to make sure passing in this bypass session history flag doesn't break
anything since just about every url we run in mail runs through one of the
LoadURI calls I changed in this patch.
I have attached a patch to docshell that will create ShEntry (mLSHE) only if
there is a sessionHistory (mSessionHistory) object in the current frame or in
the root docshell. I have done cursory testing of this patch in browser on the
trunk. I need to test it further before I can go for r and sr. I hope this fixes
the mailnews problem too. 
hey radha, just wanted to let you know that your patch to docshell also fixes
the problem for me. 
Trunk build 2001-08-11-04: WinMe, Linux RH 6.2
Mac build not available yet.

This does not appear fixed since the scrollbar remains in the same position as 
the previous message. It should scroll to the top.
nbaca, we haven't checked in the fix yet so it's not going to work =). I'll put
the vtrunk keyword in when we check in a fix for you.
sr=mscott on your change. The code looks good and I've verified that it fixes
the problem.
Whiteboard: [PDT+] → [PDT+], awaiting code review.
r=valeski
Whiteboard: [PDT+], awaiting code review. → [PDT+], Need approval
we need to check this into the trunk for testing b4 we can check it into the 
branch. Radha, do you want to drive this patch in or do you want to do it? I'm 
fine either way!
I will check it into the trunk and later into the branch.
Whiteboard: [PDT+], Need approval → [PDT+], have reviews, ready for the branch
radha, i hope you don't mind but the tree just opened *finally* a few minutes 
ago. I'd like to make sure this gets checked in tonight so it will be in 
tomorrow's trunk builds. Then hopefully PDT will let us check the fix into the 
branch tomorrow night. So I'm going to check your patch into the trunk. I 
wouldn't do this to you if it wasn't for the tree opening so late tonight! =(

Adding vtrunk keyword.
Keywords: vtrunk
Verified FIXED on the *trunk* using builds:

RedHat 7.1 - 2001-07-13-08
Windows 2K - 2001-07-13-04
Mac OS 9.1 - 2001-07-13-09

Criteria: Selected a message that required scrollbars, adjusted the scrollbar to
the middle of the scroll position.  Clicked on another message that requires
scrollbars and noticed that the scrollbar's position was reset to the top of the
document.

Thanks for the quick turn around Stephen. This will help a lot when we go to PDT
today. Radha, do you have a QA person you'd like to run some session history
tests on before we move this onto the branch today?
This bug's already been given a PDT+, so if it's been tested and everyone's
happy with the fix I think it can go on the branch.  I agree with Scott's
comments that it would be good to get someone who can help with session history
testing.

cc'ing claudius who is listed as the default Session History QA Contact.
I've gone ahead and checked this into the branch. However, it would be great if
Claudius could do some session history QA'ing to make sure the patch didn't
introduce a regression in session history.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Scott, Thanks for checking in the fix on my behalf. Due to an emergency 
situation, I had to take the day off today. 
esther - pls get claudius' verification before marking this bug verified.
Keywords: vtrunk → vbranch
Using branch builds 2001-07-17 on win, mac and linux and the original scenario &
stephens listed criteria this is fixed for mail.  Passing QA on to claudius for
regression testing around this fix in his area and for verification.
QA Contact: esther → claudius
I'm satisfied that there aren't any regressions in SH due to this checkin. Looking at branch
builds w/ and w/o the fix I don't see any new adverse behavior w/ frames, SH and scrollpositions.

Marking VERIFIED Fixed with 2001071803 branch build. removing vbranch keyword.
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: