Not sure about the right component... If you click a document internal link (href="#someref"), the document is reloaded before it is scrolled. This is a regression (don't remember when this broke). This breaks, among other things, the TOC demo (viewer demo 15): if you have the TOC open and click a TOC link, the document is reloaded and the TOC disappears).

Adding M16 and nsbeta2 keyword for review.

The duplicate bug is older and a bit more general. *** This bug has been marked as a duplicate of 36104 ***

vrfy

Crap. The bug I marked dup was marked worksforme, but this bug was not fixed. Reopening. Steps to reproduce: 1. Open Debug > Viewer Demos > #15 XML IRS 2. Click "Contents" button 3. Click on any TOC link Expected results: The URL bar should update to show the #ref. No scrolling currently, other XML bugs prevent this. What happens: The URL bar updates, but the document is reloaded so the TOC disappears. No scrolling currently, other XML bugs prevent this. To show this is not XML only issue, open some long HTML doc, like: http://www.w3.org/XML Click on "Namespaces" link. It will reload the document before scrolling, you can see this because it takes so long to load the doc, and the progress bar shows doc load as well. Still not sure about component. Could be layout or webshell.

DocShell was the right component after all.

This is IMO a must for beta2! Seems like the nsbeta2 keyword was lost from this bug, re-nominating, and adding regression keyword.

I just talked with Heikki on the phone and he said that he just realized that the attached fix is not complete, it lacks a check that the #ref is actually a ref in the same document, Heikki wasn't able to get to a computer so he asked me to add this comment.

[nsbeta2+]

OK, I have a fix for this bug including the site comparison stuff. I'm just trying to clean up the session history issues now before I will do a check in.

Ok, the checkin is done.

*** Bug 25712 has been marked as a duplicate of this bug. ***

Couple of small points: 1. You have a typo on the second arg test. 2. I believe we should set the outgoing aWasAnchor earlier, as soon as we see that the URIs are the same and the new URI has a ref part. Also, if we cannot scroll to the ref for some reason, we should do the same as we do with empty ref, that is, scroll to top of page. As it is now we would reload the page. See the diff below: Index: nsDocShell.cpp =================================================================== RCS file: /cvsroot/mozilla/docshell/base/nsDocShell.cpp,v retrieving revision 1.128 diff -u -r1.128 nsDocShell.cpp --- nsDocShell.cpp 2000/05/23 00:52:59 1.128 +++ nsDocShell.cpp 2000/05/23 09:09:47 @@ -2684,7 +2684,7 @@ NS_IMETHODIMP nsDocShell::ScrollIfAnchor(nsIURI* aURI, PRBool* aWasAnchor) { NS_ASSERTION(aURI, "null uri arg"); - NS_ASSERTION(aURI, "null anchor arg"); + NS_ASSERTION(aWasAnchor, "null anchor arg"); if (aURI == nsnull || aWasAnchor == nsnull) { @@ -2765,6 +2765,8 @@ // Both the new and current URIs refer to the same page. We can now // browse to the hash stored in the new URI. + + *aWasAnchor = PR_TRUE; if (!sNewRef.IsEmpty()) { @@ -2773,9 +2775,10 @@ if (NS_SUCCEEDED(rv) && shell) { rv = shell->GoToAnchor(sNewRef); - if (NS_SUCCEEDED(rv)) + if (NS_FAILED(rv)) { - *aWasAnchor = PR_TRUE; + // A bit of a hack - scroll to the top of the page. + SetCurScrollPos(ScrollOrientation_Y, 0); } } } @@ -2783,7 +2786,6 @@ { // A bit of a hack - scroll to the top of the page. SetCurScrollPos(ScrollOrientation_Y, 0); - *aWasAnchor = PR_TRUE; } return NS_OK;

heikki, the bottom line is that the orginal reload problem is fixed. Yes? If so, could you please mark this bug verified, and write new one(s) for new problems. Thanks!

It is not completely fixed. If we have a ref that is not in the doc, we STILL reload the doc. In my opinion it is this bug's responsibility, it is part of, or a subset of, this bug. The typo is a minor issue. Also, NS 4.7 (for example) scrolls to the top of the current doc if that happens. You can test this easily: open any page, scroll somewhere, add #heikkiguaranteesthisisnothere to the urlbar and press enter. You will end up at the top. Ok, I am reopening then, to make sure it gets tracked...

Oh boy, still more comments... When we get the hashNew and if it is less than zero (meaning there is no ref in the new URI), shouldn't we bail out at THAT point? Now we just continue and if the new and current left parts are the same we set the aWasAnchor to true, which is wrong. Or maybe I am hallucinating... So, instead of: sNewLeft = sNew; why not just call return NS_OK; Btw, there is also nsIURL interface that has GetRef method (Johnny pointed this out to me while I was bitching about the lack of GetRef method).

Re: the last comment. No we can't bail out if the new URI has no #. Imagine I'm looking a http://www.foo.com/bar#abc and I click on a link to http://www.foo.com/bar. If we bail out, it causes the page to be reloaded when all we need do is scroll to the top. Re: Modified behaviour for non-existent hashes. Yes I can scroll to the top if you like, though that behaviour might be very annoying if the user is halfway down a document that is loading and clicks on a link to ref that has not been loaded. The page would end up scrolling to the top and the user will not know why. Maybe it would be better to do nothing at all when the ref does not exist (and ensure it doesn't get put into the session history either). Re: the assert typo. I will fix that asap. So give me a consensus on whether to scroll to the top for a non-existant ref or leave the page where it is. I prefer the latter but I'm open to reason.

Good, the foo.com sample made it clear, hadn't thought about that. As to what to do if ref is not in doc, I agree with Adam. I would not want the page to scroll to top. NS 4.7 scrolls to top, but I dislike it. A real pain if you are opening a large doc and scroll a little and click a link whose target has not yet been appended to the doc... Happens with W3C specs all the time.

Please review proposed patch to do nothing when the specified anchor does not exist or has not been loaded yet.

If the ref is not found, we update the status bar but do not scroll. This looks fine to me. But you cannot press back. I think you should be able to press the back button to clear the not-found-ref from the URL bar. That is what NS 4.7 does (it seems to reload the page which is unnecessary). If you always set aWasValidAnchor to true, or remove it, this is how it seems to work. Is there some other reason why it is there? I found something new that looks weird. To test, open http://www.mozilla.org/build/win32.html and resize your window so that both scrollbars appear and that the target to 2.3 is not visible. Then click the link 2.3. Then click back. We will scroll to top, which is correct, BUT WE WILL ALSO SCROLL TO RIGHT, which is wrong. Hmm, I'll attach a test case and my suggestion diff for impl.

Ok, the final fix has gone in for this. It scrolls to the top left (I hope there are no international issues about this!), and the checkin also fixes the assert. If you scroll to a non-existant anchor you won't scroll to the top but a history entry is made. I've kept the setting of wasAnchor to PR_TRUE in both branches of the if statement rather than moving it in front so that if the code can't get the presshell it will proceed with a normal load.

Yes, the scroll to top left is not correct for all languages. Top right should be the functionality for some. But that is outside the scope of this bug. Marking verified.