Closed Bug 59774 Opened 24 years ago Closed 22 years ago

scroll position not remembered if URL contains named anchor

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.0

People

(Reporter: bulbul, Assigned: radha)

References

Details

(Keywords: topembed+, Whiteboard: [nsbeta1][ADT2])

Attachments

(1 file, 6 obsolete files)

When returning to a page (with the back button, for example), the scroll
position of that page will not be remembered if the URL contains a named anchor.

You can try it out by going to a named anchor of one of my pages:

     <http://fizzylogic.com/users/bulbul/links.html#software>

Scroll to another position on that page and click on a document-external link.
Then hit the back button. You will be returned not to the last position you were
at, but to the named anchor. This may or may not be related to bug #59773 (last
named anchor selected sometimes becomes unforgettable).
worksforme linux trunk 2000110908 if I scroll _down_.

But I can see the bug if I scroll _up_.
I didn't think to test both scrolling up and scrolling down. Sorry. I should
have said scrooling *up*, since that's how i was testing it. But now that i've
tested both ways, the story gets wierder. I'm getting funny behaviour both ways,
but *different* behaviours:
     * When i scroll *up*, the situation is as described above.
     * When i scroll *down*, i am initially taken to the position
       of the named anchor, but then after a moment i am returned
       to the previously-scrolled-to position. (The delay can be
       almost two seconds, at least in my current session on Linux.)
Maybe this is what's *supposed to* happen, only faster.
The delay of the scroll *down* behaviour seems to be dependent on the loading
time for the page. The browser moves from the named anchor position to the
scroll position just when the M/dinosaur icon in the right corner goes to "M",
indicating that the page has finished loading.
--> history, seeing this as well on newest trunk
Assignee: asa → radha
Status: UNCONFIRMED → NEW
Component: Browser-General → History
Ever confirmed: true
QA Contact: doronr → claudius
Giving it to pollmann. May be sometihng regressed.
Assignee: radha → pollmann
*** Bug 61472 has been marked as a duplicate of this bug. ***
nav triage team: needed for next beta. P2. 
Keywords: nsbeta1
Priority: P3 → P2
nav triage team:

Marking nsbeta1+
Whiteboard: [nsbeta1+]
eric: will this make it for mozilla 0.8 or 0.9? thanks, Vishy
*** Bug 67927 has been marked as a duplicate of this bug. ***
Setting milestone to future
Target Milestone: --- → Future
Keywords: mozilla0.9.1
I've got strong feeling that this one is related to bug #83673.
*** Bug 83673 has been marked as a duplicate of this bug. ***
See also bug 86330.
In mozilla 0.9.2 (build 2001070116), I have the following: If in a page I click
on a link of the form <A HREF=#id>...</A> (in the same page), and then click on
the back button, I don't return to the same position I was but rather to the rop
of the page. Thatis, the scoll position is lost. See
http://JV.Gilead.org.il/gallica/ilemyst/1/02.html - hit on a picture caption, it
will take you down the same page, and hitting back will get you to the top of
the page instead of back to the caption! I use this feature of anchors at the
same page for foot-notes, and you need to return to the same place!
Yes, this is another side-effect of the same problem. Check bug #83673 for
information on why this happens and for more debugging data.
Is the scroll position not remembered or is the browser just not using it?

I believe that the anchor causes the browser to force the position to where the
anchor is. So what can be done? ;)

I think the browser should load the page without the anchor but show the anchor
in the urlbar when using back and forward navigation.
ok.. after looking at the code more carefully and tested mozilla's behavior +
ns4's behavior, I've come to the conclusion that the current mozilla behavior is
following ns4's behavior. winie5 otoh doesn't use the named anchors at all when
loading from session history.

I believe the code that is causing this behavior is in nsDocShell.cpp

4355 if ((aLoadType == LOAD_NORMAL ||
4356 aLoadType == LOAD_NORMAL_REPLACE ||
4357 aLoadType == LOAD_HISTORY ||
4358 aLoadType == LOAD_LINK) && (aPostData == nsnull)) {
4359 PRBool wasAnchor = PR_FALSE;
4360 NS_ENSURE_SUCCESS(ScrollIfAnchor(aURI, &wasAnchor), NS_ERROR_FAILURE);

the ScrollIfAnchor method will scroll to where the anchor is. I'm wondering if
removing LOAD_HISTORY from the '||' list will solve the problem, or will there
need to be more code make sure that session history doesn't break.
Nope. NS4.x handles anchors correctly.

Also, answering "Is the scroll position not remembered or is the browser just
not using it?" - the position is not remembered and if it wasn't for side-effect
of jumping to anchors whenever they show up, loading anchored URLs from history
would always land you on the top of the page.

Please read my analysis of bug #83673.

Umm no, the code that scrolls the page when an anchor is used defaults to the
top of the page if the anchor is empty.
from nsDocShell.cpp

4988 else {
4989 // A bit of a hack - scroll to the top of the page.
4990 SetCurScrollPosEx(0, 0);
4991 *aWasAnchor = PR_TRUE;
4992 }

I believe that the page is scrolled to the right position but the anchor
scrolling code forces it to be scrolled to the anchor or to the top of the page,
I could be wrong though.
Well, not exactly. This code gets executed when the old URL is an anchor and the
new one isn't. So, if this situation happens when you're moving back in session
history, Mozilla has no idea where to scroll current page to (as that position
in the page is not stored anywhere) and it does the next sensible thing - it
just jumps to the top. If you comment out

SetCurScrollPosEx(0, 0);

Mozilla does nothing and hence no scrolling happens.

I am not sure if the above is clear, so if you wish I can write a more thorough
description with examples.

Anyway, it would be beneficial if someone in the know would find correct people
to CC: to get answers to my questions from bug #83673.

*** Bug 94779 has been marked as a duplicate of this bug. ***
*** Bug 94971 has been marked as a duplicate of this bug. ***
is this now considered a helpwanted bug?
radha, who is pollmann's manager? maybe if we assign this bug to them we can get
this assigned to whomever will own this bit of code going forward.
Assignee: pollmann → radha
Kevin Mcklusky.  But I wil take a look before I pass it on to him.
*** Bug 102484 has been marked as a duplicate of this bug. ***
Blocks: 104166
*** Bug 104505 has been marked as a duplicate of this bug. ***
So, is anyone working on this bug? Or is it helpwanted?

This bug is the only remaining reason I still have to launch NS4.x on regular basis.
I can probably look at it for 0.9.7
Status: NEW → ASSIGNED
Target Milestone: Future → mozilla0.9.7
Target Milestone: mozilla0.9.7 → mozilla0.9.9
I looked in this. session history is properly storing the handle to
nsILayoutHistoryState and restoring it back. I think layout ignores the
scrollbar position when it sees  an anchor in the url, thereby causing the bug.
Passing to jkeiser who may know the scroll frame code better. 
Assignee: radha → jkeiser
Status: ASSIGNED → NEW
Component: History: Session → Layout
That is a good possibility.  The scroll position *should* be remembered but we
do some work to override stuff when there is a named anchor.
This is a good thing to fix but IMO not critical to nsbeta1.  And it looks like
it refers to a different nsbeta1 anyway :)  Un-nominating and setting milestone.
Whiteboard: [nsbeta1+] → [nsbeta1]
Target Milestone: mozilla0.9.9 → mozilla1.1
On the contrary, I think this is quite important bug. Basically, it makes
Mozilla unusable as a javadoc reader (not to mention other documentation formats
using anchors). And I guess there's a lot of people who read javadocs these days.

PLEASE get it fixed in time for 1.0.
I agree that this is a very important bug.  I don't read javadocs, but I do read
other sorts of specifications quite often and this bug makes mozilla useless for
that purpose unless I open a new window every time I follow a link, which is
really annoying.

A good example are the w3c specs that I'm supposed to be promoting the use of as
a member of the Mozilla community.
I'd have to agree. I know of several people complaining about this, and at least
one guy is saying something along the lines of, "I'd consider using Mozilla if
they got that fixed."
Ok, let's scratch an itch.

