Last Comment Bug 662170 - Go to top anchor "#" doesn't work in Firefox 6 and Firefox 7
: Go to top anchor "#" doesn't work in Firefox 6 and Firefox 7
Status: VERIFIED FIXED
: regression, testcase
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
Depends on:
Blocks: 640387
  Show dependency treegraph
 
Reported: 2011-06-05 10:48 PDT by bdesef
Modified: 2013-12-27 14:24 PST (History)
16 users (show)
jruderman: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
fixed
fixed


Attachments
Testcase (518 bytes, text/html)
2011-06-05 10:49 PDT, bdesef
no flags Details
Patch v1 (6.43 KB, patch)
2011-06-06 09:35 PDT, Justin Lebar (not reading bugmail)
bzbarsky: review+
Details | Diff | Review
Patch v2 (per comment 10) (6.48 KB, patch)
2011-06-06 12:28 PDT, Justin Lebar (not reading bugmail)
jst: approval‑mozilla‑aurora+
Details | Diff | Review

Description bdesef 2011-06-05 10:48:23 PDT
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 2011-06-05 10:49:01 PDT
Created attachment 537463 [details]
Testcase
Comment 2 Thomas Ahlblom 2011-06-05 11:40:02 PDT
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 2011-06-05 12:36:24 PDT
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
Comment 4 Justin Lebar (not reading bugmail) 2011-06-05 19:22:52 PDT
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) 2011-06-06 07:28:33 PDT
User-facing regression of basic browsing behavior.  Nominating tracking ff-6.
Comment 6 Justin Lebar (not reading bugmail) 2011-06-06 07:35:24 PDT
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 [:bz] 2011-06-06 08:53:26 PDT
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) 2011-06-06 09:35:07 PDT
Created attachment 537575 [details] [diff] [review]
Patch v1
Comment 9 Justin Lebar (not reading bugmail) 2011-06-06 09:35:56 PDT
Comment on attachment 537575 [details] [diff] [review]
Patch v1

This applies cleanly to aurora and trunk.
Comment 10 Boris Zbarsky [:bz] 2011-06-06 11:09:19 PDT
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) 2011-06-06 12:28:57 PDT
Created attachment 537609 [details] [diff] [review]
Patch v2 (per comment 10)
Comment 12 Justin Lebar (not reading bugmail) 2011-06-06 12:30:08 PDT
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 [:bz] 2011-06-06 12:36:00 PDT
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) 2011-06-08 15:03:37 PDT
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?) 2011-06-09 14:29:24 PDT
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] 2011-06-09 14:44:26 PDT
please provide a risk assessment. thanks.
Comment 17 Justin Lebar (not reading bugmail) 2011-06-09 14:49:08 PDT
(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 19 Justin Lebar (not reading bugmail) 2011-06-16 15:16:44 PDT
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 [:bz] 2011-06-16 19:09:39 PDT
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) 2011-06-20 08:04:11 PDT
Checked in to Aurora.

http://hg.mozilla.org/releases/mozilla-aurora/rev/aab12a5b1b2f
Comment 22 Simona B [:simonab] 2011-08-10 06:41:36 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.