Closed Bug 360511 Opened 18 years ago Closed 18 years ago

[FIX]Going back to page with URL hash (#foo) doesn't show hash part

Categories

(Core :: DOM: Navigation, defect, P1)

Product:
Core
Component:
DOM: Navigation
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.9alpha1

People

(Reporter: bugzilla-mozilla-20000923, Assigned: bzbarsky)

References

(
URL
)

Details

(Keywords: regression, verified1.8.0.10, verified1.8.1.2)

Attachments

(2 files, 1 obsolete file)

Fixes bug, as expected
(deleted), patch
jst
: review+
sicking
: superreview+
jay
: approval1.8.1.2+
jay
: approval1.8.0.10+
Details | Diff | Splinter Review
mochitest test case
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
It's unclear where the bug is here, but I can't find an existing one on the problem, and it appears to be at a lower level than the URLBar, so I'm going to go for docshell for now. Steps: 1. Load a page that has hash links, like the one in the URL, or a Bugzilla bug. 2. Click a hash link (e.g. "link here" on my page). 3. Click a normal link ("CVS log", Bugzilla attachment, etc.). 4. Click back once. Actual results: URLBar (and document.location) show the URL *without* the hash part. Expeted results: URLBar (and document.location) show the URL *with* the hash part. Note that clicking back a 2nd time will actually reload the page (seems wrong), and show the URL without the hash (correct). This occurs for me with Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9a1) Gecko/20061110 Firefox/3.0a1 ID:2006111004, and Hannibal on IRC confirmed it does not happen in his Firefox 2.
Looks like this regressed between 2005-08-02-05 and 2005-08-03-08 trunk Seamonkey builds and between . Checkins: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-08-02+05&maxdate=2005-08-03+08&cvsroot=%2Fcvsroot I would guess a regression from bug 301516, especially given that disabling bfcache fixes the problem. In fact, I bet the change from doing - SetCurrentURI(uri); to doing + SetCurrentURI(document->GetDocumentURI(), + document->GetChannel(), PR_TRUE); is the problem -- |uri| came from session history, of course: - aSHEntry->GetURI(getter_AddRefs(uri)); Except that doesn't explain why it doesn't appear on the 1.8 branch.... Maybe for some reason bfcache is not on for that page in the build in question? Or the link was clicked before the page was done loading?
Blocks: 301516
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.9?
OS: Windows Server 2003 → All
Hardware: PC → All
I see this bug on the 1.8 and 1.8.0 branches, so _that_ much of the mystery is resolved.
Assignee: nobody → bzbarsky
Priority: -- → P1
Summary: Going back to page with URL hash (#foo) doesn't show hash part → [FIX]Going back to page with URL hash (#foo) doesn't show hash part
Target Milestone: --- → mozilla1.9alpha
Attached patch Fixes bug, as expected (deleted) — — Splinter Review
Attachment #245427 - Flags: superreview?(bugmail)
Attachment #245427 - Flags: review?(jst)
For what it's worth, I do think this is worth fixing on branches...
Wow, nicely found! :) Don't know why Hannibal didn't see it on FF2, but I can't see any reason to not fix it on branch.
Attachment #245427 - Flags: superreview?(bugmail) → superreview+
Mm, upon trying to reproduce on a clean profile this morning, I can reproduce, though I should mention that whether or not you wait for the link in step 3 to load seems to affect the results. And even so, what I'm seeing is inconsistent (ie, with seemingly identical actions, results vary - I'm probably missing something).
Flags: wanted1.8.1.x?
Flags: wanted1.8.0.x?
Don't think this is a blocker for any branch security release, but we'll approve this patch once it's reviewed and trunk-baked.
Flags: wanted1.8.1.x?
Flags: wanted1.8.0.x?
Comment on attachment 245427 [details] [diff] [review] Fixes bug, as expected r=jst
Attachment #245427 - Flags: review?(jst) → review+
Fixed on trunk.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment on attachment 245427 [details] [diff] [review] Fixes bug, as expected I do think we shold fix this on branches... And I think this is a pretty safe fix.
Attachment #245427 - Flags: approval1.8.1.2?
Attachment #245427 - Flags: approval1.8.0.10?
Comment on attachment 245427 [details] [diff] [review] Fixes bug, as expected Approved for both branches, a=jay for drivers.
Attachment #245427 - Flags: approval1.8.1.2?
Attachment #245427 - Flags: approval1.8.1.2+
Attachment #245427 - Flags: approval1.8.0.10?
Attachment #245427 - Flags: approval1.8.0.10+
Fixed on branches.
Flags: blocking1.9? → blocking1.9-
Keywords: fixed1.8.0.10, fixed1.8.1.2
Verified on Trunk, 1.8.1.2 and 1.8.0.10 using the steps to reproduce from comment#1. URLBar (and document.location) show the URL with the hash part after going back. Verified on Fedora FC 6 and on Windows XP x64 with : Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9a2pre) Gecko/2007020611 Minefield/3.0a2pre for trunk Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.2pre) Gecko/20070207 BonEcho/2.0.0.2pre ID:2007020703 for 1.8.1.2 Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.0.10pre) Gecko/20070207 Firefox/1.5.0.10pre ID:2007020706 for 1.8.0.10
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.0.10, fixed1.8.1.2 → verified1.8.0.10, verified1.8.1.2
Attached patch mochitest test case (obsolete) (deleted) — — Splinter Review
Attachment #387106 - Flags: review?(bzbarsky)
A few comments here: 1) Replace (i == 1 ? false : true) with "i != 1". 2) This test assumes the browser window is under 1000px tall. That might stop being a good assumption... I would rather see the markup guarantee that we'll have something to scroll to. 3) I'd prefer that our random HTML files be in standards mode (have <!DOCTYPE html> at the top). Given 2 & 3, the markup for the test files should be something like (leaving out the head, etc): <!DOCTYPE html> <html style="height: 100%"> <body style="height: 100%"> <div style="height: 200%">hello large div</div> <a name="bottom"></a> </body> </html> That will guarantee the div being 2x the height of the viewport.
Attachment #387106 - Flags: review?(bzbarsky) → review-
Attached patch mochitest test case (deleted) — — Splinter Review
Thanks for the review Boris. I've applied all your suggestions. I've also added some additional checks that the page is really getting scrolled when we click on a fragment link. I'll also start using <!DOCTYPE html> on top of random HTML files in these tests.
Attachment #387106 - Attachment is obsolete: true
Attachment #387920 - Flags: review?(bzbarsky)
Attachment #387920 - Flags: review?(bzbarsky) → review+
Pushed test as http://hg.mozilla.org/mozilla-central/rev/cdb21eed2fdf
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: