Note: There are a few cases of duplicates in user autocompletion which are being worked on.

new builds ignore <a href="#">top</a> links and general anchor traversal broken

VERIFIED FIXED in mozilla1.0

Status

()

Core
Document Navigation
--
critical
VERIFIED FIXED
16 years ago
15 years ago

People

(Reporter: Ralf Vitasek, Assigned: Radha on family leave (not reading bugmail))

Tracking

({regression, topembed+})

Trunk
mozilla1.0
x86
Windows XP
regression, topembed+
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [FIXED ON TRUNK][ADT2])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

16 years ago
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
(Reporter)

Updated

16 years ago
Target Milestone: --- → mozilla1.0
The target milestone field is for the asignee, not the reporter

-> docshell
Assignee: Matti → adamlock
Component: Browser-General → Embedding: Docshell
QA Contact: imajes-qa → adamlock
Target Milestone: mozilla1.0 → ---

Comment 2

16 years ago
Created attachment 79800 [details]
testcase

testcase

Updated

16 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

16 years ago
Keywords: regression
Summary: actual builds ignore <a href="#">top</a> links → new builds ignore <a href="#">top</a> links

Comment 3

16 years ago
Test case. Verifying problem exists in 1.0

http://bugzilla.mozilla.org/show_bug.cgi?id=138134#

Comment 4

16 years ago
Bah humbug, Bugzilla didn't turn my testcase into a link.

CC'ing some blame candidates for nsDocShell::ScrollIfAnchor changes

Comment 5

16 years ago
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

16 years ago
Actually it was Radha's checkin for bug 59774. Patch follows.
Whiteboard: adt1

Comment 7

16 years ago
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?

Updated

16 years ago
Keywords: mozilla1.0, nsbeta1, patch, topembed
Whiteboard: adt1
It was to preserve position in history loads. I think it needs a litle bit of
work. I will comment on it later today.
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.
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.
Attachment #79806 - Attachment is obsolete: true
cc'ing Ere Maijala (Owner of bug 135679)  Please read the previous comment. 
Created attachment 79911 [details] [diff] [review]
Backout Patch

This patch is a backout of the ScrollIfAnchor changes in bug 135679.

Comment 13

16 years ago
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.
Created attachment 79913 [details] [diff] [review]
patch v1.3

here's the complete patch.
Attachment #79897 - Attachment is obsolete: true
Attachment #79911 - Attachment is obsolete: true
*** Bug 138313 has been marked as a duplicate of this bug. ***
Taking from Adam.
Assignee: adamlock → radha
Severity: major → critical
Summary: new builds ignore <a href="#">top</a> links → new builds ignore <a href="#">top</a> links and general anchor traversal broken
Target Milestone: --- → mozilla1.0
OK, so load type = LOAD_HISTORY and LOAD_RELOAD_NORMAL are the only cases where
we reload state from history in general?

Comment 18

16 years ago
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.
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. 

Updated

16 years ago
Attachment #79913 - Flags: review+

Comment 20

16 years ago
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

16 years ago
*** Bug 138422 has been marked as a duplicate of this bug. ***

Comment 22

16 years ago
Comment on attachment 79913 [details] [diff] [review]
patch v1.3

sr=alecf
Attachment #79913 - Flags: superreview+
Fixed in trunk.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Keywords: adt1.0.0
Resolution: --- → FIXED
Comment on attachment 79913 [details] [diff] [review]
patch v1.3

a=rjesup@wgate.com for branch checkin
Attachment #79913 - Flags: approval+

Comment 25

16 years ago
*** Bug 138800 has been marked as a duplicate of this bug. ***

Comment 26

16 years ago
*** Bug 138887 has been marked as a duplicate of this bug. ***

Updated

16 years ago
Keywords: nsbeta1, topembed → nsbeta1+, topembed+

Updated

16 years ago
Blocks: 138000
Whiteboard: [FIXED ON TRUNK]
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. 

Updated

16 years ago
Whiteboard: [FIXED ON TRUNK] → [FIXED ON TRUNK][ADT2]

Comment 28

16 years ago
Changing QA contact. If anyone disagrees, please change it back.
QA Contact: adamlock → moied

Comment 29

15 years ago
Marking verified - I have tested URL/testcases listed in dupes, and did not see
any problems with build 2002042608 (trunk) on winxp.
Status: RESOLVED → VERIFIED

Comment 30

15 years ago
adding adt1.0.0+.  Please check this in to the branch as soon as possible and
add the fixed1.0.0 keyword.
Keywords: adt1.0.0 → adt1.0.0+
Fixed in Mozilla 1.0 branch.
Keywords: fixed1.0.0

Comment 32

15 years ago
Marking verified fixed with branch build 20020523 using winxp, adding
KW:verified1.0.0
Keywords: verified1.0.0
You need to log in before you can comment on or make changes to this bug.