Last Comment Bug 138134 - new builds ignore <a href="#">top</a> links and general anchor traversal broken
: new builds ignore <a href="#">top</a> links and general anchor traversal broken
Status: VERIFIED FIXED
[FIXED ON TRUNK][ADT2]
: regression, topembed+
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: x86 Windows XP
: -- critical (vote)
: mozilla1.0
Assigned To: Radha on family leave (not reading bugmail)
: Moied
: Andrew Overholt [:overholt]
Mentors:
: 138313 138422 138800 138887 (view as bug list)
Depends on:
Blocks: 138000
  Show dependency treegraph
 
Reported: 2002-04-18 01:01 PDT by Ralf Vitasek
Modified: 2002-05-23 15:50 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (247 bytes, text/html)
2002-04-18 06:33 PDT, jlarsen
no flags Details
Patch (814 bytes, patch)
2002-04-18 07:37 PDT, Adam Lock
no flags Details | Diff | Splinter Review
patch v1.1 (2.05 KB, patch)
2002-04-18 16:24 PDT, Radha on family leave (not reading bugmail)
no flags Details | Diff | Splinter Review
Backout Patch (1.12 KB, patch)
2002-04-18 17:34 PDT, John Keiser (jkeiser)
no flags Details | Diff | Splinter Review
patch v1.3 (2.68 KB, patch)
2002-04-18 17:47 PDT, Radha on family leave (not reading bugmail)
adamlock: review+
alecf: superreview+
rjesup: approval+
Details | Diff | Splinter Review

Description User image Ralf Vitasek 2002-04-18 01:01:20 PDT
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:0.9.5) Gecko/20011213
BuildID:    2002041603

on earlier builds you could use <a href="#">top</a> to jump to a pages top i.e.
as in IE or older Netscapes.

actual builds just don't do anything when clicking such links.

Reproducible: Always
Steps to Reproduce:
1. insert <a href="#">top</a> at bottom om a slightly longer page
2. make sure you are scrolled down
3. push the <a href="#">top</a> link

Actual Results:  mozilla doesn't do anything

Expected Results:  schould jump on top of the page
Comment 1 User image Bradley Baetz (:bbaetz) 2002-04-18 01:39:26 PDT
The target milestone field is for the asignee, not the reporter

-> docshell
Comment 2 User image jlarsen 2002-04-18 06:33:06 PDT
Created attachment 79800 [details]
testcase

testcase
Comment 3 User image Adam Lock 2002-04-18 06:56:44 PDT
Test case. Verifying problem exists in 1.0

http://bugzilla.mozilla.org/show_bug.cgi?id=138134#
Comment 4 User image Adam Lock 2002-04-18 07:07:35 PDT
Bah humbug, Bugzilla didn't turn my testcase into a link.

CC'ing some blame candidates for nsDocShell::ScrollIfAnchor changes
Comment 5 User image Adam Lock 2002-04-18 07:21:28 PDT
Checkin for bug 135679 seems to have caused this regression. The patch does
nothing when it encounters an empty anchor ('#') whereas it used to scroll to 0, 0.

I'm working up a simple patch
Comment 6 User image Adam Lock 2002-04-18 07:34:41 PDT
Actually it was Radha's checkin for bug 59774. Patch follows.
Comment 7 User image Adam Lock 2002-04-18 07:37:03 PDT
Created attachment 79806 [details] [diff] [review]
Patch

Patch reinstates the scrolling behaviour.

Radha, can you comment why you needed to remove it? If it's to preserve the
state from load history then can you advise if this patch needs work?
Comment 8 User image Radha on family leave (not reading bugmail) 2002-04-18 09:40:17 PDT
It was to preserve position in history loads. I think it needs a litle bit of
work. I will comment on it later today.
Comment 9 User image Radha on family leave (not reading bugmail) 2002-04-18 15:31:23 PDT
thorgal, Can you comment on this, since you provided the original patch for 59774. 

The following statements should be added just before scrolling to 0,0 in the 
else { ..} segment 

if (aLoadType == LOAD_HISTORY)
  return rv;

OR, the above code segment which is already present inside if
(!sNewRef.IsEmpty()) { } should be moved outside so that it will cover both the
if and the else case.
Comment 10 User image Radha on family leave (not reading bugmail) 2002-04-18 16:24:15 PDT
Created attachment 79897 [details] [diff] [review]
patch v1.1

This patch takes care of the bug reported here. But the changes that went as
part of bug 135679 also contributes to some problems. For example,

1) Load this bug report and Click on the testcase.
2) Roll down and click on 'top'.
3) click back.
4) Move the scrollbar to the middle of the page.
5) Click forward 
6) Click back.

Expected results:
The scrollbar should be in the middle of the page, as you left it.

Actual results:
The scrollbar is in the top of the page.

