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
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 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 Bradley Baetz (:bbaetz) 2002-04-18 01:39:26 PDT
The target milestone field is for the asignee, not the reporter

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

testcase
Comment 3 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 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 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 Adam Lock 2002-04-18 07:34:41 PDT
Actually it was Radha's checkin for bug 59774. Patch follows.
Comment 7 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 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 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 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 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 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 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 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 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 Radha on family leave (not reading bugmail) 2002-04-18 22:19:44 PDT
Taking from Adam.
Comment 17 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 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 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 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 Ere Maijala (slow) 2002-04-19 12:20:48 PDT
*** Bug 138422 has been marked as a duplicate of this bug. ***
Comment 22 Alec Flett 2002-04-19 13:50:12 PDT
Comment on attachment 79913 [details] [diff] [review]
patch v1.3

sr=alecf
Comment 23 Radha on family leave (not reading bugmail) 2002-04-19 16:25:57 PDT
Fixed in trunk.
Comment 24 [:jesup] on pto until 2016/8/1 Randell 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 Boris Zbarsky [:bz] 2002-04-20 11:09:24 PDT
*** Bug 138800 has been marked as a duplicate of this bug. ***
Comment 26 timeless 2002-04-20 17:42:48 PDT
*** Bug 138887 has been marked as a duplicate of this bug. ***
Comment 27 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 gerardok 2002-04-24 12:41:39 PDT
Changing QA contact. If anyone disagrees, please change it back.
Comment 29 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 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 Radha on family leave (not reading bugmail) 2002-04-29 14:06:09 PDT
Fixed in Mozilla 1.0 branch.
Comment 32 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.