First, this bug is actually about two separate issues:

1. Going from a page with anchor to _another_ page, not necessarily with anchor.
2. Going from a page to anchor on the same page.

While these cases may seem similar, they take different paths in the code and it
just happens that _both_ are broken. I've fixed the first one (prototype patch
coming in a few minutes), the 2nd is still there, but not for long, I hope (more
below).

For the record, let me state that my fix is based mostly on intuition (as usual
when hacking Moz ;)), so someone who in fact _understands_ classes and methods I
am going to mention should look hard for possible flaws in my reasoning. So,
first issue:

1. As Radha pointed out above, session history state is indeed properly
preserving layout state. However, nsScrollBoxFrame::RestoreState() is not
immediately using these values, but rather delaying the scroll position
restoration to next call to nsScrollBoxFrame::DoLayout().
2. On page without an anchor nsScrollBoxFrame::DoLayout() is getting called
after that and in fact restores the scroll position.
3. On pages without an anchor further calls to nsScrollBoxFrame::DoLayout() do
not happen, as HTMLContentSink::ScrollToRef() flushes pending layout notifications.
4. However HTMLContentSink::DidBuildModel() is not too selective about calling
HTMLContentSink::ScrollToRef() - it does it also when loading from session
history, the result being that PresShell::GoToAnchor() gets called _always_ when
 URL being loaded contains an anchor.
5. The same happens in nsXMLContentSink::DidBuildModel(), BTW.
6. My fix is to make HTMLContentSink::DidBuildModel() obtain LoadType from
DocShell, check if it has History bit set, and if so, refrain from calling
HTMLContentSink::ScrollToRef(). This is done in rather lame way (see patch), as
I don't know how to find value of LOAD_CMD_HISTORY when in HTMLContentSink.
Please fix it.

Now for the other bug:

1. When moving around the page using anchors, nsDocShell::InternalLoad() tries
to be smart and just scrolls the page without loading anything. Contrary to what
Radha said, layout state is not preserved correctly here, because
nsDocShell::ScrollIfAnchor() is called first (potentially modifying scroll
position) and only then a call is made to nsDocShell::OnNewURI() which creates
session history entry.
2. What's more, this entry _does not_ contain persisted layout state, making its
restoration impossible anyway.
3. So, this part seems to be broken by design and in fact the quickest fix for
this problem is to remove the whole block of code dealing with anchors from
nsDocShell::InternalLoad(). The cost is that now clicking on an anchor makes the
whole page reload before jumping to it, which is rather slow (very slow, when
compared to simple scrolling).
4. The other fix would be to persist layout state before making a call to
PresShell::GoToAnchor() in nsDocShell::ScrollIfAnchor(). I did that and it
worked, however I have no idea how to restore this layout state later (when in
LOAD_HISTORY mode), so no fix for this one from me.


Hopefully above (and the forthcoming patch) will help you.
Something along these lines should probably be also done for nsXMLContentSink.
I've played with it some more and here is a revised patch that should fix all
issues related to this bug. Thanks to John Keiser for helpful suggestions.

I am pretty certain that this patch should be cleaned up and perhaps even
redone, as it changes some pretty popular structures, but I hope that the
principle will be useful and we can have this fixed for 1.0.
Attachment #68777 - Attachment is obsolete: true
Attached patch fixed patch (obsolete) — Splinter Review
I forgot to include one IDL file in my previous patch. Here comes everything
once again.

Radha, may I ask you to review this?
Attachment #69383 - Attachment is obsolete: true
Attachment #69865 - Attachment is obsolete: true
*** Bug 118088 has been marked as a duplicate of this bug. ***
By popular demand, I am uploading the final patch generated with -u option.
Assigning to Radha for review, care & feeding.
Assignee: jkeiser → radha
updated keywords.
Keywords: mozilla0.9.9patch
Nominating.  The bug has a patch that works and just needs review...
Keywords: nsbeta1
Comments on the patch:

