Bug 59877 – [chromium]: Scroll position not restored on navigation back for in-page links

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Nico Weber]

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.

[Alexey Proskuryakov]

<rdar://problem/9366060>

[Mihai Parparita]

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

[Nico Weber]

Mihai: The problem repros for me in the chrome canary, which uses webkit r85369.

[Nico Weber]

Turns out this doesn't repro in a webkit nightly. Sorry.

[Yair Yogev]

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

[Nate Chapin]

Created attachment 92312 [details]
patch

[Mihai Parparita]

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?

[Nate Chapin]

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

[Nate Chapin]

Landed: http://trac.webkit.org/changeset/85787

[WebKit Review Bot]

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