Closed Bug 662170 Opened 11 years ago Closed 11 years ago

Go to top anchor "#" doesn't work in Firefox 6 and Firefox 7

Categories

(Core :: DOM: Navigation, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla6
Tracking Status
firefox5 --- unaffected
firefox6 + fixed
firefox7 --- fixed

People

(Reporter: bdesef, Assigned: justin.lebar+bug)

References

Details

(Keywords: regression, testcase)

Attachments

(2 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:7.0a1) Gecko/20110605 Firefox/7.0a1
Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:7.0a1) Gecko/20110605 Firefox/7.0a1

A link that has the target href="#" should make the browser jump to the very top of the page. I don't know whether this is official standard but it's a very intelligent feature that every browser supports, until FF4 also Firefox. The FF7 does nothing when you click at such link.

Reproducible: Always

Steps to Reproduce:
1. Open the attachment
2 [review]. Scroll to one of the links so you cannot see the very top of the page
3. Try to navigate to the top by clicking it with a common browser. Then with FF7.

Actual Results:  
The scroll position doesn't change in FF7.


You can fix this by define a anchor (e.g. "#top" which you can see often), but sometimes it's hard to set this anchor to the very top with valid html.
Attached file Testcase
Works for me:
Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.2.17) Gecko/20110420 Firefox/3.6.17
Mozilla/5.0 (X11; Linux x86_64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Mozilla/5.0 (X11; Linux x86_64; rv:5.0) Gecko/20100101 Firefox/5.0

Reproduced:
Mozilla/5.0 (X11; Linux x86_64; rv:6.0a2) Gecko/20110604 Firefox/6.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:7.0a1) Gecko/20110605 Firefox/7.0a1


Last good nightly: 2011-04-20 First bad nightly: 2011-04-21

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=45b20f137549&tochange=46fdf12082d4
Keywords: regression
OS: Windows 7 → All
Hardware: x86 → All
Version: unspecified → Trunk
The first bad revision is:
changeset:   68370:228d41c97600
user:        Justin Lebar <justin.lebar@gmail.com>
date:        Thu Mar 31 12:35:04 2011 -0400
summary:     Bug 640387 - onhashchange should be fired when navigating between history entries created via history.pushState if the entries' URIs differ only in their hashes. r=smaug
Component: General → Document Navigation
Product: Firefox → Core
QA Contact: general → docshell
Summary: Go to top anchor "#" doesn't work in FF7 → Go to top anchor "#" doesn't work in Firefox 6 and Firefox 7
Attachment #537463 - Attachment mime type: text/plain → text/html
I'll look into this.

I guess the upside of touching this code and busting it so many times is that we'll finally have testcases defining the behavior we want...
Assignee: nobody → justin.lebar+bug
Severity: minor → normal
Status: UNCONFIRMED → NEW
Ever confirmed: true
User-facing regression of basic browsing behavior.  Nominating tracking ff-6.
This behavior is defined in the spec.  It's step 2 of the second algorithm at [1].

[1] http://www.whatwg.org/specs/web-apps/current-work/#scroll-to-fragid
The problem is this change:


   3.357 -    if (hashNew == kNotFound &&
   3.358 -        (hashCurrent == kNotFound || aLoadType != LOAD_HISTORY)) {
   3.359 +    if ((aCurHash.IsEmpty() || aLoadType != LOAD_HISTORY) &&
   3.360 +        aNewHash.IsEmpty()) {
   3.361          return NS_OK;
   3.362      }

hashNew == kNotFound is not at all the same thing as aNewHash.IsEmpty().

Or put another way, this IsEmpty() test needs to happen before stripping leading '#'.  This is true for both aNewHash and aCurHash.
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #537575 - Flags: review?(bzbarsky)
Comment on attachment 537575 [details] [diff] [review]
Patch v1

This applies cleanly to aurora and trunk.
Comment on attachment 537575 [details] [diff] [review]
Patch v1

>+    NS_ASSERTION(aNewHash.Length() >= 1 && aNewHash.CharAt(0) == '#',
>+                 "aNewHash should start with '#'.");

This could fail if !aCurHash.IsEmpty() && aLoadType == LOAD_HISTORY.  So I believe this assertion is wrong.

>+  // Scroll the iframe to the bottom.
>+  cw.scrollTo(0, 300);

For this test to make sense, you should test that cw.pageYOffset is nonzero after this.

r=me with those two problems fixed.
Attachment #537575 - Flags: review?(bzbarsky) → review+
Attachment #537575 - Attachment is obsolete: true
Comment on attachment 537609 [details] [diff] [review]
Patch v2 (per comment 10)

Do I have to get tracking-ff6+ before I can request approval-mozilla-aurora?  Requesting approval in case I don't.
fwiw, you can use isnot() in tests.

> Do I have to get tracking-ff6+ before I can request approval-mozilla-aurora?

No.
Attachment #537609 - Flags: approval-mozilla-aurora?
Checked in to m-c.  Still needs to be checked in to Aurora, so not closing the bug.

http://hg.mozilla.org/mozilla-central/rev/21bd522611ea
FWIW, you should close the bug when it lands in m-c and use approval- and tracking- flags to track the process of getting the patch into aurora and beta
please provide a risk assessment. thanks.
(In reply to comment #16)
> please provide a risk assessment. thanks.

Increases risk:
 * It's a docshell change.

Decreases risk:
 * bz reviewed the patch.
 * It's a small change.

I don't think there's a serious argument for not taking this.  It's a user-facing regression, and can break sites.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Attachment #537609 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Lots of sites use anchorless anchors, just in general.  It's not so surprising to me that we see crashes on those URLs.

Do you see any signatures which link you back to this code?
Note that of the urls listed in comment 18 only http://twitter.com/# and http://www.facebook.com/# are related to the code involved in this bug.
Target Milestone: --- → mozilla6
Mozilla/5.0 (Windows NT 5.1; rv:8.0a1) Gecko/20110810 Firefox/8.0a1
Mozilla/5.0 (Windows NT 5.1; rv:7.0a2) Gecko/20110809 Firefox/7.0a2
Mozilla/5.0 (Windows NT 5.1; rv:6.0) Gecko/20100101 Firefox/6.0

Verified issue using the test case attached in Comment 1 on Windows XP, Windows 7, Mac OS X 10.6 and Ubuntu. Issue is no longer reproducible.

Setting resolution to VERIFIED FIXED.
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.