Closed Bug 138134 Opened 22 years ago Closed 22 years ago

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

Categories

(Core :: DOM: Navigation, defect)

Product:
Core
Component:
DOM: Navigation
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.0

People

(Reporter: mozilla, Assigned: radha)

References

Details

(Keywords: regression, topembed+, Whiteboard: [FIXED ON TRUNK][ADT2])

Attachments

(2 files, 3 obsolete files)

testcase
247 bytes, text/html
Details
patch v1.3
2.68 KB, patch
adamlock
: review+
alecf
: superreview+
jesup
: approval+
Details | Diff | Splinter Review 
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
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 → ---
Attached file testcase — 
testcase
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Summary: actual builds ignore <a href="#">top</a> links → new builds ignore <a href="#">top</a> links
Test case. Verifying problem exists in 1.0

http://bugzilla.mozilla.org/show_bug.cgi?id=138134#
Bah humbug, Bugzilla didn't turn my testcase into a link.

CC'ing some blame candidates for nsDocShell::ScrollIfAnchor changes
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
Actually it was Radha's checkin for bug 59774. Patch follows.
Whiteboard: adt1
Attached patch Patch (obsolete) — — Splinter Review 
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?
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.
Attached patch patch v1.1 (obsolete) — — Splinter Review 
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.
Attached patch Backout Patch (obsolete) — — Splinter Review 
This patch is a backout of the ScrollIfAnchor changes in bug 135679.
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.
Attached patch patch v1.3 — — Splinter Review 
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?
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.
Attachment #79913 - Flags: review+
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
*** Bug 138422 has been marked as a duplicate of this bug. ***
Comment on attachment 79913 [details] [diff] [review]
patch v1.3

sr=alecf
Attachment #79913 - Flags: superreview+
Fixed in trunk.
Status: NEW → RESOLVED
Closed: 22 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+
*** Bug 138800 has been marked as a duplicate of this bug. ***
*** Bug 138887 has been marked as a duplicate of this bug. ***
Keywords: nsbeta1, topembed → nsbeta1+, topembed+
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.
Whiteboard: [FIXED ON TRUNK] → [FIXED ON TRUNK][ADT2]
Changing QA contact. If anyone disagrees, please change it back.
QA Contact: adamlock → moied
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
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
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.

Attachment

General

Creator:
Created:
Updated:
Size: