Bug 59877 - [chromium]: Scroll position not restored on navigation back for in-page links
Summary: [chromium]: Scroll position not restored on navigation back for in-page links
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nate Chapin
URL: http://www.w3.org/TR/2011/WD-css3-tex...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-05-01 10:05 PDT by Nico Weber
Modified: 2011-05-04 15:21 PDT (History)
8 users (show)

See Also:


Attachments
patch (2.87 KB, patch)
2011-05-04 13:47 PDT, Nate Chapin
mihaip: review+
japhet: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nico Weber 2011-05-01 10:05:22 PDT
1. Go to a page with in-page links, e.g. http://www.w3.org/TR/2011/WD-css3-text-20110412/
2. Scroll down to table of contens
3. Click any topic
4. Click "Back"

Expected: Scroll position from after step 2 is restored
Actual: Back button jumps to top of page.
Comment 1 Alexey Proskuryakov 2011-05-01 23:18:45 PDT
<rdar://problem/9366060>
Comment 2 Mihai Parparita 2011-05-02 14:13:35 PDT
I believe this has been fixed already by http://trac.webkit.org/changeset/84604. I'm not able to reproduce this with the most recent WebKit nightly (at r85429), but it does happen with one at r84547.

*** This bug has been marked as a duplicate of bug 57898 ***
Comment 3 Nico Weber 2011-05-02 14:15:43 PDT
Mihai: The problem repros for me in the chrome canary, which uses webkit r85369.
Comment 4 Nico Weber 2011-05-02 14:20:25 PDT
Turns out this doesn't repro in a webkit nightly. Sorry.
Comment 5 Yair Yogev 2011-05-03 23:50:34 PDT
reproducible using 
Google Chrome	13.0.754.0 (Official Build 83837) canary
WebKit	534.33 (trunk@85565)

with the steps:
1. visit
http://ruby.railstutorial.org/chapters/rails-flavored-ruby#sec:strings
2. click on another in page link in that page (to a different location, for example "4.2.1 Comments" which is just above the original link position)
3. clicking back will bring you back to "4.2.2 Strings" instead of the location you viewed when clicking on  that other link
Comment 6 Nate Chapin 2011-05-04 13:47:25 PDT
Created attachment 92312 [details]
patch
Comment 7 Mihai Parparita 2011-05-04 14:13:08 PDT
Comment on attachment 92312 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92312&action=review

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1045
> +    if (!m_inSameDocumentHistoryLoad && (m_frame->loader()->loadType() == FrameLoadTypeStandard

Any ideas as to why we save the scroll state in a getter in the first place? As far as I can tell, that's been there since this code was upstreamed, but it still seems odd.

I would be curious what breaks if we remove this altogether, but that can be done separately, since I believe this is blocking a Chromium release. Maybe just a FIXME?
Comment 8 Nate Chapin 2011-05-04 14:18:07 PDT
(In reply to comment #7)
> (From update of attachment 92312 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92312&action=review
> 
> > Source/WebKit/chromium/src/WebFrameImpl.cpp:1045
> > +    if (!m_inSameDocumentHistoryLoad && (m_frame->loader()->loadType() == FrameLoadTypeStandard
> 
> Any ideas as to why we save the scroll state in a getter in the first place? As far as I can tell, that's been there since this code was upstreamed, but it still seems odd.
> 
> I would be curious what breaks if we remove this altogether, but that can be done separately, since I believe this is blocking a Chromium release. Maybe just a FIXME?

I don't know the full story, but I assume the comment is correct in that we can clobber HistoryItems otherwise.  I've got a rare null-deref crash outstanding in this function, so I'll look into it more carefully in the near-ish future.  I'll add a FIXME before landing.
Comment 9 Nate Chapin 2011-05-04 14:32:53 PDT
Landed: http://trac.webkit.org/changeset/85787
Comment 10 WebKit Review Bot 2011-05-04 15:21:50 PDT
http://trac.webkit.org/changeset/85787 might have broken Leopard Intel Debug (Tests)
The following tests are not passing:
svg/text/text-block-child-crash.xhtml