The patch that went in part of bug 135679, causes this problem.
Comment 11 User image Radha on family leave (not reading bugmail) 2002-04-18 16:26:31 PDT
cc'ing Ere Maijala (Owner of bug 135679)  Please read the previous comment. 
Comment 12 User image John Keiser (jkeiser) 2002-04-18 17:34:35 PDT
Created attachment 79911 [details] [diff] [review]
Backout Patch

This patch is a backout of the ScrollIfAnchor changes in bug 135679.
Comment 13 User image Miloslaw Smyk 2002-04-18 17:35:59 PDT
Adam, I was a bit overzealous in removing problematic code when working on bug
#59774. It wouldn't work properly in its original form nor with the first patch
here, but patch v1.1 has it right, as it checks if we're not reloading nor
loading from history.
Comment 14 User image Radha on family leave (not reading bugmail) 2002-04-18 17:47:38 PDT
Created attachment 79913 [details] [diff] [review]
patch v1.3

here's the complete patch.
Comment 15 User image Radha on family leave (not reading bugmail) 2002-04-18 22:18:14 PDT
*** Bug 138313 has been marked as a duplicate of this bug. ***
Comment 16 User image Radha on family leave (not reading bugmail) 2002-04-18 22:19:44 PDT
Taking from Adam.
Comment 17 User image John Keiser (jkeiser) 2002-04-18 22:45:53 PDT
OK, so load type = LOAD_HISTORY and LOAD_RELOAD_NORMAL are the only cases where
we reload state from history in general?
Comment 18 User image Ere Maijala (slow) 2002-04-19 00:47:30 PDT
I understand the problem and I suppose this is backout is necessary. I will try
to create a better fix for bug 135679. Sorry for the regression.
Comment 19 User image Radha on family leave (not reading bugmail) 2002-04-19 09:38:30 PDT
LOAD_HISTORY and LOAD_NORMAL_RELOAD are the only ones where session history is
involved. In other load types, either SH is bypassed or it is not involved.

jkeiser these loadtypes are indeed amalgams of flags. Look into LOAD_CMD enum in
nsIDocShell.idl and load flags in nsIWebNavigation.idl and their amalgamation in
nsDocShell.h. 
Comment 20 User image Adam Lock 2002-04-19 10:40:40 PDT
Comment on attachment 79913 [details] [diff] [review]
patch v1.3

It's probably best to use brackets in that first "if (mOSHE &&) ..." test to
make  the && || precedence obvious. 

Otherwise r=adamlock
Comment 21 User image Ere Maijala (slow) 2002-04-19 12:20:48 PDT
*** Bug 138422 has been marked as a duplicate of this bug. ***
Comment 22 User image Alec Flett 2002-04-19 13:50:12 PDT
Comment on attachment 79913 [details] [diff] [review]
patch v1.3

sr=alecf
Comment 23 User image Radha on family leave (not reading bugmail) 2002-04-19 16:25:57 PDT
Fixed in trunk.
Comment 24 User image Randell Jesup [:jesup] 2002-04-19 16:39:50 PDT
Comment on attachment 79913 [details] [diff] [review]
patch v1.3

a=rjesup@wgate.com for branch checkin
Comment 25 User image Boris Zbarsky [:bz] (still a bit busy) 2002-04-20 11:09:24 PDT
*** Bug 138800 has been marked as a duplicate of this bug. ***
Comment 26 User image timeless 2002-04-20 17:42:48 PDT
*** Bug 138887 has been marked as a duplicate of this bug. ***
Comment 27 User image Radha on family leave (not reading bugmail) 2002-04-23 13:41:54 PDT
ADT, this is a regression caused couple of weeks ago. The patch is pretty much
backout of the bug that caused the regression and few tighter controls around
that code. Please give it a '+'. Shipping the product with this regression would
be really bad, since this has been working well in the previous milestones. 
Comment 28 User image gerardok 2002-04-24 12:41:39 PDT
Changing QA contact. If anyone disagrees, please change it back.
Comment 29 User image Moied 2002-04-26 14:08:48 PDT
Marking verified - I have tested URL/testcases listed in dupes, and did not see
any problems with build 2002042608 (trunk) on winxp.
Comment 30 User image scottputterman 2002-04-27 13:18:04 PDT
adding adt1.0.0+.  Please check this in to the branch as soon as possible and
add the fixed1.0.0 keyword.
Comment 31 User image Radha on family leave (not reading bugmail) 2002-04-29 14:06:09 PDT
Fixed in Mozilla 1.0 branch.
Comment 32 User image Moied 2002-05-23 15:50:56 PDT
Marking verified fixed with branch build 20020523 using winxp, adding
KW:verified1.0.0

Note You need to log in before you can comment on or make changes to this bug.