1) I'm not sure if the patch will work if you strictly follow the reproduction
steps. ie., Go to an anchor and then scroll to another position on the page.   I
think so because, the scroller position is obtained and saved in session history
in InternalLoad()  when we go to an anchor. If the user
scrolled elsewhere after going to an anchor, the scroller position will be
different from anchor's and it doesn't look like the patch records that new
scroller position.  

2)I think this patch will only take care of vertical scrollbar position. Nothing
to the best of my knowledge suggests that horizontal scrollbar positions are
taken care here. We want to take care of horizintal scroll positions too.

3) As per the restoration code,
if (aLoadType == LOAD_HISTORY) {
    nscoord bx, by;
    mOSHE->RestoreScrollerPosition(&bx, &by);
    SetCurScrollPosEx(bx, by);
}

Does that work if you go to an outside link after scrolling and then click back.
For example, You visit 
a) http://www.mysite.com#anchor1. 
b) Scroll away from anchor.
c) Visit http://www.mozilla.org
d) Click back.

When you click back, we will not fall inside ScrollIfAnchor() in InternalLoad()
and thereby not restore the x,y, position from mOSHE. Pleas eexplain. 


4) I'm not sure if it is a good idea to move the loadType enum from nsDocShell.h
to nsIDocShell.h  I think these loadtypes were meant for private use by
nsDocShell.cpp. Adam Lock and/or Rick Potts own that part of code and I would
like  them to go through this change.  

5)The SaveScrollerPosition() and RestoreScrollerPosition() should be changed to
an attribute and thereby map to SetScrollerPosition() GetScrollerPosition() in C++. 

I think this bug can be fixed in a better way if we take care of the problem in
layout rather than docshell playing around over what layout does. I think a
layout based solution will address horizontal scrollbar too. 
Ok, let me take these one by one:

1. The patch will work - I tested this particular case, and I encourage
others to do so, too. As to why it works, please first take a look at my
comment (#37).  The problem you describe is what I termed "issue 1",
namely "Going from a page with anchor to _another_ page, not necessarily
with anchor".

Shortly, the scroller position is preserved, but its restoration is
impossible, because when you press "back" and previous page is being
restored, HTMLContentSink::DidBuildModel() _always_ makes the call to
HTMLContentSink::ScrollToRef() which in turn makes it impossible to
restore proper scroller position by overriding it with anchor's position
(if anchor is present).

This is fixed by parts of the patch concerning
content/html/document/src/nsHTMLContentSink.cpp and
docshell/base/nsIDocShell.idl

As I wrote in comment #38, similar operation should probably be
performed in nsXMLContentSink.cpp, too.


2. On the contrary, both vertical and horizontal positions are preserved
and restored properly. I know there are some problems with horizontal
scroller (see bug #99094), but they are not related to anchors at all.


3. Again, this case is "issue 1". This has nothing to do with changes I
did to mOSHE and friends in nsDocShell::InternalLoad(), as all that I
needed to fix was in HTMLContentSink::DidBuildModel(). So, you are right
- we're not falling inside ScrollIfAnchor() - but we don't need to.
ScrollIfAnchor() and its surroundings is only used if we're going to
anchors on _the same page_.

Again, anchor jumps on the same page are termed "issue" in comment #37
and discussed there in detail.

My fix takes care for all possible cases, including restoration on
forward and backward history moves within one page. Just try it.


4. I'm not sure either. John Keiser advised me that it'd be probably ok
to put loadType in this particular file, but if there are some other
people who should have their say - let them make the decision. I would
consider it a detail that could go unchanged in 1.0 and perhaps get
fixed later, as it does not break anything. But that's just me.


5. Well, I wanted to avoid naming ambiguities, as SetScrollerPosition()
suggests that we actually influence scroller. Moreover, I am not sure
how to do what you suggest - can you help? Or anyone else?


6. No, it cannot be fixed in layout, because when you do anchor jumps
withing _the same page_, layout is not used at all. Therefore, they have
no influence.

Small correction. The final sentence of point 3 should read:

"Again, anchor jumps on the same page are termed "issue 2" in comment #37
and discussed there in detail."
re: the loadType constants ... if the loadType is not needed outside
nsDocShell.cpp, we should not have a GetLoadType() function.  Since we do, we
*should* put the loadType constants where they are accessible to users of that
function.

Regarding layout saving scroller positions: I think saving/restoring scroll
position works properly, but we need to suppress DocShell's eager anchor
restoration in order to see its effects.

The other munging, as Miloslaw pointed out, was due to the fact that layout
history doesn't happen at all when you jump intra-page.  In this case it is
*possible* to manufacture a history object and write new methods that just find
the body frame and save/restore the scroller position *only* (not any other
state).  As with most things in programming, this *can* be done, though it does
not seem desirable to me.  I am not sure the small space savings is worth the
complexity, however ... Radha, what do you think?
*** Bug 131553 has been marked as a duplicate of this bug. ***
Attached patch another final patch (obsolete) — Splinter Review
This patch uses attributes, as suggested by Radha.
Attachment #69866 - Attachment is obsolete: true
Attachment #72881 - Attachment is obsolete: true
OK. I tried out the patch and read through the explanation provided in comment
#48. It appears that,

1)Issue #1, (Going from anchor to a different site),  is fixed by the changes to
nsHTMLContentSink.cpp, nsDocShell.h, nsIDocShell.idl.(Move the loadtypes from .h
to to idl file) 

2) Issue #2, is fixed by changes to nsISHEntry.idl, nsSHEntry.h nsSHEntry.cpp
and changes to the anchor related part in InternalLoad(). You have explained in
comment #37 (the very last item) that you tried to  fix this issue by saving the
layoutHistoryState(that part worked), but did not know how to restore it.  Here
is something that comes to mind, but not sure will work. DocShell restores
historyState() by calling nsIPresShell->SetHistoryState() which just sets the
mHistoryState member in nsPresShell, which is later used by it to restore the
state. I'm not sure if nsIPresShell has any handy methods that can docshell can
callto do the restoration for us. If there is one, we can probably get away by
calling nsIPresShell->CaptureHistoryLayoutState(), saving the state in mOSHE
before we leave the current anchor and restore it by calling, 
nsIPresShell->SetHistoryState(); 
nsIPresShell->HandyMethodThatActuallyRestores(); 

If this thing works, then we don't need these attributes to nsISHEntry and
related changes to nsSHEntry.h and nsSHEntry.cpp.  But my hope on this
suggestion is slim. In that case, we will have to go with your suggestion of
storing the scroller positions in nsISHEntry and restoring it manually. Jkeiser
or someone who knows PresShell can probably comment on this better.


About the patch, on the second thought, I think I prefer the
nsISHEntry->SetScrollerPosition() and nsISHEntry->RestoreScrollerPosition()
better than the attributes. The functions seem simpler than the attributes.  

It looks good otherwise. I would like AdamLock or Rick Potts input on moving the
loadType definitions to nsIDocShell.idl. Once we get to the point to check this
in, I will  talk to them about it. 
Status: NEW → ASSIGNED
Target Milestone: mozilla1.1alpha → mozilla1.0
Re: HandyMethodThatActuallyRestores() function. Yes, I also wanted to solve it
like this. Unfortunately, it looks like there is no function that restores
scrolling positions without messing with layout.
I think the LOAD_CMD_* values in nsIDocShell.idl should be defined in the
interface as constants rather than as an escaped C++ enum.
I think nsXMLContentSink.cpp should also be addressd right away.
This patch cleans up the previous one a little bit and incorporates Adam's
suggestion on load command definitions.
Attachment #74846 - Attachment is obsolete: true
plussed, not a high priority, but worthwhile. I've gotten bit by this bug too.
Keywords: nsbeta1nsbeta1+, topembed+
Comment on attachment 76290 [details] [diff] [review]
yet Another patch

Does mScrollPositionX & Y need initialising somewhere?

r=adamlock otherwise
Attachment #76290 - Flags: review+
Adam, these values are only used if:

