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

VERIFIED FIXED in Firefox 6

Status

()

Core
Document Navigation
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: bdesef, Assigned: Justin Lebar (not reading bugmail))

Tracking

({regression, testcase})

Trunk
mozilla6
regression, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox5 unaffected, firefox6+ fixed, firefox7 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

6 years ago
Created attachment 537463 [details]
Testcase

Comment 2

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

Comment 3

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

Updated

6 years ago
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
Blocks: 640387
Keywords: testcase
Attachment #537463 - Attachment mime type: text/plain → text/html
(Assignee)

Comment 4

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

Updated

6 years ago
Severity: minor → normal
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 5

6 years ago
User-facing regression of basic browsing behavior.  Nominating tracking ff-6.
tracking-firefox6: --- → ?
(Assignee)

Comment 6

6 years ago
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.
(Assignee)

Comment 8

6 years ago
Created attachment 537575 [details] [diff] [review]
Patch v1
Attachment #537575 - Flags: review?(bzbarsky)
(Assignee)

Comment 9

6 years ago
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+
(Assignee)

Comment 11

6 years ago
Created attachment 537609 [details] [diff] [review]
Patch v2 (per comment 10)
(Assignee)

Updated

6 years ago
Attachment #537575 - Attachment is obsolete: true
(Assignee)

Comment 12

6 years ago
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.
(Assignee)

Updated

6 years ago
Attachment #537609 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 14

6 years ago
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
tracking-firefox6: ? → +
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
tracking-firefox6: + → ?
tracking-firefox6: ? → +

Comment 16

6 years ago
please provide a risk assessment. thanks.
(Assignee)

Comment 17

6 years ago
(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.
(Assignee)

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Attachment #537609 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 18

6 years ago
lots of places where use of # in urls for cases like described here that show up in crash data.

3084 http://www.facebook.com/#!/
 690 http://twitter.com/#!/
 643 http://nk.pl/#aplikacje/lesnaprzygoda
 446 http://www.tuenti.com/#m=Home&func=index
 437 http://nk.pl/#main
 335 http://nk.pl/#aplikacje/wiejskiezycie
 243 http://www.facebook.com/#!/?sk=lf
 234 http://www.odnoklassniki.ru/#/game/farmandia
 222 https://www.facebook.com/#!/
 190 http://www.tuenti.com/#m=Home&func=view_home
 168 http://twitter.com/#
 166 http://www.pandora.com/#/
 130 http://www.facebook.com/#
 117 http://www.odnoklassniki.ru/#/game/nl
 110 http://mail.yandex.ru/neo2/#inbox
 107 http://twitter.com/#!/mentions
  91 http://www.odnoklassniki.ru/#/game/mega
  85 http://www.pandora.com/#/paused
  83 https://twitter.com/#!/
(Assignee)

Comment 19

6 years ago
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.
(Assignee)

Comment 21

6 years ago
Checked in to Aurora.

http://hg.mozilla.org/releases/mozilla-aurora/rev/aab12a5b1b2f
(Assignee)

Updated

6 years ago
status-firefox5: --- → unaffected
status-firefox6: --- → fixed
status-firefox7: --- → fixed
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

Updated

6 years ago
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.