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

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[bdesef]

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.

Comment 1

[bdesef]

Attachment: Testcase

Comment 2

[Thomas Ahlblom]

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

Comment 3

[Thomas Ahlblom]

The first bad revision is:
changeset:   68370:228d41c97600
user:        Justin Lebar <[e-mail]>
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

Comment 4

[Justin Lebar (not reading bugmail)]

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...

Comment 5

[Justin Lebar (not reading bugmail)]

User-facing regression of basic browsing behavior.  Nominating tracking ff-6.

Comment 6

[Justin Lebar (not reading bugmail)]

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 8

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

Comment 9

[Justin Lebar (not reading bugmail)]

Comment on attachment 537575 [details] [diff] [review]
Patch v1

This applies cleanly to aurora and trunk.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.

Comment 11

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2 (per comment 10)

Comment 12

[Justin Lebar (not reading bugmail)]

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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

fwiw, you can use isnot() in tests.

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

No.

Comment 14

[Justin Lebar (not reading bugmail)]

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

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 16

[Asa Dotzler [:asa]]

please provide a risk assessment. thanks.

Comment 17

[Justin Lebar (not reading bugmail)]

(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.

Comment 18

[chris hofmann]

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/#!/

Comment 19

[Justin Lebar (not reading bugmail)]

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?

Comment 20

[Boris Zbarsky [:bzbarsky]]

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.

Comment 21

[Justin Lebar (not reading bugmail)]

Checked in to Aurora.

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

Comment 22

[Simona Badau, Desktop QA]

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.