1) you are doing intra-page jump
and
2) aLoadType is LOAD_HISTORY

Since the mScrollPositionX/Y values are always set for intra-jumped pages, no
additional initialization is necessary.
Comment on attachment 76290 [details] [diff] [review]
yet Another patch

Can we just call this "scroll possition" as opposed to "scroller position"?
Also, remember leadingCaps

so 
SetScrollerPosition should be setScrollPosition, etc.

(and thus mScrollerPosition should be mScrollPosition)

the logic of the patch looks correct though.
The scary thing is, I'm beginning to understand this stuff :)

sr=alecf with those API name-changes.
Attachment #76290 - Flags: superreview+
alec, Thanks for catching the leading caps issue. I have the changes in my tree
and   will check those in. Thanks.
Keywords: adt1.0.0
Comment on attachment 76290 [details] [diff] [review]
yet Another patch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #76290 - Flags: approval+
Here is a short description of the bug, fix and the risk factors:

Problem: scroll position not remembered if URL contains named anchor. This
happens on intra-page jumps between anchors and while traversing from an anchor
in one page to another page. 

Fix: For intra-page anchor jumps, save scroll bar positions in session history
and use it for later restoration. For inter-page traversals, prevent layout from
going to the anchor by default, when the load was initiated by history mechanisms.

Risk factor:  AFAIK None.

Whiteboard: [nsbeta1] → [nsbeta1][ADT2]
I have built this on linux and  W2K. The patch is really XP.
Will this patch affect page load performance?
Yes, by several nanoseconds. ;)
Will this patch only affect scrollbar position when clicking on an url with a
named anchor?  Will session history scrollbar position for any other type of
page go through this code path and if so have those cases been tested thoroughly?
Major part of the patch is concerned with intra-page anchor jumps -- so yes,
this involves only anchored pages. There is also a small code path (in
nsHTMLContentSink.cpp) taken for all pages, but it is very simple. I've been
using it a lot with no problems at all.
adt1.0.0+ (on ADT's behalf) approval for checkin to 1.0.
Keywords: adt1.0.0adt1.0.0+
*** Bug 135591 has been marked as a duplicate of this bug. ***
This was fixed last week. 
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Was this landed on the branch (or before the branch)? 
sorry. ignore me. I'm confused.
In Reload, it is not fixed although surely checked.
Please refer to "http://bugzilla.mozilla.org/show_bug.cgi?id=133678".
This patch seems to be fixed also in Reload.
I think this is still broken (in a strange weird way).

RC1 Build: 2002041617
Platform: Win2k+SP2

Go to:

http://www.jerrypournelle.com/mail/currentmail.html.

Scroll down a bit and click on the "Wednesday" link - this skips you 
forward in the current page to beginning of the Wednesday mail. 
The URL is:

http://www.jerrypournelle.com/mail/currentmail.html#Wednesday

Without scrolling, click Back and you go back to the top of 
the page (where you scrolled to in order to see the Wednesday
link).

Click Forward again, and the URL changes back to the Wednesday anchor,
but the web page itself stays put.

Or is it supposed to work this way?
Heck, this is not "still broken". This is "broken again". Something regressed
badly, as I see this bug again on my other testcases (javadocs etc). Mozilla
2002041709/Linux-686

Radha, should we reopen?
Damn, this is not "still broken". This is "broken again". Something regressed
badly, as I see this bug again on my other testcases (javadocs etc). Mozilla
2002041709/Linux-686

Radha, should we reopen?
Ok, found it. The reason for regression is a fix for bug #135679, which landed
as nsDocShell.cpp version 1.443.

This is pretty major stuff, as anchor's history is basically fubar, but I am
unsure if (and where) I should set blocker status. Radha? John?
I posted the bug #138313 about that. It has a patch attached.
Is this fixed?
Yes. Why?
Miloslaw: because the most recent comments said it had regressed :)  Jaime: it
is indeed still working.
yep, w4m using 2003.02.19 comm trunk bits on linux rh8.0.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: